Message-ID: <8917bbe175172b4fd9c9f92bf2a52443eb2b1827.camel@themaw.net>
Date:   Tue, 18 Dec 2018 20:42:50 +0800
From:   Ian Kent <raven@...maw.net>
To:     Dmitry Vyukov <dvyukov@...gle.com>
Cc:     Al Viro <viro@...iv.linux.org.uk>,
        syzbot <syzbot+5399ed0832693e29f392@...kaller.appspotmail.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>
Subject: Re: kernel BUG at fs/inode.c:LINE!

On Tue, 2018-12-18 at 13:27 +0100, Dmitry Vyukov wrote:
> On Tue, Dec 18, 2018 at 12:35 PM Ian Kent <raven@...maw.net> wrote:
> > 
> > On Tue, 2018-12-18 at 18:42 +0800, Ian Kent wrote:
> > > On Mon, 2018-12-17 at 07:21 +0000, Al Viro wrote:
> > > > On Sun, Dec 16, 2018 at 10:11:04PM -0800, syzbot wrote:
> > > > > Hello,
> > > > > 
> > > > > syzbot found the following crash on:
> > > > > 
> > > > > HEAD commit:    d14b746c6c1c Add linux-next specific files for
> > > > > 20181214
> > > > > git tree:       linux-next
> > > > > console output: 
> > > > > https://syzkaller.appspot.com/x/log.txt?x=13706347400000
> > > > > kernel config:  
> > > > > https://syzkaller.appspot.com/x/.config?x=1da6d2d18f803140
> > > > > dashboard link:
> > > > > https://syzkaller.appspot.com/bug?extid=5399ed0832693e29f392
> > > > > compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
> > > > > syz repro:      
> > > > > https://syzkaller.appspot.com/x/repro.syz?x=101032b3400000
> > > > > C reproducer:   
> > > > > https://syzkaller.appspot.com/x/repro.c?x=16534063400000
> > > > > 
> > > > > IMPORTANT: if you fix the bug, please add the following tag to the
> > > > > commit:
> > > > > Reported-by: syzbot+5399ed0832693e29f392@...kaller.appspotmail.com
> > > > > 
> > > > >  slab_pre_alloc_hook mm/slab.h:423 [inline]
> > > > >  slab_alloc mm/slab.c:3365 [inline]
> > > > >  kmem_cache_alloc+0x2c4/0x730 mm/slab.c:3539
> > > > >  __d_alloc+0xc8/0xb90 fs/dcache.c:1599
> > > > > ------------[ cut here ]------------
> > > > > kernel BUG at fs/inode.c:1566!
> > > > >  d_alloc_anon fs/dcache.c:1698 [inline]
> > > > >  d_make_root+0x43/0xc0 fs/dcache.c:1885
> > > > >  autofs_fill_super+0x6f1/0x1c30 fs/autofs/inode.c:273
> > > > 
> > > > Huh?  BUG is in iput(), AFAICS, so the stack trace is rather
> > > > misreported.
> > > > iput() can be called by d_make_root(), provided that dentry allocation
> > > > fails.  So the most straightforward interpretation would be that we
> > > > had an allocation failure (injected?), followed by iput() of the inode
> > > > passed to d_make_root().  Which happened to find I_CLEAR in ->i_state
> > > > of that inode somehow, which should be impossible short of seriously
> > > > buggered inode refcounting somewhere - the inode has just been returned
> > > > by new_inode(), which clears i_state, and it would have to have passed
> > > > clear_inode() (i.e. has been through inode eviction) since then...
> > > 
> > > Sorry Al, that's my bad.
> > > 
> > > See
> > > 
https://www.ozlabs.org/~akpm/mmotm/broken-out/autofs-fix-possible-inode-leak-in-autofs_fill_super.patch
> > > 
> > > I think this will fix it, I'll forward it to Andrew if you agree:
> > 
> > Actually, looking at it again the above patch is plain not needed,
> > dropping it and updating the patch which follows it in the series
> > is what needs to be done.
> > 
> > Andrew, what should I do to make this easiest for you to handle,
> > a respost with v2 in the subject of the patch affected by dropping
> > the above patch?
> > 
> > Or I could repost the series with above patch dropped and the affected
> > patch corrected?
> 
> Hi Ian,
> 
> If you going to amend any commits, please add:
>     Tested-by: syzbot+5399ed0832693e29f392@...kaller.appspotmail.com
> otherwise:
>     Reported-by: syzbot+5399ed0832693e29f392@...kaller.appspotmail.com

I was thinking about how to handle loosing that information.

I don't think this amounts amending commits since Andrews mmotm is
based on patch series so dropping a patch and updating patches before
being merged won't capture this.

Adding the "Tested-by" attribution to the updated patch prior to syzbot
actually performing the test might be ok since it will get tested along
the way. Although the problem patch itself won't exist any more so ... ?

OTOH, if I repost the series I think I can send them to syzbot for testing
before forwarding to Andrew (I've done something like that before but can't
remember how now) and add the attribution to the series.

But this all depends on what is best for Andrew and what Al would like to
see done.

> 
> Thanks
> 
> > > autofs - fix handling of d_make_root() return in autofs_fill_super()
> > > 
> > > From: Ian Kent <raven@...maw.net>
> > > 
> > > A previous change to handle a possible inode leak in autofs_fill_super()
> > > added an iput() on d_make_root() failure but d_make_root() already puts
> > > the passed in inode on failure.
> > > 
> > > Reported-by: syzbot+5399ed0832693e29f392@...kaller.appspotmail.com
> > > Signed-off-by: Ian Kent <raven@...maw.net>
> > > ---
> > >  fs/autofs/inode.c |    4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/autofs/inode.c b/fs/autofs/inode.c
> > > index 501833cc49a8..953f76b95172 100644
> > > --- a/fs/autofs/inode.c
> > > +++ b/fs/autofs/inode.c
> > > @@ -271,7 +271,7 @@ int autofs_fill_super(struct super_block *s, void
> > > *data,
> > > int silent)
> > >       }
> > >       root = d_make_root(root_inode);
> > >       if (!root)
> > > -             goto fail_iput;
> > > +             goto fail_ino;
> > >       pipe = NULL;
> > > 
> > >       root->d_fsdata = ino;
> > > @@ -347,8 +347,6 @@ int autofs_fill_super(struct super_block *s, void
> > > *data,
> > > int silent)
> > >  fail_dput:
> > >       dput(root);
> > >       goto fail_free;
> > > -fail_iput:
> > > -     iput(root_inode);
> > >  fail_ino:
> > >       autofs_free_ino(ino);
> > >  fail_free:
> > > 

Powered by blists - more mailing lists