[Devel] [PATCH RHEL7 COMMIT] ms/namespace: only take read lock in do_reconfigure_mnt()

[Konstantin Khorenko]

The commit is pushed to "branch-rh7-3.10.0-1160.88.1.vz7.195.x-ovz" and will appear at https://src.openvz.org/scm/ovz/vzkernel.git
after rh7-3.10.0-1160.88.1.vz7.195.2
------>
commit ec3ead48b0e1a68afb61dbd1f802e5c747b33223
Author: Christian Brauner <christian.brauner at ubuntu.com>
Date:   Thu Apr 13 18:47:21 2023 +0800

    ms/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/20210121131959.646623-32-christian.brauner@ubuntu.com
    Cc: David Howells <dhowells at redhat.com>
    Cc: Al Viro <viro at zeniv.linux.org.uk>
    Cc: linux-fsdevel at vger.kernel.org
    Reviewed-by: Christoph Hellwig <hch at lst.de>
    Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
    
    (cherry picked from commit e58ace1a0fa9d578f85f556b4b88c5fe9b871d08)
    https://jira.sw.ru/browse/PSBM-144416
    Signed-off-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
    
    =================
    Patchset description:
    mount: Port move_mount_set_group and mount_setattr
    
    We need this as in Virtuozzo criu after rebase to mainstream criu in u20
    we will switch to this new API for sharing group setting accross mounts.
    
    https://jira.vzint.dev/browse/PSBM-144416
---
 fs/namespace.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 9e58a2ff16ea..c03e21575d08 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2701,10 +2701,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;
@@ -2732,13 +2728,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);
 	return ret;
 }