lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri,  4 Dec 2020 00:56:59 +0100
From:   Christian Brauner <>
To:     Alexander Viro <>,
        Christoph Hellwig <>,
Cc:     John Johansen <>,
        James Morris <>,
        Mimi Zohar <>,
        Dmitry Kasatkin <>,
        Stephen Smalley <>,
        Casey Schaufler <>,
        Arnd Bergmann <>,
        Andreas Dilger <>,
        OGAWA Hirofumi <>,
        Geoffrey Thomas <>,
        Mrunal Patel <>,
        Josh Triplett <>,
        Andy Lutomirski <>,
        Theodore Tso <>, Alban Crequy <>,
        Tycho Andersen <>,
        David Howells <>,
        James Bottomley <>,
        Seth Forshee <>,
        St├ęphane Graber <>,
        Aleksa Sarai <>,
        Lennart Poettering <>,
        "Eric W. Biederman" <>,,
        Phil Estes <>, Serge Hallyn <>,
        Kees Cook <>,
        Todd Kjos <>, Paul Moore <>,
        Jonathan Corbet <>,,,,,,,
        Christian Brauner <>,
        Christoph Hellwig <>
Subject: [PATCH v4 03/40] namespace: only take read lock in do_reconfigure_mnt()

do_reconfigure_mnt() used to take the down_write(&sb->s_umount) lock
which seems unnecessary since we're not changing the superblock. We're
only checking whether it is already read-only. Setting other mount
attributes is protected by lock_mount_hash() afaict and not by s_umount.

So I think the history of down_write(&sb->s_umount) lock being taken
when setting mount attributes dates back to the introduction of
MNT_READONLY in [2]. Afaict, this introduced the concept of having
read-only mounts in contrast to just having a read-only superblock. When
it got introduced it was simply plumbed into do_remount() which already
took down_write(&sb->s_umount) because it was only used to actually
change the superblock before [2]. Afaict, it would've already been
possible back then to only use down_read(&sb->s_umount) for MS_BIND |
MS_REMOUNT since actual mount options were protected by the vfsmount
lock already. But that would've meant special casing the locking for
MS_BIND | MS_REMOUNT in do_remount() which people might not have
considered worth it.
Then in [1] MS_BIND | MS_REMOUNT mount option changes were split out of
do_remount() into do_reconfigure_mnt() but the down_write(&sb->s_umount)
lock was simply copied over.
Now that we have this be a separate helper only take the
down_read(&sb->s_umount) lock since we're only interested in checking
whether the super block is currently read-only and blocking any writers
from changing it. Essentially, checking that the super block is
read-only has the advantage that we can avoid having to go into the
slowpath and through MNT_WRITE_HOLD and can simply set the read-only
flag on the mount in set_mount_attributes().

[1]: commit 43f5e655eff7 ("vfs: Separate changing mount flags full remount")
[2]: commit 2e4b7fcd9260 ("[PATCH] r/o bind mounts: honor mount writer counts at remount")
Cc: David Howells <>
Cc: Al Viro <>
Signed-off-by: Christian Brauner <>
Reviewed-by: Christoph Hellwig <>
/* v2 */

/* v3 */

/* v4 */
 fs/namespace.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 798bbf4f48ad..8497d149ecaa 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2512,10 +2512,6 @@ static int change_mount_ro_state(struct mount *mnt, unsigned int mnt_flags)
 	return 0;
- * Update the user-settable attributes on a mount.  The caller must hold
- * sb->s_umount for writing.
- */
 static void set_mount_attributes(struct mount *mnt, unsigned int mnt_flags)
 	mnt_flags |= mnt->mnt.mnt_flags & ~MNT_USER_SETTABLE_MASK;
@@ -2565,13 +2561,17 @@ static int do_reconfigure_mnt(struct path *path, unsigned int mnt_flags)
 	if (!can_change_locked_flags(mnt, mnt_flags))
 		return -EPERM;
-	down_write(&sb->s_umount);
+	/*
+	 * We're only checking whether the superblock is read-only not changing
+	 * it, so only take down_read(&sb->s_umount).
+	 */
+	down_read(&sb->s_umount);
 	ret = change_mount_ro_state(mnt, mnt_flags);
 	if (ret == 0)
 		set_mount_attributes(mnt, mnt_flags);
-	up_write(&sb->s_umount);
+	up_read(&sb->s_umount);
 	mnt_warn_timestamp_expiry(path, &mnt->mnt);

Powered by blists - more mailing lists