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