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@xxxxxxxxxxxxxxxxxx>
---
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)) {
