Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances

Alexey Gladkov [off-list ref] writes:

On Mon, Feb 10, 2020 at 07:36:08PM -0600, Eric W. Biederman wrote:

quoted

Alexey Gladkov [off-list ref] writes:

quoted

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.

quoted

Cc: Kees Cook <redacted>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Djalal Harouni <redacted>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Alexey Gladkov <redacted>
---
 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:

quoted

+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.

quoted

 	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!

quoted

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.

quoted

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.

quoted

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

On Wed, Feb 12, 2020 at 08:59:29AM -0600, Eric W. Biederman wrote:

Alexey Gladkov [off-list ref] writes:

quoted

On Mon, Feb 10, 2020 at 07:36:08PM -0600, Eric W. Biederman wrote:

quoted

Alexey Gladkov [off-list ref] writes:

quoted

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.

quoted

Cc: Kees Cook <redacted>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Djalal Harouni <redacted>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Alexey Gladkov <redacted>
---
 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:

quoted

+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.

quoted

 	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!

quoted

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.

quoted

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.

quoted

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

On Wed, Feb 12, 2020 at 7:01 AM Eric W. Biederman [off-list ref] wrote:

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'm pretty sure it's still important. It's very easy to generate a
_ton_ of dentries with /proc.

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.

It shouldn't be the dentries themselves that are a freeing problem.
They're being RCU-free'd anyway because of lookup. It's the
proc_mounts list that is the problem, isn't it?

So it's just fs_info that needs to be rcu-delayed because it contains
that list. Or is there something else?

               Linus

On Wed, Feb 12, 2020 at 10:45:06AM -0800, Linus Torvalds wrote:

On Wed, Feb 12, 2020 at 7:01 AM Eric W. Biederman [off-list ref] wrote:

quoted

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'm pretty sure it's still important. It's very easy to generate a
_ton_ of dentries with /proc.

quoted

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.

It shouldn't be the dentries themselves that are a freeing problem.
They're being RCU-free'd anyway because of lookup. It's the
proc_mounts list that is the problem, isn't it?

So it's just fs_info that needs to be rcu-delayed because it contains
that list. Or is there something else?

Large part of the headache is the possibility that some joker has
done something like mounting tmpfs on /proc/<pid>/map_files, or
binding /dev/null on top of /proc/<pid>/syscall, etc.

IOW, that d_invalidate() can very well have to grab namespace_sem.
And possibly do a full-blown fs shutdown of something NFS-mounted,
etc...