Re: [RFC] move_mount(2): still breakage around new mount detection

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

On Tue, Apr 29, 2025 at 09:56:14AM +0200, Christian Brauner wrote:
> On Tue, Apr 29, 2025 at 05:03:58AM +0100, Al Viro wrote:

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

Rule of the thumb: whenever you see a barrier, the need to understand
everything that's going on with the function goes up a _lot_.
Especially when one of those suckers is in a seriously hot codepath
(and __legitimize_mnt() qualifies).  I certainly agree that this one is
not critical - said so right in commit message, but one thing we need in
the area is to review the locking; not only the majority of comments are
badly obsolete (anything that mentions vfsmount_lock, for starters), but
we also have fun questions about the mount_lock users - which ones need
to touch seqcount component (and full lock_mount_hash()) and which ony
need the spinlock side.  Same for RCU delays - witness the thread that
got me started on reviewing the locking/refcounting/lifetimes in there.

Another piece of fun: there are grades of struct mount accessibility;
to a very limited extent it becomes reachable as soon as we put it into
the per-superblock list, even before the damn thing is returned to caller
of clone_mnt().  There's only one thing that can get to them that way -
sb_prepare_remount_readonly().  But that includes modifications of
->mnt_flags - setting and clearing MNT_WRITE_HOLD.  Which makes
CLEAR_MNT_SHARED(mnt) in clone_mnt() (as well as set_mnt_shared(mnt)
in CL_MAKE_SHARED case) racy, and not harmlessly so.  This one is
trivial to fix (set ->mnt_flags before attaching to superblock and
inserting into the list), but there are several places in similar spirit
where reordering doesn't solve the problem (e.g. fs/overlayfs/super.c
playing with the flags on clone_private_mnt() results).  I *really*
don't want to export mount_lock and only slightly less so - a generic
helper for adding to/removing from flags, so sane API needed in
this case is an interesting question...

I'm putting together documentation on struct mount, will post it
once it's reasonably complete.