linux-kernel - [PATCH v2] fs/namespace: handle mount(MS_BIND|MS_REMOUNT) without locking sb->s

lists.openwall.net		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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <158497590858.7371.9311902565121473436.stgit@buzz>
Date:   Mon, 23 Mar 2020 18:05:08 +0300
From:   Konstantin Khlebnikov <khlebnikov@...dex-team.ru>
To:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Alexander Viro <viro@...iv.linux.org.uk>
Cc:     David Howells <dhowells@...hat.com>,
        Dmitry Monakhov <dmtrmonakhov@...dex-team.ru>,
        Matthew Wilcox <willy@...radead.org>
Subject: [PATCH v2] fs/namespace: handle mount(MS_BIND|MS_REMOUNT) without
 locking sb->s_umount

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.

v2:
- inline helpers into change_mount_ro_state
- verify lock with lockdep_assert_held_write

Link: https://lore.kernel.org/lkml/158454107541.4470.14819321770893756073.stgit@buzz/ (v1)
Signed-off-by: Konstantin Khlebnikov <khlebnikov@...dex-team.ru>
---
 fs/namespace.c |   58 +++++++++++++++++++++++++-------------------------------
 1 file changed, 26 insertions(+), 32 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 85b5f7bea82e..d394a4c414a2 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -459,11 +459,22 @@ void mnt_drop_write_file(struct file *file)
 }
 EXPORT_SYMBOL(mnt_drop_write_file);
 
-static int mnt_make_readonly(struct mount *mnt)
+/* 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);
 	int ret = 0;
 
-	lock_mount_hash();
+	lockdep_assert_held_write(&mount_lock.seqcount);
+
+	if (readonly_request == __mnt_is_readonly(&mnt->mnt))
+		goto out;
+
+	if (!readonly_request) {
+		mnt->mnt.mnt_flags &= ~MNT_READONLY;
+		goto out;
+	}
+
 	mnt->mnt.mnt_flags |= MNT_WRITE_HOLD;
 	/*
 	 * After storing MNT_WRITE_HOLD, we'll read the counters. This store
@@ -497,16 +508,9 @@ static int mnt_make_readonly(struct mount *mnt)
 	 */
 	smp_wmb();
 	mnt->mnt.mnt_flags &= ~MNT_WRITE_HOLD;
-	unlock_mount_hash();
-	return ret;
-}
 
-static int __mnt_unmake_readonly(struct mount *mnt)
-{
-	lock_mount_hash();
-	mnt->mnt.mnt_flags &= ~MNT_READONLY;
-	unlock_mount_hash();
-	return 0;
+out:
+	return ret;
 }
 
 int sb_prepare_remount_readonly(struct super_block *sb)
@@ -2440,30 +2444,16 @@ static bool can_change_locked_flags(struct mount *mnt, unsigned int mnt_flags)
 	return true;
 }
 
-static int change_mount_ro_state(struct mount *mnt, unsigned int mnt_flags)
-{
-	bool readonly_request = (mnt_flags & MNT_READONLY);
-
-	if (readonly_request == __mnt_is_readonly(&mnt->mnt))
-		return 0;
-
-	if (readonly_request)
-		return mnt_make_readonly(mnt);
-
-	return __mnt_unmake_readonly(mnt);
-}
-
 /*
- * 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();
+	lockdep_assert_held_write(&mount_lock.seqcount);
 	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 +2485,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 +2497,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 +2542,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);
 	}