On Mon, Aug 25, 2025 at 05:40:46AM +0100, Al Viro wrote:
> Most of this pile is basically an attempt to see how well do
> cleanup.h-style mechanisms apply in mount handling. That stuff lives in
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.mount
> Rebased to -rc3 (used to be a bit past -rc2, branched at mount fixes merge)
> Individual patches in followups.
>
> Please, help with review and testing. It seems to survive the
> local beating and code generation seems to be OK, but more testing
> would be a good thing and I would really like to see comments on that
> stuff.
Btw, I just realized that basically none of your commits have any lore
links in them. That kinda sucks because I very very often just look at a
commit and then use the link to jump to the mailing list discussion for
more context about a change and how it came about.
So pretty please can you start adding lore links to your commits when
applying if it's not fucking up your workflow too much?
>
> This is not all I've got around mount handling, but I'd rather
> get that thing out for review before starting to sort out other local
> mount-related branches.
>
> Series overview:
>
> Part 1: guards.
>
> This part starts with infrastructure, followed by one-by-one
> conversions to the guard/scoped_guard in some of the places that fit
> that well enough. Note that one of those places turned out to be taking
> mount_lock for no reason whatsoever; I already see places where we do
> write_seqlock when read_seqlock_excl would suffice, etc.
>
> Folks, _please_ don't do any bulk conversions in that area.
> IMO one area where RAII becomes dangerous is locking; usually it's not
> a big deal to delay freeing some object a bit, but delay dropping a
> lock and you risk introducing deadlocks that will be bloody hard to spot.
> It _has_ to be done carefully; we had trouble in that area several times
> over the last year or so in fs/namespace.c alone. Another fun problem
> is that quite a few comments regarding the locking in there are stale.
> We still have the comments that talk about mount lock as if it had been
> an rwlock-like thing. It hadn't been that for more than a decade now.
> It needs to be documented sanely; so do the access rules to the data
> structures involved. I hope to get some of that into the tree this cycle,
> but it's still in progress.
>
> 1/52) fs/namespace.c: fix the namespace_sem guard mess
> New guards: namespace_excl and namespace_shared. The former implies
> the latter, as for anything rwsem-like. No inode locks, no dropping the final
> references, no opening files, etc. in scope of those.
> 2/52) introduced guards for mount_lock
> New guards: mount_writer, mount_locked_reader. That's write_seqlock
> and read_seqlock_excl on mount_lock; obviously, nothing blocking should be
> done in scope of those.
> 3/52) fs/namespace.c: allow to drop vfsmount references via __free(mntput)
> Missing DEFINE_FREE (for mntput()); local in fs/namespace.c, to be
> used only for keeping shit out of namespace_... and mount_... scopes.
> 4/52) __detach_mounts(): use guards
> 5/52) __is_local_mountpoint(): use guards
> 6/52) do_change_type(): use guards
> 7/52) do_set_group(): use guards
> 8/52) mark_mounts_for_expiry(): use guards
> 9/52) put_mnt_ns(): use guards
> 10/52) mnt_already_visible(): use guards
> a bunch of clear-cut conversions, with explanations of the reasons
> why this or that guard is needed.
> 11/52) check_for_nsfs_mounts(): no need to take locks
> ... and here we have one where it turns out that locking had been
> excessive. Iterating through a subtree in mount_locked_reader scope is
> safe, all right, but (1) mount_writer is not needed here at all and (2)
> namespace_shared + a reference held to the root of subtree is also enough.
> All callers had (2) already. Documented the locking requirements for
> function, removed {,un}lock_mount_hash() in it...
> 12/52) propagate_mnt(): use scoped_guard(mount_locked_reader) for mnt_set_mountpoint()
> This one is interesting - existing code had been equivalent to
> scoped_guard(mount_locked_reader), and it's right for that call. However,
> mnt_set_mountpoint() generally requires mount_writer - the only reason we
> get away with that here is that the mount in question never had been
> reachable from the mounts visible to other threads.
> 13/52) has_locked_children(): use guards
> 14/52) mnt_set_expiry(): use guards
> 15/52) path_is_under(): use guards
> more clear-cut conversions with explanations.
> 16/52) current_chrooted(): don't bother with follow_down_one()
> 17/52) current_chrooted(): use guards
> this pair might be better off with #16 taken to the beginning
> of the series (or to a separate branch merge into this one); no better
> reason to do as I had than wanting to keep the guard infrastructure
> in the very beginning.
>
> Part 2: turning unlock_mount() into __cleanup.
>
> Environment for mounting something on given location consists of:
> 1) namespace_excl scope
> 2) parent mount - the one we'll be attaching things to.
> 3) mountpoint to be, protected from disappearing under us.
> 4) inode of that mountpoint's dentry held exclusive.
> Unfortunately, we can't take inode locks in namespace_excl scopes.
> And we want to cope with the possibility that somebody has managed to
> mount something on that place while we'd been taking locks. "Cope" part
> is simple for finish_automount() ("drop our mount and go away quietly;
> somebody triggered it before we did"), but for everything else it's
> trickier - "use whatever's overmounting that place now (with the right
> locks, please)".
> lock_mount() does all of that (do_lock_mount(), actually), with
> unlock_mount() closing the scope. And it's definitely a good candidate
> for __cleanup()-based approach, except that
> * the damn thing can return an error and conditional variants of that
> infrastructure are too revolting.
> * parent mount is returned in a fucking awful way - we modify the struct
> path passed to us as location to mount on and then its ->mnt is the parent
> to be... except for the "beneath" variant where we play convoluted games
> with "no, here we want the parent of that". Implementation is also
> vulnerable to umount propagtion races.
> * the structure we set up (everything except the parent) is inserted
> into a linked list by lock_mount(). That excludes DEFINE_CLASS() -
> it wants the value formed and then copied to the variable we are
> defining.
> * it contains an implicit namespace_excl scope, so path_put() and its
> ilk *must* be done after the unlock_mount(). And most of the users have
> gotos past that.
> The first two problems are solved by adding an explicit pointer
> to parent mount into struct pinned_mountpoint. Having lock_mount()
> failure reported by setting it to ERR_PTR(-E...) allows to avoid the
> problem with expressing the constructor failure. The third one is dealt
> with by defining local macros to be used instead of CLASS - I went with
> LOCK_MOUNT(mp, path) which defines struct pinned_mountpoint mp with
> __cleanup(unlock_mount) and sets it up. If anybody has better suggestions,
> I'll be glad to hear those.
> The last one is dealt with by massaging the users to form that
> would have all post-unlock_mount() stuff done by __free().
>
> First, several trivial cleanups:
> 18/52) do_move_mount(): trim local variables
> 19/52) do_move_mount(): deal with the checks on old_path early
> 20/52) move_mount(2): take sanity checks in 'beneath' case into do_lock_mount()
> 21/52) finish_automount(): simplify the ELOOP check
>
> Getting rid of post-unlock_mount() stuff:
> 22/52) do_loopback(): use __free(path_put) to deal with old_path
> 23/52) pivot_root(2): use __free() to deal with struct path in it
> 24/52) finish_automount(): take the lock_mount() analogue into a helper
> this one turns the open-coded logics into lock_mount_exact() with
> the same kind of calling conventions as lock_mount() and do_lock_mount()
> 25/52) do_new_mount_rc(): use __free() to deal with dropping mnt on failure
> 26/52) finish_automount(): use __free() to deal with dropping mnt on failure
>
> This is the main part:
> 27/52) change calling conventions for lock_mount() et.al.
>
> Followups, cleaning up the games with parent mount in the user:
> 28/52) do_move_mount(): use the parent mount returned by do_lock_mount()
> 29/52) do_add_mount(): switch to passing pinned_mountpoint instead of mountpoint + path
> 30/52) graft_tree(), attach_recursive_mnt() - pass pinned_mountpoint
>
> Part 3: getting rid of mutating struct path there.
>
> do_lock_mount() is still playing silly buggers with struct path it
> had been given - the logics in that thing hadn't changed. It's not a pretty
> function and it's racy as well; the thing is, by this point its users have
> almost no use for the changed contents of struct path - dentry can be derived
> from struct mountpoint, parent mount to use is provided directly and we
> want that a lot more than modified path->mnt. There's only one place
> (in can_move_mount_beneath()) where we still want that and it's not hard
> to reconstruct the value by *original* path->mnt value + parent mount to
> be used.
>
> Getting rid of ->dentry uses.
> 31/52) pivot_root(2): use old_mp.mp->m_dentry instead of old.dentry
> 32/52) don't bother passing new_path->dentry to can_move_mount_beneath()
>
> A helper, already open-coded in a couple of places; carved out of
> the next patch to keep it reasonably small
> 33/52) new helper: topmost_overmount()
>
> Rewrite of do_lock_mount() to keep path constant + trivial change
> in do_move_mount() to adjust the argument it passes to can_move_mount_beneath():
> 34/52) do_lock_mount(): don't modify path.
>
>
> Part 5: a bunch of trivial cleanups (mostly constifications)
>
> 35/52) constify check_mnt()
> 36/52) do_mount_setattr(): constify path argument
> 37/52) do_set_group(): constify path arguments
> 38/52) drop_collected_paths(): constify arguments
> 39/52) collect_paths(): constify the return value
> 40/52) do_move_mount(), vfs_move_mount(), do_move_mount_old(): constify struct path argument(s)
> 41/52) mnt_warn_timestamp_expiry(): constify struct path argument
> 42/52) do_new_mount{,_fc}(): constify struct path argument
> 43/52) do_{loopback,change_type,remount,reconfigure_mnt}(): constify struct path argument
> 44/52) path_mount(): constify struct path argument
> 45/52) may_copy_tree(), __do_loopback(): constify struct path argument
> 46/52) path_umount(): constify struct path argument
> 47/52) constify can_move_mount_beneath() arguments
> 48/52) do_move_mount_old(): use __free(path_put)
> 49/52) do_mount(): use __free(path_put)
>
> Part 6: assorted stuff, will grow.
>
> 50/52) umount_tree(): take all victims out of propagation graph at once
> [had been earlier]
> For each removed mount we need to calculate where the slaves
> will end up. To avoid duplicating that work, do it for all mounts to be
> removed at once, taking the mounts themselves out of propagation graph as
> we go, then do all transfers; the duplicate work on finding destinations
> is avoided since if we run into a mount that already had destination
> found, we don't need to trace the rest of the way. That's guaranteed
> O(removed mounts) for finding destinations and removing from propagation
> graph and O(surviving mounts that have master removed) for transfers.
>
> 51/52) ecryptfs: get rid of pointless mount references in ecryptfs dentries
> ->lower_path.mnt has the same value for all dentries on given
> ecryptfs instance and if somebody goes for mountpoint-crossing variant
> where that would not be true, we can deal with that when it happens
> (and _not_ with duplicating these reference into each dentry).
> As it is, we are better off just sticking a reference into
> ecryptfs-private part of superblock and keeping it pinned until
> ->kill_sb().
> That way we can stick a reference to underlying dentry right into
> ->d_fsdata of ecryptfs one, getting rid of indirection through struct
> ecryptfs_dentry_info, along with the entire struct ecryptfs_dentry_info
> machinery.
>
> 52/52) fs/namespace.c: sanitize descriptions for {__,}lookup_mnt()
> Comments regarding "shadow mounts" were stale - no such thing
> anymore. Document the locking requirements for __lookup_mnt()...
>
>
> FWIW, the current diffstat:
>
> fs/ecryptfs/dentry.c | 14 +-
> fs/ecryptfs/ecryptfs_kernel.h | 27 +-
> fs/ecryptfs/file.c | 15 +-
> fs/ecryptfs/inode.c | 19 +-
> fs/ecryptfs/main.c | 24 +-
> fs/internal.h | 4 +-
> fs/mount.h | 12 +
> fs/namespace.c | 775 +++++++++++++++++++-----------------------
> fs/pnode.c | 75 ++--
> fs/pnode.h | 1 +
> include/linux/mount.h | 4 +-
> kernel/audit_tree.c | 12 +-
> 12 files changed, 464 insertions(+), 518 deletions(-)
