Re: [PATCH][RFC] ->mnt_devname is never NULL

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

On Tue, Apr 22, 2025 at 09:31:14AM +0200, Christian Brauner wrote:
> On Mon, Apr 21, 2025 at 05:29:47PM +0100, Al Viro wrote:
> > On Mon, Apr 21, 2025 at 09:56:20AM +0200, Christian Brauner wrote:
> > > On Mon, Apr 21, 2025 at 04:35:09AM +0100, Al Viro wrote:
> > > > Not since 8f2918898eb5 "new helpers: vfs_create_mount(), fc_mount()"
> > > > back in 2018.  Get rid of the dead checks...
> > > >     
> > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > > > ---
> > > 
> > > Good idea. Fwiw, I've put this into vfs-6.16.mount with some other minor
> > > stuff. If you're keeping it yourself let me know.
> > 
> > Not sure...  I'm going through documenting the struct mount lifecycle/locking/etc.
> > and it already looks like there will be more patches, but then some are going
> > to be #fixes fodder.
> > 
> > Example caught just a couple of minutes ago: do_lock_mount()
> >                 if (beneath) {
> >                         m = real_mount(mnt);
> >                         read_seqlock_excl(&mount_lock);
> >                         dentry = dget(m->mnt_mountpoint);
> >                         read_sequnlock_excl(&mount_lock);
> >                 } else {
> >                         dentry = path->dentry;
> >                 }
> > 
> >                 inode_lock(dentry->d_inode);
> > What's to prevent the 'beneath' case from getting mnt mount --move'd
> > away *AND* the ex-parent from getting unmounted while we are blocked
> > in inode_lock?  At this point we are not holding any locks whatsoever
> > (and all mount-related locks nest inside inode_lock(), so we couldn't
> > hold them there anyway).
> > 
> > Hit that race and watch a very unhappy umount...
> 
> If it gets unmounted or moved we immediately detect this in the next line:
> 
> if (beneath && (!is_mounted(mnt) || m->mnt_mountpoint != dentry)) {

Sure, we would - *AFTER* we get through that inode_lock().

Consider the following setup:

mkdir foo
mkdir bar
mkdir splat
mount -t tmpfs none foo		# mount 1
mount -t tmpfs none bar		# mount 2
mkdir bar/baz
mount -t tmpfs none bar/baz	# mount 3

then

A: move_mount(AT_FDCWD, "foo", AT_FDCWD, "bar/baz", MOVE_MOUNT_BENEATH)
gets to do_move_mount() and into do_lock_mount() called by it.

path->mnt points to mount 3, path->dentry - to its root.  Both are pinned.
do_lock_mount() goes into the first iteration of loop.  beneath is true,
so it picks dentry - that of #3 mountpoint, i.e. "/baz" on #2 tmpfs instance.

At that point refcount of that dentry is 3 - one from being a positive on
tmpfs, one from being a mountpoint and one more just grabbed by do_lock_mount().

Now we enter inode_lock(dentry->d_inode).  Note that at that point A is not
holding any locks.  Suppose it gets preempted at this moment for whatever reason.

B: mount --move bar/baz splat
Proceeds without any problems, mount #3 gets moved to "splat".  Now refcount
of mount #2 is not pinned by anything and refcount of "/baz" on it is 2, since
it's no longer a mountpoint.

B: umount bar
... and now it hits the fan, since the refcount of mount #2 is not elevated by
anything, so we do not hit -EBUSY and proceed through umount(2) all the way to
kill_litter_super(), which drops the refcount of "/baz" to 1 and calls kill_anon_super().
Which gets to shrink_dcache_for_umount() and from there - to umount_check() on
that dentry.  You get yelled at, then you get yelled at again for busy inodes
after umount (that dentry is pinning the inode down), etc.  Superblock of #2
is freed.

A: regains CPU.  is_mounted() is true (now at splat instead of bar/baz, but still
mounted), ->mnt_mountpoint does not match.
All right, inode_unlock(dentry->d_inode), then dput(dentry) and now the refcount
of that dentry finally hits zero.  We get iput() on its inode, followed by
shmem_evict_inode() which is where we finally oops.

As for the second issue...  Normal callers of unlock_mount() do have a struct path
somewhere that pins the location we are dealing with.  However, 'beneath' case
of do_move_mount() does not - it relies upon the sucker being a mountpoint all
along.  Which is fine until you drop namespace_sem.  As soon as namespace_unlock()
has been called, there's no warranty that it will _stay_ a mountpoint.  Moving
that inode_unlock() before the namespace_unlock() avoids that scenario.