Messages in this thread | | | Subject | Re: [PATCH 00/10] Cover letter: fix a race in release_task when flushing the dentry | From | Wen Yang <> | Date | Thu, 17 Dec 2020 10:26:23 +0800 |
| |
在 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 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> >
Hi Greg,
Could you kindly give some suggestions?
Thanks,
| |