The dentries such as /proc/<pid>/ns/ have the DCACHE_OP_DELETE flag, they should be deleted when the process exits.
Suppose the following race appears：
release_task dput -> proc_flush_task -> dentry->d_op->d_delete(dentry) -> __exit_signal -> dentry->d_lockref.count-- and return.
In the proc_flush_task(), if another process is using this dentry, it will not be deleted. At the same time, in dput(), d_op->d_delete() can be executed before __exit_signal(pid has not been hashed), d_delete returns false, so this dentry still cannot be deleted.
This dentry will always be cached (although its count is 0 and the DCACHE_OP_DELETE flag is set), its parent denry will also be cached too, and these dentries can only be deleted when drop_caches is manually triggered.
This will result in wasted memory. What's more troublesome is that these dentries reference pid, according to the commit f333c700c610 ("pidns: Add a limit on the number of pid namespaces"), if the pid cannot be released, it may result in the inability to create a new pid_ns.
This problem occurred in our cluster environment (Linux 4.9 LTS). We could reproduce it by manually constructing a test program + adding some debugging switches in the kernel: * A test program to open the directory (/proc/<pid>/ns) [1] * Adding some debugging switches to the kernel, adding a delay between proc_flush_task and __exit_signal in release_task() [2]
The test process is as follows:
A, terminal #1
Turn on the debug switch: echo 1> /proc/sys/vm/dentry_debug_trace
Execute the following unshare command: sudo unshare --pid --fork --mount-proc bash
B, terminal #2
Find the pid of the unshare process:
# pstree -p | grep unshare | `-sshd(716)---bash(718)--sudo(816)---unshare(817)---bash(818)
Find the corresponding dentry: # dmesg | grep pid=818 [70.424722] XXX proc_pid_instantiate:3119 pid=818 tid=818 entry=818/ffff8802c7b670e8
C, terminal #3
Execute the opendir program, it will always open the /proc/818/ns/ directory:
# ./a.out /proc/818/ns/ pid: 876 . .. net uts ipc pid user mnt cgroup
D, go back to terminal #2
Turn on the debugging switches to construct the race: # echo 818> /proc/sys/vm/dentry_debug_pid # echo 1> /proc/sys/vm/dentry_debug_delay
Kill the unshare process (pid 818). Since the debugging switches have been turned on, it will get stuck in release_task(): # kill -9 818
Then kill the process that opened the /proc/818/ns/ directory: # kill -9 876
Then turn off these debugging switches to allow the 818 process to exit: # echo 0> /proc/sys/vm/dentry_debug_delay # echo 0> /proc/sys/vm/dentry_debug_pid
Checking the dmesg, we will find that the dentry(/proc/818/ns) ’s count is 0, and the flag is 2800cc (#define DCACHE_OP_DELETE 0x00000008), but it is still cached: # dmesg | grep ffff8802a3999548 … [565.559156] XXX dput:853 dentry=ns/ffff8802bea7b528, flag=2800cc, cnt=0, inode=ffff8802b38c2010, pdentry=818/ffff8802c7b670e8, pflag=20008c, pcnt=1, pinode=ffff8802c7812010, keywords: be cached
It could also be verified via the crash tool:
crash> dentry.d_flags,d_iname,d_inode,d_lockref -x ffff8802bea7b528 d_flags = 0x2800cc d_iname = "ns\000kkkkkkkkkkkkkkkkkkkkkkkkkkkk" d_inode = 0xffff8802b38c2010 d_lockref = { { lock_count = 0x0, { lock = { { rlock = { raw_lock = { { val = { counter = 0x0 }, { locked = 0x0, pending = 0x0 }, { locked_pending = 0x0, tail = 0x0 } } } } } }, count = 0x0 } } } crash> kmem ffff8802bea7b528 CACHE OBJSIZE ALLOCATED TOTAL SLABS SSIZE NAME ffff8802dd5f5900 192 23663 26130 871 16k dentry SLAB MEMORY NODE TOTAL ALLOCATED FREE ffffea000afa9e00 ffff8802bea78000 0 30 25 5 FREE / [ALLOCATED] [ffff8802bea7b520]
PAGE PHYSICAL MAPPING INDEX CNT FLAGS ffffea000afa9ec0 2bea7b000 dead000000000400 0 0 2fffff80000000 crash>
This series of patches is to fix this issue.
Regards, Wen
Alexey Dobriyan (1): proc: use %u for pid printing and slightly less stack
Andreas Gruenbacher (1): proc: Pass file mode to proc_pid_make_inode
Christian Brauner (1): clone: add CLONE_PIDFD
Eric W. Biederman (6): proc: Better ownership of files for non-dumpable tasks in user namespaces proc: Rename in proc_inode rename sysctl_inodes sibling_inodes proc: Generalize proc_sys_prune_dcache into proc_prune_siblings_dcache proc: Clear the pieces of proc_inode that proc_evict_inode cares about proc: Use d_invalidate in proc_prune_siblings_dcache proc: Use a list of inodes to flush from proc
Joel Fernandes (Google) (1): pidfd: add polling support
fs/proc/base.c | 242 ++++++++++++++++++++------------------------- fs/proc/fd.c | 20 +--- fs/proc/inode.c | 67 ++++++++++++- fs/proc/internal.h | 22 ++--- fs/proc/namespaces.c | 3 +- fs/proc/proc_sysctl.c | 45 ++------- fs/proc/self.c | 6 +- fs/proc/thread_self.c | 5 +- include/linux/pid.h | 5 + include/linux/proc_fs.h | 4 +- include/uapi/linux/sched.h | 1 + kernel/exit.c | 5 +- kernel/fork.c | 145 ++++++++++++++++++++++++++- kernel/pid.c | 3 + kernel/signal.c | 11 +++ security/selinux/hooks.c | 1 + 16 files changed, 357 insertions(+), 228 deletions(-)
[1] A test program to open the directory (/proc/<pid>/ns) #include <stdio.h> #include <sys/types.h> #include <dirent.h> #include <errno.h>
int main(int argc, char *argv[]) { DIR *dip; struct dirent *dit;
if (argc < 2) { printf("Usage :%s <directory>\n", argv[0]); return -1; }
if ((dip = opendir(argv[1])) == NULL) { perror("opendir"); return -1; }
printf("pid: %d\n", getpid()); while((dit = readdir (dip)) != NULL) { printf("%s\n", dit->d_name); }
while (1) sleep (1);
return 0; }
[2] Adding some debugging switches to the kernel, also adding a delay between proc_flush_task and __exit_signal in release_task():
diff --git a/fs/dcache.c b/fs/dcache.c index 05bad55..fafad37 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -84,6 +84,9 @@ int sysctl_vfs_cache_pressure __read_mostly = 100; EXPORT_SYMBOL_GPL(sysctl_vfs_cache_pressure);
+int sysctl_dentry_debug_trace __read_mostly = 0; +EXPORT_SYMBOL_GPL(sysctl_dentry_debug_trace); + __cacheline_aligned_in_smp DEFINE_SEQLOCK(rename_lock);
EXPORT_SYMBOL(rename_lock); @@ -758,6 +761,26 @@ static inline bool fast_dput(struct dentry *dentry) return 0; }
+#define DENTRY_DEBUG_TRACE(dentry, keywords) \ +do { \ + if (sysctl_dentry_debug_trace) \ + printk("XXX %s:%d " \ + "dentry=%s/%p, flag=%x, cnt=%d, inode=%p, " \ + "pdentry=%s/%p, pflag=%x, pcnt=%d, pinode=%p, " \ + "keywords: %s\n", \ + __func__, __LINE__, \ + dentry->d_name.name, \ + dentry, \ + dentry->d_flags, \ + dentry->d_lockref.count, \ + dentry->d_inode, \ + dentry->d_parent->d_name.name, \ + dentry->d_parent, \ + dentry->d_parent->d_flags, \ + dentry->d_parent->d_lockref.count, \ + dentry->d_parent->d_inode, \ + keywords); \ +} while (0)
/* * This is dput @@ -804,6 +827,8 @@ void dput(struct dentry *dentry)
WARN_ON(d_in_lookup(dentry));
+ DENTRY_DEBUG_TRACE(dentry, "be checked"); + /* Unreachable? Get rid of it */ if (unlikely(d_unhashed(dentry))) goto kill_it; @@ -812,8 +837,10 @@ void dput(struct dentry *dentry) goto kill_it;
if (unlikely(dentry->d_flags & DCACHE_OP_DELETE)) { - if (dentry->d_op->d_delete(dentry)) + if (dentry->d_op->d_delete(dentry)) { + DENTRY_DEBUG_TRACE(dentry, "be killed"); goto kill_it; + } }
if (!(dentry->d_flags & DCACHE_REFERENCED)) @@ -822,6 +849,9 @@ void dput(struct dentry *dentry)
dentry->d_lockref.count--; spin_unlock(&dentry->d_lock); + + DENTRY_DEBUG_TRACE(dentry, "be cached"); + return;
kill_it: diff --git a/fs/proc/base.c b/fs/proc/base.c index b9e4183..419a409 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -3090,6 +3090,8 @@ void proc_flush_task(struct task_struct *task) } }
+extern int sysctl_dentry_debug_trace; + static int proc_pid_instantiate(struct inode *dir, struct dentry * dentry, struct task_struct *task, const void *ptr) @@ -3111,6 +3113,12 @@ static int proc_pid_instantiate(struct inode *dir, d_set_d_op(dentry, &pid_dentry_operations);
d_add(dentry, inode); + + if (sysctl_dentry_debug_trace) + printk("XXX %s:%d pid=%d tid=%d entry=%s/%p\n", + __func__, __LINE__, task->pid, task->tgid, + dentry->d_name.name, dentry); + /* Close the race of the process dying before we return the dentry */ if (pid_revalidate(dentry, 0)) return 0; diff --git a/kernel/exit.c b/kernel/exit.c index 27f4168..2b3e1b6 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -55,6 +55,8 @@ #include <linux/shm.h> #include <linux/kcov.h>
+#include <linux/delay.h> + #include <asm/uaccess.h> #include <asm/unistd.h> #include <asm/pgtable.h> @@ -164,6 +166,8 @@ static void delayed_put_task_struct(struct rcu_head *rhp) put_task_struct(tsk); }
+int sysctl_dentry_debug_delay __read_mostly = 0; +int sysctl_dentry_debug_pid __read_mostly = 0;
void release_task(struct task_struct *p) { @@ -178,6 +182,11 @@ void release_task(struct task_struct *p)
proc_flush_task(p);
+ if (sysctl_dentry_debug_delay && p->pid == sysctl_dentry_debug_pid) { + while (sysctl_dentry_debug_delay) + mdelay(1); + } + write_lock_irq(&tasklist_lock); ptrace_release_task(p); __exit_signal(p); diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 513e6da..27f1395 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -282,6 +282,10 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write, static int max_extfrag_threshold = 1000; #endif
+extern int sysctl_dentry_debug_trace; +extern int sysctl_dentry_debug_delay; +extern int sysctl_dentry_debug_pid; + static struct ctl_table kern_table[] = { { .procname = "sched_child_runs_first", @@ -1498,6 +1502,30 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write, .proc_handler = proc_dointvec, .extra1 = &zero, }, + { + .procname = "dentry_debug_trace", + .data = &sysctl_dentry_debug_trace, + .maxlen = sizeof(sysctl_dentry_debug_trace), + .mode = 0644, + .proc_handler = proc_dointvec, + .extra1 = &zero, + }, + { + .procname = "dentry_debug_delay", + .data = &sysctl_dentry_debug_delay, + .maxlen = sizeof(sysctl_dentry_debug_delay), + .mode = 0644, + .proc_handler = proc_dointvec, + .extra1 = &zero, + }, + { + .procname = "dentry_debug_pid", + .data = &sysctl_dentry_debug_pid, + .maxlen = sizeof(sysctl_dentry_debug_pid), + .mode = 0644, + .proc_handler = proc_dointvec, + .extra1 = &zero, + }, #ifdef HAVE_ARCH_PICK_MMAP_LAYOUT { .procname = "legacy_va_layout",
Signed-off-by: Wen Yang wenyang@linux.alibaba.com Cc: Pavel Emelyanov xemul@openvz.org Cc: Oleg Nesterov oleg@tv-sign.ru Cc: Sukadev Bhattiprolu sukadev@us.ibm.com Cc: Paul Menage menage@google.com Cc: "Eric W. Biederman" ebiederm@xmission.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: stable@vger.kernel.org
From: Christian Brauner christian@brauner.io
[ Upstream commit b3e5838252665ee4cfa76b82bdf1198dca81e5be ]
This patchset makes it possible to retrieve pid file descriptors at process creation time by introducing the new flag CLONE_PIDFD to the clone() system call. Linus originally suggested to implement this as a new flag to clone() instead of making it a separate system call. As spotted by Linus, there is exactly one bit for clone() left.
CLONE_PIDFD creates file descriptors based on the anonymous inode implementation in the kernel that will also be used to implement the new mount api. They serve as a simple opaque handle on pids. Logically, this makes it possible to interpret a pidfd differently, narrowing or widening the scope of various operations (e.g. signal sending). Thus, a pidfd cannot just refer to a tgid, but also a tid, or in theory - given appropriate flag arguments in relevant syscalls - a process group or session. A pidfd does not represent a privilege. This does not imply it cannot ever be that way but for now this is not the case.
A pidfd comes with additional information in fdinfo if the kernel supports procfs. The fdinfo file contains the pid of the process in the callers pid namespace in the same format as the procfs status file, i.e. "Pid:\t%d".
As suggested by Oleg, with CLONE_PIDFD the pidfd is returned in the parent_tidptr argument of clone. This has the advantage that we can give back the associated pid and the pidfd at the same time.
To remove worries about missing metadata access this patchset comes with a sample program that illustrates how a combination of CLONE_PIDFD, and pidfd_send_signal() can be used to gain race-free access to process metadata through /proc/<pid>. The sample program can easily be translated into a helper that would be suitable for inclusion in libc so that users don't have to worry about writing it themselves.
Suggested-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Christian Brauner christian@brauner.io Co-developed-by: Jann Horn jannh@google.com Signed-off-by: Jann Horn jannh@google.com Reviewed-by: Oleg Nesterov oleg@redhat.com Cc: Arnd Bergmann arnd@arndb.de Cc: "Eric W. Biederman" ebiederm@xmission.com Cc: Kees Cook keescook@chromium.org Cc: Thomas Gleixner tglx@linutronix.de Cc: David Howells dhowells@redhat.com Cc: "Michael Kerrisk (man-pages)" mtk.manpages@gmail.com Cc: Andy Lutomirsky luto@kernel.org Cc: Andrew Morton akpm@linux-foundation.org Cc: Aleksa Sarai cyphar@cyphar.com Cc: Linus Torvalds torvalds@linux-foundation.org Cc: Al Viro viro@zeniv.linux.org.uk Cc: stable@vger.kernel.org # 4.9.x (clone: fix up cherry-pick conflicts for b3e583825266) Signed-off-by: Wen Yang wenyang@linux.alibaba.com --- include/linux/pid.h | 1 + include/uapi/linux/sched.h | 1 + kernel/fork.c | 119 +++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 117 insertions(+), 4 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h index 97b745d..7599a78 100644 --- a/include/linux/pid.h +++ b/include/linux/pid.h @@ -73,6 +73,7 @@ struct pid_link struct hlist_node node; struct pid *pid; }; +extern const struct file_operations pidfd_fops;
static inline struct pid *get_pid(struct pid *pid) { diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h index 5f0fe01..ed6e31d 100644 --- a/include/uapi/linux/sched.h +++ b/include/uapi/linux/sched.h @@ -9,6 +9,7 @@ #define CLONE_FS 0x00000200 /* set if fs info shared between processes */ #define CLONE_FILES 0x00000400 /* set if open files shared between processes */ #define CLONE_SIGHAND 0x00000800 /* set if signal handlers and blocked signals shared */ +#define CLONE_PIDFD 0x00001000 /* set if a pidfd should be placed in parent */ #define CLONE_PTRACE 0x00002000 /* set if we want to let tracing continue on the child too */ #define CLONE_VFORK 0x00004000 /* set if the parent wants the child to wake it up on mm_release */ #define CLONE_PARENT 0x00008000 /* set if we want to have the same parent as the cloner */ diff --git a/kernel/fork.c b/kernel/fork.c index b64efec..076297a 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -11,7 +11,22 @@ * management can be a bitch. See 'mm/memory.c': 'copy_page_range()' */
+#include <linux/anon_inodes.h> #include <linux/slab.h> +#if 0 +#include <linux/sched/autogroup.h> +#include <linux/sched/mm.h> +#include <linux/sched/coredump.h> +#include <linux/sched/user.h> +#include <linux/sched/numa_balancing.h> +#include <linux/sched/stat.h> +#include <linux/sched/task.h> +#include <linux/sched/task_stack.h> +#include <linux/sched/cputime.h> +#include <linux/seq_file.h> +#include <linux/rtmutex.h> +>>>>>>> b3e58382... clone: add CLONE_PIDFD +#endif #include <linux/init.h> #include <linux/unistd.h> #include <linux/module.h> @@ -1460,6 +1475,58 @@ static void posix_cpu_timers_init(struct task_struct *tsk) task->pids[type].pid = pid; }
+static int pidfd_release(struct inode *inode, struct file *file) +{ + struct pid *pid = file->private_data; + + file->private_data = NULL; + put_pid(pid); + return 0; +} + +#ifdef CONFIG_PROC_FS +static void pidfd_show_fdinfo(struct seq_file *m, struct file *f) +{ + struct pid_namespace *ns = file_inode(m->file)->i_sb->s_fs_info; + struct pid *pid = f->private_data; + + seq_put_decimal_ull(m, "Pid:\t", pid_nr_ns(pid, ns)); + seq_putc(m, '\n'); +} +#endif + +const struct file_operations pidfd_fops = { + .release = pidfd_release, +#ifdef CONFIG_PROC_FS + .show_fdinfo = pidfd_show_fdinfo, +#endif +}; + +/** + * pidfd_create() - Create a new pid file descriptor. + * + * @pid: struct pid that the pidfd will reference + * + * This creates a new pid file descriptor with the O_CLOEXEC flag set. + * + * Note, that this function can only be called after the fd table has + * been unshared to avoid leaking the pidfd to the new process. + * + * Return: On success, a cloexec pidfd is returned. + * On error, a negative errno number will be returned. + */ +static int pidfd_create(struct pid *pid) +{ + int fd; + + fd = anon_inode_getfd("[pidfd]", &pidfd_fops, get_pid(pid), + O_RDWR | O_CLOEXEC); + if (fd < 0) + put_pid(pid); + + return fd; +} + /* * This creates a new process as a copy of the old one, * but does not actually start it yet. @@ -1472,13 +1539,14 @@ static __latent_entropy struct task_struct *copy_process( unsigned long clone_flags, unsigned long stack_start, unsigned long stack_size, + int __user *parent_tidptr, int __user *child_tidptr, struct pid *pid, int trace, unsigned long tls, int node) { - int retval; + int pidfd = -1, retval; struct task_struct *p;
if ((clone_flags & (CLONE_NEWNS|CLONE_FS)) == (CLONE_NEWNS|CLONE_FS)) @@ -1526,6 +1594,30 @@ static __latent_entropy struct task_struct *copy_process( retval = security_task_create(clone_flags); if (retval) goto fork_out; + if (clone_flags & CLONE_PIDFD) { + int reserved; + + /* + * - CLONE_PARENT_SETTID is useless for pidfds and also + * parent_tidptr is used to return pidfds. + * - CLONE_DETACHED is blocked so that we can potentially + * reuse it later for CLONE_PIDFD. + * - CLONE_THREAD is blocked until someone really needs it. + */ + if (clone_flags & + (CLONE_DETACHED | CLONE_PARENT_SETTID | CLONE_THREAD)) + return ERR_PTR(-EINVAL); + + /* + * Verify that parent_tidptr is sane so we can potentially + * reuse it later. + */ + if (get_user(reserved, parent_tidptr)) + return ERR_PTR(-EFAULT); + + if (reserved != 0) + return ERR_PTR(-EINVAL); + }
retval = -ENOMEM; p = dup_task_struct(current, node); @@ -1703,6 +1795,22 @@ static __latent_entropy struct task_struct *copy_process( } }
+ /* + * This has to happen after we've potentially unshared the file + * descriptor table (so that the pidfd doesn't leak into the child + * if the fd table isn't shared). + */ + if (clone_flags & CLONE_PIDFD) { + retval = pidfd_create(pid); + if (retval < 0) + goto bad_fork_free_pid; + + pidfd = retval; + retval = put_user(pidfd, parent_tidptr); + if (retval) + goto bad_fork_put_pidfd; + } + #ifdef CONFIG_BLOCK p->plug = NULL; #endif @@ -1758,7 +1866,7 @@ static __latent_entropy struct task_struct *copy_process( */ retval = cgroup_can_fork(p); if (retval) - goto bad_fork_free_pid; + goto bad_fork_put_pidfd;
/* * From this point on we must avoid any synchronous user-space @@ -1869,6 +1977,9 @@ static __latent_entropy struct task_struct *copy_process( spin_unlock(¤t->sighand->siglock); write_unlock_irq(&tasklist_lock); cgroup_cancel_fork(p); +bad_fork_put_pidfd: + if (clone_flags & CLONE_PIDFD) + __close_fd(current->files, pidfd); bad_fork_free_pid: threadgroup_change_end(current); if (pid != &init_struct_pid) @@ -1928,7 +2039,7 @@ static inline void init_idle_pids(struct pid_link *links) struct task_struct *fork_idle(int cpu) { struct task_struct *task; - task = copy_process(CLONE_VM, 0, 0, NULL, &init_struct_pid, 0, 0, + task = copy_process(CLONE_VM, 0, 0, NULL, NULL, &init_struct_pid, 0, 0, cpu_to_node(cpu)); if (!IS_ERR(task)) { init_idle_pids(task->pids); @@ -1973,7 +2084,7 @@ long _do_fork(unsigned long clone_flags, trace = 0; }
- p = copy_process(clone_flags, stack_start, stack_size, + p = copy_process(clone_flags, stack_start, stack_size, parent_tidptr, child_tidptr, NULL, trace, tls, NUMA_NO_NODE); add_latent_entropy(); /*
From: "Joel Fernandes (Google)" joel@joelfernandes.org
[ Upstream commit b53b0b9d9a613c418057f6cb921c2f40a6f78c24 ]
This patch adds polling support to pidfd.
Android low memory killer (LMK) needs to know when a process dies once it is sent the kill signal. It does so by checking for the existence of /proc/pid which is both racy and slow. For example, if a PID is reused between when LMK sends a kill signal and checks for existence of the PID, since the wrong PID is now possibly checked for existence. Using the polling support, LMK will be able to get notified when a process exists in race-free and fast way, and allows the LMK to do other things (such as by polling on other fds) while awaiting the process being killed to die.
For notification to polling processes, we follow the same existing mechanism in the kernel used when the parent of the task group is to be notified of a child's death (do_notify_parent). This is precisely when the tasks waiting on a poll of pidfd are also awakened in this patch.
We have decided to include the waitqueue in struct pid for the following reasons: 1. The wait queue has to survive for the lifetime of the poll. Including it in task_struct would not be option in this case because the task can be reaped and destroyed before the poll returns.
2. By including the struct pid for the waitqueue means that during de_thread(), the new thread group leader automatically gets the new waitqueue/pid even though its task_struct is different.
Appropriate test cases are added in the second patch to provide coverage of all the cases the patch is handling.
Cc: Andy Lutomirski luto@amacapital.net Cc: Steven Rostedt rostedt@goodmis.org Cc: Daniel Colascione dancol@google.com Cc: Jann Horn jannh@google.com Cc: Tim Murray timmurray@google.com Cc: Jonathan Kowalski bl0pbl33p@gmail.com Cc: Linus Torvalds torvalds@linux-foundation.org Cc: Al Viro viro@zeniv.linux.org.uk Cc: Kees Cook keescook@chromium.org Cc: David Howells dhowells@redhat.com Cc: Oleg Nesterov oleg@redhat.com Cc: kernel-team@android.com Reviewed-by: Oleg Nesterov oleg@redhat.com Co-developed-by: Daniel Colascione dancol@google.com Signed-off-by: Daniel Colascione dancol@google.com Signed-off-by: Joel Fernandes (Google) joel@joelfernandes.org Signed-off-by: Christian Brauner christian@brauner.io Cc: stable@vger.kernel.org # 4.9.x: b3e5838: clone: add CLONE_PIDFD Cc: stable@vger.kernel.org # 4.9.x (pidfd: fix up cherry-pick conflicts for b53b0b9d9a61) Signed-off-by: Wen Yang wenyang@linux.alibaba.com --- include/linux/pid.h | 3 +++ kernel/fork.c | 26 ++++++++++++++++++++++++++ kernel/pid.c | 2 ++ kernel/signal.c | 11 +++++++++++ 4 files changed, 42 insertions(+)
diff --git a/include/linux/pid.h b/include/linux/pid.h index 7599a78..f5552ba 100644 --- a/include/linux/pid.h +++ b/include/linux/pid.h @@ -2,6 +2,7 @@ #define _LINUX_PID_H
#include <linux/rcupdate.h> +#include <linux/wait.h>
enum pid_type { @@ -62,6 +63,8 @@ struct pid unsigned int level; /* lists of tasks that use this pid */ struct hlist_head tasks[PIDTYPE_MAX]; + /* wait queue for pidfd notifications */ + wait_queue_head_t wait_pidfd; struct rcu_head rcu; struct upid numbers[1]; }; diff --git a/kernel/fork.c b/kernel/fork.c index 076297a..ac57f91 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1495,8 +1495,34 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f) } #endif
+/* + * Poll support for process exit notification. + */ +static unsigned int pidfd_poll(struct file *file, struct poll_table_struct *pts) +{ + struct task_struct *task; + struct pid *pid = file->private_data; + int poll_flags = 0; + + poll_wait(file, &pid->wait_pidfd, pts); + + rcu_read_lock(); + task = pid_task(pid, PIDTYPE_PID); + /* + * Inform pollers only when the whole thread group exits. + * If the thread group leader exits before all other threads in the + * group, then poll(2) should block, similar to the wait(2) family. + */ + if (!task || (task->exit_state && thread_group_empty(task))) + poll_flags = POLLIN | POLLRDNORM; + rcu_read_unlock(); + + return poll_flags; +} + const struct file_operations pidfd_fops = { .release = pidfd_release, + .poll = pidfd_poll, #ifdef CONFIG_PROC_FS .show_fdinfo = pidfd_show_fdinfo, #endif diff --git a/kernel/pid.c b/kernel/pid.c index fa704f8..e605398 100644 --- a/kernel/pid.c +++ b/kernel/pid.c @@ -333,6 +333,8 @@ struct pid *alloc_pid(struct pid_namespace *ns) for (type = 0; type < PIDTYPE_MAX; ++type) INIT_HLIST_HEAD(&pid->tasks[type]);
+ init_waitqueue_head(&pid->wait_pidfd); + upid = pid->numbers + ns->level; spin_lock_irq(&pidmap_lock); if (!(ns->nr_hashed & PIDNS_HASH_ADDING)) diff --git a/kernel/signal.c b/kernel/signal.c index bedca16..053de87a 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -1632,6 +1632,14 @@ int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group) return ret; }
+static void do_notify_pidfd(struct task_struct *task) +{ + struct pid *pid; + + pid = task_pid(task); + wake_up_all(&pid->wait_pidfd); +} + /* * Let a parent know about the death of a child. * For a stopped/continued status change, use do_notify_parent_cldstop instead. @@ -1655,6 +1663,9 @@ bool do_notify_parent(struct task_struct *tsk, int sig) BUG_ON(!tsk->ptrace && (tsk->group_leader != tsk || !thread_group_empty(tsk)));
+ /* Wake up all pidfd waiters */ + do_notify_pidfd(tsk); + if (sig != SIGCHLD) { /* * This is only possible if parent == real_parent.
From: Andreas Gruenbacher agruenba@redhat.com
[ Upstream commit db978da8fa1d0819b210c137d31a339149b88875 ]
Pass the file mode of the proc inode to be created to proc_pid_make_inode. In proc_pid_make_inode, initialize inode->i_mode before calling security_task_to_inode. This allows selinux to set isec->sclass right away without introducing "half-initialized" inode security structs.
Signed-off-by: Andreas Gruenbacher agruenba@redhat.com Signed-off-by: Paul Moore paul@paul-moore.com Cc: stable@vger.kernel.org # 4.9.x Signed-off-by: Wen Yang wenyang@linux.alibaba.com --- fs/proc/base.c | 23 +++++++++-------------- fs/proc/fd.c | 6 ++---- fs/proc/internal.h | 2 +- fs/proc/namespaces.c | 3 +-- security/selinux/hooks.c | 1 + 5 files changed, 14 insertions(+), 21 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c index b9e4183..ee2e0ec 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -1676,7 +1676,8 @@ static int proc_pid_readlink(struct dentry * dentry, char __user * buffer, int b
/* building an inode */
-struct inode *proc_pid_make_inode(struct super_block * sb, struct task_struct *task) +struct inode *proc_pid_make_inode(struct super_block * sb, + struct task_struct *task, umode_t mode) { struct inode * inode; struct proc_inode *ei; @@ -1690,6 +1691,7 @@ struct inode *proc_pid_make_inode(struct super_block * sb, struct task_struct *t
/* Common stuff */ ei = PROC_I(inode); + inode->i_mode = mode; inode->i_ino = get_next_ino(); inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode); inode->i_op = &proc_def_inode_operations; @@ -2041,7 +2043,9 @@ struct map_files_info { struct proc_inode *ei; struct inode *inode;
- inode = proc_pid_make_inode(dir->i_sb, task); + inode = proc_pid_make_inode(dir->i_sb, task, S_IFLNK | + ((mode & FMODE_READ ) ? S_IRUSR : 0) | + ((mode & FMODE_WRITE) ? S_IWUSR : 0)); if (!inode) return -ENOENT;
@@ -2050,12 +2054,6 @@ struct map_files_info {
inode->i_op = &proc_map_files_link_inode_operations; inode->i_size = 64; - inode->i_mode = S_IFLNK; - - if (mode & FMODE_READ) - inode->i_mode |= S_IRUSR; - if (mode & FMODE_WRITE) - inode->i_mode |= S_IWUSR;
d_set_d_op(dentry, &tid_map_files_dentry_operations); d_add(dentry, inode); @@ -2409,12 +2407,11 @@ static int proc_pident_instantiate(struct inode *dir, struct inode *inode; struct proc_inode *ei;
- inode = proc_pid_make_inode(dir->i_sb, task); + inode = proc_pid_make_inode(dir->i_sb, task, p->mode); if (!inode) goto out;
ei = PROC_I(inode); - inode->i_mode = p->mode; if (S_ISDIR(inode->i_mode)) set_nlink(inode, 2); /* Use getattr to fix if necessary */ if (p->iop) @@ -3096,11 +3093,10 @@ static int proc_pid_instantiate(struct inode *dir, { struct inode *inode;
- inode = proc_pid_make_inode(dir->i_sb, task); + inode = proc_pid_make_inode(dir->i_sb, task, S_IFDIR | S_IRUGO | S_IXUGO); if (!inode) goto out;
- inode->i_mode = S_IFDIR|S_IRUGO|S_IXUGO; inode->i_op = &proc_tgid_base_inode_operations; inode->i_fop = &proc_tgid_base_operations; inode->i_flags|=S_IMMUTABLE; @@ -3391,11 +3387,10 @@ static int proc_task_instantiate(struct inode *dir, struct dentry *dentry, struct task_struct *task, const void *ptr) { struct inode *inode; - inode = proc_pid_make_inode(dir->i_sb, task); + inode = proc_pid_make_inode(dir->i_sb, task, S_IFDIR | S_IRUGO | S_IXUGO);
if (!inode) goto out; - inode->i_mode = S_IFDIR|S_IRUGO|S_IXUGO; inode->i_op = &proc_tid_base_inode_operations; inode->i_fop = &proc_tid_base_operations; inode->i_flags|=S_IMMUTABLE; diff --git a/fs/proc/fd.c b/fs/proc/fd.c index d21dafe..4274f83 100644 --- a/fs/proc/fd.c +++ b/fs/proc/fd.c @@ -183,14 +183,13 @@ static int proc_fd_link(struct dentry *dentry, struct path *path) struct proc_inode *ei; struct inode *inode;
- inode = proc_pid_make_inode(dir->i_sb, task); + inode = proc_pid_make_inode(dir->i_sb, task, S_IFLNK); if (!inode) goto out;
ei = PROC_I(inode); ei->fd = fd;
- inode->i_mode = S_IFLNK; inode->i_op = &proc_pid_link_inode_operations; inode->i_size = 64;
@@ -322,14 +321,13 @@ int proc_fd_permission(struct inode *inode, int mask) struct proc_inode *ei; struct inode *inode;
- inode = proc_pid_make_inode(dir->i_sb, task); + inode = proc_pid_make_inode(dir->i_sb, task, S_IFREG | S_IRUSR); if (!inode) goto out;
ei = PROC_I(inode); ei->fd = fd;
- inode->i_mode = S_IFREG | S_IRUSR; inode->i_fop = &proc_fdinfo_file_operations;
d_set_d_op(dentry, &tid_fd_dentry_operations); diff --git a/fs/proc/internal.h b/fs/proc/internal.h index c0bdece..5bc057b 100644 --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -163,7 +163,7 @@ extern int proc_pid_statm(struct seq_file *, struct pid_namespace *, extern const struct dentry_operations pid_dentry_operations; extern int pid_getattr(struct vfsmount *, struct dentry *, struct kstat *); extern int proc_setattr(struct dentry *, struct iattr *); -extern struct inode *proc_pid_make_inode(struct super_block *, struct task_struct *); +extern struct inode *proc_pid_make_inode(struct super_block *, struct task_struct *, umode_t); extern int pid_revalidate(struct dentry *, unsigned int); extern int pid_delete_dentry(const struct dentry *); extern int proc_pid_readdir(struct file *, struct dir_context *); diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c index 51b8b0a..766f0c6 100644 --- a/fs/proc/namespaces.c +++ b/fs/proc/namespaces.c @@ -92,12 +92,11 @@ static int proc_ns_instantiate(struct inode *dir, struct inode *inode; struct proc_inode *ei;
- inode = proc_pid_make_inode(dir->i_sb, task); + inode = proc_pid_make_inode(dir->i_sb, task, S_IFLNK | S_IRWXUGO); if (!inode) goto out;
ei = PROC_I(inode); - inode->i_mode = S_IFLNK|S_IRWXUGO; inode->i_op = &proc_ns_link_inode_operations; ei->ns_ops = ns_ops;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index a2b63a6..98b5f40 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3967,6 +3967,7 @@ static void selinux_task_to_inode(struct task_struct *p, struct inode_security_struct *isec = inode->i_security; u32 sid = task_sid(p);
+ isec->sclass = inode_mode_to_security_class(inode->i_mode); isec->sid = sid; isec->initialized = LABEL_INITIALIZED; }
From: "Eric W. Biederman" ebiederm@xmission.com
[ Upstream commit 68eb94f16227336a5773b83ecfa8290f1d6b78ce ]
Instead of making the files owned by the GLOBAL_ROOT_USER. Make non-dumpable files whose mm has always lived in a user namespace owned by the user namespace root. This allows the container root to have things work as expected in a container.
Signed-off-by: "Eric W. Biederman" ebiederm@xmission.com Cc: stable@vger.kernel.org # 4.9.x Signed-off-by: Wen Yang wenyang@linux.alibaba.com --- fs/proc/base.c | 102 ++++++++++++++++++++++++++++++----------------------- fs/proc/fd.c | 12 +------ fs/proc/internal.h | 16 ++------- 3 files changed, 61 insertions(+), 69 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c index ee2e0ec..5bfdb61 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -1676,12 +1676,63 @@ static int proc_pid_readlink(struct dentry * dentry, char __user * buffer, int b
/* building an inode */
+void task_dump_owner(struct task_struct *task, mode_t mode, + kuid_t *ruid, kgid_t *rgid) +{ + /* Depending on the state of dumpable compute who should own a + * proc file for a task. + */ + const struct cred *cred; + kuid_t uid; + kgid_t gid; + + /* Default to the tasks effective ownership */ + rcu_read_lock(); + cred = __task_cred(task); + uid = cred->euid; + gid = cred->egid; + rcu_read_unlock(); + + /* + * Before the /proc/pid/status file was created the only way to read + * the effective uid of a /process was to stat /proc/pid. Reading + * /proc/pid/status is slow enough that procps and other packages + * kept stating /proc/pid. To keep the rules in /proc simple I have + * made this apply to all per process world readable and executable + * directories. + */ + if (mode != (S_IFDIR|S_IRUGO|S_IXUGO)) { + struct mm_struct *mm; + task_lock(task); + mm = task->mm; + /* Make non-dumpable tasks owned by some root */ + if (mm) { + if (get_dumpable(mm) != SUID_DUMP_USER) { + struct user_namespace *user_ns = mm->user_ns; + + uid = make_kuid(user_ns, 0); + if (!uid_valid(uid)) + uid = GLOBAL_ROOT_UID; + + gid = make_kgid(user_ns, 0); + if (!gid_valid(gid)) + gid = GLOBAL_ROOT_GID; + } + } else { + uid = GLOBAL_ROOT_UID; + gid = GLOBAL_ROOT_GID; + } + task_unlock(task); + } + *ruid = uid; + *rgid = gid; +} + struct inode *proc_pid_make_inode(struct super_block * sb, struct task_struct *task, umode_t mode) { struct inode * inode; struct proc_inode *ei; - const struct cred *cred;
/* We need a new inode */
@@ -1703,13 +1754,7 @@ struct inode *proc_pid_make_inode(struct super_block * sb, if (!ei->pid) goto out_unlock;
- if (task_dumpable(task)) { - rcu_read_lock(); - cred = __task_cred(task); - inode->i_uid = cred->euid; - inode->i_gid = cred->egid; - rcu_read_unlock(); - } + task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid); security_task_to_inode(task, inode);
out: @@ -1724,7 +1769,6 @@ int pid_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat) { struct inode *inode = d_inode(dentry); struct task_struct *task; - const struct cred *cred; struct pid_namespace *pid = dentry->d_sb->s_fs_info;
generic_fillattr(inode, stat); @@ -1742,12 +1786,7 @@ int pid_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat) */ return -ENOENT; } - if ((inode->i_mode == (S_IFDIR|S_IRUGO|S_IXUGO)) || - task_dumpable(task)) { - cred = __task_cred(task); - stat->uid = cred->euid; - stat->gid = cred->egid; - } + task_dump_owner(task, inode->i_mode, &stat->uid, &stat->gid); } rcu_read_unlock(); return 0; @@ -1763,18 +1802,11 @@ int pid_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat) * Rewrite the inode's ownerships here because the owning task may have * performed a setuid(), etc. * - * Before the /proc/pid/status file was created the only way to read - * the effective uid of a /process was to stat /proc/pid. Reading - * /proc/pid/status is slow enough that procps and other packages - * kept stating /proc/pid. To keep the rules in /proc simple I have - * made this apply to all per process world readable and executable - * directories. */ int pid_revalidate(struct dentry *dentry, unsigned int flags) { struct inode *inode; struct task_struct *task; - const struct cred *cred;
if (flags & LOOKUP_RCU) return -ECHILD; @@ -1783,17 +1815,8 @@ int pid_revalidate(struct dentry *dentry, unsigned int flags) task = get_proc_task(inode);
if (task) { - if ((inode->i_mode == (S_IFDIR|S_IRUGO|S_IXUGO)) || - task_dumpable(task)) { - rcu_read_lock(); - cred = __task_cred(task); - inode->i_uid = cred->euid; - inode->i_gid = cred->egid; - rcu_read_unlock(); - } else { - inode->i_uid = GLOBAL_ROOT_UID; - inode->i_gid = GLOBAL_ROOT_GID; - } + task_dump_owner(task, inode->i_mode, &inode->i_uid, &inode->i_gid); + inode->i_mode &= ~(S_ISUID | S_ISGID); security_task_to_inode(task, inode); put_task_struct(task); @@ -1915,7 +1938,6 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags) bool exact_vma_exists = false; struct mm_struct *mm = NULL; struct task_struct *task; - const struct cred *cred; struct inode *inode; int status = 0;
@@ -1940,16 +1962,8 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags) mmput(mm);
if (exact_vma_exists) { - if (task_dumpable(task)) { - rcu_read_lock(); - cred = __task_cred(task); - inode->i_uid = cred->euid; - inode->i_gid = cred->egid; - rcu_read_unlock(); - } else { - inode->i_uid = GLOBAL_ROOT_UID; - inode->i_gid = GLOBAL_ROOT_GID; - } + task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid); + security_task_to_inode(task, inode); status = 1; } diff --git a/fs/proc/fd.c b/fs/proc/fd.c index 4274f83..00ce153 100644 --- a/fs/proc/fd.c +++ b/fs/proc/fd.c @@ -84,7 +84,6 @@ static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags) { struct files_struct *files; struct task_struct *task; - const struct cred *cred; struct inode *inode; unsigned int fd;
@@ -108,16 +107,7 @@ static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags) rcu_read_unlock(); put_files_struct(files);
- if (task_dumpable(task)) { - rcu_read_lock(); - cred = __task_cred(task); - inode->i_uid = cred->euid; - inode->i_gid = cred->egid; - rcu_read_unlock(); - } else { - inode->i_uid = GLOBAL_ROOT_UID; - inode->i_gid = GLOBAL_ROOT_GID; - } + task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid);
if (S_ISLNK(inode->i_mode)) { unsigned i_mode = S_IFLNK; diff --git a/fs/proc/internal.h b/fs/proc/internal.h index 5bc057b..103435f 100644 --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -98,20 +98,8 @@ static inline struct task_struct *get_proc_task(struct inode *inode) return get_pid_task(proc_pid(inode), PIDTYPE_PID); }
-static inline int task_dumpable(struct task_struct *task) -{ - int dumpable = 0; - struct mm_struct *mm; - - task_lock(task); - mm = task->mm; - if (mm) - dumpable = get_dumpable(mm); - task_unlock(task); - if (dumpable == SUID_DUMP_USER) - return 1; - return 0; -} +void task_dump_owner(struct task_struct *task, mode_t mode, + kuid_t *ruid, kgid_t *rgid);
static inline unsigned name_to_int(const struct qstr *qstr) {
From: Alexey Dobriyan adobriyan@gmail.com
[ Upstream commit e3912ac37e07a13c70675cd75020694de4841c74 ]
PROC_NUMBUF is 13 which is enough for "negative int + \n + \0".
However PIDs and TGIDs are never negative and newline is not a concern, so use just 10 per integer.
Link: http://lkml.kernel.org/r/20171120203005.GA27743@avx2 Signed-off-by: Alexey Dobriyan adobriyan@gmail.com Cc: Alexander Viro viro@ftp.linux.org.uk Signed-off-by: Andrew Morton akpm@linux-foundation.org Signed-off-by: Linus Torvalds torvalds@linux-foundation.org Cc: stable@vger.kernel.org # 4.9.x Signed-off-by: Wen Yang wenyang@linux.alibaba.com --- fs/proc/base.c | 16 ++++++++-------- fs/proc/fd.c | 2 +- fs/proc/self.c | 6 +++--- fs/proc/thread_self.c | 5 ++--- 4 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c index 5bfdb61..3502a40 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -3018,11 +3018,11 @@ static struct dentry *proc_tgid_base_lookup(struct inode *dir, struct dentry *de static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid) { struct dentry *dentry, *leader, *dir; - char buf[PROC_NUMBUF]; + char buf[10 + 1]; struct qstr name;
name.name = buf; - name.len = snprintf(buf, sizeof(buf), "%d", pid); + name.len = snprintf(buf, sizeof(buf), "%u", pid); /* no ->d_hash() rejects on procfs */ dentry = d_hash_and_lookup(mnt->mnt_root, &name); if (dentry) { @@ -3034,7 +3034,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid) return;
name.name = buf; - name.len = snprintf(buf, sizeof(buf), "%d", tgid); + name.len = snprintf(buf, sizeof(buf), "%u", tgid); leader = d_hash_and_lookup(mnt->mnt_root, &name); if (!leader) goto out; @@ -3046,7 +3046,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid) goto out_put_leader;
name.name = buf; - name.len = snprintf(buf, sizeof(buf), "%d", pid); + name.len = snprintf(buf, sizeof(buf), "%u", pid); dentry = d_hash_and_lookup(dir, &name); if (dentry) { d_invalidate(dentry); @@ -3226,14 +3226,14 @@ int proc_pid_readdir(struct file *file, struct dir_context *ctx) for (iter = next_tgid(ns, iter); iter.task; iter.tgid += 1, iter = next_tgid(ns, iter)) { - char name[PROC_NUMBUF]; + char name[10 + 1]; int len;
cond_resched(); if (!has_pid_permissions(ns, iter.task, 2)) continue;
- len = snprintf(name, sizeof(name), "%d", iter.tgid); + len = snprintf(name, sizeof(name), "%u", iter.tgid); ctx->pos = iter.tgid + TGID_OFFSET; if (!proc_fill_cache(file, ctx, name, len, proc_pid_instantiate, iter.task, NULL)) { @@ -3557,10 +3557,10 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx) for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns); task; task = next_tid(task), ctx->pos++) { - char name[PROC_NUMBUF]; + char name[10 + 1]; int len; tid = task_pid_nr_ns(task, ns); - len = snprintf(name, sizeof(name), "%d", tid); + len = snprintf(name, sizeof(name), "%u", tid); if (!proc_fill_cache(file, ctx, name, len, proc_task_instantiate, task, NULL)) { /* returning this tgid failed, save it as the first diff --git a/fs/proc/fd.c b/fs/proc/fd.c index 00ce153..390c2fe 100644 --- a/fs/proc/fd.c +++ b/fs/proc/fd.c @@ -235,7 +235,7 @@ static int proc_readfd_common(struct file *file, struct dir_context *ctx, for (fd = ctx->pos - 2; fd < files_fdtable(files)->max_fds; fd++, ctx->pos++) { - char name[PROC_NUMBUF]; + char name[10 + 1]; int len;
if (!fcheck_files(files, fd)) diff --git a/fs/proc/self.c b/fs/proc/self.c index f6e2e3f..dd06755 100644 --- a/fs/proc/self.c +++ b/fs/proc/self.c @@ -35,11 +35,11 @@ static const char *proc_self_get_link(struct dentry *dentry,
if (!tgid) return ERR_PTR(-ENOENT); - /* 11 for max length of signed int in decimal + NULL term */ - name = kmalloc(12, dentry ? GFP_KERNEL : GFP_ATOMIC); + /* max length of unsigned int in decimal + NULL term */ + name = kmalloc(10 + 1, dentry ? GFP_KERNEL : GFP_ATOMIC); if (unlikely(!name)) return dentry ? ERR_PTR(-ENOMEM) : ERR_PTR(-ECHILD); - sprintf(name, "%d", tgid); + sprintf(name, "%u", tgid); set_delayed_call(done, kfree_link, name); return name; } diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c index 02d1db8..44e0921 100644 --- a/fs/proc/thread_self.c +++ b/fs/proc/thread_self.c @@ -30,11 +30,10 @@ static const char *proc_thread_self_get_link(struct dentry *dentry,
if (!pid) return ERR_PTR(-ENOENT); - name = kmalloc(PROC_NUMBUF + 6 + PROC_NUMBUF, - dentry ? GFP_KERNEL : GFP_ATOMIC); + name = kmalloc(10 + 6 + 10 + 1, dentry ? GFP_KERNEL : GFP_ATOMIC); if (unlikely(!name)) return dentry ? ERR_PTR(-ENOMEM) : ERR_PTR(-ECHILD); - sprintf(name, "%d/task/%d", tgid, pid); + sprintf(name, "%u/task/%u", tgid, pid); set_delayed_call(done, kfree_link, name); return name; }
From: "Eric W. Biederman" ebiederm@xmission.com
[ Upstream commit 0afa5ca82212247456f9de1468b595a111fee633 ]
I about to need and use the same functionality for pid based inodes and there is no point in adding a second field when this field is already here and serving the same purporse.
Just give the field a generic name so it is clear that it is no longer sysctl specific.
Also for good measure initialize sibling_inodes when proc_inode is initialized.
Signed-off-by: Eric W. Biederman ebiederm@xmission.com Cc: stable@vger.kernel.org # 4.9.x Signed-off-by: Wen Yang wenyang@linux.alibaba.com --- fs/proc/inode.c | 1 + fs/proc/internal.h | 2 +- fs/proc/proc_sysctl.c | 8 ++++---- 3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/fs/proc/inode.c b/fs/proc/inode.c index a289349..14d9c1d 100644 --- a/fs/proc/inode.c +++ b/fs/proc/inode.c @@ -67,6 +67,7 @@ static struct inode *proc_alloc_inode(struct super_block *sb) ei->pde = NULL; ei->sysctl = NULL; ei->sysctl_entry = NULL; + INIT_HLIST_NODE(&ei->sibling_inodes); ei->ns_ops = NULL; inode = &ei->vfs_inode; return inode; diff --git a/fs/proc/internal.h b/fs/proc/internal.h index 103435f..409b5c5 100644 --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -65,7 +65,7 @@ struct proc_inode { struct proc_dir_entry *pde; struct ctl_table_header *sysctl; struct ctl_table *sysctl_entry; - struct hlist_node sysctl_inodes; + struct hlist_node sibling_inodes; const struct proc_ns_operations *ns_ops; struct inode vfs_inode; }; diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 191573a..671490e 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -272,9 +272,9 @@ static void proc_sys_prune_dcache(struct ctl_table_header *head) node = hlist_first_rcu(&head->inodes); if (!node) break; - ei = hlist_entry(node, struct proc_inode, sysctl_inodes); + ei = hlist_entry(node, struct proc_inode, sibling_inodes); spin_lock(&sysctl_lock); - hlist_del_init_rcu(&ei->sysctl_inodes); + hlist_del_init_rcu(&ei->sibling_inodes); spin_unlock(&sysctl_lock);
inode = &ei->vfs_inode; @@ -480,7 +480,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, } ei->sysctl = head; ei->sysctl_entry = table; - hlist_add_head_rcu(&ei->sysctl_inodes, &head->inodes); + hlist_add_head_rcu(&ei->sibling_inodes, &head->inodes); head->count++; spin_unlock(&sysctl_lock);
@@ -511,7 +511,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, void proc_sys_evict_inode(struct inode *inode, struct ctl_table_header *head) { spin_lock(&sysctl_lock); - hlist_del_init_rcu(&PROC_I(inode)->sysctl_inodes); + hlist_del_init_rcu(&PROC_I(inode)->sibling_inodes); if (!--head->count) kfree_rcu(head, rcu); spin_unlock(&sysctl_lock);
From: "Eric W. Biederman" ebiederm@xmission.com
[ Upstream commit 26dbc60f385ff9cff475ea2a3bad02e80fd6fa43 ]
This prepares the way for allowing the pid part of proc to use this dcache pruning code as well.
Signed-off-by: Eric W. Biederman ebiederm@xmission.com Cc: stable@vger.kernel.org # 4.9.x (proc: fix up cherry-pick conflicts for 26dbc60f385f) Signed-off-by: Wen Yang wenyang@linux.alibaba.com --- fs/proc/inode.c | 38 ++++++++++++++++++++++++++++++++++++++ fs/proc/internal.h | 1 + fs/proc/proc_sysctl.c | 35 +---------------------------------- 3 files changed, 40 insertions(+), 34 deletions(-)
diff --git a/fs/proc/inode.c b/fs/proc/inode.c index 14d9c1d..920c761 100644 --- a/fs/proc/inode.c +++ b/fs/proc/inode.c @@ -101,6 +101,44 @@ void __init proc_init_inodecache(void) init_once); }
+void proc_prune_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock) +{ + struct inode *inode; + struct proc_inode *ei; + struct hlist_node *node; + struct super_block *sb; + + rcu_read_lock(); + for (;;) { + node = hlist_first_rcu(inodes); + if (!node) + break; + ei = hlist_entry(node, struct proc_inode, sibling_inodes); + spin_lock(lock); + hlist_del_init_rcu(&ei->sibling_inodes); + spin_unlock(lock); + + inode = &ei->vfs_inode; + sb = inode->i_sb; + if (!atomic_inc_not_zero(&sb->s_active)) + continue; + inode = igrab(inode); + rcu_read_unlock(); + if (unlikely(!inode)) { + deactivate_super(sb); + rcu_read_lock(); + continue; + } + + d_prune_aliases(inode); + iput(inode); + deactivate_super(sb); + + rcu_read_lock(); + } + rcu_read_unlock(); +} + static int proc_show_options(struct seq_file *seq, struct dentry *root) { struct super_block *sb = root->d_sb; diff --git a/fs/proc/internal.h b/fs/proc/internal.h index 409b5c5..9bc44a1 100644 --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -200,6 +200,7 @@ struct pde_opener { extern const struct inode_operations proc_pid_link_inode_operations;
extern void proc_init_inodecache(void); +void proc_prune_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock); extern struct inode *proc_get_inode(struct super_block *, struct proc_dir_entry *); extern int proc_fill_super(struct super_block *, void *data, int flags); extern void proc_entry_rundown(struct proc_dir_entry *); diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 671490e..f19063b 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -262,40 +262,7 @@ static void unuse_table(struct ctl_table_header *p)
static void proc_sys_prune_dcache(struct ctl_table_header *head) { - struct inode *inode; - struct proc_inode *ei; - struct hlist_node *node; - struct super_block *sb; - - rcu_read_lock(); - for (;;) { - node = hlist_first_rcu(&head->inodes); - if (!node) - break; - ei = hlist_entry(node, struct proc_inode, sibling_inodes); - spin_lock(&sysctl_lock); - hlist_del_init_rcu(&ei->sibling_inodes); - spin_unlock(&sysctl_lock); - - inode = &ei->vfs_inode; - sb = inode->i_sb; - if (!atomic_inc_not_zero(&sb->s_active)) - continue; - inode = igrab(inode); - rcu_read_unlock(); - if (unlikely(!inode)) { - deactivate_super(sb); - rcu_read_lock(); - continue; - } - - d_prune_aliases(inode); - iput(inode); - deactivate_super(sb); - - rcu_read_lock(); - } - rcu_read_unlock(); + proc_prune_siblings_dcache(&head->inodes, &sysctl_lock); }
/* called under sysctl_lock, will reacquire if has to wait */
From: "Eric W. Biederman" ebiederm@xmission.com
[ Upstream commit 71448011ea2a1cd36d8f5cbdab0ed716c454d565 ]
This just keeps everything tidier, and allows for using flags like SLAB_TYPESAFE_BY_RCU where slabs are not always cleared before reuse. I don't see reuse without reinitializing happening with the proc_inode but I had a false alarm while reworking flushing of proc dentries and indoes when a process dies that caused me to tidy this up.
The code is a little easier to follow and reason about this way so I figured the changes might as well be kept.
Signed-off-by: "Eric W. Biederman" ebiederm@xmission.com Cc: stable@vger.kernel.org # 4.9.x Signed-off-by: Wen Yang wenyang@linux.alibaba.com --- fs/proc/inode.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/fs/proc/inode.c b/fs/proc/inode.c index 920c761..739fb9c 100644 --- a/fs/proc/inode.c +++ b/fs/proc/inode.c @@ -32,21 +32,27 @@ static void proc_evict_inode(struct inode *inode) { struct proc_dir_entry *de; struct ctl_table_header *head; + struct proc_inode *ei = PROC_I(inode);
truncate_inode_pages_final(&inode->i_data); clear_inode(inode);
/* Stop tracking associated processes */ - put_pid(PROC_I(inode)->pid); + if (ei->pid) { + put_pid(ei->pid); + ei->pid = NULL; + }
/* Let go of any associated proc directory entry */ - de = PDE(inode); - if (de) + de = ei->pde; + if (de) { pde_put(de); + ei->pde = NULL; + }
- head = PROC_I(inode)->sysctl; + head = ei->sysctl; if (head) { - RCU_INIT_POINTER(PROC_I(inode)->sysctl, NULL); + RCU_INIT_POINTER(ei->sysctl, NULL); proc_sys_evict_inode(inode, head); } }
From: "Eric W. Biederman" ebiederm@xmission.com
[ Upstream commit f90f3cafe8d56d593fc509a4185da1d5800efea4 ]
The function d_prune_aliases has the problem that it will only prune aliases thare are completely unused. It will not remove aliases for the dcache or even think of removing mounts from the dcache. For that behavior d_invalidate is needed.
To use d_invalidate replace d_prune_aliases with d_find_alias followed by d_invalidate and dput.
For completeness the directory and the non-directory cases are separated because in theory (although not in currently in practice for proc) directories can only ever have a single dentry while non-directories can have hardlinks and thus multiple dentries. As part of this separation use d_find_any_alias for directories to spare d_find_alias the extra work of doing that.
Plus the differences between d_find_any_alias and d_find_alias makes it clear why the directory and non-directory code and not share code.
To make it clear these routines now invalidate dentries rename proc_prune_siblings_dache to proc_invalidate_siblings_dcache, and rename proc_sys_prune_dcache proc_sys_invalidate_dcache.
V2: Split the directory and non-directory cases. To make this code robust to future changes in proc.
Signed-off-by: "Eric W. Biederman" ebiederm@xmission.com Cc: stable@vger.kernel.org # 4.9.x (proc: fix up cherry-pick conflicts for f90f3cafe8d5) Signed-off-by: Wen Yang wenyang@linux.alibaba.com --- fs/proc/inode.c | 16 ++++++++++++++-- fs/proc/internal.h | 2 +- fs/proc/proc_sysctl.c | 8 ++++---- 3 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/fs/proc/inode.c b/fs/proc/inode.c index 739fb9c..2af9f4f 100644 --- a/fs/proc/inode.c +++ b/fs/proc/inode.c @@ -107,7 +107,7 @@ void __init proc_init_inodecache(void) init_once); }
-void proc_prune_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock) +void proc_invalidate_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock) { struct inode *inode; struct proc_inode *ei; @@ -136,7 +136,19 @@ void proc_prune_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock) continue; }
- d_prune_aliases(inode); + if (S_ISDIR(inode->i_mode)) { + struct dentry *dir = d_find_any_alias(inode); + if (dir) { + d_invalidate(dir); + dput(dir); + } + } else { + struct dentry *dentry; + while ((dentry = d_find_alias(inode))) { + d_invalidate(dentry); + dput(dentry); + } + } iput(inode); deactivate_super(sb);
diff --git a/fs/proc/internal.h b/fs/proc/internal.h index 9bc44a1..6a1d679 100644 --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -200,7 +200,7 @@ struct pde_opener { extern const struct inode_operations proc_pid_link_inode_operations;
extern void proc_init_inodecache(void); -void proc_prune_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock); +void proc_invalidate_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock); extern struct inode *proc_get_inode(struct super_block *, struct proc_dir_entry *); extern int proc_fill_super(struct super_block *, void *data, int flags); extern void proc_entry_rundown(struct proc_dir_entry *); diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index f19063b..b6668a5 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -260,9 +260,9 @@ static void unuse_table(struct ctl_table_header *p) complete(p->unregistering); }
-static void proc_sys_prune_dcache(struct ctl_table_header *head) +static void proc_sys_invalidate_dcache(struct ctl_table_header *head) { - proc_prune_siblings_dcache(&head->inodes, &sysctl_lock); + proc_invalidate_siblings_dcache(&head->inodes, &sysctl_lock); }
/* called under sysctl_lock, will reacquire if has to wait */ @@ -284,10 +284,10 @@ static void start_unregistering(struct ctl_table_header *p) spin_unlock(&sysctl_lock); } /* - * Prune dentries for unregistered sysctls: namespaced sysctls + * Invalidate dentries for unregistered sysctls: namespaced sysctls * can have duplicate names and contaminate dcache very badly. */ - proc_sys_prune_dcache(p); + proc_sys_invalidate_dcache(p); /* * do not remove from the list until nobody holds it; walking the * list in do_sysctl() relies on that.
From: "Eric W. Biederman" ebiederm@xmission.com
[ Upstream commit 7bc3e6e55acf065500a24621f3b313e7e5998acf ]
Rework the flushing of proc to use a list of directory inodes that need to be flushed.
The list is kept on struct pid not on struct task_struct, as there is a fixed connection between proc inodes and pids but at least for the case of de_thread the pid of a task_struct changes.
This removes the dependency on proc_mnt which allows for different mounts of proc having different mount options even in the same pid namespace and this allows for the removal of proc_mnt which will trivially the first mount of proc to honor it's mount options.
This flushing remains an optimization. The functions pid_delete_dentry and pid_revalidate ensure that ordinary dcache management will not attempt to use dentries past the point their respective task has died. When unused the shrinker will eventually be able to remove these dentries.
There is a case in de_thread where proc_flush_pid can be called early for a given pid. Which winds up being safe (if suboptimal) as this is just an optiimization.
Only pid directories are put on the list as the other per pid files are children of those directories and d_invalidate on the directory will get them as well.
So that the pid can be used during flushing it's reference count is taken in release_task and dropped in proc_flush_pid. Further the call of proc_flush_pid is moved after the tasklist_lock is released in release_task so that it is certain that the pid has already been unhashed when flushing it taking place. This removes a small race where a dentry could recreated.
As struct pid is supposed to be small and I need a per pid lock I reuse the only lock that currently exists in struct pid the the wait_pidfd.lock.
The net result is that this adds all of this functionality with just a little extra list management overhead and a single extra pointer in struct pid.
v2: Initialize pid->inodes. I somehow failed to get that initialization into the initial version of the patch. A boot failure was reported by "kernel test robot lkp@intel.com", and failure to initialize that pid->inodes matches all of the reported symptoms.
Signed-off-by: Eric W. Biederman ebiederm@xmission.com Fixes: f333c700c610 ("pidns: Add a limit on the number of pid namespaces") Fixes: 60347f6716aa ("pid namespaces: prepare proc_flust_task() to flush entries from multiple proc trees") Cc: stable@vger.kernel.org # 4.9.x: b3e5838: clone: add CLONE_PIDFD Cc: stable@vger.kernel.org # 4.9.x: b53b0b9: pidfd: add polling support Cc: stable@vger.kernel.org # 4.9.x: db978da: proc: Pass file mode to proc_pid_make_inode Cc: stable@vger.kernel.org # 4.9.x: 68eb94f: proc: Better ownership of files for non-dumpable tasks in user namespaces Cc: stable@vger.kernel.org # 4.9.x: e3912ac: proc: use %u for pid printing and slightly less stack Cc: stable@vger.kernel.org # 4.9.x: 0afa5ca: proc: Rename in proc_inode rename sysctl_inodes sibling_inodes Cc: stable@vger.kernel.org # 4.9.x: 26dbc60: proc: Generalize proc_sys_prune_dcache into proc_prune_siblings_dcache Cc: stable@vger.kernel.org # 4.9.x: 7144801: proc: Clear the pieces of proc_inode that proc_evict_inode cares about Cc: stable@vger.kernel.org # 4.9.x: f90f3ca: Use d_invalidate in proc_prune_siblings_dcache Cc: stable@vger.kernel.org # 4.9.x (proc: fix up cherry-pick conflicts for 7bc3e6e55acf) Signed-off-by: Wen Yang wenyang@linux.alibaba.com --- fs/proc/base.c | 111 ++++++++++++++++-------------------------------- fs/proc/inode.c | 2 +- fs/proc/internal.h | 1 + include/linux/pid.h | 1 + include/linux/proc_fs.h | 4 +- kernel/exit.c | 5 ++- kernel/pid.c | 1 + 7 files changed, 45 insertions(+), 80 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c index 3502a40..11caf35 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -1728,11 +1728,25 @@ void task_dump_owner(struct task_struct *task, mode_t mode, *rgid = gid; }
+void proc_pid_evict_inode(struct proc_inode *ei) +{ + struct pid *pid = ei->pid; + + if (S_ISDIR(ei->vfs_inode.i_mode)) { + spin_lock(&pid->wait_pidfd.lock); + hlist_del_init_rcu(&ei->sibling_inodes); + spin_unlock(&pid->wait_pidfd.lock); + } + + put_pid(pid); +} + struct inode *proc_pid_make_inode(struct super_block * sb, struct task_struct *task, umode_t mode) { struct inode * inode; struct proc_inode *ei; + struct pid *pid;
/* We need a new inode */
@@ -1750,10 +1764,18 @@ struct inode *proc_pid_make_inode(struct super_block * sb, /* * grab the reference to task. */ - ei->pid = get_task_pid(task, PIDTYPE_PID); - if (!ei->pid) + pid = get_task_pid(task, PIDTYPE_PID); + if (!pid) goto out_unlock;
+ /* Let the pid remember us for quick removal */ + ei->pid = pid; + if (S_ISDIR(mode)) { + spin_lock(&pid->wait_pidfd.lock); + hlist_add_head_rcu(&ei->sibling_inodes, &pid->inodes); + spin_unlock(&pid->wait_pidfd.lock); + } + task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid); security_task_to_inode(task, inode);
@@ -3015,90 +3037,29 @@ static struct dentry *proc_tgid_base_lookup(struct inode *dir, struct dentry *de .permission = proc_pid_permission, };
-static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid) -{ - struct dentry *dentry, *leader, *dir; - char buf[10 + 1]; - struct qstr name; - - name.name = buf; - name.len = snprintf(buf, sizeof(buf), "%u", pid); - /* no ->d_hash() rejects on procfs */ - dentry = d_hash_and_lookup(mnt->mnt_root, &name); - if (dentry) { - d_invalidate(dentry); - dput(dentry); - } - - if (pid == tgid) - return; - - name.name = buf; - name.len = snprintf(buf, sizeof(buf), "%u", tgid); - leader = d_hash_and_lookup(mnt->mnt_root, &name); - if (!leader) - goto out; - - name.name = "task"; - name.len = strlen(name.name); - dir = d_hash_and_lookup(leader, &name); - if (!dir) - goto out_put_leader; - - name.name = buf; - name.len = snprintf(buf, sizeof(buf), "%u", pid); - dentry = d_hash_and_lookup(dir, &name); - if (dentry) { - d_invalidate(dentry); - dput(dentry); - } - - dput(dir); -out_put_leader: - dput(leader); -out: - return; -} - /** - * proc_flush_task - Remove dcache entries for @task from the /proc dcache. - * @task: task that should be flushed. + * proc_flush_pid - Remove dcache entries for @pid from the /proc dcache. + * @pid: pid that should be flushed. * - * When flushing dentries from proc, one needs to flush them from global - * proc (proc_mnt) and from all the namespaces' procs this task was seen - * in. This call is supposed to do all of this job. - * - * Looks in the dcache for - * /proc/@pid - * /proc/@tgid/task/@pid - * if either directory is present flushes it and all of it'ts children - * from the dcache. + * This function walks a list of inodes (that belong to any proc + * filesystem) that are attached to the pid and flushes them from + * the dentry cache. * * It is safe and reasonable to cache /proc entries for a task until * that task exits. After that they just clog up the dcache with * useless entries, possibly causing useful dcache entries to be - * flushed instead. This routine is proved to flush those useless - * dcache entries at process exit time. + * flushed instead. This routine is provided to flush those useless + * dcache entries when a process is reaped. * * NOTE: This routine is just an optimization so it does not guarantee - * that no dcache entries will exist at process exit time it - * just makes it very unlikely that any will persist. + * that no dcache entries will exist after a process is reaped + * it just makes it very unlikely that any will persist. */
-void proc_flush_task(struct task_struct *task) +void proc_flush_pid(struct pid *pid) { - int i; - struct pid *pid, *tgid; - struct upid *upid; - - pid = task_pid(task); - tgid = task_tgid(task); - - for (i = 0; i <= pid->level; i++) { - upid = &pid->numbers[i]; - proc_flush_task_mnt(upid->ns->proc_mnt, upid->nr, - tgid->numbers[i].nr); - } + proc_invalidate_siblings_dcache(&pid->inodes, &pid->wait_pidfd.lock); + put_pid(pid); }
static int proc_pid_instantiate(struct inode *dir, diff --git a/fs/proc/inode.c b/fs/proc/inode.c index 2af9f4f..8503444 100644 --- a/fs/proc/inode.c +++ b/fs/proc/inode.c @@ -39,7 +39,7 @@ static void proc_evict_inode(struct inode *inode)
/* Stop tracking associated processes */ if (ei->pid) { - put_pid(ei->pid); + proc_pid_evict_inode(ei); ei->pid = NULL; }
diff --git a/fs/proc/internal.h b/fs/proc/internal.h index 6a1d679..0c6ca639 100644 --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -151,6 +151,7 @@ extern int proc_pid_statm(struct seq_file *, struct pid_namespace *, extern const struct dentry_operations pid_dentry_operations; extern int pid_getattr(struct vfsmount *, struct dentry *, struct kstat *); extern int proc_setattr(struct dentry *, struct iattr *); +extern void proc_pid_evict_inode(struct proc_inode *); extern struct inode *proc_pid_make_inode(struct super_block *, struct task_struct *, umode_t); extern int pid_revalidate(struct dentry *, unsigned int); extern int pid_delete_dentry(const struct dentry *); diff --git a/include/linux/pid.h b/include/linux/pid.h index f5552ba..04b4aaa 100644 --- a/include/linux/pid.h +++ b/include/linux/pid.h @@ -63,6 +63,7 @@ struct pid unsigned int level; /* lists of tasks that use this pid */ struct hlist_head tasks[PIDTYPE_MAX]; + struct hlist_head inodes; /* wait queue for pidfd notifications */ wait_queue_head_t wait_pidfd; struct rcu_head rcu; diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h index b97bf2e..d3580f5 100644 --- a/include/linux/proc_fs.h +++ b/include/linux/proc_fs.h @@ -12,7 +12,7 @@ #ifdef CONFIG_PROC_FS
extern void proc_root_init(void); -extern void proc_flush_task(struct task_struct *); +extern void proc_flush_pid(struct pid *);
extern struct proc_dir_entry *proc_symlink(const char *, struct proc_dir_entry *, const char *); @@ -48,7 +48,7 @@ static inline void proc_root_init(void) { }
-static inline void proc_flush_task(struct task_struct *task) +static inline void proc_flush_pid(struct pid *pid) { }
diff --git a/kernel/exit.c b/kernel/exit.c index f9943ef..5e66030 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -168,6 +168,7 @@ static void delayed_put_task_struct(struct rcu_head *rhp) void release_task(struct task_struct *p) { struct task_struct *leader; + struct pid *thread_pid; int zap_leader; repeat: /* don't need to get the RCU readlock here - the process is dead and @@ -176,10 +177,9 @@ void release_task(struct task_struct *p) atomic_dec(&__task_cred(p)->user->processes); rcu_read_unlock();
- proc_flush_task(p); - write_lock_irq(&tasklist_lock); ptrace_release_task(p); + thread_pid = get_pid(p->pids[PIDTYPE_PID].pid); __exit_signal(p);
/* @@ -202,6 +202,7 @@ void release_task(struct task_struct *p) }
write_unlock_irq(&tasklist_lock); + proc_flush_pid(thread_pid); release_thread(p); call_rcu(&p->rcu, delayed_put_task_struct);
diff --git a/kernel/pid.c b/kernel/pid.c index e605398..fb32a81 100644 --- a/kernel/pid.c +++ b/kernel/pid.c @@ -334,6 +334,7 @@ struct pid *alloc_pid(struct pid_namespace *ns) INIT_HLIST_HEAD(&pid->tasks[type]);
init_waitqueue_head(&pid->wait_pidfd); + INIT_HLIST_HEAD(&pid->inodes);
upid = pid->numbers + ns->level; spin_lock_irq(&pidmap_lock);
On Fri, Dec 04, 2020 at 02:31:59AM +0800, Wen Yang wrote:
From: Alexey Dobriyan adobriyan@gmail.com
[ Upstream commit e3912ac37e07a13c70675cd75020694de4841c74 ]
PROC_NUMBUF is 13 which is enough for "negative int + \n + \0".
However PIDs and TGIDs are never negative and newline is not a concern, so use just 10 per integer.
Link: http://lkml.kernel.org/r/20171120203005.GA27743@avx2 Signed-off-by: Alexey Dobriyan adobriyan@gmail.com Cc: Alexander Viro viro@ftp.linux.org.uk Signed-off-by: Andrew Morton akpm@linux-foundation.org Signed-off-by: Linus Torvalds torvalds@linux-foundation.org Cc: stable@vger.kernel.org # 4.9.x
A what? How does this belong to stable?
在 2020/12/4 上午2:31, Wen Yang 写道:
The dentries such as /proc/<pid>/ns/ have the DCACHE_OP_DELETE flag, they should be deleted when the process exits.
Suppose the following race appears：
release_task dput -> proc_flush_task -> dentry->d_op->d_delete(dentry) -> __exit_signal -> dentry->d_lockref.count-- and return.
In the proc_flush_task(), if another process is using this dentry, it will not be deleted. At the same time, in dput(), d_op->d_delete() can be executed before __exit_signal(pid has not been hashed), d_delete returns false, so this dentry still cannot be deleted.
This dentry will always be cached (although its count is 0 and the DCACHE_OP_DELETE flag is set), its parent denry will also be cached too, and these dentries can only be deleted when drop_caches is manually triggered.
This will result in wasted memory. What's more troublesome is that these dentries reference pid, according to the commit f333c700c610 ("pidns: Add a limit on the number of pid namespaces"), if the pid cannot be released, it may result in the inability to create a new pid_ns.
This problem occurred in our cluster environment (Linux 4.9 LTS). We could reproduce it by manually constructing a test program + adding some debugging switches in the kernel:
- A test program to open the directory (/proc/<pid>/ns) [1]
- Adding some debugging switches to the kernel, adding a delay between proc_flush_task and __exit_signal in release_task() [2]
The test process is as follows:
A, terminal #1
Turn on the debug switch: echo 1> /proc/sys/vm/dentry_debug_trace
Execute the following unshare command: sudo unshare --pid --fork --mount-proc bash
B, terminal #2
Find the pid of the unshare process:
# pstree -p | grep unshare | `-sshd(716)---bash(718)--sudo(816)---unshare(817)---bash(818)
Find the corresponding dentry: # dmesg | grep pid=818 [70.424722] XXX proc_pid_instantiate:3119 pid=818 tid=818 entry=818/ffff8802c7b670e8
C, terminal #3
Execute the opendir program, it will always open the /proc/818/ns/ directory:
# ./a.out /proc/818/ns/ pid: 876 . .. net uts ipc pid user mnt cgroup
D, go back to terminal #2
Turn on the debugging switches to construct the race: # echo 818> /proc/sys/vm/dentry_debug_pid # echo 1> /proc/sys/vm/dentry_debug_delay
Kill the unshare process (pid 818). Since the debugging switches have been turned on, it will get stuck in release_task(): # kill -9 818
Then kill the process that opened the /proc/818/ns/ directory: # kill -9 876
Then turn off these debugging switches to allow the 818 process to exit: # echo 0> /proc/sys/vm/dentry_debug_delay # echo 0> /proc/sys/vm/dentry_debug_pid
Checking the dmesg, we will find that the dentry(/proc/818/ns) ’s count is 0, and the flag is 2800cc (#define DCACHE_OP_DELETE 0x00000008), but it is still cached: # dmesg | grep ffff8802a3999548 … [565.559156] XXX dput:853 dentry=ns/ffff8802bea7b528, flag=2800cc, cnt=0, inode=ffff8802b38c2010, pdentry=818/ffff8802c7b670e8, pflag=20008c, pcnt=1, pinode=ffff8802c7812010, keywords: be cached
It could also be verified via the crash tool:
crash> dentry.d_flags,d_iname,d_inode,d_lockref -x ffff8802bea7b528 d_flags = 0x2800cc d_iname = "ns\000kkkkkkkkkkkkkkkkkkkkkkkkkkkk" d_inode = 0xffff8802b38c2010 d_lockref = { { lock_count = 0x0, { lock = { { rlock = { raw_lock = { { val = { counter = 0x0 }, { locked = 0x0, pending = 0x0 }, { locked_pending = 0x0, tail = 0x0 } } } } } }, count = 0x0 } } } crash> kmem ffff8802bea7b528 CACHE OBJSIZE ALLOCATED TOTAL SLABS SSIZE NAME ffff8802dd5f5900 192 23663 26130 871 16k dentry SLAB MEMORY NODE TOTAL ALLOCATED FREE ffffea000afa9e00 ffff8802bea78000 0 30 25 5 FREE / [ALLOCATED] [ffff8802bea7b520]
PAGE PHYSICAL MAPPING INDEX CNT FLAGSffffea000afa9ec0 2bea7b000 dead000000000400 0 0 2fffff80000000 crash>
This series of patches is to fix this issue.
Regards, Wen
Alexey Dobriyan (1): proc: use %u for pid printing and slightly less stack
Andreas Gruenbacher (1): proc: Pass file mode to proc_pid_make_inode
Christian Brauner (1): clone: add CLONE_PIDFD
Eric W. Biederman (6): proc: Better ownership of files for non-dumpable tasks in user namespaces proc: Rename in proc_inode rename sysctl_inodes sibling_inodes proc: Generalize proc_sys_prune_dcache into proc_prune_siblings_dcache proc: Clear the pieces of proc_inode that proc_evict_inode cares about proc: Use d_invalidate in proc_prune_siblings_dcache proc: Use a list of inodes to flush from proc
Joel Fernandes (Google) (1): pidfd: add polling support
fs/proc/base.c | 242 ++++++++++++++++++++------------------------- fs/proc/fd.c | 20 +--- fs/proc/inode.c | 67 ++++++++++++- fs/proc/internal.h | 22 ++--- fs/proc/namespaces.c | 3 +- fs/proc/proc_sysctl.c | 45 ++------- fs/proc/self.c | 6 +- fs/proc/thread_self.c | 5 +- include/linux/pid.h | 5 + include/linux/proc_fs.h | 4 +- include/uapi/linux/sched.h | 1 + kernel/exit.c | 5 +- kernel/fork.c | 145 ++++++++++++++++++++++++++- kernel/pid.c | 3 + kernel/signal.c | 11 +++ security/selinux/hooks.c | 1 + 16 files changed, 357 insertions(+), 228 deletions(-)
[1] A test program to open the directory (/proc/<pid>/ns) #include <stdio.h> #include <sys/types.h> #include <dirent.h> #include <errno.h>
int main(int argc, char *argv[]) { DIR *dip; struct dirent *dit;
if (argc < 2) { printf("Usage :%s <directory>\n", argv[0]); return -1; }
if ((dip = opendir(argv[1])) == NULL) { perror("opendir"); return -1; }
printf("pid: %d\n", getpid()); while((dit = readdir (dip)) != NULL) { printf("%s\n", dit->d_name); }
while (1) sleep (1);
return 0; }
[2] Adding some debugging switches to the kernel, also adding a delay between proc_flush_task and __exit_signal in release_task():
diff --git a/fs/dcache.c b/fs/dcache.c index 05bad55..fafad37 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -84,6 +84,9 @@ int sysctl_vfs_cache_pressure __read_mostly = 100; EXPORT_SYMBOL_GPL(sysctl_vfs_cache_pressure);
+int sysctl_dentry_debug_trace __read_mostly = 0; +EXPORT_SYMBOL_GPL(sysctl_dentry_debug_trace);
__cacheline_aligned_in_smp DEFINE_SEQLOCK(rename_lock);
EXPORT_SYMBOL(rename_lock);
@@ -758,6 +761,26 @@ static inline bool fast_dput(struct dentry *dentry) return 0; }
+#define DENTRY_DEBUG_TRACE(dentry, keywords) \ +do { \
- if (sysctl_dentry_debug_trace) \
printk("XXX %s:%d " \"dentry=%s/%p, flag=%x, cnt=%d, inode=%p, " \"pdentry=%s/%p, pflag=%x, pcnt=%d, pinode=%p, " \"keywords: %s\n", \__func__, __LINE__, \dentry->d_name.name, \dentry, \dentry->d_flags, \dentry->d_lockref.count, \dentry->d_inode, \dentry->d_parent->d_name.name, \dentry->d_parent, \dentry->d_parent->d_flags, \dentry->d_parent->d_lockref.count, \dentry->d_parent->d_inode, \keywords); \+} while (0)
/*
- This is dput
@@ -804,6 +827,8 @@ void dput(struct dentry *dentry)
WARN_ON(d_in_lookup(dentry));
- DENTRY_DEBUG_TRACE(dentry, "be checked");
- /* Unreachable? Get rid of it */ if (unlikely(d_unhashed(dentry))) goto kill_it;
@@ -812,8 +837,10 @@ void dput(struct dentry *dentry) goto kill_it;
if (unlikely(dentry->d_flags & DCACHE_OP_DELETE)) {
if (dentry->d_op->d_delete(dentry))
if (dentry->d_op->d_delete(dentry)) {DENTRY_DEBUG_TRACE(dentry, "be killed"); goto kill_it;}}
if (!(dentry->d_flags & DCACHE_REFERENCED))
@@ -822,6 +849,9 @@ void dput(struct dentry *dentry)
dentry->d_lockref.count--; spin_unlock(&dentry->d_lock);
DENTRY_DEBUG_TRACE(dentry, "be cached");
return;
kill_it:
diff --git a/fs/proc/base.c b/fs/proc/base.c index b9e4183..419a409 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -3090,6 +3090,8 @@ void proc_flush_task(struct task_struct *task) } }
+extern int sysctl_dentry_debug_trace;
- static int proc_pid_instantiate(struct inode *dir, struct dentry * dentry, struct task_struct *task, const void *ptr)
@@ -3111,6 +3113,12 @@ static int proc_pid_instantiate(struct inode *dir, d_set_d_op(dentry, &pid_dentry_operations);
d_add(dentry, inode);
- if (sysctl_dentry_debug_trace)
printk("XXX %s:%d pid=%d tid=%d entry=%s/%p\n",__func__, __LINE__, task->pid, task->tgid,dentry->d_name.name, dentry);- /* Close the race of the process dying before we return the dentry */ if (pid_revalidate(dentry, 0)) return 0;
diff --git a/kernel/exit.c b/kernel/exit.c index 27f4168..2b3e1b6 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -55,6 +55,8 @@ #include <linux/shm.h> #include <linux/kcov.h>
+#include <linux/delay.h>
- #include <asm/uaccess.h> #include <asm/unistd.h> #include <asm/pgtable.h>
@@ -164,6 +166,8 @@ static void delayed_put_task_struct(struct rcu_head *rhp) put_task_struct(tsk); }
+int sysctl_dentry_debug_delay __read_mostly = 0; +int sysctl_dentry_debug_pid __read_mostly = 0;
void release_task(struct task_struct *p) { @@ -178,6 +182,11 @@ void release_task(struct task_struct *p)
proc_flush_task(p);
- if (sysctl_dentry_debug_delay && p->pid == sysctl_dentry_debug_pid) {
while (sysctl_dentry_debug_delay)mdelay(1);- }
- write_lock_irq(&tasklist_lock); ptrace_release_task(p); __exit_signal(p);
diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 513e6da..27f1395 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -282,6 +282,10 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write, static int max_extfrag_threshold = 1000; #endif
+extern int sysctl_dentry_debug_trace; +extern int sysctl_dentry_debug_delay; +extern int sysctl_dentry_debug_pid;
- static struct ctl_table kern_table[] = { { .procname = "sched_child_runs_first",
@@ -1498,6 +1502,30 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write, .proc_handler = proc_dointvec, .extra1 = &zero, },
- {
.procname = "dentry_debug_trace",.data = &sysctl_dentry_debug_trace,.maxlen = sizeof(sysctl_dentry_debug_trace),.mode = 0644,.proc_handler = proc_dointvec,.extra1 = &zero,- },
- {
.procname = "dentry_debug_delay",.data = &sysctl_dentry_debug_delay,.maxlen = sizeof(sysctl_dentry_debug_delay),.mode = 0644,.proc_handler = proc_dointvec,.extra1 = &zero,- },
- {
.procname = "dentry_debug_pid",.data = &sysctl_dentry_debug_pid,.maxlen = sizeof(sysctl_dentry_debug_pid),.mode = 0644,.proc_handler = proc_dointvec,.extra1 = &zero,- }, #ifdef HAVE_ARCH_PICK_MMAP_LAYOUT { .procname = "legacy_va_layout",
Signed-off-by: Wen Yang wenyang@linux.alibaba.com Cc: Pavel Emelyanov xemul@openvz.org Cc: Oleg Nesterov oleg@tv-sign.ru Cc: Sukadev Bhattiprolu sukadev@us.ibm.com Cc: Paul Menage menage@google.com Cc: "Eric W. Biederman" ebiederm@xmission.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: stable@vger.kernel.org
Hi Greg,
Could you kindly give some suggestions?
Thanks,
On Thu, Dec 17, 2020 at 10:26:23AM +0800, Wen Yang wrote:
在 2020/12/4 上午2:31, Wen Yang 写道:
The dentries such as /proc/<pid>/ns/ have the DCACHE_OP_DELETE flag, they should be deleted when the process exits.
Suppose the following race appears：
release_task dput -> proc_flush_task -> dentry->d_op->d_delete(dentry) -> __exit_signal -> dentry->d_lockref.count-- and return.
In the proc_flush_task(), if another process is using this dentry, it will not be deleted. At the same time, in dput(), d_op->d_delete() can be executed before __exit_signal(pid has not been hashed), d_delete returns false, so this dentry still cannot be deleted.
This dentry will always be cached (although its count is 0 and the DCACHE_OP_DELETE flag is set), its parent denry will also be cached too, and these dentries can only be deleted when drop_caches is manually triggered.
This will result in wasted memory. What's more troublesome is that these dentries reference pid, according to the commit f333c700c610 ("pidns: Add a limit on the number of pid namespaces"), if the pid cannot be released, it may result in the inability to create a new pid_ns.
This problem occurred in our cluster environment (Linux 4.9 LTS). We could reproduce it by manually constructing a test program + adding some debugging switches in the kernel:
- A test program to open the directory (/proc/<pid>/ns) [1]
- Adding some debugging switches to the kernel, adding a delay between proc_flush_task and __exit_signal in release_task() [2]
The test process is as follows:
A, terminal #1
Turn on the debug switch: echo 1> /proc/sys/vm/dentry_debug_trace
Execute the following unshare command: sudo unshare --pid --fork --mount-proc bash
B, terminal #2
Find the pid of the unshare process:
# pstree -p | grep unshare | `-sshd(716)---bash(718)--sudo(816)---unshare(817)---bash(818)
Find the corresponding dentry: # dmesg | grep pid=818 [70.424722] XXX proc_pid_instantiate:3119 pid=818 tid=818 entry=818/ffff8802c7b670e8
C, terminal #3
Execute the opendir program, it will always open the /proc/818/ns/ directory:
# ./a.out /proc/818/ns/ pid: 876 . .. net uts ipc pid user mnt cgroup
D, go back to terminal #2
Turn on the debugging switches to construct the race: # echo 818> /proc/sys/vm/dentry_debug_pid # echo 1> /proc/sys/vm/dentry_debug_delay
Kill the unshare process (pid 818). Since the debugging switches have been turned on, it will get stuck in release_task(): # kill -9 818
Then kill the process that opened the /proc/818/ns/ directory: # kill -9 876
Then turn off these debugging switches to allow the 818 process to exit: # echo 0> /proc/sys/vm/dentry_debug_delay # echo 0> /proc/sys/vm/dentry_debug_pid
Checking the dmesg, we will find that the dentry(/proc/818/ns) ’s count is 0, and the flag is 2800cc (#define DCACHE_OP_DELETE 0x00000008), but it is still cached: # dmesg | grep ffff8802a3999548 … [565.559156] XXX dput:853 dentry=ns/ffff8802bea7b528, flag=2800cc, cnt=0, inode=ffff8802b38c2010, pdentry=818/ffff8802c7b670e8, pflag=20008c, pcnt=1, pinode=ffff8802c7812010, keywords: be cached
It could also be verified via the crash tool:
crash> dentry.d_flags,d_iname,d_inode,d_lockref -x ffff8802bea7b528 d_flags = 0x2800cc d_iname = "ns\000kkkkkkkkkkkkkkkkkkkkkkkkkkkk" d_inode = 0xffff8802b38c2010 d_lockref = { { lock_count = 0x0, { lock = { { rlock = { raw_lock = { { val = { counter = 0x0 }, { locked = 0x0, pending = 0x0 }, { locked_pending = 0x0, tail = 0x0 } } } } } }, count = 0x0 } } } crash> kmem ffff8802bea7b528 CACHE OBJSIZE ALLOCATED TOTAL SLABS SSIZE NAME ffff8802dd5f5900 192 23663 26130 871 16k dentry SLAB MEMORY NODE TOTAL ALLOCATED FREE ffffea000afa9e00 ffff8802bea78000 0 30 25 5 FREE / [ALLOCATED] [ffff8802bea7b520]
PAGE PHYSICAL MAPPING INDEX CNT FLAGSffffea000afa9ec0 2bea7b000 dead000000000400 0 0 2fffff80000000 crash>
This series of patches is to fix this issue.
Regards, Wen
Alexey Dobriyan (1): proc: use %u for pid printing and slightly less stack
Andreas Gruenbacher (1): proc: Pass file mode to proc_pid_make_inode
Christian Brauner (1): clone: add CLONE_PIDFD
Eric W. Biederman (6): proc: Better ownership of files for non-dumpable tasks in user namespaces proc: Rename in proc_inode rename sysctl_inodes sibling_inodes proc: Generalize proc_sys_prune_dcache into proc_prune_siblings_dcache proc: Clear the pieces of proc_inode that proc_evict_inode cares about proc: Use d_invalidate in proc_prune_siblings_dcache proc: Use a list of inodes to flush from proc
Joel Fernandes (Google) (1): pidfd: add polling support
fs/proc/base.c | 242 ++++++++++++++++++++------------------------- fs/proc/fd.c | 20 +--- fs/proc/inode.c | 67 ++++++++++++- fs/proc/internal.h | 22 ++--- fs/proc/namespaces.c | 3 +- fs/proc/proc_sysctl.c | 45 ++------- fs/proc/self.c | 6 +- fs/proc/thread_self.c | 5 +- include/linux/pid.h | 5 + include/linux/proc_fs.h | 4 +- include/uapi/linux/sched.h | 1 + kernel/exit.c | 5 +- kernel/fork.c | 145 ++++++++++++++++++++++++++- kernel/pid.c | 3 + kernel/signal.c | 11 +++ security/selinux/hooks.c | 1 + 16 files changed, 357 insertions(+), 228 deletions(-)
[1] A test program to open the directory (/proc/<pid>/ns) #include <stdio.h> #include <sys/types.h> #include <dirent.h> #include <errno.h>
int main(int argc, char *argv[]) { DIR *dip; struct dirent *dit;
if (argc < 2) { printf("Usage :%s <directory>\n", argv[0]); return -1; }
if ((dip = opendir(argv[1])) == NULL) { perror("opendir"); return -1; }
printf("pid: %d\n", getpid()); while((dit = readdir (dip)) != NULL) { printf("%s\n", dit->d_name); }
while (1) sleep (1);
return 0; }
[2] Adding some debugging switches to the kernel, also adding a delay between proc_flush_task and __exit_signal in release_task():
diff --git a/fs/dcache.c b/fs/dcache.c index 05bad55..fafad37 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -84,6 +84,9 @@ int sysctl_vfs_cache_pressure __read_mostly = 100; EXPORT_SYMBOL_GPL(sysctl_vfs_cache_pressure);
+int sysctl_dentry_debug_trace __read_mostly = 0; +EXPORT_SYMBOL_GPL(sysctl_dentry_debug_trace);
__cacheline_aligned_in_smp DEFINE_SEQLOCK(rename_lock);
EXPORT_SYMBOL(rename_lock);
@@ -758,6 +761,26 @@ static inline bool fast_dput(struct dentry *dentry) return 0; }
+#define DENTRY_DEBUG_TRACE(dentry, keywords) \ +do { \
- if (sysctl_dentry_debug_trace) \
printk("XXX %s:%d " \"dentry=%s/%p, flag=%x, cnt=%d, inode=%p, " \"pdentry=%s/%p, pflag=%x, pcnt=%d, pinode=%p, " \"keywords: %s\n", \__func__, __LINE__, \dentry->d_name.name, \dentry, \dentry->d_flags, \dentry->d_lockref.count, \dentry->d_inode, \dentry->d_parent->d_name.name, \dentry->d_parent, \dentry->d_parent->d_flags, \dentry->d_parent->d_lockref.count, \dentry->d_parent->d_inode, \keywords); \+} while (0)
/*
- This is dput
@@ -804,6 +827,8 @@ void dput(struct dentry *dentry)
WARN_ON(d_in_lookup(dentry));
- DENTRY_DEBUG_TRACE(dentry, "be checked");
- /* Unreachable? Get rid of it */ if (unlikely(d_unhashed(dentry))) goto kill_it;
@@ -812,8 +837,10 @@ void dput(struct dentry *dentry) goto kill_it;
if (unlikely(dentry->d_flags & DCACHE_OP_DELETE)) {
if (dentry->d_op->d_delete(dentry))
if (dentry->d_op->d_delete(dentry)) {DENTRY_DEBUG_TRACE(dentry, "be killed"); goto kill_it;}}
if (!(dentry->d_flags & DCACHE_REFERENCED))
@@ -822,6 +849,9 @@ void dput(struct dentry *dentry)
dentry->d_lockref.count--; spin_unlock(&dentry->d_lock);
DENTRY_DEBUG_TRACE(dentry, "be cached");
return;
kill_it:
diff --git a/fs/proc/base.c b/fs/proc/base.c index b9e4183..419a409 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -3090,6 +3090,8 @@ void proc_flush_task(struct task_struct *task) } }
+extern int sysctl_dentry_debug_trace;
- static int proc_pid_instantiate(struct inode *dir, struct dentry * dentry, struct task_struct *task, const void *ptr)
@@ -3111,6 +3113,12 @@ static int proc_pid_instantiate(struct inode *dir, d_set_d_op(dentry, &pid_dentry_operations);
d_add(dentry, inode);
- if (sysctl_dentry_debug_trace)
printk("XXX %s:%d pid=%d tid=%d entry=%s/%p\n",__func__, __LINE__, task->pid, task->tgid,dentry->d_name.name, dentry);- /* Close the race of the process dying before we return the dentry */ if (pid_revalidate(dentry, 0)) return 0;
diff --git a/kernel/exit.c b/kernel/exit.c index 27f4168..2b3e1b6 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -55,6 +55,8 @@ #include <linux/shm.h> #include <linux/kcov.h>
+#include <linux/delay.h>
- #include <asm/uaccess.h> #include <asm/unistd.h> #include <asm/pgtable.h>
@@ -164,6 +166,8 @@ static void delayed_put_task_struct(struct rcu_head *rhp) put_task_struct(tsk); }
+int sysctl_dentry_debug_delay __read_mostly = 0; +int sysctl_dentry_debug_pid __read_mostly = 0;
void release_task(struct task_struct *p) { @@ -178,6 +182,11 @@ void release_task(struct task_struct *p)
proc_flush_task(p);
- if (sysctl_dentry_debug_delay && p->pid == sysctl_dentry_debug_pid) {
while (sysctl_dentry_debug_delay)mdelay(1);- }
- write_lock_irq(&tasklist_lock); ptrace_release_task(p); __exit_signal(p);
diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 513e6da..27f1395 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -282,6 +282,10 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write, static int max_extfrag_threshold = 1000; #endif
+extern int sysctl_dentry_debug_trace; +extern int sysctl_dentry_debug_delay; +extern int sysctl_dentry_debug_pid;
- static struct ctl_table kern_table[] = { { .procname = "sched_child_runs_first",
@@ -1498,6 +1502,30 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write, .proc_handler = proc_dointvec, .extra1 = &zero, },
- {
.procname = "dentry_debug_trace",.data = &sysctl_dentry_debug_trace,.maxlen = sizeof(sysctl_dentry_debug_trace),.mode = 0644,.proc_handler = proc_dointvec,.extra1 = &zero,- },
- {
.procname = "dentry_debug_delay",.data = &sysctl_dentry_debug_delay,.maxlen = sizeof(sysctl_dentry_debug_delay),.mode = 0644,.proc_handler = proc_dointvec,.extra1 = &zero,- },
- {
.procname = "dentry_debug_pid",.data = &sysctl_dentry_debug_pid,.maxlen = sizeof(sysctl_dentry_debug_pid),.mode = 0644,.proc_handler = proc_dointvec,.extra1 = &zero,- }, #ifdef HAVE_ARCH_PICK_MMAP_LAYOUT { .procname = "legacy_va_layout",
Signed-off-by: Wen Yang wenyang@linux.alibaba.com Cc: Pavel Emelyanov xemul@openvz.org Cc: Oleg Nesterov oleg@tv-sign.ru Cc: Sukadev Bhattiprolu sukadev@us.ibm.com Cc: Paul Menage menage@google.com Cc: "Eric W. Biederman" ebiederm@xmission.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: stable@vger.kernel.org
Hi Greg,
Could you kindly give some suggestions?
I'm sorry, but I don't really understand what this patch series is for.
Why is there a patch in the 00/10 email? What stable kernel(s) should this be backported to? And why can't you just use a newer kernel (like 4.19) if you are hitting this issue with much older ones?
confused,
greg k-h
在 2020/12/31 下午5:22, Greg Kroah-Hartman 写道:
On Thu, Dec 17, 2020 at 10:26:23AM +0800, Wen Yang wrote:
在 2020/12/4 上午2:31, Wen Yang 写道:
The dentries such as /proc/<pid>/ns/ have the DCACHE_OP_DELETE flag, they should be deleted when the process exits.
Suppose the following race appears：
release_task dput -> proc_flush_task -> dentry->d_op->d_delete(dentry) -> __exit_signal -> dentry->d_lockref.count-- and return.
In the proc_flush_task(), if another process is using this dentry, it will not be deleted. At the same time, in dput(), d_op->d_delete() can be executed before __exit_signal(pid has not been hashed), d_delete returns false, so this dentry still cannot be deleted.
This dentry will always be cached (although its count is 0 and the DCACHE_OP_DELETE flag is set), its parent denry will also be cached too, and these dentries can only be deleted when drop_caches is manually triggered.
This will result in wasted memory. What's more troublesome is that these dentries reference pid, according to the commit f333c700c610 ("pidns: Add a limit on the number of pid namespaces"), if the pid cannot be released, it may result in the inability to create a new pid_ns.
This problem occurred in our cluster environment (Linux 4.9 LTS). We could reproduce it by manually constructing a test program + adding some debugging switches in the kernel:
- A test program to open the directory (/proc/<pid>/ns) [1]
- Adding some debugging switches to the kernel, adding a delay between proc_flush_task and __exit_signal in release_task() [2]
The test process is as follows:
A, terminal #1
Turn on the debug switch: echo 1> /proc/sys/vm/dentry_debug_trace
Execute the following unshare command: sudo unshare --pid --fork --mount-proc bash
B, terminal #2
Find the pid of the unshare process:
# pstree -p | grep unshare | `-sshd(716)---bash(718)--sudo(816)---unshare(817)---bash(818)
Find the corresponding dentry: # dmesg | grep pid=818 [70.424722] XXX proc_pid_instantiate:3119 pid=818 tid=818 entry=818/ffff8802c7b670e8
C, terminal #3
Execute the opendir program, it will always open the /proc/818/ns/ directory:
# ./a.out /proc/818/ns/ pid: 876 . .. net uts ipc pid user mnt cgroup
D, go back to terminal #2
Turn on the debugging switches to construct the race: # echo 818> /proc/sys/vm/dentry_debug_pid # echo 1> /proc/sys/vm/dentry_debug_delay
Kill the unshare process (pid 818). Since the debugging switches have been turned on, it will get stuck in release_task(): # kill -9 818
Then kill the process that opened the /proc/818/ns/ directory: # kill -9 876
Then turn off these debugging switches to allow the 818 process to exit: # echo 0> /proc/sys/vm/dentry_debug_delay # echo 0> /proc/sys/vm/dentry_debug_pid
Checking the dmesg, we will find that the dentry(/proc/818/ns) ’s count is 0, and the flag is 2800cc (#define DCACHE_OP_DELETE 0x00000008), but it is still cached: # dmesg | grep ffff8802a3999548 … [565.559156] XXX dput:853 dentry=ns/ffff8802bea7b528, flag=2800cc, cnt=0, inode=ffff8802b38c2010, pdentry=818/ffff8802c7b670e8, pflag=20008c, pcnt=1, pinode=ffff8802c7812010, keywords: be cached
It could also be verified via the crash tool:
crash> dentry.d_flags,d_iname,d_inode,d_lockref -x ffff8802bea7b528 d_flags = 0x2800cc d_iname = "ns\000kkkkkkkkkkkkkkkkkkkkkkkkkkkk" d_inode = 0xffff8802b38c2010 d_lockref = { { lock_count = 0x0, { lock = { { rlock = { raw_lock = { { val = { counter = 0x0 }, { locked = 0x0, pending = 0x0 }, { locked_pending = 0x0, tail = 0x0 } } } } } }, count = 0x0 } } } crash> kmem ffff8802bea7b528 CACHE OBJSIZE ALLOCATED TOTAL SLABS SSIZE NAME ffff8802dd5f5900 192 23663 26130 871 16k dentry SLAB MEMORY NODE TOTAL ALLOCATED FREE ffffea000afa9e00 ffff8802bea78000 0 30 25 5 FREE / [ALLOCATED] [ffff8802bea7b520]
PAGE PHYSICAL MAPPING INDEX CNT FLAGSffffea000afa9ec0 2bea7b000 dead000000000400 0 0 2fffff80000000 crash>
This series of patches is to fix this issue.
Regards, Wen
Alexey Dobriyan (1): proc: use %u for pid printing and slightly less stack
Andreas Gruenbacher (1): proc: Pass file mode to proc_pid_make_inode
Christian Brauner (1): clone: add CLONE_PIDFD
Eric W. Biederman (6): proc: Better ownership of files for non-dumpable tasks in user namespaces proc: Rename in proc_inode rename sysctl_inodes sibling_inodes proc: Generalize proc_sys_prune_dcache into proc_prune_siblings_dcache proc: Clear the pieces of proc_inode that proc_evict_inode cares about proc: Use d_invalidate in proc_prune_siblings_dcache proc: Use a list of inodes to flush from proc
Joel Fernandes (Google) (1): pidfd: add polling support
fs/proc/base.c | 242 ++++++++++++++++++++------------------------- fs/proc/fd.c | 20 +--- fs/proc/inode.c | 67 ++++++++++++- fs/proc/internal.h | 22 ++--- fs/proc/namespaces.c | 3 +- fs/proc/proc_sysctl.c | 45 ++------- fs/proc/self.c | 6 +- fs/proc/thread_self.c | 5 +- include/linux/pid.h | 5 + include/linux/proc_fs.h | 4 +- include/uapi/linux/sched.h | 1 + kernel/exit.c | 5 +- kernel/fork.c | 145 ++++++++++++++++++++++++++- kernel/pid.c | 3 + kernel/signal.c | 11 +++ security/selinux/hooks.c | 1 + 16 files changed, 357 insertions(+), 228 deletions(-)
[1] A test program to open the directory (/proc/<pid>/ns) #include <stdio.h> #include <sys/types.h> #include <dirent.h> #include <errno.h>
int main(int argc, char *argv[]) { DIR *dip; struct dirent *dit;
if (argc < 2) { printf("Usage :%s <directory>\n", argv[0]); return -1; }
if ((dip = opendir(argv[1])) == NULL) { perror("opendir"); return -1; }
printf("pid: %d\n", getpid()); while((dit = readdir (dip)) != NULL) { printf("%s\n", dit->d_name); }
while (1) sleep (1);
return 0; }
[2] Adding some debugging switches to the kernel, also adding a delay between proc_flush_task and __exit_signal in release_task():
diff --git a/fs/dcache.c b/fs/dcache.c index 05bad55..fafad37 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -84,6 +84,9 @@ int sysctl_vfs_cache_pressure __read_mostly = 100; EXPORT_SYMBOL_GPL(sysctl_vfs_cache_pressure);
+int sysctl_dentry_debug_trace __read_mostly = 0; +EXPORT_SYMBOL_GPL(sysctl_dentry_debug_trace);
__cacheline_aligned_in_smp DEFINE_SEQLOCK(rename_lock);
EXPORT_SYMBOL(rename_lock);
@@ -758,6 +761,26 @@ static inline bool fast_dput(struct dentry *dentry) return 0; }
+#define DENTRY_DEBUG_TRACE(dentry, keywords) \ +do { \
- if (sysctl_dentry_debug_trace) \
printk("XXX %s:%d " \"dentry=%s/%p, flag=%x, cnt=%d, inode=%p, " \"pdentry=%s/%p, pflag=%x, pcnt=%d, pinode=%p, " \"keywords: %s\n", \__func__, __LINE__, \dentry->d_name.name, \dentry, \dentry->d_flags, \dentry->d_lockref.count, \dentry->d_inode, \dentry->d_parent->d_name.name, \dentry->d_parent, \dentry->d_parent->d_flags, \dentry->d_parent->d_lockref.count, \dentry->d_parent->d_inode, \keywords); \+} while (0)
/* * This is dput @@ -804,6 +827,8 @@ void dput(struct dentry *dentry)
WARN_ON(d_in_lookup(dentry));
- DENTRY_DEBUG_TRACE(dentry, "be checked");
- /* Unreachable? Get rid of it */ if (unlikely(d_unhashed(dentry))) goto kill_it;
@@ -812,8 +837,10 @@ void dput(struct dentry *dentry) goto kill_it;
if (unlikely(dentry->d_flags & DCACHE_OP_DELETE)) {
if (dentry->d_op->d_delete(dentry))
if (dentry->d_op->d_delete(dentry)) {DENTRY_DEBUG_TRACE(dentry, "be killed"); goto kill_it;}}
if (!(dentry->d_flags & DCACHE_REFERENCED))
@@ -822,6 +849,9 @@ void dput(struct dentry *dentry)
dentry->d_lockref.count--; spin_unlock(&dentry->d_lock);
DENTRY_DEBUG_TRACE(dentry, "be cached");
return;
kill_it:
diff --git a/fs/proc/base.c b/fs/proc/base.c index b9e4183..419a409 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -3090,6 +3090,8 @@ void proc_flush_task(struct task_struct *task) } }
+extern int sysctl_dentry_debug_trace;
- static int proc_pid_instantiate(struct inode *dir, struct dentry * dentry, struct task_struct *task, const void *ptr)
@@ -3111,6 +3113,12 @@ static int proc_pid_instantiate(struct inode *dir, d_set_d_op(dentry, &pid_dentry_operations);
d_add(dentry, inode);
- if (sysctl_dentry_debug_trace)
printk("XXX %s:%d pid=%d tid=%d entry=%s/%p\n",__func__, __LINE__, task->pid, task->tgid,dentry->d_name.name, dentry);- /* Close the race of the process dying before we return the dentry */ if (pid_revalidate(dentry, 0)) return 0;
diff --git a/kernel/exit.c b/kernel/exit.c index 27f4168..2b3e1b6 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -55,6 +55,8 @@ #include <linux/shm.h> #include <linux/kcov.h>
+#include <linux/delay.h>
- #include <asm/uaccess.h> #include <asm/unistd.h> #include <asm/pgtable.h>
@@ -164,6 +166,8 @@ static void delayed_put_task_struct(struct rcu_head *rhp) put_task_struct(tsk); }
+int sysctl_dentry_debug_delay __read_mostly = 0; +int sysctl_dentry_debug_pid __read_mostly = 0;
void release_task(struct task_struct *p) { @@ -178,6 +182,11 @@ void release_task(struct task_struct *p)
proc_flush_task(p);
- if (sysctl_dentry_debug_delay && p->pid == sysctl_dentry_debug_pid) {
while (sysctl_dentry_debug_delay)mdelay(1);- }
- write_lock_irq(&tasklist_lock); ptrace_release_task(p); __exit_signal(p);
diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 513e6da..27f1395 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -282,6 +282,10 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write, static int max_extfrag_threshold = 1000; #endif
+extern int sysctl_dentry_debug_trace; +extern int sysctl_dentry_debug_delay; +extern int sysctl_dentry_debug_pid;
- static struct ctl_table kern_table[] = { { .procname = "sched_child_runs_first",
@@ -1498,6 +1502,30 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write, .proc_handler = proc_dointvec, .extra1 = &zero, },
- {
.procname = "dentry_debug_trace",.data = &sysctl_dentry_debug_trace,.maxlen = sizeof(sysctl_dentry_debug_trace),.mode = 0644,.proc_handler = proc_dointvec,.extra1 = &zero,- },
- {
.procname = "dentry_debug_delay",.data = &sysctl_dentry_debug_delay,.maxlen = sizeof(sysctl_dentry_debug_delay),.mode = 0644,.proc_handler = proc_dointvec,.extra1 = &zero,- },
- {
.procname = "dentry_debug_pid",.data = &sysctl_dentry_debug_pid,.maxlen = sizeof(sysctl_dentry_debug_pid),.mode = 0644,.proc_handler = proc_dointvec,.extra1 = &zero,- }, #ifdef HAVE_ARCH_PICK_MMAP_LAYOUT { .procname = "legacy_va_layout",
Signed-off-by: Wen Yang wenyang@linux.alibaba.com Cc: Pavel Emelyanov xemul@openvz.org Cc: Oleg Nesterov oleg@tv-sign.ru Cc: Sukadev Bhattiprolu sukadev@us.ibm.com Cc: Paul Menage menage@google.com Cc: "Eric W. Biederman" ebiederm@xmission.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: stable@vger.kernel.org
Hi Greg,
Could you kindly give some suggestions?
I'm sorry, but I don't really understand what this patch series is for.
Why is there a patch in the 00/10 email? What stable kernel(s) should this be backported to? And why can't you just use a newer kernel (like 4.19) if you are hitting this issue with much older ones?
This bug was introduced by 60347f6716aa ("pid namespaces: prepare proc_flust_task() to flush entries from multiple proc trees"), exposed by f333c700c610 ("pidns: Add a limit on the number of pid namespaces"), and then fixed by 7bc3e6e55acf (“proc: Use a list of inodes to flush from proc”)
4.19 LTS did not solve it either.
The one that fixes the bug is this patch (10/10): https://lkml.org/lkml/2020/12/3/1046 The previous 9 patches (from 01/10 to 09/10) are its pre-dependencies.
The 00/10 is some of our test programs, including a user mode program (open the /proc/<pid>/ns directory), and some hacks added to the kernel (just add log printing and some delays for easy construction this race).
Thanks.
On Fri, Dec 04, 2020 at 02:31:55AM +0800, Wen Yang wrote:
From: Christian Brauner christian@brauner.io
[ Upstream commit b3e5838252665ee4cfa76b82bdf1198dca81e5be ]
This patchset makes it possible to retrieve pid file descriptors at process creation time by introducing the new flag CLONE_PIDFD to the clone() system call. Linus originally suggested to implement this as a new flag to clone() instead of making it a separate system call. As spotted by Linus, there is exactly one bit for clone() left.
CLONE_PIDFD creates file descriptors based on the anonymous inode implementation in the kernel that will also be used to implement the new mount api. They serve as a simple opaque handle on pids. Logically, this makes it possible to interpret a pidfd differently, narrowing or widening the scope of various operations (e.g. signal sending). Thus, a pidfd cannot just refer to a tgid, but also a tid, or in theory - given appropriate flag arguments in relevant syscalls - a process group or session. A pidfd does not represent a privilege. This does not imply it cannot ever be that way but for now this is not the case.
A pidfd comes with additional information in fdinfo if the kernel supports procfs. The fdinfo file contains the pid of the process in the callers pid namespace in the same format as the procfs status file, i.e. "Pid:\t%d".
As suggested by Oleg, with CLONE_PIDFD the pidfd is returned in the parent_tidptr argument of clone. This has the advantage that we can give back the associated pid and the pidfd at the same time.
To remove worries about missing metadata access this patchset comes with a sample program that illustrates how a combination of CLONE_PIDFD, and pidfd_send_signal() can be used to gain race-free access to process metadata through /proc/<pid>. The sample program can easily be translated into a helper that would be suitable for inclusion in libc so that users don't have to worry about writing it themselves.
Suggested-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Christian Brauner christian@brauner.io Co-developed-by: Jann Horn jannh@google.com Signed-off-by: Jann Horn jannh@google.com Reviewed-by: Oleg Nesterov oleg@redhat.com Cc: Arnd Bergmann arnd@arndb.de Cc: "Eric W. Biederman" ebiederm@xmission.com Cc: Kees Cook keescook@chromium.org Cc: Thomas Gleixner tglx@linutronix.de Cc: David Howells dhowells@redhat.com Cc: "Michael Kerrisk (man-pages)" mtk.manpages@gmail.com Cc: Andy Lutomirsky luto@kernel.org Cc: Andrew Morton akpm@linux-foundation.org Cc: Aleksa Sarai cyphar@cyphar.com Cc: Linus Torvalds torvalds@linux-foundation.org Cc: Al Viro viro@zeniv.linux.org.uk Cc: stable@vger.kernel.org # 4.9.x (clone: fix up cherry-pick conflicts for b3e583825266) Signed-off-by: Wen Yang wenyang@linux.alibaba.com
include/linux/pid.h | 1 + include/uapi/linux/sched.h | 1 + kernel/fork.c | 119 +++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 117 insertions(+), 4 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h index 97b745d..7599a78 100644 --- a/include/linux/pid.h +++ b/include/linux/pid.h @@ -73,6 +73,7 @@ struct pid_link struct hlist_node node; struct pid *pid; }; +extern const struct file_operations pidfd_fops; static inline struct pid *get_pid(struct pid *pid) { diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h index 5f0fe01..ed6e31d 100644 --- a/include/uapi/linux/sched.h +++ b/include/uapi/linux/sched.h @@ -9,6 +9,7 @@ #define CLONE_FS 0x00000200 /* set if fs info shared between processes */ #define CLONE_FILES 0x00000400 /* set if open files shared between processes */ #define CLONE_SIGHAND 0x00000800 /* set if signal handlers and blocked signals shared */ +#define CLONE_PIDFD 0x00001000 /* set if a pidfd should be placed in parent */ #define CLONE_PTRACE 0x00002000 /* set if we want to let tracing continue on the child too */ #define CLONE_VFORK 0x00004000 /* set if the parent wants the child to wake it up on mm_release */ #define CLONE_PARENT 0x00008000 /* set if we want to have the same parent as the cloner */ diff --git a/kernel/fork.c b/kernel/fork.c index b64efec..076297a 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -11,7 +11,22 @@
- management can be a bitch. See 'mm/memory.c': 'copy_page_range()'
*/ +#include <linux/anon_inodes.h> #include <linux/slab.h> +#if 0 +#include <linux/sched/autogroup.h> +#include <linux/sched/mm.h> +#include <linux/sched/coredump.h> +#include <linux/sched/user.h> +#include <linux/sched/numa_balancing.h> +#include <linux/sched/stat.h> +#include <linux/sched/task.h> +#include <linux/sched/task_stack.h> +#include <linux/sched/cputime.h> +#include <linux/seq_file.h> +#include <linux/rtmutex.h> +>>>>>>> b3e58382... clone: add CLONE_PIDFD +#endif
That looks odd :(
Can you please refresh this patch series, and make sure it is correct and resend it?
thanks,
greg k-h
On Mon, Jan 04, 2021 at 02:03:14PM +0100, Greg Kroah-Hartman wrote:
On Fri, Dec 04, 2020 at 02:31:55AM +0800, Wen Yang wrote:
From: Christian Brauner christian@brauner.io
[ Upstream commit b3e5838252665ee4cfa76b82bdf1198dca81e5be ]
This patchset makes it possible to retrieve pid file descriptors at process creation time by introducing the new flag CLONE_PIDFD to the clone() system call. Linus originally suggested to implement this as a new flag to clone() instead of making it a separate system call. As spotted by Linus, there is exactly one bit for clone() left.
CLONE_PIDFD creates file descriptors based on the anonymous inode implementation in the kernel that will also be used to implement the new mount api. They serve as a simple opaque handle on pids. Logically, this makes it possible to interpret a pidfd differently, narrowing or widening the scope of various operations (e.g. signal sending). Thus, a pidfd cannot just refer to a tgid, but also a tid, or in theory - given appropriate flag arguments in relevant syscalls - a process group or session. A pidfd does not represent a privilege. This does not imply it cannot ever be that way but for now this is not the case.
A pidfd comes with additional information in fdinfo if the kernel supports procfs. The fdinfo file contains the pid of the process in the callers pid namespace in the same format as the procfs status file, i.e. "Pid:\t%d".
As suggested by Oleg, with CLONE_PIDFD the pidfd is returned in the parent_tidptr argument of clone. This has the advantage that we can give back the associated pid and the pidfd at the same time.
To remove worries about missing metadata access this patchset comes with a sample program that illustrates how a combination of CLONE_PIDFD, and pidfd_send_signal() can be used to gain race-free access to process metadata through /proc/<pid>. The sample program can easily be translated into a helper that would be suitable for inclusion in libc so that users don't have to worry about writing it themselves.
Suggested-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Christian Brauner christian@brauner.io Co-developed-by: Jann Horn jannh@google.com Signed-off-by: Jann Horn jannh@google.com Reviewed-by: Oleg Nesterov oleg@redhat.com Cc: Arnd Bergmann arnd@arndb.de Cc: "Eric W. Biederman" ebiederm@xmission.com Cc: Kees Cook keescook@chromium.org Cc: Thomas Gleixner tglx@linutronix.de Cc: David Howells dhowells@redhat.com Cc: "Michael Kerrisk (man-pages)" mtk.manpages@gmail.com Cc: Andy Lutomirsky luto@kernel.org Cc: Andrew Morton akpm@linux-foundation.org Cc: Aleksa Sarai cyphar@cyphar.com Cc: Linus Torvalds torvalds@linux-foundation.org Cc: Al Viro viro@zeniv.linux.org.uk Cc: stable@vger.kernel.org # 4.9.x (clone: fix up cherry-pick conflicts for b3e583825266) Signed-off-by: Wen Yang wenyang@linux.alibaba.com
include/linux/pid.h | 1 + include/uapi/linux/sched.h | 1 + kernel/fork.c | 119 +++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 117 insertions(+), 4 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h index 97b745d..7599a78 100644 --- a/include/linux/pid.h +++ b/include/linux/pid.h @@ -73,6 +73,7 @@ struct pid_link struct hlist_node node; struct pid *pid; }; +extern const struct file_operations pidfd_fops; static inline struct pid *get_pid(struct pid *pid) { diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h index 5f0fe01..ed6e31d 100644 --- a/include/uapi/linux/sched.h +++ b/include/uapi/linux/sched.h @@ -9,6 +9,7 @@ #define CLONE_FS 0x00000200 /* set if fs info shared between processes */ #define CLONE_FILES 0x00000400 /* set if open files shared between processes */ #define CLONE_SIGHAND 0x00000800 /* set if signal handlers and blocked signals shared */ +#define CLONE_PIDFD 0x00001000 /* set if a pidfd should be placed in parent */ #define CLONE_PTRACE 0x00002000 /* set if we want to let tracing continue on the child too */ #define CLONE_VFORK 0x00004000 /* set if the parent wants the child to wake it up on mm_release */ #define CLONE_PARENT 0x00008000 /* set if we want to have the same parent as the cloner */ diff --git a/kernel/fork.c b/kernel/fork.c index b64efec..076297a 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -11,7 +11,22 @@
- management can be a bitch. See 'mm/memory.c': 'copy_page_range()'
*/ +#include <linux/anon_inodes.h> #include <linux/slab.h> +#if 0 +#include <linux/sched/autogroup.h> +#include <linux/sched/mm.h> +#include <linux/sched/coredump.h> +#include <linux/sched/user.h> +#include <linux/sched/numa_balancing.h> +#include <linux/sched/stat.h> +#include <linux/sched/task.h> +#include <linux/sched/task_stack.h> +#include <linux/sched/cputime.h> +#include <linux/seq_file.h> +#include <linux/rtmutex.h> +>>>>>>> b3e58382... clone: add CLONE_PIDFD +#endif
That looks odd :(
Can you please refresh this patch series, and make sure it is correct and resend it?
Uhm, this patch series has been merged at least a year ago so this looks like an accidental send. This probably isn't meant for upstream but for some alibaba specific kernel I'd reckon.
Thanks! Christian
On Mon, Jan 04, 2021 at 02:13:42PM +0100, Christian Brauner wrote:
On Mon, Jan 04, 2021 at 02:03:14PM +0100, Greg Kroah-Hartman wrote:
On Fri, Dec 04, 2020 at 02:31:55AM +0800, Wen Yang wrote:
From: Christian Brauner christian@brauner.io
[ Upstream commit b3e5838252665ee4cfa76b82bdf1198dca81e5be ]
This patchset makes it possible to retrieve pid file descriptors at process creation time by introducing the new flag CLONE_PIDFD to the clone() system call. Linus originally suggested to implement this as a new flag to clone() instead of making it a separate system call. As spotted by Linus, there is exactly one bit for clone() left.
CLONE_PIDFD creates file descriptors based on the anonymous inode implementation in the kernel that will also be used to implement the new mount api. They serve as a simple opaque handle on pids. Logically, this makes it possible to interpret a pidfd differently, narrowing or widening the scope of various operations (e.g. signal sending). Thus, a pidfd cannot just refer to a tgid, but also a tid, or in theory - given appropriate flag arguments in relevant syscalls - a process group or session. A pidfd does not represent a privilege. This does not imply it cannot ever be that way but for now this is not the case.
A pidfd comes with additional information in fdinfo if the kernel supports procfs. The fdinfo file contains the pid of the process in the callers pid namespace in the same format as the procfs status file, i.e. "Pid:\t%d".
As suggested by Oleg, with CLONE_PIDFD the pidfd is returned in the parent_tidptr argument of clone. This has the advantage that we can give back the associated pid and the pidfd at the same time.
To remove worries about missing metadata access this patchset comes with a sample program that illustrates how a combination of CLONE_PIDFD, and pidfd_send_signal() can be used to gain race-free access to process metadata through /proc/<pid>. The sample program can easily be translated into a helper that would be suitable for inclusion in libc so that users don't have to worry about writing it themselves.
Suggested-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Christian Brauner christian@brauner.io Co-developed-by: Jann Horn jannh@google.com Signed-off-by: Jann Horn jannh@google.com Reviewed-by: Oleg Nesterov oleg@redhat.com Cc: Arnd Bergmann arnd@arndb.de Cc: "Eric W. Biederman" ebiederm@xmission.com Cc: Kees Cook keescook@chromium.org Cc: Thomas Gleixner tglx@linutronix.de Cc: David Howells dhowells@redhat.com Cc: "Michael Kerrisk (man-pages)" mtk.manpages@gmail.com Cc: Andy Lutomirsky luto@kernel.org Cc: Andrew Morton akpm@linux-foundation.org Cc: Aleksa Sarai cyphar@cyphar.com Cc: Linus Torvalds torvalds@linux-foundation.org Cc: Al Viro viro@zeniv.linux.org.uk Cc: stable@vger.kernel.org # 4.9.x (clone: fix up cherry-pick conflicts for b3e583825266) Signed-off-by: Wen Yang wenyang@linux.alibaba.com
include/linux/pid.h | 1 + include/uapi/linux/sched.h | 1 + kernel/fork.c | 119 +++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 117 insertions(+), 4 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h index 97b745d..7599a78 100644 --- a/include/linux/pid.h +++ b/include/linux/pid.h @@ -73,6 +73,7 @@ struct pid_link struct hlist_node node; struct pid *pid; }; +extern const struct file_operations pidfd_fops; static inline struct pid *get_pid(struct pid *pid) { diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h index 5f0fe01..ed6e31d 100644 --- a/include/uapi/linux/sched.h +++ b/include/uapi/linux/sched.h @@ -9,6 +9,7 @@ #define CLONE_FS 0x00000200 /* set if fs info shared between processes */ #define CLONE_FILES 0x00000400 /* set if open files shared between processes */ #define CLONE_SIGHAND 0x00000800 /* set if signal handlers and blocked signals shared */ +#define CLONE_PIDFD 0x00001000 /* set if a pidfd should be placed in parent */ #define CLONE_PTRACE 0x00002000 /* set if we want to let tracing continue on the child too */ #define CLONE_VFORK 0x00004000 /* set if the parent wants the child to wake it up on mm_release */ #define CLONE_PARENT 0x00008000 /* set if we want to have the same parent as the cloner */ diff --git a/kernel/fork.c b/kernel/fork.c index b64efec..076297a 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -11,7 +11,22 @@
- management can be a bitch. See 'mm/memory.c': 'copy_page_range()'
*/ +#include <linux/anon_inodes.h> #include <linux/slab.h> +#if 0 +#include <linux/sched/autogroup.h> +#include <linux/sched/mm.h> +#include <linux/sched/coredump.h> +#include <linux/sched/user.h> +#include <linux/sched/numa_balancing.h> +#include <linux/sched/stat.h> +#include <linux/sched/task.h> +#include <linux/sched/task_stack.h> +#include <linux/sched/cputime.h> +#include <linux/seq_file.h> +#include <linux/rtmutex.h> +>>>>>>> b3e58382... clone: add CLONE_PIDFD +#endif
That looks odd :(
Can you please refresh this patch series, and make sure it is correct and resend it?
Uhm, this patch series has been merged at least a year ago so this looks like an accidental send. This probably isn't meant for upstream but for some alibaba specific kernel I'd reckon.
This was ment for the 4.19.y kernel to solve a problem reported in patch 00/XX of the series.
thanks,
greg k-h
On Mon, Jan 04, 2021 at 02:17:40PM +0100, Greg Kroah-Hartman wrote:
On Mon, Jan 04, 2021 at 02:13:42PM +0100, Christian Brauner wrote:
On Mon, Jan 04, 2021 at 02:03:14PM +0100, Greg Kroah-Hartman wrote:
On Fri, Dec 04, 2020 at 02:31:55AM +0800, Wen Yang wrote:
From: Christian Brauner christian@brauner.io
[ Upstream commit b3e5838252665ee4cfa76b82bdf1198dca81e5be ]
This patchset makes it possible to retrieve pid file descriptors at process creation time by introducing the new flag CLONE_PIDFD to the clone() system call. Linus originally suggested to implement this as a new flag to clone() instead of making it a separate system call. As spotted by Linus, there is exactly one bit for clone() left.
CLONE_PIDFD creates file descriptors based on the anonymous inode implementation in the kernel that will also be used to implement the new mount api. They serve as a simple opaque handle on pids. Logically, this makes it possible to interpret a pidfd differently, narrowing or widening the scope of various operations (e.g. signal sending). Thus, a pidfd cannot just refer to a tgid, but also a tid, or in theory - given appropriate flag arguments in relevant syscalls - a process group or session. A pidfd does not represent a privilege. This does not imply it cannot ever be that way but for now this is not the case.
A pidfd comes with additional information in fdinfo if the kernel supports procfs. The fdinfo file contains the pid of the process in the callers pid namespace in the same format as the procfs status file, i.e. "Pid:\t%d".
As suggested by Oleg, with CLONE_PIDFD the pidfd is returned in the parent_tidptr argument of clone. This has the advantage that we can give back the associated pid and the pidfd at the same time.
To remove worries about missing metadata access this patchset comes with a sample program that illustrates how a combination of CLONE_PIDFD, and pidfd_send_signal() can be used to gain race-free access to process metadata through /proc/<pid>. The sample program can easily be translated into a helper that would be suitable for inclusion in libc so that users don't have to worry about writing it themselves.
Suggested-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Christian Brauner christian@brauner.io Co-developed-by: Jann Horn jannh@google.com Signed-off-by: Jann Horn jannh@google.com Reviewed-by: Oleg Nesterov oleg@redhat.com Cc: Arnd Bergmann arnd@arndb.de Cc: "Eric W. Biederman" ebiederm@xmission.com Cc: Kees Cook keescook@chromium.org Cc: Thomas Gleixner tglx@linutronix.de Cc: David Howells dhowells@redhat.com Cc: "Michael Kerrisk (man-pages)" mtk.manpages@gmail.com Cc: Andy Lutomirsky luto@kernel.org Cc: Andrew Morton akpm@linux-foundation.org Cc: Aleksa Sarai cyphar@cyphar.com Cc: Linus Torvalds torvalds@linux-foundation.org Cc: Al Viro viro@zeniv.linux.org.uk Cc: stable@vger.kernel.org # 4.9.x (clone: fix up cherry-pick conflicts for b3e583825266) Signed-off-by: Wen Yang wenyang@linux.alibaba.com
include/linux/pid.h | 1 + include/uapi/linux/sched.h | 1 + kernel/fork.c | 119 +++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 117 insertions(+), 4 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h index 97b745d..7599a78 100644 --- a/include/linux/pid.h +++ b/include/linux/pid.h @@ -73,6 +73,7 @@ struct pid_link struct hlist_node node; struct pid *pid; }; +extern const struct file_operations pidfd_fops; static inline struct pid *get_pid(struct pid *pid) { diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h index 5f0fe01..ed6e31d 100644 --- a/include/uapi/linux/sched.h +++ b/include/uapi/linux/sched.h @@ -9,6 +9,7 @@ #define CLONE_FS 0x00000200 /* set if fs info shared between processes */ #define CLONE_FILES 0x00000400 /* set if open files shared between processes */ #define CLONE_SIGHAND 0x00000800 /* set if signal handlers and blocked signals shared */ +#define CLONE_PIDFD 0x00001000 /* set if a pidfd should be placed in parent */ #define CLONE_PTRACE 0x00002000 /* set if we want to let tracing continue on the child too */ #define CLONE_VFORK 0x00004000 /* set if the parent wants the child to wake it up on mm_release */ #define CLONE_PARENT 0x00008000 /* set if we want to have the same parent as the cloner */ diff --git a/kernel/fork.c b/kernel/fork.c index b64efec..076297a 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -11,7 +11,22 @@
- management can be a bitch. See 'mm/memory.c': 'copy_page_range()'
*/ +#include <linux/anon_inodes.h> #include <linux/slab.h> +#if 0 +#include <linux/sched/autogroup.h> +#include <linux/sched/mm.h> +#include <linux/sched/coredump.h> +#include <linux/sched/user.h> +#include <linux/sched/numa_balancing.h> +#include <linux/sched/stat.h> +#include <linux/sched/task.h> +#include <linux/sched/task_stack.h> +#include <linux/sched/cputime.h> +#include <linux/seq_file.h> +#include <linux/rtmutex.h> +>>>>>>> b3e58382... clone: add CLONE_PIDFD +#endif
That looks odd :(
Can you please refresh this patch series, and make sure it is correct and resend it?
Uhm, this patch series has been merged at least a year ago so this looks like an accidental send. This probably isn't meant for upstream but for some alibaba specific kernel I'd reckon.
This was ment for the 4.19.y kernel to solve a problem reported in patch 00/XX of the series.
Ah, ok. Then just as an fyi: the Cc line seems to indicate 4.9 and not 4.19.
Christian
linux-stable-mirror@lists.linaro.org
-
Alexey Dobriyan -
Christian Brauner -
Greg Kroah-Hartman -
Wen Yang