linux-kernel - Re: kernel BUG at fs/inode.c:LINE!

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190411022257.GB2217@ZenIV.linux.org.uk>
Date:   Thu, 11 Apr 2019 03:22:57 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Ian Kent <raven@...maw.net>
Cc:     Dmitry Vyukov <dvyukov@...gle.com>,
        syzbot <syzbot+5399ed0832693e29f392@...kaller.appspotmail.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Amir Goldstein <amir73il@...il.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>
Subject: Re: kernel BUG at fs/inode.c:LINE!

On Thu, Apr 11, 2019 at 08:50:17AM +0800, Ian Kent wrote:
> On Wed, 2019-04-10 at 14:41 +0200, Dmitry Vyukov wrote:
> > On Wed, Apr 10, 2019 at 2:12 PM Al Viro <viro@...iv.linux.org.uk> wrote:
> > > 
> > > On Wed, Apr 10, 2019 at 08:07:15PM +0800, Ian Kent wrote:
> > > 
> > > > > I'm unable to find a branch matching the line numbers.
> > > > > 
> > > > > Given that, on the face of it, the scenario is impossible I'm
> > > > > seeking clarification on what linux-next to look at for the
> > > > > sake of accuracy.
> > > > > 
> > > > > So I'm wondering if this testing done using the master branch
> > > > > or one of the daily branches one would use to check for conflicts
> > > > > before posting?
> > > > 
> > > > Sorry those are tags not branches.
> > > 
> > > FWIW, that's next-20181214; it is what master had been in mid-December
> > > and master is rebased every day.  Can it be reproduced with the current
> > > tree?
> > 
> > From the info on the dashboard we know that it happened only once on
> > d14b746c (the second one is result of reproducing the first one). So
> > it was either fixed or just hard to trigger.
> 
> Looking at the source of tag next-20181214 in linux-next-history I see
> this is mistake I made due to incorrect error handling which I fixed
> soon after (there was in fact a double iput()).

Right - "autofs: fix possible inode leak in autofs_fill_super()" had been
broken (and completely pointless), leading to double iput() in that failure
case.  And yes, double iput() can trigger that BUG_ON(), and with non-zero
odds do so with that stack trace.

As far as I'm concerned, case closed - bug had been in a misguided "fix"
for inexistent leak (coming from misreading the calling conventions for
d_make_root()), introduced in -next at next-20181130 and kicked out of
there in next-20181219.  Dropped by Ian's request in 
Message-ID: <66d497c00cffb3e4109ca0d5287c8277954d7132.camel@...maw.net>
which has fixed that crap.  Moreover, that posting had been in reply to
that very syzcaller report, AFAICS.

I don't know how to tell the bot to STFU and close the report in this
situation; up to you, folks.

As an aside, the cause of that bug is that d_make_root() calling conventions
are insufficiently documented.  All we have is

||[mandatory]
||        d_alloc_root() is gone, along with a lot of bugs caused by code
||misusing it.  Replacement: d_make_root(inode).  The difference is,
||d_make_root() drops the reference to inode if dentry allocation fails.

in Documentation/filesystems/porting, and that's not good enough.  Anyone
willing to take a shot at that?  FWIW, the calling conventions are:

	d_make_root(inode) normally allocates and returns a new dentry.
On failure NULL is returned.  A reference to inode is consumed in all
cases (on success it is transferred to new dentry, on failure it is
dropped), so failure handling does not need anything done to inode.
d_make_root(NULL) quietly returns NULL, which further simplifies the
error handling in typical caller.  Usually it's something like
	inode = foofs_new_inode(....);
	s->s_root = d_make_inode(inode);
	if (!s->s_root)
		bugger off, no need to undo inode allocation
	success
We do not need to check if foofs_new_inode() has returned NULL and we
do not need any special cleanups in case of failure - not for the
undoing the inode allocation.

If anyone cares to convert that into coherent (and printable) documentation,
patches are welcome...