Re: [PATCH] nilfs2: Fix nilfs_sufile_mark_dirty() not set segment usage as dirty

[Ryusuke Konishi]

Hi Andrew,

On Sat, Nov 19, 2022 at 7:11 AM Andrew Morton wrote:
>
> On Fri, 18 Nov 2022 14:33:04 +0800 Chen Zhongjin wrote:
>
> > In nilfs_sufile_mark_dirty(), the buffer and inode are set dirty, but
> > nilfs_segment_usage is not set dirty, which makes it can be found by
> > nilfs_sufile_alloc() because it checks nilfs_segment_usage_clean(su).
> >
> > This will cause the problem reported by syzkaller:
> > https://syzkaller.appspot.com/bug?id=c7c4748e11ffcc367cef04f76e02e931833cbd24
> >
> > It's because the case starts with segbuf1.segnum = 3, nextnum = 4, and
> > nilfs_sufile_alloc() not called to allocate a new segment.
> >
> > The first time nilfs_segctor_extend_segments() allocated segment
> > segbuf2.segnum = segbuf1.nextnum = 4, then nilfs_sufile_alloc() found
> > nextnextnum = 4 segment because its su is not set dirty.
> > So segbuf2.nextnum = 4, which causes next segbuf3.segnum = 4.
> >
> > sb_getblk() will get same bh for segbuf2 and segbuf3, and this bh is
> > added to both buffer lists of two segbuf.
> > It makes the list head of second list linked to the first one. When
> > iterating the first one, it will access and deref the head of second,
> > which causes NULL pointer dereference.

Acked-by: Ryusuke Konishi <konishi.ryusuke@xxxxxxxxx>
Tested-by: Ryusuke Konishi <konishi.ryusuke@xxxxxxxxx>

This is a simple and side-effect-free nice patch for this problem,
even though the functionality of nilfs_sufile_mark_dirty() has changed
from what it is supposed to do.

Making the segment usage dirty was the responsibility of another
function, nilfs_sufile_alloc().
However, it turns out that the current implementation is insufficient
for corrupted disk images like those created by syzbot. This patch
complements that.

If you don't mind, please add the following tag as well:

Reported-by: Liu Shixin <liushixin2@xxxxxxxxxx>

He initially tried to fix this issue [1]:

[1] https://lkml.kernel.org/r/20221108022928.497746-1-liushixin2@xxxxxxxxxx

> >
> > Fixes: 9ff05123e3bf ("nilfs2: segment constructor")
>
> Merged in 2009!

This is not wrong.  Sorry for the long term bug.
Because mkfs.nilfs2 and the nilfs2 module don't create broken metadata
that would require this patch, so we never encountered this bug until
syzbot reported it.

>
> > --- a/fs/nilfs2/sufile.c
> > +++ b/fs/nilfs2/sufile.c
> > @@ -495,12 +495,18 @@ void nilfs_sufile_do_free(struct inode *sufile, __u64 segnum,
> >  int nilfs_sufile_mark_dirty(struct inode *sufile, __u64 segnum)
> >  {
> >       struct buffer_head *bh;
> > +     void *kaddr;
> > +     struct nilfs_segment_usage *su;
> >       int ret;
> >
> >       ret = nilfs_sufile_get_segment_usage_block(sufile, segnum, 0, &bh);
> >       if (!ret) {
> >               mark_buffer_dirty(bh);
> >               nilfs_mdt_mark_dirty(sufile);
> > +             kaddr = kmap_atomic(bh->b_page);
> > +             su = nilfs_sufile_block_get_segment_usage(sufile, segnum, bh, kaddr);
> > +             nilfs_segment_usage_set_dirty(su);
> > +             kunmap_atomic(kaddr);
> >               brelse(bh);
> >       }
> >       return ret;
>
> Do we feel that this fix should be backported into -stable kernels?

Yes, it should be. Please do so.

Thanks,
Ryusuke Konishi