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.
