Re: [PATCH] nilfs2: fix missing continue after -ENOENT in nilfs_ioctl_mark_blocks_dirty()

[Ryusuke Konishi]

(added Junjie Cao to CC)

Hi Deepanshu,

On Mon, Mar 30, 2026 at 7:00 PM Ryusuke Konishi wrote:
>
> Hi Deepanshu,
>
> On Mon, Mar 30, 2026 at 6:40 PM Deepanshu Kartikey wrote:
> >
> > On Fri, Mar 20, 2026 at 11:02 PM Ryusuke Konishi
> > <konishi.ryusuke@xxxxxxxxx> wrote:
> > >
> > > Thank you, Deepanshu.
> > >
> > > On Thu, Mar 19, 2026 at 6:19 PM Deepanshu Kartikey wrote:
> > > >
> > > > nilfs_ioctl_mark_blocks_dirty() calls nilfs_bmap_lookup_at_level() to
> > > > get the current block number of each block descriptor. When the lookup
> > > > returns -ENOENT, meaning the block does not exist, it sets bd_blocknr
> > > > to 0 and continues processing.
> > > >
> > > > However, if bd_oblocknr is also 0, the subsequent check:
> > > >
> > > >   if (bdescs[i].bd_blocknr != bdescs[i].bd_oblocknr)
> > > >           continue;
> > > >
> > > > will not skip the block, and nilfs_bmap_mark() will be called on a
> > > > non-existent block. This causes nilfs_btree_do_lookup() to return
> > > > -ENOENT, triggering the WARN_ON(ret == -ENOENT).
> > > >
> > > > Fix this by adding a continue statement after setting bd_blocknr to 0
> > > > when the lookup returns -ENOENT, so that dead blocks are always skipped
> > > > regardless of the value of bd_oblocknr.
> > > >
> > > > Fixes: 7942b919f732 ("nilfs2: ioctl operations")
> > > > Reported-by: syzbot+98a040252119df0506f8@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > > Closes: https://syzkaller.appspot.com/bug?extid=98a040252119df0506f8
> > > > Signed-off-by: Deepanshu Kartikey <Kartikey406@xxxxxxxxx>
> > > > ---
> > > >  fs/nilfs2/ioctl.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > >
> > > Since this implementation interacts with userland GC, I will check
> > > whether this is a simple missing 'continue' statement or if it was
> > > intentional.
> > > If it is as you pointed out, I will pick it up and send it upstream.
> > >
> > > Thanks,
> > > Ryusuke Konishi
> > >
> >
> > gentle ping on this patch. Would like to know the status.
> > Let me know anything is required from my side
> >
> > Thanks
> >
> > Deepanshu
>
> Please wait a little longer.
>
> I have finished reviewing your other patch and am currently testing it.
>
> I'm sorry for the delay.  I was unable to work last week due to a
> family bereavement and various administrative procedures that
> followed.
>
> Regards,
> Ryusuke Konishi

I checked this and found that this fallthrough was intentional, aiming
to detect and skip a dead block by the subsequent comparison with
bd_oblocknr.

The problem is that it does not reject cases where bd_oblocknr takes
an unexpected value of 0.

This bd_oblocknr parameter stores the location where the userland GC
library found the target data block or the target intermediate block
of the DAT file.
As long as it is valid, it can never be block 0, which typically
stores the primary superblock and others.

However, due to the missing check for that anomalous value, the
corrupted ioctl request pattern generated by syzbot triggers the
assertion failure you pointed out.
I think a check like the following should be inserted at the beginning
of each iteration of the loop:

if (unlikely(!bdescs[i].bd_oblocknr))
    return -EINVAL;

Could you please restructure the patch in that direction?

Thanks,
Ryusuke Konishi