Re: [PATCH] shmem: don't call put_super() when fill_super() failed.

[Eric Biggers <ebiggers3@gmail.com>]

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