Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: LKML <linux-kernel@vger.kernel.org>
Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>,
	Linux API <linux-api@vger.kernel.org>,
	Linux FS Devel <linux-fsdevel@vger.kernel.org>,
	Linux Security Module <linux-security-module@vger.kernel.org>,
	Akinobu Mita <akinobu.mita@gmail.com>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Alexey Dobriyan <adobriyan@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Andy Lutomirski <luto@kernel.org>,
	Daniel Micay <danielmicay@gmail.com>,
	Djalal Harouni <tixxdz@gmail.com>,
	"Dmitry V . Levin" <ldv@altlinux.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Ingo Molnar <mingo@kernel.org>,
	"J . Bruce Fields" <bfields@fieldses.org>,
	Jeff Layton <jlayton@poochiereds.net>,
	Jonathan Corbet <corbet@lwn.net>,
	Kees Cook <keescook@chromium.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Oleg Nesterov <oleg@redhat.com>,
	Solar Designer <solar@openwall.com>
Subject: Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
Date: Wed, 12 Feb 2020 08:59:29 -0600
Message-ID: <87tv3vkg1a.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <20200212144921.sykucj4mekcziicz@comp-core-i7-2640m-0182e6> (Alexey Gladkov's message of "Wed, 12 Feb 2020 15:49:21 +0100")

Alexey Gladkov <gladkov.alexey@gmail.com> writes:

> On Mon, Feb 10, 2020 at 07:36:08PM -0600, Eric W. Biederman wrote:
>> Alexey Gladkov <gladkov.alexey@gmail.com> 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 <keescook@chromium.org>
>> > Cc: Andy Lutomirski <luto@kernel.org>
>> > Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
>> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>> > Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
>> > ---
>> >  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


  reply index

Thread overview: 85+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-10 15:05 [PATCH v8 00/11] proc: modernize proc to support multiple private instances Alexey Gladkov
2020-02-10 15:05 ` [PATCH v8 01/11] proc: Rename struct proc_fs_info to proc_fs_opts Alexey Gladkov
2020-02-10 15:05 ` [PATCH v8 02/11] proc: add proc_fs_info struct to store proc information Alexey Gladkov
2020-02-10 15:05 ` [PATCH v8 03/11] proc: move /proc/{self|thread-self} dentries to proc_fs_info Alexey Gladkov
2020-02-10 18:23   ` Andy Lutomirski
2020-02-12 15:00     ` Alexey Gladkov
2020-02-10 15:05 ` [PATCH v8 04/11] proc: move hide_pid, pid_gid from pid_namespace " Alexey Gladkov
2020-02-10 15:05 ` [PATCH v8 05/11] proc: add helpers to set and get proc hidepid and gid mount options Alexey Gladkov
2020-02-10 18:30   ` Andy Lutomirski
2020-02-12 14:57     ` Alexey Gladkov
2020-02-10 15:05 ` [PATCH v8 06/11] proc: support mounting procfs instances inside same pid namespace Alexey Gladkov
2020-02-10 15:05 ` [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances Alexey Gladkov
2020-02-10 17:46   ` Linus Torvalds
2020-02-10 19:23     ` Al Viro
2020-02-11  1:36   ` Eric W. Biederman
2020-02-11  4:01     ` Eric W. Biederman
2020-02-12 14:49     ` Alexey Gladkov
2020-02-12 14:59       ` Eric W. Biederman [this message]
2020-02-12 17:08         ` Alexey Gladkov
2020-02-12 18:45         ` Linus Torvalds
2020-02-12 19:16           ` Eric W. Biederman
2020-02-12 19:49             ` Linus Torvalds
2020-02-12 20:03               ` Al Viro
2020-02-12 20:35                 ` Linus Torvalds
2020-02-12 20:38                   ` Al Viro
2020-02-12 20:41                     ` Al Viro
2020-02-12 21:02                       ` Linus Torvalds
2020-02-12 21:46                         ` Eric W. Biederman
2020-02-13  0:48                           ` Linus Torvalds
2020-02-13  4:37                             ` Eric W. Biederman
2020-02-13  5:55                               ` Al Viro
2020-02-13 21:30                                 ` Linus Torvalds
2020-02-13 22:23                                   ` Al Viro
2020-02-13 22:47                                     ` Linus Torvalds
2020-02-14 14:15                                       ` Eric W. Biederman
2020-02-14  3:48                                 ` Eric W. Biederman
2020-02-20 20:46                               ` [PATCH 0/7] proc: Dentry flushing without proc_mnt Eric W. Biederman
2020-02-20 20:47                                 ` [PATCH 1/7] proc: Rename in proc_inode rename sysctl_inodes sibling_inodes Eric W. Biederman
2020-02-20 20:48                                 ` [PATCH 2/7] proc: Generalize proc_sys_prune_dcache into proc_prune_siblings_dcache Eric W. Biederman
2020-02-20 20:49                                 ` [PATCH 3/7] proc: Mov rcu_read_(lock|unlock) in proc_prune_siblings_dcache Eric W. Biederman
2020-02-20 22:33                                   ` Linus Torvalds
2020-02-20 20:49                                 ` [PATCH 4/7] proc: Use d_invalidate " Eric W. Biederman
2020-02-20 22:43                                   ` Linus Torvalds
2020-02-20 22:54                                   ` Al Viro
2020-02-20 23:00                                     ` Linus Torvalds
2020-02-20 23:03                                     ` Al Viro
2020-02-20 23:39                                       ` Eric W. Biederman
2020-02-20 20:51                                 ` [PATCH 5/7] proc: Clear the pieces of proc_inode that proc_evict_inode cares about Eric W. Biederman
2020-02-20 20:52                                 ` [PATCH 6/7] proc: Use a list of inodes to flush from proc Eric W. Biederman
2020-02-20 20:52                                 ` [PATCH 7/7] proc: Ensure we see the exit of each process tid exactly once Eric W. Biederman
2020-02-21 16:50                                   ` Oleg Nesterov
2020-02-22 15:46                                     ` Eric W. Biederman
2020-02-20 23:02                                 ` [PATCH 0/7] proc: Dentry flushing without proc_mnt Linus Torvalds
2020-02-20 23:07                                   ` Al Viro
2020-02-20 23:37                                     ` Eric W. Biederman
2020-02-24 16:25                                 ` [PATCH v2 0/6] " Eric W. Biederman
2020-02-24 16:26                                   ` [PATCH v2 1/6] proc: Rename in proc_inode rename sysctl_inodes sibling_inodes Eric W. Biederman
2020-02-24 16:27                                   ` [PATCH v2 2/6] proc: Generalize proc_sys_prune_dcache into proc_prune_siblings_dcache Eric W. Biederman
2020-02-24 16:27                                   ` [PATCH v2 3/6] proc: In proc_prune_siblings_dcache cache an aquired super block Eric W. Biederman
2020-02-24 16:28                                   ` [PATCH v2 4/6] proc: Use d_invalidate in proc_prune_siblings_dcache Eric W. Biederman
2020-02-24 16:28                                   ` [PATCH v2 5/6] proc: Clear the pieces of proc_inode that proc_evict_inode cares about Eric W. Biederman
2020-02-24 16:29                                   ` [PATCH v2 6/6] proc: Use a list of inodes to flush from proc Eric W. Biederman
2020-02-28 20:17                                   ` [PATCH 0/3] proc: Actually honor the mount options Eric W. Biederman
2020-02-28 20:18                                     ` [PATCH 1/3] uml: Don't consult current to find the proc_mnt in mconsole_proc Eric W. Biederman
2020-02-28 20:18                                     ` [PATCH 2/3] uml: Create a private mount of proc for mconsole Eric W. Biederman
2020-02-28 20:30                                       ` Christian Brauner
2020-02-28 21:28                                         ` Eric W. Biederman
2020-02-28 21:59                                           ` Christian Brauner
2020-02-28 20:19                                     ` [PATCH 3/3] proc: Remove the now unnecessary internal mount of proc Eric W. Biederman
2020-02-28 20:39                                       ` Christian Brauner
2020-02-28 21:40                                         ` Eric W. Biederman
2020-02-28 22:34                                     ` [PATCH 4/3] pid: Improve the comment about waiting in zap_pid_ns_processes Eric W. Biederman
2020-02-29  2:59                                       ` Christian Brauner
2020-02-14  3:49                     ` [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances Eric W. Biederman
2020-02-12 19:47           ` Al Viro
2020-02-11 22:45   ` Al Viro
2020-02-12 14:26     ` Alexey Gladkov
2020-02-10 15:05 ` [PATCH v8 08/11] proc: instantiate only pids that we can ptrace on 'hidepid=4' mount option Alexey Gladkov
2020-02-10 16:29   ` Jordan Glover
2020-02-12 14:34     ` Alexey Gladkov
2020-02-10 15:05 ` [PATCH v8 09/11] proc: add option to mount only a pids subset Alexey Gladkov
2020-02-10 15:05 ` [PATCH v8 10/11] docs: proc: add documentation for "hidepid=4" and "subset=pidfs" options and new mount behavior Alexey Gladkov
2020-02-10 18:29   ` Andy Lutomirski
2020-02-12 16:03     ` Alexey Gladkov
2020-02-10 15:05 ` [PATCH v8 11/11] proc: Move hidepid values to uapi as they are user interface to mount Alexey Gladkov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87tv3vkg1a.fsf@x220.int.ebiederm.org \
    --to=ebiederm@xmission.com \
    --cc=adobriyan@gmail.com \
    --cc=akinobu.mita@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=bfields@fieldses.org \
    --cc=corbet@lwn.net \
    --cc=danielmicay@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jlayton@poochiereds.net \
    --cc=keescook@chromium.org \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=ldv@altlinux.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mingo@kernel.org \
    --cc=oleg@redhat.com \
    --cc=solar@openwall.com \
    --cc=tixxdz@gmail.com \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git