Kernel-hardening Archive on lore.kernel.org
 help / color / Atom feed
From: Alexey Gladkov <gladkov.alexey@gmail.com>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
	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 18:08:37 +0100
Message-ID: <20200212170836.kiqogl4cqdpyjjk3@comp-core-i7-2640m-0182e6> (raw)
In-Reply-To: <87tv3vkg1a.fsf@x220.int.ebiederm.org>

On Wed, Feb 12, 2020 at 08:59:29AM -0600, Eric W. Biederman wrote:
> 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.

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


  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
2020-02-12 17:08         ` Alexey Gladkov [this message]
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=20200212170836.kiqogl4cqdpyjjk3@comp-core-i7-2640m-0182e6 \
    --to=gladkov.alexey@gmail.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=ebiederm@xmission.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

Kernel-hardening Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kernel-hardening/0 kernel-hardening/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 kernel-hardening kernel-hardening/ https://lore.kernel.org/kernel-hardening \
		kernel-hardening@lists.openwall.com
	public-inbox-index kernel-hardening

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/com.openwall.lists.kernel-hardening


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