On Thu, Apr 11, 2019 at 4:23 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > > 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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx> > 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. Please see the following for this: > syzbot will keep track of this bug report. See: > https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with syzbot. There are just 3 operations: mark as fixed by a commit, mark as invalid, mark as duplicate. I won't be always around. Tracking statuses of bug reports is in the interests of kernel quality. > 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...
