Received: by 2002:a25:1506:0:0:0:0:0 with SMTP id 6csp6279794ybv; Wed, 12 Feb 2020 09:10:53 -0800 (PST) X-Google-Smtp-Source: APXvYqwAv4Q0tAZkndOjJQrtIdlSGW35ZbsIH0PZlxXbIHQCoOvJJvTLknRJBaoyeyMcEAliagKf X-Received: by 2002:a9d:51c1:: with SMTP id d1mr9610377oth.136.1581527452955; Wed, 12 Feb 2020 09:10:52 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1581527452; cv=none; d=google.com; s=arc-20160816; b=B7N+MeevPffz7EtCp5k1bPPCZaYypfn/o53pcauHFYBov5N5w4lxM6fFYTJpZeLqfN OgW4G1MoFXWu11P+VCuP77hJvGkCoIeU3Uk7sTH7S+mnrYFAcj5xsFfa2KZNmh27K3c0 kdgMh81bvaw84MwtgoaKUPGV9LxbQehaAQupB9HL8lwqklRx3GThGrNOa0TqP887lsH6 x0w1SeRlYO5EohNMCSqJqahknMzJNBuGaiy5NLC5iVA120GePkAJZ0Vx0EvQZ00WWIEW jcaE7cfkGETz0bXOmpbw2uaa0BCqn6se8jeqN5XsYkvjhCBEaHQJDV4czhdfF3M2XGhb L90Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:in-reply-to:content-disposition :mime-version:references:mail-followup-to:message-id:subject:cc:to :from:date; bh=xb+On1lvImUVf4eU+QnsRAifycIKRTwOPwx+zyjqE0U=; b=MpeDml5LVlr9cmcP/K60Tk/lNNDMua/I0E1Fd+mLTXIp+dd5cjCt+9AY2Zp41ol1w4 U66uiE02UP/bbvJN93Oz+8u0mDR2bi54k3EujfYdw3hycKyO3QDzJGkb3DLbgFHI12d9 Nq8PAJLhNv0p1xyFhwMXEaIOVONv94EezW53QY1JjzEXSA54Es67owRWj7OLtnxZ/oFO GCystCNQB8POEMPuUd1KSddRDIYvS3z4eMMiTMVbMxGgdwec4X9vHSt+NO3CIe7qkSIX O+DWRHwOAXskvZgQ/1WwGKhdyRr56ewdL7+b/PlioDAr5Syik6nj3bgoCnq7meaeEjba pw8Q== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 7si3298097oix.49.2020.02.12.09.10.40; Wed, 12 Feb 2020 09:10:52 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728931AbgBLRIn (ORCPT + 99 others); Wed, 12 Feb 2020 12:08:43 -0500 Received: from monster.unsafe.ru ([5.9.28.80]:37610 "EHLO mail.unsafe.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728098AbgBLRIm (ORCPT ); Wed, 12 Feb 2020 12:08:42 -0500 Received: from comp-core-i7-2640m-0182e6 (nat-pool-brq-t.redhat.com [213.175.37.10]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.unsafe.ru (Postfix) with ESMTPSA id 97063C61AB0; Wed, 12 Feb 2020 17:08:38 +0000 (UTC) Date: Wed, 12 Feb 2020 18:08:37 +0100 From: Alexey Gladkov To: "Eric W. Biederman" Cc: LKML , Kernel Hardening , Linux API , Linux FS Devel , Linux Security Module , Akinobu Mita , Alexander Viro , Alexey Dobriyan , Andrew Morton , Andy Lutomirski , Daniel Micay , Djalal Harouni , "Dmitry V . Levin" , Greg Kroah-Hartman , Ingo Molnar , "J . Bruce Fields" , Jeff Layton , Jonathan Corbet , Kees Cook , Linus Torvalds , Oleg Nesterov , Solar Designer Subject: Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances Message-ID: <20200212170836.kiqogl4cqdpyjjk3@comp-core-i7-2640m-0182e6> Mail-Followup-To: "Eric W. Biederman" , LKML , Kernel Hardening , Linux API , Linux FS Devel , Linux Security Module , Akinobu Mita , Alexander Viro , Alexey Dobriyan , Andrew Morton , Andy Lutomirski , Daniel Micay , Djalal Harouni , "Dmitry V . Levin" , Greg Kroah-Hartman , Ingo Molnar , "J . Bruce Fields" , Jeff Layton , Jonathan Corbet , Kees Cook , Linus Torvalds , Oleg Nesterov , Solar Designer References: <20200210150519.538333-1-gladkov.alexey@gmail.com> <20200210150519.538333-8-gladkov.alexey@gmail.com> <87v9odlxbr.fsf@x220.int.ebiederm.org> <20200212144921.sykucj4mekcziicz@comp-core-i7-2640m-0182e6> <87tv3vkg1a.fsf@x220.int.ebiederm.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87tv3vkg1a.fsf@x220.int.ebiederm.org> Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Feb 12, 2020 at 08:59:29AM -0600, Eric W. Biederman wrote: > Alexey Gladkov writes: > > > On Mon, Feb 10, 2020 at 07:36:08PM -0600, Eric W. Biederman wrote: > >> Alexey Gladkov writes: > >> > >> > This allows to flush dcache entries of a task on multiple procfs mounts > >> > per pid namespace. > >> > > >> > The RCU lock is used because the number of reads at the task exit time > >> > is much larger than the number of procfs mounts. > >> > >> A couple of quick comments. > >> > >> > Cc: Kees Cook > >> > Cc: Andy Lutomirski > >> > Signed-off-by: Djalal Harouni > >> > Suggested-by: Linus Torvalds > >> > Signed-off-by: Alexey Gladkov > >> > --- > >> > fs/proc/base.c | 20 +++++++++++++++----- > >> > fs/proc/root.c | 27 ++++++++++++++++++++++++++- > >> > include/linux/pid_namespace.h | 2 ++ > >> > include/linux/proc_fs.h | 2 ++ > >> > 4 files changed, 45 insertions(+), 6 deletions(-) > >> > > >> > diff --git a/fs/proc/base.c b/fs/proc/base.c > >> > index 4ccb280a3e79..24b7c620ded3 100644 > >> > --- a/fs/proc/base.c > >> > +++ b/fs/proc/base.c > >> > @@ -3133,7 +3133,7 @@ static const struct inode_operations proc_tgid_base_inode_operations = { > >> > .permission = proc_pid_permission, > >> > }; > >> > > >> > -static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid) > >> > +static void proc_flush_task_mnt_root(struct dentry *mnt_root, pid_t pid, pid_t tgid) > >> Perhaps just rename things like: > >> > +static void proc_flush_task_root(struct dentry *root, pid_t pid, pid_t tgid) > >> > { > >> > >> I don't think the mnt_ prefix conveys any information, and it certainly > >> makes everything longer and more cumbersome. > >> > >> > struct dentry *dentry, *leader, *dir; > >> > char buf[10 + 1]; > >> > @@ -3142,7 +3142,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid) > >> > 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); > >> > + dentry = d_hash_and_lookup(mnt_root, &name); > >> > if (dentry) { > >> > d_invalidate(dentry); > >> > dput(dentry); > >> > @@ -3153,7 +3153,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid) > >> > > >> > name.name = buf; > >> > name.len = snprintf(buf, sizeof(buf), "%u", tgid); > >> > - leader = d_hash_and_lookup(mnt->mnt_root, &name); > >> > + leader = d_hash_and_lookup(mnt_root, &name); > >> > if (!leader) > >> > goto out; > >> > > >> > @@ -3208,14 +3208,24 @@ void proc_flush_task(struct task_struct *task) > >> > int i; > >> > struct pid *pid, *tgid; > >> > struct upid *upid; > >> > + struct dentry *mnt_root; > >> > + struct proc_fs_info *fs_info; > >> > > >> > 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); > >> > + > >> > + rcu_read_lock(); > >> > + list_for_each_entry_rcu(fs_info, &upid->ns->proc_mounts, pidns_entry) { > >> > + mnt_root = fs_info->m_super->s_root; > >> > + proc_flush_task_mnt_root(mnt_root, upid->nr, tgid->numbers[i].nr); > >> > + } > >> > + rcu_read_unlock(); > >> > + > >> > + mnt_root = upid->ns->proc_mnt->mnt_root; > >> > + proc_flush_task_mnt_root(mnt_root, upid->nr, tgid->numbers[i].nr); > >> > >> I don't think this following of proc_mnt is needed. It certainly > >> shouldn't be. The loop through all of the super blocks should be > >> enough. > > > > Yes, thanks! > > > >> Once this change goes through. UML can be given it's own dedicated > >> proc_mnt for the initial pid namespace, and proc_mnt can be removed > >> entirely. > > > > After you deleted the old sysctl syscall we could probably do it. > > > >> Unless something has changed recently UML is the only other user of > >> pid_ns->proc_mnt. That proc_mnt really only exists to make the loop in > >> proc_flush_task easy to write. > > > > Now I think, is there any way to get rid of proc_mounts or even > > proc_flush_task somehow. > > > >> It also probably makes sense to take the rcu_read_lock() over > >> that entire for loop. > > > > Al Viro pointed out to me that I cannot use rcu locks here :( > > Fundamentally proc_flush_task is an optimization. Just getting rid of > dentries earlier. At least at one point it was an important > optimization because the old process dentries would just sit around > doing nothing for anyone. > > I wonder if instead of invalidating specific dentries we could instead > fire wake up a shrinker and point it at one or more instances of proc. > > The practical challenge I see is something might need to access the > dentries to see that they are invalid. > > We definitely could try without this optimization and see what happens. When Linus said that a semaphore for proc_mounts is a bad idea, I tried to come up with some kind of asynchronous way to clear it per superblock. I gave up with the asynchronous GC because userspace can quite easily get ahead of it. Without this optimization the kernel starts to consume a lot of memory during intensive reading /proc. I tried to do: while :; do for x in `seq 0 9`; do sleep 0.1; done; ls /proc/[0-9]*; done >/dev/null; and memory consumption went up without proc_flush_task. Since we have mounted procfs in each container, this is dangerous. -- Rgrds, legion