[PATCH 7/8] fs: don't let a migrating task hide its reference from do_umount()

[Christian Brauner <brauner@kernel.org>]

propagate_mount_busy() uses mnt_get_count() via do_refcount_check() to
figure out whether a synchronous umount may proceed or not by summing
the per-CPU mnt_count counter under mount_lock.

The fastpath for mntget() and mntput() doesn't require that lock. A task
that already holds a reference and takes another one on a cpu the loop
has summed can get migrated and drops that reference again on another
cpu. The loop might not have reached that cpu yet. That ends up
undercounting. The original reference of the task is missed and umount()
ends up succeeding with the file still open. The mount is destroyed
later when the task mntput()s so it degrades to a lazy umount.

That's basically the umount(2) variant of commit 9ea0a46ca2c3 ("fix
mntput/mntput race") closed for the final mntput(). The same
cpu-migration caused a UAF.

Luckily this isn't a UAF. It's just a hidden reference pinning the
mount but it's wrong nonetheless.

Like the other race it's a narrow window but I can reproduce it reliably
with some patching. The debug patch below prints the sum the loop
computed next to an immediate re-sum of the same counters. It reports
who drops the last reference to that mount later:

  --- a/fs/namespace.c
  +++ b/fs/namespace.c
  @@ -34,6 +34,8 @@
   #include <linux/mnt_idmapping.h>
   #include <linux/pidfs.h>
   #include <linux/nstree.h>
  +#include <linux/debugfs.h>
  +#include <linux/delay.h>

   #include "pnode.h"
   #include "internal.h"
  @@ -263,17 +265,48 @@
   #endif
   }

  +static u32 mnt_get_count_delay_ms;
  +static int mnt_get_count_debug_id;
  +
  +static int __init mnt_get_count_debug_init(void)
  +{
  +	debugfs_create_u32("mnt_get_count_delay_ms", 0600, NULL,
  +			   &mnt_get_count_delay_ms);
  +	return 0;
  +}
  +late_initcall(mnt_get_count_debug_init);
  +
   /*
    * vfsmount lock must be held for write
    */
   int mnt_get_count(struct mount *mnt)
   {
   #ifdef CONFIG_SMP
  -	int count = 0;
  +	static int seen[NR_CPUS];
  +	bool slow = mnt_get_count_delay_ms &&
  +		    !(mnt->mnt.mnt_flags & MNT_UMOUNT);
  +	int count = 0, again = 0;
   	int cpu;

   	for_each_possible_cpu(cpu) {
  -		count += per_cpu_ptr(mnt->mnt_pcp, cpu)->mnt_count;
  +		seen[cpu] = per_cpu_ptr(mnt->mnt_pcp, cpu)->mnt_count;
  +		count += seen[cpu];
  +		if (slow)
  +			mdelay(mnt_get_count_delay_ms);
  +	}
  +	if (slow) {
  +		for_each_possible_cpu(cpu) {
  +			int now;
  +
  +			now = per_cpu_ptr(mnt->mnt_pcp, cpu)->mnt_count;
  +			again += now;
  +			pr_err("mount %d: cpu%d read %d, now %d\n",
  +			       mnt->mnt_id, cpu, seen[cpu], now);
  +		}
  +		mnt_get_count_debug_id = mnt->mnt_id;
  +		pr_err("mount %d: sum %d, re-sum %d, %ps\n",
  +		       mnt->mnt_id, count, again,
  +		       __builtin_return_address(0));
   	}

   	return count;
  @@ -1350,6 +1383,9 @@
   	smp_mb();
   	mnt_add_count(mnt, -1);
   	count = mnt_get_count(mnt);
  +	if (count == 0 && mnt->mnt_id == mnt_get_count_debug_id)
  +		pr_err("mount %d: last put by %s (%d)\n", mnt->mnt_id,
  +		       current->comm, task_pid_nr(current));
   	if (count != 0) {
   		WARN_ON(count < 0);
   		rcu_read_unlock();

A task holds open a directory fd on a tmpfs. A synchronous rumount() of
the tmpfs sums up. Concurrently a task calls fchdir() on that fd on
CPU 0 and fchdir()s back to / on CPU 3. This will cause a path_get() and
a path_put() with a migration in between without mount_lock held.

Adding a delay per CPU makes the count loop miss the get in a cpu it
already visited but count the put in one it hasn't.

The reproducer, run as root with debugfs mounted:

  /*
   * gcc -static -o umount-torn umount-torn.c
   */
  #define _GNU_SOURCE
  #include <errno.h>
  #include <fcntl.h>
  #include <sched.h>
  #include <stdio.h>
  #include <string.h>
  #include <unistd.h>
  #include <sys/mount.h>
  #include <sys/stat.h>
  #include <sys/wait.h>

  #define KNOB "/sys/kernel/debug/mnt_get_count_delay_ms"
  #define MNT "/mnt/x"

  static int kmsg;

  static void pin(int cpu)
  {
  	cpu_set_t s;

  	CPU_ZERO(&s);
  	CPU_SET(cpu, &s);
  	sched_setaffinity(0, sizeof(s), &s);
  }

  static void knob(const char *v)
  {
  	int fd = open(KNOB, O_WRONLY);

  	write(fd, v, strlen(v));
  	close(fd);
  }

  int main(void)
  {
  	int last = sysconf(_SC_NPROCESSORS_ONLN) - 1;
  	int p[2], fd, ret;
  	pid_t pid;
  	char c;

  	kmsg = open("/dev/kmsg", O_WRONLY);
  	mkdir(MNT, 0755);
  	mount("none", MNT, "tmpfs", 0, NULL);
  	fd = open(MNT "/f", O_WRONLY | O_CREAT, 0644);
  	write(fd, "still here\n", 11);
  	close(fd);

  	pipe(p);
  	pid = fork();
  	if (pid == 0) {
  		int dirfd = open(MNT, O_RDONLY | O_DIRECTORY);
  		int rootfd = open("/", O_RDONLY | O_DIRECTORY);
  		struct stat st;
  		char buf[16];

  		/* one get/put pair while the umount is summing */
  		usleep(150000);
  		pin(0);
  		fchdir(dirfd);		/* mntget() */
  		dprintf(kmsg, "holder %d: get on cpu %d\n", getpid(),
  			sched_getcpu());
  		pin(last);
  		fchdir(rootfd);		/* mntput() */
  		dprintf(kmsg, "holder %d: put on cpu %d\n", getpid(),
  			sched_getcpu());

  		/* the umount has returned, is the fd still good? */
  		read(p[0], &c, 1);
  		fd = openat(dirfd, "f", O_RDONLY);
  		ret = read(fd, buf, sizeof(buf));
  		dprintf(kmsg, "holder %d: fstat %d openat %d read %d\n",
  			getpid(), fstat(dirfd, &st), fd, ret);
  		_exit(0);
  	}

  	pin(1);
  	knob("100");
  	ret = umount2(MNT, 0);
  	knob("0");
  	if (ret)
  		dprintf(kmsg, "umount: %s\n", strerror(errno));
  	else
  		dprintf(kmsg, "umount: 0\n");
  	write(p[1], "x", 1);
  	waitpid(pid, NULL, 0);
  	return 0;
  }

The log shows the busy check summing 2 and when read right after it
reads 3. The loop read CPU 0 before the get landed there and CPU 3
after the put did. The umount succeeds while there's still someone
with an fd. The mount is then dropped when the fd goes:

  [    2.511944] holder 90: get on cpu 0
  [    2.512023] holder 90: put on cpu 3
  [    2.671388] mount 28: cpu0 read 1, now 2
  [    2.671398] mount 28: cpu1 read 2, now 2
  [    2.671401] mount 28: cpu2 read 0, now 0
  [    2.671404] mount 28: cpu3 read -1, now -1
  [    2.671407] mount 28: sum 2, re-sum 3, propagate_mount_busy
  [    2.671773] umount: 0
  [    2.672963] holder 90: fstat 0 openat 8 read 11
  [    2.673663] mount 28: last put by umount-torn (90)

Easy fix would be to order mntget() against the loop that sums up. But
that's a full barrier and a flag test in mntget() afaict. No bueno...
That would cost every path_get().

So learn from SRCU. Keep gets and puts in separate per-cpu counters and
make mnt_get_count() sum all puts first and all gets second with a full
barrier in between.

A put is always after its own get (hopefully...) independent of the cpu
the task is on. So a put the first pass counted must have its get
visible to the second pass. Ergo, we can't undercount.

This is the two pass sum srcu_readers_active_idx_check() does for
srcu_locks and srcu_unlocks. mntget() keeps a single
this_cpu_inc(). mntput() gains an smp_wmb() before its increment, a
compiler barrier on x86 and a store fence on the weakly ordered
architectures, since the two counters are two words and a put stored
after a get could otherwise become visible before it. The counters
are unsigned so the difference stays right after they wrap.

The count can still come out high when a get is counted whose put
the first pass did not see. We don't care. A task can only take a new
reference if:

(1) it holds one already
(2) through __legitimize_mnt() which the seqcount orders
    against the write side

So a get in this window means someone has a reference and -EBUSY is
correct. The final mntput() is unchanged. Once ->mnt_ns is NULL every
put takes the slow path under mount_lock and so no put is in flight
while it counts. Here's an attempted proof for that crap:

(i) A put never becomes visible before its get. Within one task the
    smp_wmb() in mntput_no_expire() orders every earlier store before the
    increment of the puts.

    Any task that migrated in between had its stores flushed by the
    scheduler. When that reference is handed to another task that's
    trivially true.

(ii) The double count in mnt_get_count() is ordered via smp_mb(). That
     keeps every read of the puts counter before every read of the gets
     counter.

(iii) We never undercount by (ii).

(iv) There are no spurious counts/differences. A get is counted only if
     it happened and a put is left out only if it hasn't happened when
     the counter was read. With (iii) this means it's equivalent to the
     old count whenever nothing is taken or dropped during the loop.

(v) do_umount() holds a reference and mount_lock hence. New references
    come from: (v.a) __legitimize_mnt() managed by the seqcount. Either
    it's smp_mb() made the get visible to the loop or it takes the lock
    and waits for the loop. By (iii) a mount with a additional
    references is refused. By (iv) refusing the umount just means that
    any additional reference existed, same as the old counter.

(vi) mntput_no_expire_slowpath() runs with ->mnt_ns already NULL. Hence,
     every other put must take the slowpath as well and hangs on mount_lock.

    The grace period in namespace_unlock() will make every fast path do
    a put that still saw ->mnt_ns.

    No put is in flight during the loop.

    A get that is in flight comes from a task holding another reference.
    That must be included by the count or from __legitimize_mnt() as in
    (v).

    Ergo, zero means the last reference and non-zero means a reference
    is live.

(vii) may_umount() uses propagate_mount_busy() as well and
      may_umount_tree() sums mnt_get_count() itself.

With this patch the race is dead:

  [    3.066786] mount 28: sum 3, re-sum 3, propagate_mount_busy
  [    3.105867] umount: Device or resource busy
  [    3.106047] holder 90: fstat 0 openat 8 read 11

Fixes: b3e19d924b6e ("fs: scale mntget/mntput")
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/mount.h     |  3 ++-
 fs/namespace.c | 48 ++++++++++++++++++++++++++++++------------------
 2 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/fs/mount.h b/fs/mount.h
index 94fcc306d21e..85f136786bbc 100644
--- a/fs/mount.h
+++ b/fs/mount.h
@@ -33,7 +33,8 @@ struct mnt_namespace {
 } __randomize_layout;
 
 struct mnt_pcp {
-	int mnt_count;
+	unsigned int mnt_gets;
+	unsigned int mnt_puts;
 	int mnt_writers;
 };
 
diff --git a/fs/namespace.c b/fs/namespace.c
index d842fc1f1f1a..73215f84e04d 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -249,16 +249,24 @@ void mnt_release_group_id(struct mount *mnt)
 	mnt->mnt_group_id = 0;
 }
 
-/*
- * vfsmount lock must be held for read
- */
-static inline void mnt_add_count(struct mount *mnt, int n)
+static inline void mnt_inc_count(struct mount *mnt)
+{
+#ifdef CONFIG_SMP
+	this_cpu_inc(mnt->mnt_pcp->mnt_gets);
+#else
+	preempt_disable();
+	mnt->mnt_count++;
+	preempt_enable();
+#endif
+}
+
+static inline void mnt_dec_count(struct mount *mnt)
 {
 #ifdef CONFIG_SMP
-	this_cpu_add(mnt->mnt_pcp->mnt_count, n);
+	this_cpu_inc(mnt->mnt_pcp->mnt_puts);
 #else
 	preempt_disable();
-	mnt->mnt_count += n;
+	mnt->mnt_count--;
 	preempt_enable();
 #endif
 }
@@ -269,14 +277,17 @@ static inline void mnt_add_count(struct mount *mnt, int n)
 int mnt_get_count(struct mount *mnt)
 {
 #ifdef CONFIG_SMP
-	int count = 0;
+	unsigned int gets = 0, puts = 0;
 	int cpu;
 
-	for_each_possible_cpu(cpu) {
-		count += per_cpu_ptr(mnt->mnt_pcp, cpu)->mnt_count;
-	}
+	/* puts first, so a put counted here has its get counted below */
+	for_each_possible_cpu(cpu)
+		puts += per_cpu_ptr(mnt->mnt_pcp, cpu)->mnt_puts;
+	smp_mb();	/* pairs with the smp_wmb() in mntput_no_expire() */
+	for_each_possible_cpu(cpu)
+		gets += per_cpu_ptr(mnt->mnt_pcp, cpu)->mnt_gets;
 
-	return count;
+	return gets - puts;
 #else
 	return mnt->mnt_count;
 #endif
@@ -305,7 +316,7 @@ static struct mount *alloc_vfsmnt(const char *name)
 		if (!mnt->mnt_pcp)
 			goto out_free_devname;
 
-		this_cpu_add(mnt->mnt_pcp->mnt_count, 1);
+		this_cpu_inc(mnt->mnt_pcp->mnt_gets);
 #else
 		mnt->mnt_count = 1;
 		mnt->mnt_writers = 0;
@@ -746,13 +757,13 @@ int __legitimize_mnt(struct vfsmount *bastard, unsigned seq)
 	if (bastard == NULL)
 		return 0;
 	mnt = real_mount(bastard);
-	mnt_add_count(mnt, 1);
-	smp_mb();		// see mntput_no_expire() and do_umount()
+	mnt_inc_count(mnt);
+	smp_mb();	/* see mntput_no_expire_slowpath() and do_umount() */
 	if (likely(!read_seqretry(&mount_lock, seq)))
 		return 0;
 	lock_mount_hash();
 	if (unlikely(bastard->mnt_flags & (MNT_SYNC_UMOUNT | MNT_DOOMED))) {
-		mnt_add_count(mnt, -1);
+		mnt_dec_count(mnt);
 		unlock_mount_hash();
 		return 1;
 	}
@@ -1348,7 +1359,7 @@ static void noinline mntput_no_expire_slowpath(struct mount *mnt)
 	 * mount_lock, we'll see their refcount increment here.
 	 */
 	smp_mb();
-	mnt_add_count(mnt, -1);
+	mnt_dec_count(mnt);
 	count = mnt_get_count(mnt);
 	if (count != 0) {
 		WARN_ON(count < 0);
@@ -1405,7 +1416,8 @@ static void mntput_no_expire(struct mount *mnt)
 		 * non-NULL under rcu_read_lock(), the reference
 		 * we are dropping is not the final one.
 		 */
-		mnt_add_count(mnt, -1);
+		smp_wmb();	/* pairs with the smp_mb() in mnt_get_count() */
+		mnt_dec_count(mnt);
 		rcu_read_unlock();
 		return;
 	}
@@ -1427,7 +1439,7 @@ EXPORT_SYMBOL(mntput);
 struct vfsmount *mntget(struct vfsmount *mnt)
 {
 	if (mnt)
-		mnt_add_count(real_mount(mnt), 1);
+		mnt_inc_count(real_mount(mnt));
 	return mnt;
 }
 EXPORT_SYMBOL(mntget);

-- 
2.53.0