Re: [PATCH 5/8] fs: don't silently unmount busy mounts

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

[My apologies for being MIA since July - had been sick; should be
back to normal now]

On Wed, Sep 23, 2026 at 02:27:57PM +0200, Christian Brauner wrote:
> The propagate_mount_busy() helper exists to decide whether a synchronous
> can suceed. It does that by looking at the copies of the victims in the
> mounts its parent propagates to. One of those functions from propagation
> hell.
> 
> It skips a copy that has child mounts except for the case of a single
> child mount covering the copy's root. That algorithm used to be exact.
> It isn't anymore.
> 
> Originally only childless copies were unmounted during propagation.
> And 1064f874abc0 ("mnt: Tuck mounts under others instead of creating
> shadow/side mounts.") ended up adding covered mounts to both sides in
> one go.
> 
> Along came 99b19d16471e ("mnt: In propgate_umount handle visiting mounts
> in any order") and that started to unmount more than before:
> 
> [1]: A copy of a mount is unmounted when each of its children is either
>      its overmount or a copy of the victim itself. In the former case
>      the overmount gets reparented. In the latter case the copy of the
>      victim will end up being unmounted.
> 
> So propagate_umount() ended up being rewritten without adjusting
> propagate_mount_busy(). And currently propagate_mount_busy() doesn't
> look at copies of the victim that have two or more children nor a single
> child that is not the overmount.
> 
> So any such copy in [1] is not checked for
> references. The result is that a synchronous unmount succeeds, pulling
> out a mount that is still in use.
> 
> A container can run into this with its own mounts. The host shares a
> tree with the container and the host has a mount in that tree:
> 
>   mount -t tmpfs none /mnt
>   mount --make-shared /mnt
nit: mkdir /mnt/a
>   mount -t tmpfs none /mnt/a
> 
> The container's /mnt is a slave copy of the host mount. The container
> now takes a detached copy of the tree and mounts that copy beneath its
> propagated copy of /mnt/a and keeps the file descriptor to the tree
> open:
> 
>   unshare -m --propagation unchanged
>   mount --make-rslave /
>   fd = open_tree(AT_FDCWD, "/mnt", OPEN_TREE_CLONE | AT_RECURSIVE)
>   move_mount(fd, "", AT_FDCWD, "/mnt/a", MOVE_MOUNT_BENEATH)
> 
>   grep /mnt /proc/self/mountinfo
>   775 355 ... /mnt ...     master:646
>   777 775 ... /mnt/a ...   master:646
>   776 777 ... /mnt/a ...   master:647
>   778 777 ... /mnt/a/a ... master:647
> 
> The now attached tree (777) is now mounted beneath the container's copy
> of /mnt/a. That's also where the host mount sits (as seen from the host
> mount namespace). So the propagated copy (776) of the host's mount is on
> top of its root and the copy of that (778) is inside it. Now the host
> runs:
> 
>   umount /mnt/a
> 
> and it succeeds. The moved tree tree and the copy inside of it are gone
> from the container. Now only the propagated copy is left and it got
> reparented to where it was before:
> 
>   grep /mnt /proc/self/mountinfo
>   775 355 ... /mnt ... master:646
>   776 775 ... /mnt/a ...
> 
> But the file descriptor still refers to the moved tree. So a synchronous
> umount detached a mount that is still in use.
> 
> Teach propagate_mount_busy() the same checks that propagate_umount()
> does. It now checks for a single victim mount without children
> for which it can assume that MNT_LOCKED is cleared on every copy like
> propagate_umount() does.

Huh?  propagate_umount() does *not* assume anything of that sort - look for
those IS_MNT_LOCKED() in there.

> The copies form chains. IOW, when a copy itself
> receives propagation from the victim's parent then the mount at the same
> mountpoint inside that copy is a copy as well. Whether a mount receives
> propagation from the victim's can simply be checked by walking its
> masters. Walk each chain once from its top so we are sure that nested
> copies stay linear. Check every reference once of every copy that gets
> unmounted.

Explanation is somewhat misleading here, IMO...  Sure, all candidate
victims have identical mountpoints, so they must have distinct parents,
etc., but "copy" is not a good word here - all of them *are* copies of
copies of the same mount, but the history of copying might have nothing
to do with your chains.

	FWIW, there are two related bugs here; one is the horror with
MOVE_MOUNT_BENEATH (and the more I'm thinking of interplay between that
and propagation, the less I like the entire situation).  Another does
not involve MOVE_MOUNT_BENEATH at all - 

; mount --type tmpfs none /mnt    # M0
; mount --make-private /mnt
; mount --make-shared /mnt
; mkdir /mnt/a
; mkdir /mnt/b
; mount --bind /mnt /mnt/a        # M1 = M0/a
; mount --make-shared /mnt/a
; mount --bind /mnt/a /mnt/b      # M2 = M0/b
; mount --make-slave /mnt/a
; mount --make-slave /mnt/b
; umount /mnt/a/b
; mount --bind /mnt/b /mnt/b/a    # M3 = M2/a
; mount --bind /mnt/b /mnt/b/a/a  # M4 = M3/a
; cd /mnt/b/a			# M3 is busy, isn't it?
; /bin/pwd
/mnt/b/a
; umount /mnt/a			# takes out M1, M3 and M4
; /bin/pwd
pwd: couldn't find directory entry in '..' with matching i-node

What happens is that propagate_mount_busy() sees that M3 has something
mounted _not_ on its root and decides that propagate_umount() will not
take it out.  M1 and M4 are not busy, so we proceed to propagate_umount(),
which notices what propagate_mount_busy() hadn't - the thing mounted on
M3/a is something we are taking out, so M3 is not pinned by remaining
mounts...

I'm not fond of the patch itself, TBH - the amount of work done under
mount_lock would better be linear by the number of mounts involved and
your variant is at least quadratic.  That's not hard to fix, but that'll
obviously have to wait - Saturday after -rc7 is not a good time for
that.