Message-Id: <20210121131959.646623-32-christian.brauner@ubuntu.com>
Date:   Thu, 21 Jan 2021 14:19:50 +0100
From:   Christian Brauner <christian.brauner@...ntu.com>
To:     Alexander Viro <viro@...iv.linux.org.uk>,
        Christoph Hellwig <hch@....de>, linux-fsdevel@...r.kernel.org
Cc:     John Johansen <john.johansen@...onical.com>,
        James Morris <jmorris@...ei.org>,
        Mimi Zohar <zohar@...ux.ibm.com>,
        Dmitry Kasatkin <dmitry.kasatkin@...il.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Casey Schaufler <casey@...aufler-ca.com>,
        Arnd Bergmann <arnd@...db.de>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>,
        Geoffrey Thomas <geofft@...reload.com>,
        Mrunal Patel <mpatel@...hat.com>,
        Josh Triplett <josh@...htriplett.org>,
        Andy Lutomirski <luto@...nel.org>,
        Theodore Tso <tytso@....edu>, Alban Crequy <alban@...volk.io>,
        Tycho Andersen <tycho@...ho.ws>,
        David Howells <dhowells@...hat.com>,
        James Bottomley <James.Bottomley@...senpartnership.com>,
        Seth Forshee <seth.forshee@...onical.com>,
        Stéphane Graber <stgraber@...ntu.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Aleksa Sarai <cyphar@...har.com>,
        Lennart Poettering <lennart@...ttering.net>,
        "Eric W. Biederman" <ebiederm@...ssion.com>, smbarber@...omium.org,
        Phil Estes <estesp@...il.com>, Serge Hallyn <serge@...lyn.com>,
        Kees Cook <keescook@...omium.org>,
        Todd Kjos <tkjos@...gle.com>, Paul Moore <paul@...l-moore.com>,
        Jonathan Corbet <corbet@....net>,
        containers@...ts.linux-foundation.org,
        linux-security-module@...r.kernel.org, linux-api@...r.kernel.org,
        linux-ext4@...r.kernel.org, linux-xfs@...r.kernel.org,
        linux-integrity@...r.kernel.org, selinux@...r.kernel.org,
        Christian Brauner <christian.brauner@...ntu.com>
Subject: [PATCH v6 31/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.

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].
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")

Link: https://lore.kernel.org/r/20210112220124.837960-4-christian.brauner@ubuntu.com
Cc: David Howells <dhowells@...hat.com>
Cc: Al Viro <viro@...iv.linux.org.uk>
Cc: linux-fsdevel@...r.kernel.org
Reviewed-by: Christoph Hellwig <hch@....de>
Signed-off-by: Christian Brauner <christian.brauner@...ntu.com>
---
/* v2 */
unchanged

/* v3 */
unchanged

/* v4 */
unchanged

/* v5 */
unchanged
base-commit: 7c53f6b671f4aba70ff15e1b05148b10d58c2837

/* v6 */
unchanged
base-commit: 19c329f6808995b142b3966301f217c831e7cf31
---
 fs/namespace.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 5fceb2854395..367f1c7cb6db 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2527,10 +2527,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;
@@ -2580,13 +2576,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);
 	lock_mount_hash();
 	ret = change_mount_ro_state(mnt, mnt_flags);
 	if (ret == 0)
 		set_mount_attributes(mnt, mnt_flags);
 	unlock_mount_hash();
-	up_write(&sb->s_umount);
+	up_read(&sb->s_umount);
 
 	mnt_warn_timestamp_expiry(path, &mnt->mnt);
 
-- 
2.30.0

Powered by blists - more mailing lists