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

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

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...

BTW, a stylistic note: 'beneath' and '!beneath' cases have very little
in common; I'm pretty sure it would be cleaner to split this function
in two, putting the '!beneath' case back into lock_mount() and calling
the rest lock_mount_beneath()...  This kind of boolean arguments is
a bad idea, IME - especially when they affect locking or lifetimes
in any way.