Re: [RFC] move_mount(2): still breakage around new mount detection

[Al Viro <viro@zeniv.linux.org.uk>]

On Mon, Apr 28, 2025 at 07:53:18PM +0100, Al Viro wrote:

> FWIW, I've a series of cleanups falling out of audit of struct mount
> handling; it's still growing, but I'll post the stable parts for review
> tonight or tomorrow...

_Another_ fun one, this time around do_umount().  Take a look
at this chunk in mntput_no_expire():
        lock_mount_hash();
	/*
	 * make sure that if __legitimize_mnt() has not seen us grab
	 * mount_lock, we'll see their refcount increment here.
	 */
	smp_mb();
	mnt_add_count(mnt, -1);
	count = mnt_get_count(mnt);

... and note that we do *not* have such a barrier in do_umount(), between
        lock_mount_hash();
and
                shrink_submounts(mnt);
		retval = -EBUSY;
		if (!propagate_mount_busy(mnt, 2)) {
making it possible to __legitimize_mnt() fail to see lock_mount_hash() in
do_umount(), with do_umount() not noticing the increment of refcount done
by __legitimize_mnt().  It is considerably harder to hit, but I wouldn't
bet on it being impossible...

The sky is not falling (the worst we'll get is a successful sync umount(2)
ending up like a lazy one would; sucks if you see that umount(2) has succeeded
and e.g. pull a USB stick out, of course, but...)

But AFAICS we need a barrier here, to make sure that either legitimize_mnt()
fails seqcount check, grabs mount_lock, sees MNT_SYNC_UMOUNT and quitely
decrements refcount and buggers off or umount(2) sees the increment in
legitimize_mnt() and fails with -EBUSY.

It's really the same situation as with mntput_no_expire(), except that
there the corresponding flag is MNT_DOOMED...

[PATCH] do_umount(): add missing barrier before refcount checks in sync case
    
do_umount() analogue of the race fixed in 119e1ef80ecf "fix
__legitimize_mnt()/mntput() race".  Here we want to make sure that
if __legitimize_mnt() doesn't notice our lock_mount_hash(), we will
notice their refcount increment.  Harder to hit than mntput_no_expire()
one, fortunately, and consequences are milder (sync umount acting
like umount -l on a rare race with RCU pathwalk hitting at just the
wrong time instead of use-after-free galore mntput_no_expire()
counterpart used to be hit).  Still a bug...
 
Fixes: 48a066e72d97 ("RCU'd vsfmounts")
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/namespace.c b/fs/namespace.c
index eba4748388b1..d8a344d0a80a 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -787,7 +787,7 @@ int __legitimize_mnt(struct vfsmount *bastard, unsigned seq)
 		return 0;
 	mnt = real_mount(bastard);
 	mnt_add_count(mnt, 1);
-	smp_mb();			// see mntput_no_expire()
+	smp_mb();		// see mntput_no_expire() and do_umount()
 	if (likely(!read_seqretry(&mount_lock, seq)))
 		return 0;
 	lock_mount_hash();
@@ -2044,6 +2044,7 @@ static int do_umount(struct mount *mnt, int flags)
 			umount_tree(mnt, UMOUNT_PROPAGATE);
 		retval = 0;
 	} else {
+		smp_mb(); // paired with __legitimize_mnt()
 		shrink_submounts(mnt);
 		retval = -EBUSY;
 		if (!propagate_mount_busy(mnt, 2)) {