On Tue, Apr 29, 2025 at 05:03:58AM +0100, Al Viro wrote:
> 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...
Most of these issues are almost impossible to hit in real workloads or
it's so rare that it doesn't matter. This one in particular seems like a
really uninteresting one. I mean, yes we should probably add that
barrier there but also nobody would care if we didn't.
>
> 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")
That's an ancient bug at that...
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> ---
Reviewed-by: Christian Brauner <brauner@xxxxxxxxxx>
> 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)) {
