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

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

[Chen Zhongjin]

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")
Reported-by: syzbot+77e4f0...@syzkaller.appspotmail.com
Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 fs/nilfs2/sufile.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/nilfs2/sufile.c b/fs/nilfs2/sufile.c
index 77ff8e95421f..2962f9071490 100644
--- 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;
-- 
2.17.1

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

[Andrew Morton]

On Fri, 18 Nov 2022 14:33:04 +0800 Chen Zhongjin <chenzhongjin@huawei.com> 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!

> --- 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?

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@gmail.com>
Tested-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>

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@huawei.com>

He initially tried to fix this issue [1]:

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

> >
> > 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

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

[Chen Zhongjin]

On 2022/11/19 6:11, Andrew Morton wrote:
> On Fri, 18 Nov 2022 14:33:04 +0800 Chen Zhongjin <chenzhongjin@huawei.com> 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.

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@huawei.com> 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

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

[Ryusuke Konishi]

Hi Chen Zhongjin,

On Sat, Nov 19, 2022 at 2:29 PM Chen Zhongjin wrote:
>
>
> 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@huawei.com> 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().
> >

Could you please resubmit the patch reflecting the following comments ?

After I replied to Andrew, I noticed them.
Also, When reposting, it would be helpful if you could add all the
tags I asked for Andrew in advance.

Comments:
1) Please change nilfs_sufile_mark_dirty() so that it protects the
segusage modification
with &NILFS_MDT(sufile)->mi_sem:

> --- 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);

+       down_write(&NILFS_MDT(sufile)->mi_sem);
>       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);
>       }
+       up_write(&NILFS_MDT(sufile)->mi_sem);
>       return ret;

All functions that modify metadata on the sufile need protection with
this R/W semaphore.
You may not see this protection for some sufile functions as is, but
in that case, the wrapper function that uses them acquires this R/W
semaphore instead.

Since I retested for this change as well, you don't have to drop my
"Tested-by" tag.

2) Please use the following complete email address for the
"Reported-by" tag of syzbot.

Reported-by: syzbot+77e4f005cb899d4268d1@syzkaller.appspotmail.com

Your tag is partially abbreviated.  I don't know that abbreviation is
valid, but there are very few examples of such.
And even if it's valid for syzbot, I don't think that omission is
desirable as some tools may not support it.

Thanks,
Ryusuke Konishi

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

[Ryusuke Konishi]

On Sat, Nov 19, 2022 at 4:17 PM Ryusuke Konishi wrote:
>
> Hi Chen Zhongjin,
>
> On Sat, Nov 19, 2022 at 2:29 PM Chen Zhongjin wrote:
> >
> >
> > 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@huawei.com> 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().
> > >
>
> Could you please resubmit the patch reflecting the following comments ?
>
> After I replied to Andrew, I noticed them.
> Also, When reposting, it would be helpful if you could add all the
> tags I asked for Andrew in advance.
>
> Comments:
> 1) Please change nilfs_sufile_mark_dirty() so that it protects the
> segusage modification
> with &NILFS_MDT(sufile)->mi_sem:
>
> > --- 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);
>
> +       down_write(&NILFS_MDT(sufile)->mi_sem);

Sorry, the location of this down_write() was wrong in this email.
In my tested change, I put it before
nilfs_sufile_get_segment_usage_block() like others.

> +       down_write(&NILFS_MDT(sufile)->mi_sem);
> >       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);
> >       }
> +       up_write(&NILFS_MDT(sufile)->mi_sem);
> >       return ret;
>
> All functions that modify metadata on the sufile need protection with
> this R/W semaphore.
> You may not see this protection for some sufile functions as is, but
> in that case, the wrapper function that uses them acquires this R/W
> semaphore instead.


Regards,
Ryusuke Konishi


>
> Since I retested for this change as well, you don't have to drop my
> "Tested-by" tag.
>
> 2) Please use the following complete email address for the
> "Reported-by" tag of syzbot.
>
> Reported-by: syzbot+77e4f005cb899d4268d1@syzkaller.appspotmail.com
>
> Your tag is partially abbreviated.  I don't know that abbreviation is
> valid, but there are very few examples of such.
> And even if it's valid for syzbot, I don't think that omission is
> desirable as some tools may not support it.
>
> Thanks,
> Ryusuke Konishi

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

[Chen Zhongjin]

On 2022/11/19 15:17, Ryusuke Konishi wrote:
> Hi Chen Zhongjin,
>
> On Sat, Nov 19, 2022 at 2:29 PM Chen Zhongjin wrote:
>>
>> 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@huawei.com> 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().
>>>
> Could you please resubmit the patch reflecting the following comments ?
>
> After I replied to Andrew, I noticed them.
> Also, When reposting, it would be helpful if you could add all the
> tags I asked for Andrew in advance.
>
> Comments:
> 1) Please change nilfs_sufile_mark_dirty() so that it protects the
> segusage modification
> with &NILFS_MDT(sufile)->mi_sem:
>
>> --- 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);
> +       down_write(&NILFS_MDT(sufile)->mi_sem);
>>        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);
>>        }
> +       up_write(&NILFS_MDT(sufile)->mi_sem);
>>        return ret;
> All functions that modify metadata on the sufile need protection with
> this R/W semaphore.
> You may not see this protection for some sufile functions as is, but
> in that case, the wrapper function that uses them acquires this R/W
> semaphore instead.
>
> Since I retested for this change as well, you don't have to drop my
> "Tested-by" tag.
>
> 2) Please use the following complete email address for the
> "Reported-by" tag of syzbot.
>
> Reported-by: syzbot+77e4f005cb899d4268d1@syzkaller.appspotmail.com
>
> Your tag is partially abbreviated.  I don't know that abbreviation is
> valid, but there are very few examples of such.
> And even if it's valid for syzbot, I don't think that omission is
> desirable as some tools may not support it.

Thanks for suggestions! I have sent v2 for all of them, please check.


Best,

Chen

> Thanks,
> Ryusuke Konishi