From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 27E56C2BA83 for ; Wed, 12 Feb 2020 15:01:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id F286C20714 for ; Wed, 12 Feb 2020 15:01:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728446AbgBLPBc (ORCPT ); Wed, 12 Feb 2020 10:01:32 -0500 Received: from out01.mta.xmission.com ([166.70.13.231]:45584 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727231AbgBLPBc (ORCPT ); Wed, 12 Feb 2020 10:01:32 -0500 Received: from in02.mta.xmission.com ([166.70.13.52]) by out01.mta.xmission.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1j1tW3-00061z-6s; Wed, 12 Feb 2020 08:01:27 -0700 Received: from ip68-227-160-95.om.om.cox.net ([68.227.160.95] helo=x220.xmission.com) by in02.mta.xmission.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.87) (envelope-from ) id 1j1tW0-0002sE-6R; Wed, 12 Feb 2020 08:01:27 -0700 From: ebiederm@xmission.com (Eric W. Biederman) To: LKML Cc: 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> Date: Wed, 12 Feb 2020 08:59:29 -0600 In-Reply-To: <20200212144921.sykucj4mekcziicz@comp-core-i7-2640m-0182e6> (Alexey Gladkov's message of "Wed, 12 Feb 2020 15:49:21 +0100") Message-ID: <87tv3vkg1a.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1j1tW0-0002sE-6R;;;mid=<87tv3vkg1a.fsf@x220.int.ebiederm.org>;;;hst=in02.mta.xmission.com;;;ip=68.227.160.95;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX1/y0OtQ5QQujeXudLM/V7FEvgZrY+iG/uQ= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org 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. Eric