[PATCH] fs/namespace: handle mount(MS_BIND|MS_REMOUNT) without locking sb->s_umount

[PATCH] fs/namespace: handle mount(MS_BIND|MS_REMOUNT) without locking sb->s_umount

[Konstantin Khlebnikov]

Writeback grabs sb->s_umount for read during I/O. This blocks bind-remount
for a long time. Bind-remount actually does not need sb->s_umount locked
for read or write because it does not alter superblock, only mnt_flags.
All mnt_flags are serialized by global mount_lock.

This patch moves locking into callers to handle remount atomically.
Also grab namespace_sem to synchronize with /proc/mounts and mountinfo.
Function do_change_type() uses the same locking combination.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 fs/namespace.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 85b5f7bea82e..c6c03be5cc4e 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -459,11 +459,11 @@ void mnt_drop_write_file(struct file *file)
 }
 EXPORT_SYMBOL(mnt_drop_write_file);
 
+/* mount_lock must be held */
 static int mnt_make_readonly(struct mount *mnt)
 {
 	int ret = 0;
 
-	lock_mount_hash();
 	mnt->mnt.mnt_flags |= MNT_WRITE_HOLD;
 	/*
 	 * After storing MNT_WRITE_HOLD, we'll read the counters. This store
@@ -497,15 +497,14 @@ static int mnt_make_readonly(struct mount *mnt)
 	 */
 	smp_wmb();
 	mnt->mnt.mnt_flags &= ~MNT_WRITE_HOLD;
-	unlock_mount_hash();
+
 	return ret;
 }
 
+/* mount_lock must be held */
 static int __mnt_unmake_readonly(struct mount *mnt)
 {
-	lock_mount_hash();
 	mnt->mnt.mnt_flags &= ~MNT_READONLY;
-	unlock_mount_hash();
 	return 0;
 }
 
@@ -2440,6 +2439,7 @@ static bool can_change_locked_flags(struct mount *mnt, unsigned int mnt_flags)
 	return true;
 }
 
+/* mount_lock must be held */
 static int change_mount_ro_state(struct mount *mnt, unsigned int mnt_flags)
 {
 	bool readonly_request = (mnt_flags & MNT_READONLY);
@@ -2454,16 +2454,14 @@ static int change_mount_ro_state(struct mount *mnt, unsigned int mnt_flags)
 }
 
 /*
- * Update the user-settable attributes on a mount.  The caller must hold
- * sb->s_umount for writing.
+ * Update the user-settable attributes on a mount.
+ * mount_lock must be held.
  */
 static void set_mount_attributes(struct mount *mnt, unsigned int mnt_flags)
 {
-	lock_mount_hash();
 	mnt_flags |= mnt->mnt.mnt_flags & ~MNT_USER_SETTABLE_MASK;
 	mnt->mnt.mnt_flags = mnt_flags;
 	touch_mnt_namespace(mnt->mnt_ns);
-	unlock_mount_hash();
 }
 
 static void mnt_warn_timestamp_expiry(struct path *mountpoint, struct vfsmount *mnt)
@@ -2495,7 +2493,6 @@ static void mnt_warn_timestamp_expiry(struct path *mountpoint, struct vfsmount *
  */
 static int do_reconfigure_mnt(struct path *path, unsigned int mnt_flags)
 {
-	struct super_block *sb = path->mnt->mnt_sb;
 	struct mount *mnt = real_mount(path->mnt);
 	int ret;
 
@@ -2508,11 +2505,13 @@ 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);
+	namespace_lock();
+	lock_mount_hash();
 	ret = change_mount_ro_state(mnt, mnt_flags);
 	if (ret == 0)
 		set_mount_attributes(mnt, mnt_flags);
-	up_write(&sb->s_umount);
+	unlock_mount_hash();
+	namespace_unlock();
 
 	mnt_warn_timestamp_expiry(path, &mnt->mnt);
 
@@ -2551,8 +2550,11 @@ static int do_remount(struct path *path, int ms_flags, int sb_flags,
 		err = -EPERM;
 		if (ns_capable(sb->s_user_ns, CAP_SYS_ADMIN)) {
 			err = reconfigure_super(fc);
-			if (!err)
+			if (!err) {
+				lock_mount_hash();
 				set_mount_attributes(mnt, mnt_flags);
+				unlock_mount_hash();
+			}
 		}
 		up_write(&sb->s_umount);
 	}

Re: [PATCH] fs/namespace: handle mount(MS_BIND|MS_REMOUNT) without locking sb->s_umount

[Matthew Wilcox]

On Wed, Mar 18, 2020 at 05:17:55PM +0300, Konstantin Khlebnikov wrote:
> @@ -459,11 +459,11 @@ void mnt_drop_write_file(struct file *file)
>  }
>  EXPORT_SYMBOL(mnt_drop_write_file);
>  
> +/* mount_lock must be held */
>  static int mnt_make_readonly(struct mount *mnt)
>  {
>  	int ret = 0;
>  
> -	lock_mount_hash();

I'd rather see
+	lockdep_assert_held_write(&mount_lock);
than a comment.  Maybe wrapped up into a macro like assert_hash_locked().