[RFC] MNT_WRITE_HOLD mess

[RFC] MNT_WRITE_HOLD mess

[Al Viro]

	Scalable handling of writers count on a mount had been added
back in 2009, when the counter got split into per-cpu components.
The tricky part, of course, was on the "make it read-only" side and the
way it had been done was to add a flag for "we are adding that shit up,
feel free to increment, but don't get through the mnt_get_write_access()
until we are finished".

	Details are in mnt_get_write_access() and mnt_{,un}hold_writers().
Current rules:
	* mnt_hold_writers()/mnt_unhold_writers() should be in the same
mount_lock scope.
	* any successful mnt_hold_writers() must be followed by mnt_unhold_writers()
within the same scope.
	* mnt_get_write_access() *MAY* take and release mount_lock, but only
if there's somebody currently playing with mnt_hold_writers() on the same mount.

	The non-obvious trouble came in 2011 (in 4ed5e82fe77f "vfs: protect
remounting superblock read-only") when we got sb_prepare_remount_readonly(),
esssentially doing mnt_hold_writers() for each mount over given superblock.
I hadn't realized the implications until this year ;-/

	The trouble is, as soon as mount has been added to ->s_mounts
(currently in vfs_create_mount() and clone_mnt()) it becomes accessible
to other threads, even if only in a very limited way.

	That breaks very natural assumptions, just lightly enough to make
the resulting races hard to detect.  Note, for example, this in ovl_get_upper():
	upper_mnt = clone_private_mount(upperpath);
	err = PTR_ERR(upper_mnt);
	if (IS_ERR(upper_mnt)) {
		pr_err("failed to clone upperpath\n");
		goto out;
	}

	/* Don't inherit atime flags */
	upper_mnt->mnt_flags &= ~(MNT_NOATIME | MNT_NODIRATIME | MNT_RELATIME);
See the problem?  upper_mnt has been returned by clone_private_mount() and at
the moment it has no references to it anywhere other than this local variable.
It is not a part of any mount tree, it is not hashed, it is not reachable via
propagation graph, it is not on any expiry lists, etc.  Except that it *is*
on ->s_mounts of underlying superblock (linked via ->mnt_instance) and should
anybody try to call sb_prepare_remount_readonly(), we may end up fucking
the things up in all kinds of interesting ways - e.g.
	mnt_hold_writers() sets MNT_WRITE_HOLD
	we fetch ->mnt_flags
	mnt_unhold_writers() clears MNT_WRITE_HOLD
	we remove the atime bits and store ->mnt_flags, bringing MNT_WRITE_HOLD
back.  Makes for a very unhappy mnt_get_write_access() down the road...

	The races are real.  Some of them can be dealt with by grabbing
mount_lock around the problematic modifications of ->mnt_flags, but that's
not an option outside of fs/namespace.c - I really don't want to export
mount_lock *or* provide a "clear these flags and set these ones" exported
primitive.

	In any case, the underlying problem is that we have this state
of struct mount when it's almost, but not entirely thread-local.  That's
likely to cause future bugs of the same sort.

	I'd been playing with delaying the insertion into ->s_mounts
and making mnt_get_write_access() fail and complain if mount hasn't been
inserted yet.  It's... doable, but not pleasant.  However, there's
another approach that might be better.

	We have ->mnt_instance, which is used only under mount_lock.
We have two places where we insert, one place where we remove and
two places (both in sb_prepare_remount_readonly()) where we iterate
through the list.

	What if we steal LSB of ->mnt_instance.prev for what MNT_WRITE_HOLD
is currently used for?  list_for_each_entry() won't give a damn;
list_del() and list_add_tail() are called in mount_lock scopes that do
not contain any calls of mnt_hold_writers(), so they'll see that LSB
clear and work as usual.  Loop in mnt_get_write_access() could just
as easily do
        while ((unsigned long)READ_ONCE(mnt->mnt_instance.prev) & 1) {
as the current
        while (READ_ONCE(mnt->mnt.mnt_flags) & MNT_WRITE_HOLD) {
and no barriers need to be changed.  We might want to move
->mnt_instance closer to ->mnt or to ->mnt_pcp, for the sake of
the number of cachelines we are accessing, but then ->mnt_instance
is pretty close to ->mnt_pcp as it is.

	Something like the delta below, modulo stale comments, etc.
AFAICS, that removes this "slightly exposed" state, along with the
races caused by it.  This is on top of mainline and it should be
easy to backport.

	Comments?

diff --git a/fs/namespace.c b/fs/namespace.c
index 54c59e091919..7e3145baac8f 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -506,7 +506,7 @@ int mnt_get_write_access(struct vfsmount *m)
 	 */
 	smp_mb();
 	might_lock(&mount_lock.lock);
-	while (READ_ONCE(mnt->mnt.mnt_flags) & MNT_WRITE_HOLD) {
+	while ((unsigned long)READ_ONCE(mnt->mnt_instance.prev) & 1) {
 		if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
 			cpu_relax();
 		} else {
 			preempt_enable();
-			lock_mount_hash();
-			unlock_mount_hash();
+			read_seqlock_excl(&mount_lock);
+			read_sequnlock_excl(&mount_lock);
 			preempt_disable();
 		}
 	}
@@ -650,6 +650,11 @@ void mnt_drop_write_file(struct file *file)
 }
 EXPORT_SYMBOL(mnt_drop_write_file);
 
+static inline unsigned long __mnt_writers_held(const struct mount *m)
+{
+	return (unsigned long)m->mnt_instance.prev;
+}
+
 /**
  * mnt_hold_writers - prevent write access to the given mount
  * @mnt: mnt to prevent write access to
@@ -670,7 +675,7 @@ EXPORT_SYMBOL(mnt_drop_write_file);
  */
 static inline int mnt_hold_writers(struct mount *mnt)
 {
-	mnt->mnt.mnt_flags |= MNT_WRITE_HOLD;
+	mnt->mnt_instance.prev = (void *)(__mnt_writers_held(mnt) | 1);
 	/*
 	 * After storing MNT_WRITE_HOLD, we'll read the counters. This store
 	 * should be visible before we do.
@@ -718,7 +723,7 @@ static inline void mnt_unhold_writers(struct mount *mnt)
 	 * that become unheld will see MNT_READONLY.
 	 */
 	smp_wmb();
-	mnt->mnt.mnt_flags &= ~MNT_WRITE_HOLD;
+	mnt->mnt_instance.prev = (void *)(__mnt_writers_held(mnt) & ~1);
 }
 
 static int mnt_make_readonly(struct mount *mnt)
@@ -755,8 +760,9 @@ int sb_prepare_remount_readonly(struct super_block *sb)
 	if (!err)
 		sb_start_ro_state_change(sb);
 	list_for_each_entry(mnt, &sb->s_mounts, mnt_instance) {
-		if (mnt->mnt.mnt_flags & MNT_WRITE_HOLD)
-			mnt->mnt.mnt_flags &= ~MNT_WRITE_HOLD;
+		unsigned long v = __mnt_writers_held(mnt);
+		if (v & 1)
+			mnt->mnt_instance.prev = (void *)(v & ~1);
 	}
 	unlock_mount_hash();
 
@@ -1349,7 +1355,7 @@ static struct mount *clone_mnt(struct mount *old, struct dentry *root,
 	}
 
 	mnt->mnt.mnt_flags = old->mnt.mnt_flags;
-	mnt->mnt.mnt_flags &= ~(MNT_WRITE_HOLD|MNT_MARKED|MNT_INTERNAL);
+	mnt->mnt.mnt_flags &= ~(MNT_MARKED|MNT_INTERNAL);
 
 	atomic_inc(&sb->s_active);
 	mnt->mnt.mnt_idmap = mnt_idmap_get(mnt_idmap(&old->mnt));
@@ -4959,7 +4965,7 @@ static int mount_setattr_prepare(struct mount_kattr *kattr, struct mount *mnt)
 		 */
 		for (p = mnt; p; p = next_mnt(p, mnt)) {
 			/* If we had to hold writers unblock them. */
-			if (p->mnt.mnt_flags & MNT_WRITE_HOLD)
+			if (__mnt_writers_held(mnt) & 1)
 				mnt_unhold_writers(p);
 
 			/*
@@ -4999,7 +5005,7 @@ static void mount_setattr_commit(struct mount_kattr *kattr, struct mount *mnt)
 		WRITE_ONCE(m->mnt.mnt_flags, flags);
 
 		/* If we had to hold writers unblock them. */
-		if (m->mnt.mnt_flags & MNT_WRITE_HOLD)
+		if (__mnt_writers_held(mnt) & 1)
 			mnt_unhold_writers(m);
 
 		if (kattr->propagation)
diff --git a/include/linux/mount.h b/include/linux/mount.h
index 1a508beba446..1728bc50d02b 100644
--- a/include/linux/mount.h
+++ b/include/linux/mount.h
@@ -33,7 +33,6 @@ enum mount_flags {
 	MNT_NOSYMFOLLOW	= 0x80,
 
 	MNT_SHRINKABLE	= 0x100,
-	MNT_WRITE_HOLD	= 0x200,
 
 	MNT_SHARED	= 0x1000, /* if the vfsmount is a shared mount */
 	MNT_UNBINDABLE	= 0x2000, /* if the vfsmount is a unbindable mount */
@@ -64,7 +63,7 @@ enum mount_flags {
 				  | MNT_READONLY | MNT_NOSYMFOLLOW,
 	MNT_ATIME_MASK = MNT_NOATIME | MNT_NODIRATIME | MNT_RELATIME,
 
-	MNT_INTERNAL_FLAGS = MNT_SHARED | MNT_WRITE_HOLD | MNT_INTERNAL |
+	MNT_INTERNAL_FLAGS = MNT_SHARED | MNT_INTERNAL |
 			     MNT_DOOMED | MNT_SYNC_UMOUNT | MNT_MARKED |
 			     MNT_LOCKED,
 };

Re: [RFC] MNT_WRITE_HOLD mess

[Linus Torvalds]

On Fri, 4 Jul 2025 at 12:44, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
>         What if we steal LSB of ->mnt_instance.prev for what MNT_WRITE_HOLD
> is currently used for?

Ugh. I don't hate the concept, but if we do this, I think it needs to
be better abstracted out.

And you may be right that things like list_for_each_entry() won't
care, but I would not be surprised there is list debugging code that
could care deeply. Or if anybody uses things like "list_is_first()",
it will work 99+_% of the time, but then break horribly if the low bit
of the prev pointer is set.

So we obviously use the low bits of pointers in many other situations,
but I do think that it needs to have some kind of clear abstraction
and type safety to make sure that people don't use the "normal" list
handling helpers silently by mistake when they won't actually work.

Yes, that tends to involve a fair amount of duplication - exactly like
<linux/list_bl.h>, which is obviously the exact same thing except it
uses the low bit of the list head rather than the list entry. But if
the uses are limited enough - and they obviously need to be limited to
things that never look at 'prev' - maybe that duplication can also be
fairly limited.

I suspect we should also have another level of abstraction - we do
those "low bits of pointer" things often enough now that we probably
should have actual helpers for it, rather than have people do the
whole "cast to unsigned long and extract/insert bits by hand".

But that's a separate issue and largely independent (except that you'd
introduce a new use-case).

            Linus

Re: [RFC] MNT_WRITE_HOLD mess

[Al Viro]

On Fri, Jul 04, 2025 at 12:57:39PM -0700, Linus Torvalds wrote:

> Ugh. I don't hate the concept, but if we do this, I think it needs to
> be better abstracted out.
> 
> And you may be right that things like list_for_each_entry() won't
> care, but I would not be surprised there is list debugging code that
> could care deeply. Or if anybody uses things like "list_is_first()",
> it will work 99+_% of the time, but then break horribly if the low bit
> of the prev pointer is set.
> 
> So we obviously use the low bits of pointers in many other situations,
> but I do think that it needs to have some kind of clear abstraction
> and type safety to make sure that people don't use the "normal" list
> handling helpers silently by mistake when they won't actually work.

Point, but in this case I'd be tempted to turn the damn thing into
pointer + unsigned long right in the struct mount, and deal with it
explicitly.  And put a big note on it, along the lines of "we might want
to abstract that someday".

Backporting would be easier that way, if nothing else...

Re: [RFC] MNT_WRITE_HOLD mess

[Al Viro]

On Fri, Jul 04, 2025 at 09:23:37PM +0100, Al Viro wrote:
> On Fri, Jul 04, 2025 at 12:57:39PM -0700, Linus Torvalds wrote:
> 
> > Ugh. I don't hate the concept, but if we do this, I think it needs to
> > be better abstracted out.
> > 
> > And you may be right that things like list_for_each_entry() won't
> > care, but I would not be surprised there is list debugging code that
> > could care deeply. Or if anybody uses things like "list_is_first()",
> > it will work 99+_% of the time, but then break horribly if the low bit
> > of the prev pointer is set.
> > 
> > So we obviously use the low bits of pointers in many other situations,
> > but I do think that it needs to have some kind of clear abstraction
> > and type safety to make sure that people don't use the "normal" list
> > handling helpers silently by mistake when they won't actually work.
> 
> Point, but in this case I'd be tempted to turn the damn thing into
> pointer + unsigned long right in the struct mount, and deal with it
> explicitly.  And put a big note on it, along the lines of "we might want
> to abstract that someday".
> 
> Backporting would be easier that way, if nothing else...

FWIW, several observations around that thing:
	* mnt_get_write_access(), vfs_create_mount() and clone_mnt()
definitely do not need to touch the seqcount component of mount_lock.
read_seqlock_excl() is enough there.
	* AFAICS, the same goes for sb_prepare_remount_readonly(),
do_remount() and do_reconfigure_mnt() - no point bumping the seqcount
side of mount_lock there; only spinlock is needed.
	* failure exit in mount_setattr_prepare() needs only clearing the
bit; smp_wmb() is pointless there (especially done for each mount involved).
	* both vfs_create_mount() and clone_mnt() add mount to the tail
of ->s_mounts.  Is there any reason why that would be better than adding
to head?  I don't remember if that had been covered back in 2010/2011
discussion of per-mount r/o patchset; quick search on lore hasn't turned
up anything...  Miklos?

Re: [RFC] MNT_WRITE_HOLD mess

[Miklos Szeredi]

On Sat, 5 Jul 2025 at 02:01, Al Viro <viro@zeniv.linux.org.uk> wrote:
>         * both vfs_create_mount() and clone_mnt() add mount to the tail
> of ->s_mounts.  Is there any reason why that would be better than adding
> to head?  I don't remember if that had been covered back in 2010/2011
> discussion of per-mount r/o patchset; quick search on lore hasn't turned
> up anything...  Miklos?

I don't see why it would matter.  And I tend to choose tail if it
doesn't matter.

Thanks,
Miklos

Re: [RFC] MNT_WRITE_HOLD mess

[Al Viro]

On Sat, Jul 05, 2025 at 06:52:51AM +0200, Miklos Szeredi wrote:
> On Sat, 5 Jul 2025 at 02:01, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >         * both vfs_create_mount() and clone_mnt() add mount to the tail
> > of ->s_mounts.  Is there any reason why that would be better than adding
> > to head?  I don't remember if that had been covered back in 2010/2011
> > discussion of per-mount r/o patchset; quick search on lore hasn't turned
> > up anything...  Miklos?
> 
> I don't see why it would matter.  And I tend to choose tail if it
> doesn't matter.

Just the question of imitation of list_head vs. that of hlist...
Completely untested variant (fs/{namespace,super}.o compiles, but I hadn't even
tried to build the kernel, let alone boot and test it) follows:

diff --git a/fs/mount.h b/fs/mount.h
index ad7173037924..10fde24ecf5d 100644
--- a/fs/mount.h
+++ b/fs/mount.h
@@ -65,7 +65,9 @@ struct mount {
 #endif
 	struct list_head mnt_mounts;	/* list of children, anchored here */
 	struct list_head mnt_child;	/* and going through their mnt_child */
-	struct list_head mnt_instance;	/* mount instance on sb->s_mounts */
+	struct mount *mnt_next_for_sb;	/* the next two fields are hlist_node */
+	unsigned long mnt_prev_for_sb;	/* except that LSB of pprev is stolen */
+#define WRITE_HOLD 1			/* ... for use by mnt_hold_writers() */
 	const char *mnt_devname;	/* Name of device e.g. /dev/dsk/hda1 */
 	struct list_head mnt_list;
 	struct list_head mnt_expire;	/* link in fs-specific expiry list */
diff --git a/fs/namespace.c b/fs/namespace.c
index 54c59e091919..797d0bd7b674 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -501,31 +501,31 @@ int mnt_get_write_access(struct vfsmount *m)
 	mnt_inc_writers(mnt);
 	/*
 	 * The store to mnt_inc_writers must be visible before we pass
-	 * MNT_WRITE_HOLD loop below, so that the slowpath can see our
-	 * incremented count after it has set MNT_WRITE_HOLD.
+	 * WRITE_HOLD loop below, so that the slowpath can see our
+	 * incremented count after it has set WRITE_HOLD.
 	 */
 	smp_mb();
 	might_lock(&mount_lock.lock);
-	while (READ_ONCE(mnt->mnt.mnt_flags) & MNT_WRITE_HOLD) {
+	while (READ_ONCE(mnt->mnt_prev_for_sb) & WRITE_HOLD) {
 		if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
 			cpu_relax();
 		} else {
 			/*
 			 * This prevents priority inversion, if the task
-			 * setting MNT_WRITE_HOLD got preempted on a remote
-			 * CPU, and it prevents life lock if the task setting
-			 * MNT_WRITE_HOLD has a lower priority and is bound to
+			 * setting WRITE_HOLD got preempted on a remote
+			 * CPU, and it prevents life lock if such task
+			 * has a lower priority and is bound to
 			 * the same CPU as the task that is spinning here.
 			 */
 			preempt_enable();
-			lock_mount_hash();
-			unlock_mount_hash();
+			read_seqlock_excl(&mount_lock);
+			read_sequnlock_excl(&mount_lock);
 			preempt_disable();
 		}
 	}
 	/*
 	 * The barrier pairs with the barrier sb_start_ro_state_change() making
-	 * sure that if we see MNT_WRITE_HOLD cleared, we will also see
+	 * sure that if we see WRITE_HOLD cleared, we will also see
 	 * s_readonly_remount set (or even SB_RDONLY / MNT_READONLY flags) in
 	 * mnt_is_readonly() and bail in case we are racing with remount
 	 * read-only.
@@ -663,16 +663,15 @@ EXPORT_SYMBOL(mnt_drop_write_file);
  * a call to mnt_unhold_writers() in order to stop preventing write access to
  * @mnt.
  *
- * Context: This function expects lock_mount_hash() to be held serializing
- *          setting MNT_WRITE_HOLD.
+ * Context: This function expects read_seqlock_excl(&mount_lock).
  * Return: On success 0 is returned.
  *	   On error, -EBUSY is returned.
  */
 static inline int mnt_hold_writers(struct mount *mnt)
 {
-	mnt->mnt.mnt_flags |= MNT_WRITE_HOLD;
+	mnt->mnt_prev_for_sb |= WRITE_HOLD;
 	/*
-	 * After storing MNT_WRITE_HOLD, we'll read the counters. This store
+	 * After storing WRITE_HOLD, we'll read the counters. This store
 	 * should be visible before we do.
 	 */
 	smp_mb();
@@ -688,9 +687,9 @@ static inline int mnt_hold_writers(struct mount *mnt)
 	 * sum up each counter, if we read a counter before it is incremented,
 	 * but then read another CPU's count which it has been subsequently
 	 * decremented from -- we would see more decrements than we should.
-	 * MNT_WRITE_HOLD protects against this scenario, because
+	 * WRITE_HOLD protects against this scenario, because
 	 * mnt_want_write first increments count, then smp_mb, then spins on
-	 * MNT_WRITE_HOLD, so it can't be decremented by another CPU while
+	 * WRITE_HOLD, so it can't be decremented by another CPU while
 	 * we're counting up here.
 	 */
 	if (mnt_get_writers(mnt) > 0)
@@ -709,16 +708,39 @@ static inline int mnt_hold_writers(struct mount *mnt)
  * This function can only be called after a successful call to
  * mnt_hold_writers().
  *
- * Context: This function expects lock_mount_hash() to be held.
+ * Context: This function expects read_seqlock_excl(&mount_lock) to be held.
  */
 static inline void mnt_unhold_writers(struct mount *mnt)
 {
+	if (!(mnt->mnt_prev_for_sb & WRITE_HOLD))
+		return;
 	/*
-	 * MNT_READONLY must become visible before ~MNT_WRITE_HOLD, so writers
+	 * MNT_READONLY must become visible before ~WRITE_HOLD, so writers
 	 * that become unheld will see MNT_READONLY.
 	 */
 	smp_wmb();
-	mnt->mnt.mnt_flags &= ~MNT_WRITE_HOLD;
+	mnt->mnt_prev_for_sb &= ~WRITE_HOLD;
+}
+
+static inline void mnt_del_instance(struct mount *m)
+{
+	struct mount **p = (void *)m->mnt_prev_for_sb;
+	struct mount *next = m->mnt_next_for_sb;
+
+	if (next)
+		next->mnt_prev_for_sb = (unsigned long)p;
+	*p = next;
+}
+
+static inline void mnt_add_instance(struct mount *m, struct super_block *s)
+{
+	struct mount *first = s->s_mounts;
+
+	if (first)
+		first->mnt_prev_for_sb = (unsigned long)m;
+	m->mnt_next_for_sb = first;
+	m->mnt_prev_for_sb = (unsigned long)&s->s_mounts;
+	s->s_mounts = m;
 }
 
 static int mnt_make_readonly(struct mount *mnt)
@@ -734,17 +756,16 @@ static int mnt_make_readonly(struct mount *mnt)
 
 int sb_prepare_remount_readonly(struct super_block *sb)
 {
-	struct mount *mnt;
 	int err = 0;
 
-	/* Racy optimization.  Recheck the counter under MNT_WRITE_HOLD */
+	/* Racy optimization.  Recheck the counter under WRITE_HOLD */
 	if (atomic_long_read(&sb->s_remove_count))
 		return -EBUSY;
 
-	lock_mount_hash();
-	list_for_each_entry(mnt, &sb->s_mounts, mnt_instance) {
-		if (!(mnt->mnt.mnt_flags & MNT_READONLY)) {
-			err = mnt_hold_writers(mnt);
+	read_seqlock_excl(&mount_lock);
+	for (struct mount *m = sb->s_mounts; m; m = m->mnt_next_for_sb) {
+		if (!(m->mnt.mnt_flags & MNT_READONLY)) {
+			err = mnt_hold_writers(m);
 			if (err)
 				break;
 		}
@@ -754,11 +775,11 @@ int sb_prepare_remount_readonly(struct super_block *sb)
 
 	if (!err)
 		sb_start_ro_state_change(sb);
-	list_for_each_entry(mnt, &sb->s_mounts, mnt_instance) {
-		if (mnt->mnt.mnt_flags & MNT_WRITE_HOLD)
-			mnt->mnt.mnt_flags &= ~MNT_WRITE_HOLD;
+	for (struct mount *m = sb->s_mounts; m; m = m->mnt_next_for_sb) {
+		if (m->mnt_prev_for_sb & WRITE_HOLD)
+			m->mnt_prev_for_sb &= ~WRITE_HOLD;
 	}
-	unlock_mount_hash();
+	read_sequnlock_excl(&mount_lock);
 
 	return err;
 }
@@ -1249,6 +1270,21 @@ static struct mount *skip_mnt_tree(struct mount *p)
 	return p;
 }
 
+static void setup_mnt(struct mount *m, struct dentry *root)
+{
+	struct super_block *s = root->d_sb;
+
+	atomic_inc(&s->s_active);
+	m->mnt.mnt_sb = s;
+	m->mnt.mnt_root = dget(root);
+	m->mnt_mountpoint = m->mnt.mnt_root;
+	m->mnt_parent = m;
+
+	read_seqlock_excl(&mount_lock);
+	mnt_add_instance(m, s);
+	read_sequnlock_excl(&mount_lock);
+}
+
 /**
  * vfs_create_mount - Create a mount for a configured superblock
  * @fc: The configuration context with the superblock attached
@@ -1272,15 +1308,8 @@ struct vfsmount *vfs_create_mount(struct fs_context *fc)
 	if (fc->sb_flags & SB_KERNMOUNT)
 		mnt->mnt.mnt_flags = MNT_INTERNAL;
 
-	atomic_inc(&fc->root->d_sb->s_active);
-	mnt->mnt.mnt_sb		= fc->root->d_sb;
-	mnt->mnt.mnt_root	= dget(fc->root);
-	mnt->mnt_mountpoint	= mnt->mnt.mnt_root;
-	mnt->mnt_parent		= mnt;
+	setup_mnt(mnt, fc->root);
 
-	lock_mount_hash();
-	list_add_tail(&mnt->mnt_instance, &mnt->mnt.mnt_sb->s_mounts);
-	unlock_mount_hash();
 	return &mnt->mnt;
 }
 EXPORT_SYMBOL(vfs_create_mount);
@@ -1329,7 +1358,6 @@ EXPORT_SYMBOL_GPL(vfs_kern_mount);
 static struct mount *clone_mnt(struct mount *old, struct dentry *root,
 					int flag)
 {
-	struct super_block *sb = old->mnt.mnt_sb;
 	struct mount *mnt;
 	int err;
 
@@ -1349,18 +1377,11 @@ static struct mount *clone_mnt(struct mount *old, struct dentry *root,
 	}
 
 	mnt->mnt.mnt_flags = old->mnt.mnt_flags;
-	mnt->mnt.mnt_flags &= ~(MNT_WRITE_HOLD|MNT_MARKED|MNT_INTERNAL);
+	mnt->mnt.mnt_flags &= ~(MNT_MARKED|MNT_INTERNAL);
 
-	atomic_inc(&sb->s_active);
 	mnt->mnt.mnt_idmap = mnt_idmap_get(mnt_idmap(&old->mnt));
 
-	mnt->mnt.mnt_sb = sb;
-	mnt->mnt.mnt_root = dget(root);
-	mnt->mnt_mountpoint = mnt->mnt.mnt_root;
-	mnt->mnt_parent = mnt;
-	lock_mount_hash();
-	list_add_tail(&mnt->mnt_instance, &sb->s_mounts);
-	unlock_mount_hash();
+	setup_mnt(mnt, root);
 
 	if ((flag & CL_SLAVE) ||
 	    ((flag & CL_SHARED_TO_SLAVE) && IS_MNT_SHARED(old))) {
@@ -1477,7 +1498,7 @@ static void mntput_no_expire(struct mount *mnt)
 	mnt->mnt.mnt_flags |= MNT_DOOMED;
 	rcu_read_unlock();
 
-	list_del(&mnt->mnt_instance);
+	mnt_del_instance(mnt);
 
 	if (unlikely(!list_empty(&mnt->mnt_mounts))) {
 		struct mount *p, *tmp;
@@ -4953,18 +4974,17 @@ static int mount_setattr_prepare(struct mount_kattr *kattr, struct mount *mnt)
 		struct mount *p;
 
 		/*
-		 * If we had to call mnt_hold_writers() MNT_WRITE_HOLD will
-		 * be set in @mnt_flags. The loop unsets MNT_WRITE_HOLD for all
+		 * If we had to call mnt_hold_writers() WRITE_HOLD will be set
+		 * in @mnt_prev_for_sb. The loop unsets WRITE_HOLD for all
 		 * mounts and needs to take care to include the first mount.
 		 */
 		for (p = mnt; p; p = next_mnt(p, mnt)) {
 			/* If we had to hold writers unblock them. */
-			if (p->mnt.mnt_flags & MNT_WRITE_HOLD)
-				mnt_unhold_writers(p);
+			mnt_unhold_writers(p);
 
 			/*
 			 * We're done once the first mount we changed got
-			 * MNT_WRITE_HOLD unset.
+			 * WRITE_HOLD unset.
 			 */
 			if (p == m)
 				break;
@@ -4999,8 +5019,7 @@ static void mount_setattr_commit(struct mount_kattr *kattr, struct mount *mnt)
 		WRITE_ONCE(m->mnt.mnt_flags, flags);
 
 		/* If we had to hold writers unblock them. */
-		if (m->mnt.mnt_flags & MNT_WRITE_HOLD)
-			mnt_unhold_writers(m);
+		mnt_unhold_writers(m);
 
 		if (kattr->propagation)
 			change_mnt_propagation(m, kattr->propagation);
diff --git a/fs/super.c b/fs/super.c
index 80418ca8e215..6ca4bace5476 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -323,7 +323,6 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
 	if (!s)
 		return NULL;
 
-	INIT_LIST_HEAD(&s->s_mounts);
 	s->s_user_ns = get_user_ns(user_ns);
 	init_rwsem(&s->s_umount);
 	lockdep_set_class(&s->s_umount, &type->s_umount_key);
@@ -408,7 +407,7 @@ static void __put_super(struct super_block *s)
 		list_del_init(&s->s_list);
 		WARN_ON(s->s_dentry_lru.node);
 		WARN_ON(s->s_inode_lru.node);
-		WARN_ON(!list_empty(&s->s_mounts));
+		WARN_ON(s->s_mounts);
 		call_rcu(&s->rcu, destroy_super_rcu);
 	}
 }
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b085f161ed22..004d6a63fa5a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1356,7 +1356,7 @@ struct super_block {
 	__u16 s_encoding_flags;
 #endif
 	struct hlist_bl_head	s_roots;	/* alternate root dentries for NFS */
-	struct list_head	s_mounts;	/* list of mounts; _not_ for fs use */
+	void			*s_mounts;	/* list of mounts; _not_ for fs use */
 	struct block_device	*s_bdev;	/* can go away once we use an accessor for @s_bdev_file */
 	struct file		*s_bdev_file;
 	struct backing_dev_info *s_bdi;
diff --git a/include/linux/mount.h b/include/linux/mount.h
index 1a508beba446..1728bc50d02b 100644
--- a/include/linux/mount.h
+++ b/include/linux/mount.h
@@ -33,7 +33,6 @@ enum mount_flags {
 	MNT_NOSYMFOLLOW	= 0x80,
 
 	MNT_SHRINKABLE	= 0x100,
-	MNT_WRITE_HOLD	= 0x200,
 
 	MNT_SHARED	= 0x1000, /* if the vfsmount is a shared mount */
 	MNT_UNBINDABLE	= 0x2000, /* if the vfsmount is a unbindable mount */
@@ -64,7 +63,7 @@ enum mount_flags {
 				  | MNT_READONLY | MNT_NOSYMFOLLOW,
 	MNT_ATIME_MASK = MNT_NOATIME | MNT_NODIRATIME | MNT_RELATIME,
 
-	MNT_INTERNAL_FLAGS = MNT_SHARED | MNT_WRITE_HOLD | MNT_INTERNAL |
+	MNT_INTERNAL_FLAGS = MNT_SHARED | MNT_INTERNAL |
 			     MNT_DOOMED | MNT_SYNC_UMOUNT | MNT_MARKED |
 			     MNT_LOCKED,
 };

Re: [RFC] MNT_WRITE_HOLD mess

[Al Viro]

On Sat, Jul 05, 2025 at 06:57:15AM +0100, Al Viro wrote:
> +static inline void mnt_add_instance(struct mount *m, struct super_block *s)
> +{
> +	struct mount *first = s->s_mounts;
> +
> +	if (first)
> +		first->mnt_prev_for_sb = (unsigned long)m;
					 ^^^^^^^^^^^^^^^^
					 (unsigned long)&m->mnt_next_for_sb;

that is...

Re: [RFC] MNT_WRITE_HOLD mess

[Al Viro]

On Sat, Jul 05, 2025 at 01:01:14AM +0100, Al Viro wrote:

> FWIW, several observations around that thing:
> 	* mnt_get_write_access(), vfs_create_mount() and clone_mnt()
> definitely do not need to touch the seqcount component of mount_lock.
> read_seqlock_excl() is enough there.
> 	* AFAICS, the same goes for sb_prepare_remount_readonly(),
> do_remount() and do_reconfigure_mnt() - no point bumping the seqcount
> side of mount_lock there; only spinlock is needed.
> 	* failure exit in mount_setattr_prepare() needs only clearing the
> bit; smp_wmb() is pointless there (especially done for each mount involved).

The following appears to work; writing docs now...  Incidentally, the
comment about mnt_unhold_writers() ("This function can only be called
after a successful call to mnt_hold_writers()") was dangerously wrong;
failing mnt_hold_writers() still needs to be undone (and removing it from
failure case in mnt_make_readonly() would break things badly).  OTOH,
we might make mnt_hold_writers() clean WRITE_HOLD on failure; it would
mean a minor change in mnt_make_readonly() and simplify the cleanup
loop in mount_setattr_prepare().  Not sure if it's better left to
a followup...

Re useless smp_wmb() in mount_setattr_prepare() failure path - how costly
smp_wmb() is on e.g. arm64?  In any case I don't think it's worth bothering
with for something we'd backport, but do we care about those even in
mainline?  It's a failure path, after all...

Anyway, current delta follows.  It ends up adding new mounts to the
_head_ of ->mnt_instance instead of its tail, but there's only one
place that might care - sb_prepare_remount_readonly() on something
that has mounts with positive write counts might do a different
amount of work before failing.  The cost of success case is unaffected.

Folks, how much do you hate the variant below?

diff --git a/fs/mount.h b/fs/mount.h
index ad7173037924..10fde24ecf5d 100644
--- a/fs/mount.h
+++ b/fs/mount.h
@@ -65,7 +65,9 @@ struct mount {
 #endif
 	struct list_head mnt_mounts;	/* list of children, anchored here */
 	struct list_head mnt_child;	/* and going through their mnt_child */
-	struct list_head mnt_instance;	/* mount instance on sb->s_mounts */
+	struct mount *mnt_next_for_sb;	/* the next two fields are hlist_node */
+	unsigned long mnt_prev_for_sb;	/* except that LSB of pprev is stolen */
+#define WRITE_HOLD 1			/* ... for use by mnt_hold_writers() */
 	const char *mnt_devname;	/* Name of device e.g. /dev/dsk/hda1 */
 	struct list_head mnt_list;
 	struct list_head mnt_expire;	/* link in fs-specific expiry list */
diff --git a/fs/namespace.c b/fs/namespace.c
index 54c59e091919..48f861d65c85 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -501,31 +501,31 @@ int mnt_get_write_access(struct vfsmount *m)
 	mnt_inc_writers(mnt);
 	/*
 	 * The store to mnt_inc_writers must be visible before we pass
-	 * MNT_WRITE_HOLD loop below, so that the slowpath can see our
-	 * incremented count after it has set MNT_WRITE_HOLD.
+	 * WRITE_HOLD loop below, so that the slowpath can see our
+	 * incremented count after it has set WRITE_HOLD.
 	 */
 	smp_mb();
 	might_lock(&mount_lock.lock);
-	while (READ_ONCE(mnt->mnt.mnt_flags) & MNT_WRITE_HOLD) {
+	while (READ_ONCE(mnt->mnt_prev_for_sb) & WRITE_HOLD) {
 		if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
 			cpu_relax();
 		} else {
 			/*
 			 * This prevents priority inversion, if the task
-			 * setting MNT_WRITE_HOLD got preempted on a remote
-			 * CPU, and it prevents life lock if the task setting
-			 * MNT_WRITE_HOLD has a lower priority and is bound to
+			 * setting WRITE_HOLD got preempted on a remote
+			 * CPU, and it prevents life lock if such task
+			 * has a lower priority and is bound to
 			 * the same CPU as the task that is spinning here.
 			 */
 			preempt_enable();
-			lock_mount_hash();
-			unlock_mount_hash();
+			read_seqlock_excl(&mount_lock);
+			read_sequnlock_excl(&mount_lock);
 			preempt_disable();
 		}
 	}
 	/*
 	 * The barrier pairs with the barrier sb_start_ro_state_change() making
-	 * sure that if we see MNT_WRITE_HOLD cleared, we will also see
+	 * sure that if we see WRITE_HOLD cleared, we will also see
 	 * s_readonly_remount set (or even SB_RDONLY / MNT_READONLY flags) in
 	 * mnt_is_readonly() and bail in case we are racing with remount
 	 * read-only.
@@ -663,16 +663,15 @@ EXPORT_SYMBOL(mnt_drop_write_file);
  * a call to mnt_unhold_writers() in order to stop preventing write access to
  * @mnt.
  *
- * Context: This function expects lock_mount_hash() to be held serializing
- *          setting MNT_WRITE_HOLD.
+ * Context: This function expects read_seqlock_excl(&mount_lock).
  * Return: On success 0 is returned.
  *	   On error, -EBUSY is returned.
  */
 static inline int mnt_hold_writers(struct mount *mnt)
 {
-	mnt->mnt.mnt_flags |= MNT_WRITE_HOLD;
+	mnt->mnt_prev_for_sb |= WRITE_HOLD;
 	/*
-	 * After storing MNT_WRITE_HOLD, we'll read the counters. This store
+	 * After storing WRITE_HOLD, we'll read the counters. This store
 	 * should be visible before we do.
 	 */
 	smp_mb();
@@ -688,9 +687,9 @@ static inline int mnt_hold_writers(struct mount *mnt)
 	 * sum up each counter, if we read a counter before it is incremented,
 	 * but then read another CPU's count which it has been subsequently
 	 * decremented from -- we would see more decrements than we should.
-	 * MNT_WRITE_HOLD protects against this scenario, because
+	 * WRITE_HOLD protects against this scenario, because
 	 * mnt_want_write first increments count, then smp_mb, then spins on
-	 * MNT_WRITE_HOLD, so it can't be decremented by another CPU while
+	 * WRITE_HOLD, so it can't be decremented by another CPU while
 	 * we're counting up here.
 	 */
 	if (mnt_get_writers(mnt) > 0)
@@ -706,19 +705,41 @@ static inline int mnt_hold_writers(struct mount *mnt)
  * Stop preventing write access to @mnt allowing callers to gain write access
  * to @mnt again.
  *
- * This function can only be called after a successful call to
- * mnt_hold_writers().
+ * This function can only be called after a call to mnt_hold_writers().
  *
- * Context: This function expects lock_mount_hash() to be held.
+ * Context: This function expects read_seqlock_excl(&mount_lock) to be held.
  */
 static inline void mnt_unhold_writers(struct mount *mnt)
 {
+	if (!(mnt->mnt_prev_for_sb & WRITE_HOLD))
+		return;
 	/*
-	 * MNT_READONLY must become visible before ~MNT_WRITE_HOLD, so writers
+	 * MNT_READONLY must become visible before ~WRITE_HOLD, so writers
 	 * that become unheld will see MNT_READONLY.
 	 */
 	smp_wmb();
-	mnt->mnt.mnt_flags &= ~MNT_WRITE_HOLD;
+	mnt->mnt_prev_for_sb &= ~WRITE_HOLD;
+}
+
+static inline void mnt_del_instance(struct mount *m)
+{
+	struct mount **p = (void *)m->mnt_prev_for_sb;
+	struct mount *next = m->mnt_next_for_sb;
+
+	if (next)
+		next->mnt_prev_for_sb = (unsigned long)p;
+	*p = next;
+}
+
+static inline void mnt_add_instance(struct mount *m, struct super_block *s)
+{
+	struct mount *first = s->s_mounts;
+
+	if (first)
+		first->mnt_prev_for_sb = (unsigned long)&m->mnt_next_for_sb;
+	m->mnt_next_for_sb = first;
+	m->mnt_prev_for_sb = (unsigned long)&s->s_mounts;
+	s->s_mounts = m;
 }
 
 static int mnt_make_readonly(struct mount *mnt)
@@ -734,17 +755,16 @@ static int mnt_make_readonly(struct mount *mnt)
 
 int sb_prepare_remount_readonly(struct super_block *sb)
 {
-	struct mount *mnt;
 	int err = 0;
 
-	/* Racy optimization.  Recheck the counter under MNT_WRITE_HOLD */
+	/* Racy optimization.  Recheck the counter under WRITE_HOLD */
 	if (atomic_long_read(&sb->s_remove_count))
 		return -EBUSY;
 
-	lock_mount_hash();
-	list_for_each_entry(mnt, &sb->s_mounts, mnt_instance) {
-		if (!(mnt->mnt.mnt_flags & MNT_READONLY)) {
-			err = mnt_hold_writers(mnt);
+	read_seqlock_excl(&mount_lock);
+	for (struct mount *m = sb->s_mounts; m; m = m->mnt_next_for_sb) {
+		if (!(m->mnt.mnt_flags & MNT_READONLY)) {
+			err = mnt_hold_writers(m);
 			if (err)
 				break;
 		}
@@ -754,11 +774,11 @@ int sb_prepare_remount_readonly(struct super_block *sb)
 
 	if (!err)
 		sb_start_ro_state_change(sb);
-	list_for_each_entry(mnt, &sb->s_mounts, mnt_instance) {
-		if (mnt->mnt.mnt_flags & MNT_WRITE_HOLD)
-			mnt->mnt.mnt_flags &= ~MNT_WRITE_HOLD;
+	for (struct mount *m = sb->s_mounts; m; m = m->mnt_next_for_sb) {
+		if (m->mnt_prev_for_sb & WRITE_HOLD)
+			m->mnt_prev_for_sb &= ~WRITE_HOLD;
 	}
-	unlock_mount_hash();
+	read_sequnlock_excl(&mount_lock);
 
 	return err;
 }
@@ -1249,6 +1269,21 @@ static struct mount *skip_mnt_tree(struct mount *p)
 	return p;
 }
 
+static void setup_mnt(struct mount *m, struct dentry *root)
+{
+	struct super_block *s = root->d_sb;
+
+	atomic_inc(&s->s_active);
+	m->mnt.mnt_sb = s;
+	m->mnt.mnt_root = dget(root);
+	m->mnt_mountpoint = m->mnt.mnt_root;
+	m->mnt_parent = m;
+
+	read_seqlock_excl(&mount_lock);
+	mnt_add_instance(m, s);
+	read_sequnlock_excl(&mount_lock);
+}
+
 /**
  * vfs_create_mount - Create a mount for a configured superblock
  * @fc: The configuration context with the superblock attached
@@ -1272,15 +1307,8 @@ struct vfsmount *vfs_create_mount(struct fs_context *fc)
 	if (fc->sb_flags & SB_KERNMOUNT)
 		mnt->mnt.mnt_flags = MNT_INTERNAL;
 
-	atomic_inc(&fc->root->d_sb->s_active);
-	mnt->mnt.mnt_sb		= fc->root->d_sb;
-	mnt->mnt.mnt_root	= dget(fc->root);
-	mnt->mnt_mountpoint	= mnt->mnt.mnt_root;
-	mnt->mnt_parent		= mnt;
+	setup_mnt(mnt, fc->root);
 
-	lock_mount_hash();
-	list_add_tail(&mnt->mnt_instance, &mnt->mnt.mnt_sb->s_mounts);
-	unlock_mount_hash();
 	return &mnt->mnt;
 }
 EXPORT_SYMBOL(vfs_create_mount);
@@ -1329,7 +1357,6 @@ EXPORT_SYMBOL_GPL(vfs_kern_mount);
 static struct mount *clone_mnt(struct mount *old, struct dentry *root,
 					int flag)
 {
-	struct super_block *sb = old->mnt.mnt_sb;
 	struct mount *mnt;
 	int err;
 
@@ -1349,18 +1376,11 @@ static struct mount *clone_mnt(struct mount *old, struct dentry *root,
 	}
 
 	mnt->mnt.mnt_flags = old->mnt.mnt_flags;
-	mnt->mnt.mnt_flags &= ~(MNT_WRITE_HOLD|MNT_MARKED|MNT_INTERNAL);
+	mnt->mnt.mnt_flags &= ~(MNT_MARKED|MNT_INTERNAL);
 
-	atomic_inc(&sb->s_active);
 	mnt->mnt.mnt_idmap = mnt_idmap_get(mnt_idmap(&old->mnt));
 
-	mnt->mnt.mnt_sb = sb;
-	mnt->mnt.mnt_root = dget(root);
-	mnt->mnt_mountpoint = mnt->mnt.mnt_root;
-	mnt->mnt_parent = mnt;
-	lock_mount_hash();
-	list_add_tail(&mnt->mnt_instance, &sb->s_mounts);
-	unlock_mount_hash();
+	setup_mnt(mnt, root);
 
 	if ((flag & CL_SLAVE) ||
 	    ((flag & CL_SHARED_TO_SLAVE) && IS_MNT_SHARED(old))) {
@@ -1477,7 +1497,7 @@ static void mntput_no_expire(struct mount *mnt)
 	mnt->mnt.mnt_flags |= MNT_DOOMED;
 	rcu_read_unlock();
 
-	list_del(&mnt->mnt_instance);
+	mnt_del_instance(mnt);
 
 	if (unlikely(!list_empty(&mnt->mnt_mounts))) {
 		struct mount *p, *tmp;
@@ -3334,11 +3354,11 @@ static int do_reconfigure_mnt(struct path *path, unsigned int mnt_flags)
 	 * changing it, so only take down_read(&sb->s_umount).
 	 */
 	down_read(&sb->s_umount);
-	lock_mount_hash();
+	read_seqlock_excl(&mount_lock);
 	ret = change_mount_ro_state(mnt, mnt_flags);
 	if (ret == 0)
 		set_mount_attributes(mnt, mnt_flags);
-	unlock_mount_hash();
+	read_sequnlock_excl(&mount_lock);
 	up_read(&sb->s_umount);
 
 	mnt_warn_timestamp_expiry(path, &mnt->mnt);
@@ -3385,9 +3405,9 @@ static int do_remount(struct path *path, int ms_flags, int sb_flags,
 		if (ns_capable(sb->s_user_ns, CAP_SYS_ADMIN)) {
 			err = reconfigure_super(fc);
 			if (!err) {
-				lock_mount_hash();
+				read_seqlock_excl(&mount_lock);
 				set_mount_attributes(mnt, mnt_flags);
-				unlock_mount_hash();
+				read_sequnlock_excl(&mount_lock);
 			}
 		}
 		up_write(&sb->s_umount);
@@ -4953,18 +4973,17 @@ static int mount_setattr_prepare(struct mount_kattr *kattr, struct mount *mnt)
 		struct mount *p;
 
 		/*
-		 * If we had to call mnt_hold_writers() MNT_WRITE_HOLD will
-		 * be set in @mnt_flags. The loop unsets MNT_WRITE_HOLD for all
+		 * If we had to call mnt_hold_writers() WRITE_HOLD will be set
+		 * in @mnt_prev_for_sb. The loop unsets WRITE_HOLD for all
 		 * mounts and needs to take care to include the first mount.
 		 */
 		for (p = mnt; p; p = next_mnt(p, mnt)) {
 			/* If we had to hold writers unblock them. */
-			if (p->mnt.mnt_flags & MNT_WRITE_HOLD)
-				mnt_unhold_writers(p);
+			mnt_unhold_writers(p);
 
 			/*
 			 * We're done once the first mount we changed got
-			 * MNT_WRITE_HOLD unset.
+			 * WRITE_HOLD unset.
 			 */
 			if (p == m)
 				break;
@@ -4999,8 +5018,7 @@ static void mount_setattr_commit(struct mount_kattr *kattr, struct mount *mnt)
 		WRITE_ONCE(m->mnt.mnt_flags, flags);
 
 		/* If we had to hold writers unblock them. */
-		if (m->mnt.mnt_flags & MNT_WRITE_HOLD)
-			mnt_unhold_writers(m);
+		mnt_unhold_writers(m);
 
 		if (kattr->propagation)
 			change_mnt_propagation(m, kattr->propagation);
diff --git a/fs/super.c b/fs/super.c
index 80418ca8e215..6ca4bace5476 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -323,7 +323,6 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
 	if (!s)
 		return NULL;
 
-	INIT_LIST_HEAD(&s->s_mounts);
 	s->s_user_ns = get_user_ns(user_ns);
 	init_rwsem(&s->s_umount);
 	lockdep_set_class(&s->s_umount, &type->s_umount_key);
@@ -408,7 +407,7 @@ static void __put_super(struct super_block *s)
 		list_del_init(&s->s_list);
 		WARN_ON(s->s_dentry_lru.node);
 		WARN_ON(s->s_inode_lru.node);
-		WARN_ON(!list_empty(&s->s_mounts));
+		WARN_ON(s->s_mounts);
 		call_rcu(&s->rcu, destroy_super_rcu);
 	}
 }
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b085f161ed22..004d6a63fa5a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1356,7 +1356,7 @@ struct super_block {
 	__u16 s_encoding_flags;
 #endif
 	struct hlist_bl_head	s_roots;	/* alternate root dentries for NFS */
-	struct list_head	s_mounts;	/* list of mounts; _not_ for fs use */
+	void			*s_mounts;	/* list of mounts; _not_ for fs use */
 	struct block_device	*s_bdev;	/* can go away once we use an accessor for @s_bdev_file */
 	struct file		*s_bdev_file;
 	struct backing_dev_info *s_bdi;
diff --git a/include/linux/mount.h b/include/linux/mount.h
index 1a508beba446..1728bc50d02b 100644
--- a/include/linux/mount.h
+++ b/include/linux/mount.h
@@ -33,7 +33,6 @@ enum mount_flags {
 	MNT_NOSYMFOLLOW	= 0x80,
 
 	MNT_SHRINKABLE	= 0x100,
-	MNT_WRITE_HOLD	= 0x200,
 
 	MNT_SHARED	= 0x1000, /* if the vfsmount is a shared mount */
 	MNT_UNBINDABLE	= 0x2000, /* if the vfsmount is a unbindable mount */
@@ -64,7 +63,7 @@ enum mount_flags {
 				  | MNT_READONLY | MNT_NOSYMFOLLOW,
 	MNT_ATIME_MASK = MNT_NOATIME | MNT_NODIRATIME | MNT_RELATIME,
 
-	MNT_INTERNAL_FLAGS = MNT_SHARED | MNT_WRITE_HOLD | MNT_INTERNAL |
+	MNT_INTERNAL_FLAGS = MNT_SHARED | MNT_INTERNAL |
 			     MNT_DOOMED | MNT_SYNC_UMOUNT | MNT_MARKED |
 			     MNT_LOCKED,
 };

Re: [RFC] MNT_WRITE_HOLD mess

[Al Viro]

On Sat, Jul 05, 2025 at 07:53:59PM +0100, Al Viro wrote:
> On Sat, Jul 05, 2025 at 01:01:14AM +0100, Al Viro wrote:
> 
> > FWIW, several observations around that thing:
> > 	* mnt_get_write_access(), vfs_create_mount() and clone_mnt()
> > definitely do not need to touch the seqcount component of mount_lock.
> > read_seqlock_excl() is enough there.
> > 	* AFAICS, the same goes for sb_prepare_remount_readonly(),
> > do_remount() and do_reconfigure_mnt() - no point bumping the seqcount
> > side of mount_lock there; only spinlock is needed.
> > 	* failure exit in mount_setattr_prepare() needs only clearing the
> > bit; smp_wmb() is pointless there (especially done for each mount involved).
> 
> The following appears to work; writing docs now...

	More fun questions in the area: is there any reason we have mnt_want_write()
doing
int mnt_want_write(struct vfsmount *m)
{
        int ret;

        sb_start_write(m->mnt_sb);
        ret = mnt_get_write_access(m);
        if (ret)
                sb_end_write(m->mnt_sb);
        return ret;
}
rather than
int mnt_want_write(struct vfsmount *m)
{
        int ret = mnt_get_write_access(m);
	if (!ret)
		sb_start_write(m->mnt_sb);
        return ret;
}

	Note that mnt_want_write_file() on e.g. a regular file opened
for write will have sb_start_write() done with mnt_get_write_access()
already in place since open(2).  So the nesting shouldn't be an issue
here...  The same order (mount then superblock) is used for overlayfs
copyup, for that matter.

	So if it's a matter of waiting for thaw with mount write
count already incremented, simple echo foo > pathname would already
demonstrate the same, no matter of which order mnt_want_write() uses.
What am I missing there?

	IIRC, that was Jan's stuff... <checks git log> yep - eb04c28288bb
"fs: Add freezing handling to mnt_want_write() / mnt_drop_write()" is
where it came from...

[PATCH] fix a mount write count leak in ksmbd_vfs_kern_path_locked() (was Re: [RFC] MNT_WRITE_HOLD mess)

[Al Viro]

If the call of ksmbd_vfs_lock_parent() fails, we drop the parent_path
references and return an error.  We need to drop the write access we
just got on parent_path->mnt before we drop the mount reference - callers
assume that ksmbd_vfs_kern_path_locked() returns with mount write
access grabbed if and only if it has returned 0.

Fixes: 864fb5d37163 "ksmbd: fix possible deadlock in smb2_open"
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
index 0f3aad12e495..d3437f6644e3 100644
--- a/fs/smb/server/vfs.c
+++ b/fs/smb/server/vfs.c
@@ -1282,6 +1282,7 @@ int ksmbd_vfs_kern_path_locked(struct ksmbd_work *work, char *name,
 
 		err = ksmbd_vfs_lock_parent(parent_path->dentry, path->dentry);
 		if (err) {
+			mnt_drop_write(parent_path->mnt);
 			path_put(path);
 			path_put(parent_path);
 		}

Re: [PATCH] fix a mount write count leak in ksmbd_vfs_kern_path_locked() (was Re: [RFC] MNT_WRITE_HOLD mess)

[Namjae Jeon]

On Sun, Jul 6, 2025 at 10:26 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> If the call of ksmbd_vfs_lock_parent() fails, we drop the parent_path
> references and return an error.  We need to drop the write access we
> just got on parent_path->mnt before we drop the mount reference - callers
> assume that ksmbd_vfs_kern_path_locked() returns with mount write
> access grabbed if and only if it has returned 0.
>
> Fixes: 864fb5d37163 "ksmbd: fix possible deadlock in smb2_open"
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Applied it to #ksmbd-for-next-next.
Thank you for the patch!
> ---
> diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
> index 0f3aad12e495..d3437f6644e3 100644
> --- a/fs/smb/server/vfs.c
> +++ b/fs/smb/server/vfs.c
> @@ -1282,6 +1282,7 @@ int ksmbd_vfs_kern_path_locked(struct ksmbd_work *work, char *name,
>
>                 err = ksmbd_vfs_lock_parent(parent_path->dentry, path->dentry);
>                 if (err) {
> +                       mnt_drop_write(parent_path->mnt);
>                         path_put(path);
>                         path_put(parent_path);
>                 }

Re: [RFC] MNT_WRITE_HOLD mess

[Christian Brauner]

On Fri, Jul 04, 2025 at 08:44:14PM +0100, Al Viro wrote:
> 	Scalable handling of writers count on a mount had been added
> back in 2009, when the counter got split into per-cpu components.
> The tricky part, of course, was on the "make it read-only" side and the
> way it had been done was to add a flag for "we are adding that shit up,
> feel free to increment, but don't get through the mnt_get_write_access()
> until we are finished".
> 
> 	Details are in mnt_get_write_access() and mnt_{,un}hold_writers().
> Current rules:
> 	* mnt_hold_writers()/mnt_unhold_writers() should be in the same
> mount_lock scope.
> 	* any successful mnt_hold_writers() must be followed by mnt_unhold_writers()
> within the same scope.
> 	* mnt_get_write_access() *MAY* take and release mount_lock, but only
> if there's somebody currently playing with mnt_hold_writers() on the same mount.
> 
> 	The non-obvious trouble came in 2011 (in 4ed5e82fe77f "vfs: protect
> remounting superblock read-only") when we got sb_prepare_remount_readonly(),
> esssentially doing mnt_hold_writers() for each mount over given superblock.
> I hadn't realized the implications until this year ;-/

We had issues here before. I fixed a bug where ->mnt_flags was written
back incompletely due to the sb_prepare_remount_readonly() last year.

> 
> 	The trouble is, as soon as mount has been added to ->s_mounts
> (currently in vfs_create_mount() and clone_mnt()) it becomes accessible
> to other threads, even if only in a very limited way.
> 
> 	That breaks very natural assumptions, just lightly enough to make
> the resulting races hard to detect.  Note, for example, this in ovl_get_upper():
> 	upper_mnt = clone_private_mount(upperpath);
> 	err = PTR_ERR(upper_mnt);
> 	if (IS_ERR(upper_mnt)) {
> 		pr_err("failed to clone upperpath\n");
> 		goto out;
> 	}
> 
> 	/* Don't inherit atime flags */
> 	upper_mnt->mnt_flags &= ~(MNT_NOATIME | MNT_NODIRATIME | MNT_RELATIME);
> See the problem?  upper_mnt has been returned by clone_private_mount() and at
> the moment it has no references to it anywhere other than this local variable.
> It is not a part of any mount tree, it is not hashed, it is not reachable via
> propagation graph, it is not on any expiry lists, etc.  Except that it *is*
> on ->s_mounts of underlying superblock (linked via ->mnt_instance) and should
> anybody try to call sb_prepare_remount_readonly(), we may end up fucking
> the things up in all kinds of interesting ways - e.g.
> 	mnt_hold_writers() sets MNT_WRITE_HOLD
> 	we fetch ->mnt_flags
> 	mnt_unhold_writers() clears MNT_WRITE_HOLD
> 	we remove the atime bits and store ->mnt_flags, bringing MNT_WRITE_HOLD
> back.  Makes for a very unhappy mnt_get_write_access() down the road...

I'm a bit confused by this example. Like, where would we fetch
mnt->mnt_flags with MNT_WRITE_HOLD set and then write it back
unconditonally?

But bigger picture. We should cleanup mnt->mnt_flags:

* Really, clone_private_mount() should be extended using a flags
  argument instead of messing around with the flags afterwards like it
  does now. That's just broken and will always invite for subtle bugs.
  Doing that kills any direct access to mnt->mnt_flags from overlayfs.

* secretment_init() should not mess directly with
  mnt->mnt_flags |= MNT_NOEXEC either. That's also just wrong. That
  should be replaced with SB_I_NOEXEC on the superblock itself.

* Stuff like MNT_INTERNAL does not have to be in
  mnt->mnt_flags afaict. It could easily live in struct mount and we can
  have a mnt_is_internal() thing that can be used by security/ which is
  the only non-core VFS part that cares about that.

After these changes we're left with two non-core VFS accesses to
mnt->mnt_flags: nfs and ocfs2. Both care about access time settings.

We add mnt->mnt_flags accessors that only returns proper mount
options/user flags and never any of the private VFS flags.

Afterwards we document direct access or modification of mnt->mnt_flags
outside core VFS as unsupported and unsafe.

Then we at least have clear rules.

> 
> 	The races are real.  Some of them can be dealt with by grabbing
> mount_lock around the problematic modifications of ->mnt_flags, but that's
> not an option outside of fs/namespace.c - I really don't want to export
> mount_lock *or* provide a "clear these flags and set these ones" exported
> primitive.
> 
> 	In any case, the underlying problem is that we have this state
> of struct mount when it's almost, but not entirely thread-local.  That's
> likely to cause future bugs of the same sort.
> 
> 	I'd been playing with delaying the insertion into ->s_mounts
> and making mnt_get_write_access() fail and complain if mount hasn't been
> inserted yet.  It's... doable, but not pleasant.  However, there's
> another approach that might be better.
> 
> 	We have ->mnt_instance, which is used only under mount_lock.
> We have two places where we insert, one place where we remove and
> two places (both in sb_prepare_remount_readonly()) where we iterate
> through the list.
> 
> 	What if we steal LSB of ->mnt_instance.prev for what MNT_WRITE_HOLD

Fine by me but please use proper accessor like we do in the struct fd
case and give the bit(s) proper name(s). IWe're all very attuned to this
because we know the struct fd code. But everyime someone hears about
this for the first time they look confused.

> is currently used for?  list_for_each_entry() won't give a damn;
> list_del() and list_add_tail() are called in mount_lock scopes that do
> not contain any calls of mnt_hold_writers(), so they'll see that LSB
> clear and work as usual.  Loop in mnt_get_write_access() could just
> as easily do
>         while ((unsigned long)READ_ONCE(mnt->mnt_instance.prev) & 1) {
> as the current
>         while (READ_ONCE(mnt->mnt.mnt_flags) & MNT_WRITE_HOLD) {
> and no barriers need to be changed.  We might want to move
> ->mnt_instance closer to ->mnt or to ->mnt_pcp, for the sake of
> the number of cachelines we are accessing, but then ->mnt_instance
> is pretty close to ->mnt_pcp as it is.

Re: [RFC] MNT_WRITE_HOLD mess

[Jan Kara]

On Sun 06-07-25 00:26:21, Al Viro wrote:
> On Sat, Jul 05, 2025 at 07:53:59PM +0100, Al Viro wrote:
> > On Sat, Jul 05, 2025 at 01:01:14AM +0100, Al Viro wrote:
> > 
> > > FWIW, several observations around that thing:
> > > 	* mnt_get_write_access(), vfs_create_mount() and clone_mnt()
> > > definitely do not need to touch the seqcount component of mount_lock.
> > > read_seqlock_excl() is enough there.
> > > 	* AFAICS, the same goes for sb_prepare_remount_readonly(),
> > > do_remount() and do_reconfigure_mnt() - no point bumping the seqcount
> > > side of mount_lock there; only spinlock is needed.
> > > 	* failure exit in mount_setattr_prepare() needs only clearing the
> > > bit; smp_wmb() is pointless there (especially done for each mount involved).
> > 
> > The following appears to work; writing docs now...
> 
> 	More fun questions in the area: is there any reason we have mnt_want_write()
> doing
> int mnt_want_write(struct vfsmount *m)
> {
>         int ret;
> 
>         sb_start_write(m->mnt_sb);
>         ret = mnt_get_write_access(m);
>         if (ret)
>                 sb_end_write(m->mnt_sb);
>         return ret;
> }
> rather than
> int mnt_want_write(struct vfsmount *m)
> {
>         int ret = mnt_get_write_access(m);
> 	if (!ret)
> 		sb_start_write(m->mnt_sb);
>         return ret;
> }
> 
> 	Note that mnt_want_write_file() on e.g. a regular file opened
> for write will have sb_start_write() done with mnt_get_write_access()
> already in place since open(2).  So the nesting shouldn't be an issue
> here...  The same order (mount then superblock) is used for overlayfs
> copyup, for that matter.
> 
> 	So if it's a matter of waiting for thaw with mount write
> count already incremented, simple echo foo > pathname would already
> demonstrate the same, no matter of which order mnt_want_write() uses.
> What am I missing there?
> 
> 	IIRC, that was Jan's stuff... <checks git log> yep - eb04c28288bb
> "fs: Add freezing handling to mnt_want_write() / mnt_drop_write()" is
> where it came from...

I was thinking about this for some time. I think it was just a pure caution
to hold as few resources as possible when waiting for fs to thaw - in
particular I think I wanted to avoid returning EBUSY when remounting
mountpoint of a frozen fs just because there's somebody blocked in
mnt_want_write() waiting for fs to thaw. And yes, when there's file open
for writing, we will wait for fs to thaw with write count elevated but in
that case EBUSY from remount is kind of expected outcome anyway. I was more
concerned about cases like mkdir etc. So overall I wouldn't say there's a
strong reason for the ordering I've chosen, it just seemed a bit more user
friendly.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR