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

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

[Jan Kara]

Hello,

this is a second revision of the patch series which 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.).

Since v1, I have changed the logic so that all the locking & restart magic
happens in m_show() so that we don't bail back to traverse(), then m_start(),
and then back to m_show(). Al, is it better this way?

I was not able to get rid of the hold_sb() helper which you didn't like much as
namespace_sem is private to fs/namespace.c and we need to drop it after
acquiring sb reference and before blocking on sb->s_umount semaphore. So
hold_sb() still looked like the least painful solution.

								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 |  2 ++
 fs/super.c    | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/fs/internal.h b/fs/internal.h
index d410186bc369..c2c92be1e7df 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -104,6 +104,8 @@ 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);
+extern bool mount_trylock_super(struct super_block *sb);
 
 /*
  * open.c
diff --git a/fs/super.c b/fs/super.c
index ca53a08497ed..552d6247053b 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -775,6 +775,41 @@ 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);
+}
+
+/**
+ *	mount_trylock_super - get superblock and lock it against remount
+ *	@sb: superblock to get reference for
+ *
+ *	Get reference to superblock and protect superblock against racing
+ *	remount or umount. 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).
+ */
+bool mount_trylock_super(struct super_block *sb)
+{
+	hold_sb(sb);
+	if (!down_read_trylock(&sb->s_umount)) {
+		put_super(sb);
+		return false;
+	}
+	return true;
+}
+
 /**
  * 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/namespace.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index a7f91265ea67..3b7cd4465658 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1263,7 +1263,7 @@ static void *m_next(struct seq_file *m, void *v, loff_t *pos)
 {
 	struct proc_mounts *p = m->private;
 
-	p->cached_mount = seq_list_next(v, &p->ns->list, pos);
+	p->cached_mount = seq_list_next(p->cached_mount, &p->ns->list, pos);
 	p->cached_index = *pos;
 	return p->cached_mount;
 }
@@ -1276,8 +1276,38 @@ static void m_stop(struct seq_file *m, void *v)
 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 mount *r;
+	struct super_block *sb;
+	int ret;
+
+restart:
+	r = list_entry(v, struct mount, mnt_list);
+	sb = r->mnt.mnt_sb;
+
+	/* Protect show function against racing remount */
+	if (mount_trylock_super(sb))
+		goto show;
+	/*
+	 * 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);
+	down_read(&namespace_sem);
+	v = seq_list_start(&p->ns->list, p->cached_index);
+	if (!sb->s_root || v != p->cached_mount) {
+		drop_super(sb);
+		p->cached_event = p->ns->event;
+		p->cached_mount = v;
+		goto restart;
+	}
+show:
+	ret = p->show(m, &r->mnt);
+	drop_super(sb);
+	return ret;
 }
 
 const struct seq_operations mounts_op = {
-- 
2.16.4

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

[Jan Kara]

Ping Al?

On Tue 18-12-18 14:46:40, Jan Kara wrote:
> Hello,
> 
> this is a second revision of the patch series which 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.).
> 
> Since v1, I have changed the logic so that all the locking & restart magic
> happens in m_show() so that we don't bail back to traverse(), then m_start(),
> and then back to m_show(). Al, is it better this way?
> 
> I was not able to get rid of the hold_sb() helper which you didn't like much as
> namespace_sem is private to fs/namespace.c and we need to drop it after
> acquiring sb reference and before blocking on sb->s_umount semaphore. So
> hold_sb() still looked like the least painful solution.
> 
> 								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[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 ca53a08497ed..d34b8c49af9d 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 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