fs: Add to super_blocks list after SB_BORN is set.

Commit Message

Tetsuo Handa July 13, 2018, 10:09 a.m. UTC

More simple version. Is this assumption correct?

From 52f10183f2b921cd946dd4c83d9b1d99bc48914e Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <[e-mail]>
Date: Fri, 13 Jul 2018 12:15:54 +0900
Subject: [PATCH] fs: Add to super_blocks list after SB_BORN is set.

syzbot is reporting hung tasks at iterate_supers() [1]. This is because
iterate_supers() is calling down_read(&sb->s_umount) on all superblocks
found by traversing super_blocks list while sget_userns() adds "newly
created superblock to super_blocks list with ->s_umount held by
alloc_super() for write" before "mount_fs() sets SB_BORN flag after
returning from type->mount()".

If type->mount() took long time after returning from sget() for some
reason, e.g. sync() request will be blocked on !SB_BORN superblocks.
Also, since security_sb_kern_mount() which is called after SB_BORN is
set might involve GFP_KERNEL memory allocation, we can't guarantee that
up_write(&sb->s_umount) is called as soon as SB_BORN was set.

Therefore, this patch makes sure that newly created superblock is added
to super_blocks list immediately before calling up_write(&sb->s_umount).

[1] https://syzkaller.appspot.com/bug?id=3c0c173ff55822aacb81ce7ae27a6676fba29a5c

Signed-off-by: Tetsuo Handa <[e-mail]>
---
 fs/super.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Al Viro July 13, 2018, noon UTC | #1

On Fri, Jul 13, 2018 at 07:09:03PM +0900, Tetsuo Handa wrote:
> More simple version. Is this assumption correct?

Racy, for obvious reasons (sget/sget)

Patch

diff --git a/fs/super.c b/fs/super.c
index 50728d9..8b2c8cc 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -236,6 +236,7 @@  static struct super_block *alloc_super(struct file_system_type *type, int flags,
 	s->s_flags = flags;
 	if (s->s_user_ns != &init_user_ns)
 		s->s_iflags |= SB_I_NODEV;
+	INIT_LIST_HEAD(&s->s_list);
 	INIT_HLIST_NODE(&s->s_instances);
 	INIT_HLIST_BL_HEAD(&s->s_roots);
 	mutex_init(&s->s_sync_lock);
@@ -527,8 +528,6 @@  struct super_block *sget_userns(struct file_system_type *type,
 	}
 	s->s_type = type;
 	strlcpy(s->s_id, type->name, sizeof(s->s_id));
-	list_add_tail(&s->s_list, &super_blocks);
-	hlist_add_head(&s->s_instances, &type->fs_supers);
 	spin_unlock(&sb_lock);
 	get_filesystem(type);
 	register_shrinker_prepared(&s->s_shrink);
@@ -1305,6 +1304,12 @@  mount_fs(struct file_system_type *type, int flags, const char *name, void *data)
 	WARN((sb->s_maxbytes < 0), "%s set sb->s_maxbytes to "
 		"negative value (%lld)\n", type->name, sb->s_maxbytes);
 
+	if (list_empty(&sb->s_list)) {
+		spin_lock(&sb_lock);
+		list_add_tail(&sb->s_list, &super_blocks);
+		hlist_add_head(&sb->s_instances, &type->fs_supers);
+		spin_unlock(&sb_lock);
+	}
 	up_write(&sb->s_umount);
 	free_secdata(secdata);
 	return root;