[PATCH 0/2] vfs: Fix crashes when reading /proc/mounts

[PATCH 0/2] vfs: Fix crashes when reading /proc/mounts

[Jan Kara]

Hello,

this patch series aims at fixing possible races between functions formatting
output for /proc/mounts and friends and remount. Especially ->show_options
functions of filesystems are not counting with the fact that options can
change under them and thus races can result in bogus output or outright
crashes (like was the case with ext4 handling of quota mount option, or udf
could crash when printing charset name, or xfs when printing log device name
etc.).

Admittedly the patch 2/2 is somewhat ugly because namespace_sem ranks below
sb->s_umount semaphore but I think it is bearable. Al, what do you think?

								Honza

[PATCH 1/2] vfs: Provide function to grab superblock reference

[Jan Kara]

Provide function to hold superblock reference when we are sure the
superblock is active. This function will get used by code reading
/proc/mounts and friends.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/internal.h |  1 +
 fs/super.c    | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/fs/internal.h b/fs/internal.h
index d410186bc369..608f4f276ba5 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -104,6 +104,7 @@ extern bool trylock_super(struct super_block *sb);
 extern struct dentry *mount_fs(struct file_system_type *,
 			       int, const char *, void *);
 extern struct super_block *user_get_super(dev_t);
+extern void hold_sb(struct super_block *sb);
 
 /*
  * open.c
diff --git a/fs/super.c b/fs/super.c
index f3a8c008e164..f60a07f794cc 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -775,6 +775,22 @@ struct super_block *get_super_exclusive_thawed(struct block_device *bdev)
 }
 EXPORT_SYMBOL(get_super_exclusive_thawed);
 
+/**
+ *	hold_sb - get superblock reference for active superblock
+ *	@sb: superblock to get reference for
+ *
+ *	Get reference to superblock. The caller must make sure the superblock
+ *	is currently active by other means (e.g. holding an active reference
+ * 	itself or holding namespace_sem preventing unmount).
+ */
+void hold_sb(struct super_block *sb)
+{
+	spin_lock(&sb_lock);
+	WARN_ON_ONCE(!atomic_read(&sb->s_active));
+	sb->s_count++;
+	spin_unlock(&sb_lock);
+}
+
 /**
  * get_active_super - get an active reference to the superblock of a device
  * @bdev: device to get the superblock for
-- 
2.16.4

[PATCH 2/2] proc: Protect readers of /proc/mounts from remount

[Jan Kara]

Readers of /proc/mounts (and similar files) are currently unprotected
from concurrently running remount on the filesystem they are reporting.
This can not only result in bogus reported information but also in
confusion in filesystems ->show_options callbacks resulting in
use-after-free issues or similar (for example ext4 handling of quota
file names is prone to such races).

Fix the problem by protecting showing of mount information with
sb->s_umount semaphore.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/mount.h     |  1 +
 fs/namespace.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/fs/mount.h b/fs/mount.h
index f39bc9da4d73..5ca0a9e714b3 100644
--- a/fs/mount.h
+++ b/fs/mount.h
@@ -134,6 +134,7 @@ struct proc_mounts {
 	void *cached_mount;
 	u64 cached_event;
 	loff_t cached_index;
+	struct super_block *locked_sb;
 };
 
 extern const struct seq_operations mounts_op;
diff --git a/fs/namespace.c b/fs/namespace.c
index 99186556f8d3..31270baa4c0b 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1237,26 +1237,68 @@ struct vfsmount *mnt_clone_internal(const struct path *path)
 }
 
 #ifdef CONFIG_PROC_FS
+
+static bool mounts_trylock_super(struct proc_mounts *p, struct super_block *sb)
+{
+	if (p->locked_sb == sb)
+		return true;
+	if (p->locked_sb) {
+		drop_super(p->locked_sb);
+		p->locked_sb = NULL;
+	}
+	if (down_read_trylock(&sb->s_umount)) {
+		hold_sb(sb);
+		p->locked_sb = sb;
+		return true;
+	}
+	return false;
+}
+
 /* iterator; we want it to have access to namespace_sem, thus here... */
 static void *m_start(struct seq_file *m, loff_t *pos)
 {
 	struct proc_mounts *p = m->private;
+	struct mount *mount;
+	struct super_block *sb;
 
+restart:
 	down_read(&namespace_sem);
 	if (p->cached_event == p->ns->event) {
 		void *v = p->cached_mount;
 		if (*pos == p->cached_index)
-			return v;
+			goto lock_sb;
 		if (*pos == p->cached_index + 1) {
 			v = seq_list_next(v, &p->ns->list, &p->cached_index);
-			return p->cached_mount = v;
+			p->cached_mount = v;
+			goto lock_sb;
 		}
 	}
 
 	p->cached_event = p->ns->event;
 	p->cached_mount = seq_list_start(&p->ns->list, *pos);
 	p->cached_index = *pos;
-	return p->cached_mount;
+lock_sb:
+	if (!p->cached_mount)
+		return NULL;
+	mount = list_entry(p->cached_mount, struct mount, mnt_list);
+	sb = mount->mnt.mnt_sb;
+	if (mounts_trylock_super(p, sb))
+		return p->cached_mount;
+	/*
+	 * Trylock failed. Since namepace_sem ranks below s_umount (through
+	 * sb->s_umount > dir->i_rwsem > namespace_sem in the mount path), we
+	 * have to drop it, wait for s_umount and then try again to guarantee
+	 * forward progress.
+	 */
+	hold_sb(sb);
+	up_read(&namespace_sem);
+	down_read(&sb->s_umount);
+	/*
+	 * Sb may be dead by now but that just means we won't find it in any
+	 * mount and drop lock & reference anyway.
+	 */
+	p->locked_sb = sb;
+	goto restart;
 }
 
 static void *m_next(struct seq_file *m, void *v, loff_t *pos)
@@ -1277,7 +1319,16 @@ static int m_show(struct seq_file *m, void *v)
 {
 	struct proc_mounts *p = m->private;
 	struct mount *r = list_entry(v, struct mount, mnt_list);
-	return p->show(m, &r->mnt);
+	struct super_block *sb = r->mnt.mnt_sb;
+	int ret;
+
+	/* Protect show function against racing remount */
+	if (!mounts_trylock_super(p, sb))
+		return -EAGAIN;
+	ret = p->show(m, &r->mnt);
+	drop_super(sb);
+	p->locked_sb = NULL;
+	return ret;
 }
 
 const struct seq_operations mounts_op = {
-- 
2.16.4