| Message ID | tencent_B5CA92105D925DA2993D4FD20DDD25BF8D07@qq.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: add a sanity check for csum root before fill the data csum | expand |
在 2024/10/23 21:34, Edward Adam Davis 写道: > Syzbot reported a null-ptr-deref in btrfs_lookup_csums_bitmap. > The btrfs info contains IGNOREDATACSUMS, which prevents the csum root from > being loaded. > Before filling in the csum data, check the flag BTRFS_FS_STATE_NO_DATA_CSUMS > to confirm that the csum root has been loaded. > > Reported-and-tested-by: syzbot+5d2b33d7835870519b5f@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=5d2b33d7835870519b5f > Signed-off-by: Edward Adam Davis <eadavis@qq.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/scrub.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c > index 3a3427428074..1ba4d8ba902b 100644 > --- a/fs/btrfs/scrub.c > +++ b/fs/btrfs/scrub.c > @@ -1602,7 +1602,8 @@ static int scrub_find_fill_first_stripe(struct btrfs_block_group *bg, > } > > /* Now fill the data csum. */ > - if (bg->flags & BTRFS_BLOCK_GROUP_DATA) { > + if (!test_bit(BTRFS_FS_STATE_NO_DATA_CSUMS, &fs_info->fs_state) && > + bg->flags & BTRFS_BLOCK_GROUP_DATA) { > int sector_nr; > unsigned long csum_bitmap = 0; >
On Wed, Oct 23, 2024 at 07:04:40PM +0800, Edward Adam Davis wrote: > Syzbot reported a null-ptr-deref in btrfs_lookup_csums_bitmap. > The btrfs info contains IGNOREDATACSUMS, which prevents the csum root from > being loaded. > Before filling in the csum data, check the flag BTRFS_FS_STATE_NO_DATA_CSUMS > to confirm that the csum root has been loaded. > > Reported-and-tested-by: syzbot+5d2b33d7835870519b5f@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=5d2b33d7835870519b5f > Signed-off-by: Edward Adam Davis <eadavis@qq.com> Added to for-next, thanks. > --- > fs/btrfs/scrub.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c > index 3a3427428074..1ba4d8ba902b 100644 > --- a/fs/btrfs/scrub.c > +++ b/fs/btrfs/scrub.c > @@ -1602,7 +1602,8 @@ static int scrub_find_fill_first_stripe(struct btrfs_block_group *bg, > } > > /* Now fill the data csum. */ > - if (bg->flags & BTRFS_BLOCK_GROUP_DATA) { > + if (!test_bit(BTRFS_FS_STATE_NO_DATA_CSUMS, &fs_info->fs_state) && I've updatd the coment as this is double negation that could be confusing on a quick read. > + bg->flags & BTRFS_BLOCK_GROUP_DATA) { > int sector_nr; > unsigned long csum_bitmap = 0; > > -- > 2.43.0 >
在 2024/10/26 05:14, David Sterba 写道: > On Wed, Oct 23, 2024 at 07:04:40PM +0800, Edward Adam Davis wrote: >> Syzbot reported a null-ptr-deref in btrfs_lookup_csums_bitmap. >> The btrfs info contains IGNOREDATACSUMS, which prevents the csum root from >> being loaded. >> Before filling in the csum data, check the flag BTRFS_FS_STATE_NO_DATA_CSUMS >> to confirm that the csum root has been loaded. >> >> Reported-and-tested-by: syzbot+5d2b33d7835870519b5f@syzkaller.appspotmail.com >> Closes: https://syzkaller.appspot.com/bug?extid=5d2b33d7835870519b5f >> Signed-off-by: Edward Adam Davis <eadavis@qq.com> > > Added to for-next, thanks. Wait for a second, I believe LiZhi Xu's solution is better. And sorry I didn't notice that until his patch is submitted. The problem for this fix is, although it fixes the crash, it also gives a false feel of safety that scrub is finding nothing wrong. But the truth is, there is no csum root, and everything can go wrong. Thus I'd prefer LiZhi's solution which error out and terminate the scrub immediately. Thanks, Qu > >> --- >> fs/btrfs/scrub.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c >> index 3a3427428074..1ba4d8ba902b 100644 >> --- a/fs/btrfs/scrub.c >> +++ b/fs/btrfs/scrub.c >> @@ -1602,7 +1602,8 @@ static int scrub_find_fill_first_stripe(struct btrfs_block_group *bg, >> } >> >> /* Now fill the data csum. */ >> - if (bg->flags & BTRFS_BLOCK_GROUP_DATA) { >> + if (!test_bit(BTRFS_FS_STATE_NO_DATA_CSUMS, &fs_info->fs_state) && > > I've updatd the coment as this is double negation that could be > confusing on a quick read. > >> + bg->flags & BTRFS_BLOCK_GROUP_DATA) { >> int sector_nr; >> unsigned long csum_bitmap = 0; >> >> -- >> 2.43.0 >> >
On Sat, Oct 26, 2024 at 07:45:18AM +1030, Qu Wenruo wrote: > > > 在 2024/10/26 05:14, David Sterba 写道: > > On Wed, Oct 23, 2024 at 07:04:40PM +0800, Edward Adam Davis wrote: > >> Syzbot reported a null-ptr-deref in btrfs_lookup_csums_bitmap. > >> The btrfs info contains IGNOREDATACSUMS, which prevents the csum root from > >> being loaded. > >> Before filling in the csum data, check the flag BTRFS_FS_STATE_NO_DATA_CSUMS > >> to confirm that the csum root has been loaded. > >> > >> Reported-and-tested-by: syzbot+5d2b33d7835870519b5f@syzkaller.appspotmail.com > >> Closes: https://syzkaller.appspot.com/bug?extid=5d2b33d7835870519b5f > >> Signed-off-by: Edward Adam Davis <eadavis@qq.com> > > > > Added to for-next, thanks. > > Wait for a second, I believe LiZhi Xu's solution is better. > > And sorry I didn't notice that until his patch is submitted. > > The problem for this fix is, although it fixes the crash, it also gives > a false feel of safety that scrub is finding nothing wrong. > > But the truth is, there is no csum root, and everything can go wrong. > > Thus I'd prefer LiZhi's solution which error out and terminate the scrub > immediately. Ok, I've dropped this patch from for-next. Please add "btrfs: add a sanity check for btrfs root".
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c index 3a3427428074..1ba4d8ba902b 100644 --- a/fs/btrfs/scrub.c +++ b/fs/btrfs/scrub.c @@ -1602,7 +1602,8 @@ static int scrub_find_fill_first_stripe(struct btrfs_block_group *bg, } /* Now fill the data csum. */ - if (bg->flags & BTRFS_BLOCK_GROUP_DATA) { + if (!test_bit(BTRFS_FS_STATE_NO_DATA_CSUMS, &fs_info->fs_state) && + bg->flags & BTRFS_BLOCK_GROUP_DATA) { int sector_nr; unsigned long csum_bitmap = 0;
Syzbot reported a null-ptr-deref in btrfs_lookup_csums_bitmap. The btrfs info contains IGNOREDATACSUMS, which prevents the csum root from being loaded. Before filling in the csum data, check the flag BTRFS_FS_STATE_NO_DATA_CSUMS to confirm that the csum root has been loaded. Reported-and-tested-by: syzbot+5d2b33d7835870519b5f@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=5d2b33d7835870519b5f Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- fs/btrfs/scrub.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)