| Message ID | 201805140657.w4E6vV4a035377@www262.sakura.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tetsuo, On Mon, May 14, 2018 at 03:57:31PM +0900, Tetsuo Handa wrote: > From 193d9cb8b5dfc50c693d4bdd345cedb615bbf5ae Mon Sep 17 00:00:00 2001 > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Date: Mon, 14 May 2018 15:25:13 +0900 > Subject: [PATCH] shmem: don't call put_super() when fill_super() failed. > > syzbot is reporting NULL pointer dereference at shmem_unused_huge_count() > [1]. This is because shmem_fill_super() is calling shmem_put_super() which > immediately releases memory before unregister_shrinker() is called by > deactivate_locked_super() after fill_super() in mount_nodev() failed. > Fix this by leaving the call to shmem_put_super() to > generic_shutdown_super() from kill_anon_super() from kill_litter_super() > from deactivate_locked_super(). > > [1] https://syzkaller.appspot.com/bug?id=46e792849791f4abbac898880e8522054e032391 > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Reported-by: syzbot <syzbot+d2586fde8fdcead3647f@syzkaller.appspotmail.com> > Cc: Al Viro <viro@ZenIV.linux.org.uk> > --- > mm/shmem.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/mm/shmem.c b/mm/shmem.c > index 9d6c7e5..18e134c 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -3843,7 +3843,6 @@ int shmem_fill_super(struct super_block *sb, void *data, int silent) > return 0; > > failed: > - shmem_put_super(sb); > return err; > } > > -- > 1.8.3.1 I'm not following, since generic_shutdown_super() only calls ->put_super() if ->s_root is set, which only happens at the end of shmem_fill_super(). Isn't the real problem that s_shrink is registered too early, causing super_cache_count() and shmem_unused_huge_count() to potentially run before shmem_fill_super() has completed? Or alternatively, the problem is that super_cache_count() doesn't check for SB_ACTIVE. Eric
On Mon, May 14, 2018 at 10:04:23AM -0700, Eric Biggers wrote: > Hi Tetsuo, > > On Mon, May 14, 2018 at 03:57:31PM +0900, Tetsuo Handa wrote: > > From 193d9cb8b5dfc50c693d4bdd345cedb615bbf5ae Mon Sep 17 00:00:00 2001 > > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > > Date: Mon, 14 May 2018 15:25:13 +0900 > > Subject: [PATCH] shmem: don't call put_super() when fill_super() failed. > > > > syzbot is reporting NULL pointer dereference at shmem_unused_huge_count() > > [1]. This is because shmem_fill_super() is calling shmem_put_super() which > > immediately releases memory before unregister_shrinker() is called by > > deactivate_locked_super() after fill_super() in mount_nodev() failed. > > Fix this by leaving the call to shmem_put_super() to > > generic_shutdown_super() from kill_anon_super() from kill_litter_super() > > from deactivate_locked_super(). > > > > [1] https://syzkaller.appspot.com/bug?id=46e792849791f4abbac898880e8522054e032391 > > > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > > Reported-by: syzbot <syzbot+d2586fde8fdcead3647f@syzkaller.appspotmail.com> > > Cc: Al Viro <viro@ZenIV.linux.org.uk> > > --- > > mm/shmem.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/mm/shmem.c b/mm/shmem.c > > index 9d6c7e5..18e134c 100644 > > --- a/mm/shmem.c > > +++ b/mm/shmem.c > > @@ -3843,7 +3843,6 @@ int shmem_fill_super(struct super_block *sb, void *data, int silent) > > return 0; > > > > failed: > > - shmem_put_super(sb); > > return err; > > } > > > > -- > > 1.8.3.1 > > I'm not following, since generic_shutdown_super() only calls ->put_super() if > ->s_root is set, which only happens at the end of shmem_fill_super(). Isn't the > real problem that s_shrink is registered too early, causing super_cache_count() > and shmem_unused_huge_count() to potentially run before shmem_fill_super() has > completed? Or alternatively, the problem is that super_cache_count() doesn't > check for SB_ACTIVE. > Coincidentally, this is already going to be fixed by commit 79f546a696bff259 ("fs: don't scan the inode cache before SB_BORN is set") in vfs/for-linus. - Eric
On Mon, May 14, 2018 at 10:11:54AM -0700, Eric Biggers wrote: > > I'm not following, since generic_shutdown_super() only calls ->put_super() if > > ->s_root is set, which only happens at the end of shmem_fill_super(). Isn't the > > real problem that s_shrink is registered too early, causing super_cache_count() > > and shmem_unused_huge_count() to potentially run before shmem_fill_super() has > > completed? Or alternatively, the problem is that super_cache_count() doesn't > > check for SB_ACTIVE. > > > > Coincidentally, this is already going to be fixed by commit 79f546a696bff259 > ("fs: don't scan the inode cache before SB_BORN is set") in vfs/for-linus. Exactly. While we are at it, we could add static void shmem_kill_super(struct super_block *sb) { struct shmem_sb_info *sbinfo = SHMEM_SB(sb); kill_litter_super(sb); if (sbinfo) { percpu_counter_destroy(&sbinfo->used_blocks); mpol_put(sbinfo->mpol); kfree(sbinfo); } } use that for ->kill_sb() and to hell with shmem_put_super() *and* its call in cleanup path of shmem_fill_super() - these err = -E...; goto failed; in there become simply return -E...;
Eric Biggers wrote: > > I'm not following, since generic_shutdown_super() only calls ->put_super() if > > ->s_root is set, which only happens at the end of shmem_fill_super(). Isn't the > > real problem that s_shrink is registered too early, causing super_cache_count() > > and shmem_unused_huge_count() to potentially run before shmem_fill_super() has > > completed? Or alternatively, the problem is that super_cache_count() doesn't > > check for SB_ACTIVE. > > > > Coincidentally, this is already going to be fixed by commit 79f546a696bff259 > ("fs: don't scan the inode cache before SB_BORN is set") in vfs/for-linus. Indeed. This is use before initialisation bug which will be fixed by commit 79f546a696bff259. #syz fix: fs: don't scan the inode cache before SB_BORN is set
diff --git a/mm/shmem.c b/mm/shmem.c index 9d6c7e5..18e134c 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -3843,7 +3843,6 @@ int shmem_fill_super(struct super_block *sb, void *data, int silent) return 0; failed: - shmem_put_super(sb); return err; }