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

[Chen Zhongjin]

On 2022/11/19 13:24, Chen Zhongjin wrote:
> On 2022/11/19 6:11, Andrew Morton wrote:
> > On Fri, 18 Nov 2022 14:33:04 +0800 Chen Zhongjin 
> > <chenzhongjin@xxxxxxxxxx> 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.
> > >
> > > Fixes: 9ff05123e3bf ("nilfs2: segment constructor")
> > Merged in 2009!
>
> Yes, seems it is introduced at the beginning of this file and the 
> function called nilfs_touch_segusage().
>
> Wired that this problem is not discovered utill now. So I'm wondering 
> that whether this is a real-world
> problem or just a use case constructed maliciously by syzkaller. But 
> according to the result of syzkaller bisection,
> this problem should have a history.
> > > --- 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?
> Sorry that I'm not familiar with the specific use scenarios of nilfs2. 
> So I can't offer a better advice. I think if it
> is a problem that not happen easily in normal situations there's no 
> necessary to backport it to stable.

I just noticed Ryusuke's mail so let's do it as his advice.

Thanks for your time!

Best,

Chen