On Mon, Apr 11, 2022 at 01:06:09PM +0800, Dongliang Mu wrote:
> On Mon, Apr 11, 2022 at 11:10 AM Chao Yu <[email protected]> wrote:
> >
> > On 2022/4/9 14:42, Dongliang Mu wrote:
> > > On Sat, Apr 9, 2022 at 11:46 AM Chao Yu <[email protected]> wrote:
> > >>
> > >> On 2022/4/9 9:34, Dongliang Mu wrote:
> > >>> On Sat, Apr 9, 2022 at 8:27 AM Chao Yu <[email protected]> wrote:
> > >>>>
> > >>>> On 2022/4/8 13:22, Dongliang Mu wrote:
> > >>>>> From: Dongliang Mu <[email protected]>
> > >>>>>
> > >>>>> In f2fs_is_valid_blkaddr, if type is DATA_GENERIC_ENHANCE or
> > >>>>> DATA_GENERIC_ENHANCE_READ, it invokes WARN_ON(1) not matter
> > >>>>> blkaddr is in the range or not.
> > >>>>
> > >>>> If we run into the path where we invoke WARN_ON(1) in f2fs_is_valid_blkaddr(),
> > >>>> It means f2fs image may be broken, or there is a bug in f2fs.
> > >>>>
> > >>>> So, do you suffer any related issue in your environment?
> > >>>
> > >>> related issue? Can you explain a little?
> > >>>
> > >>> If you mean if this warning occurs, any other issues or crash
> > >>
> > >> I mean have you seen any warning info printed in the path of
> > >> f2fs_is_valid_blkaddr() before applying this patch, and if so, w/ what
> > >> reproducer? or you just figure out this patch from perspective of code
> > >> review?
> > >
> > > Yes, I have seen both warning information from Syzbot [1] and my local
> > > syzkaller instance.
> > >
> > > In f2fs_is_valid_blkaddr, if the following condition is satisfied,
> > > i.e., blkaddr is not in the right range [2], it will directly invoke
> > > one WARN_ON.
> > >
> > > if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
> > > blkaddr < MAIN_BLKADDR(sbi))) {
> > >
> > > This is the case on Syzbot.
> > >
> > > Otherwise, it will jump into __is_bitmap_valid. And if the following
> > > condition is satisfied [3], it will trigger another WARN_ON.
> > >
> > > exist = f2fs_test_bit(offset, se->cur_valid_map);
> > > if (!exist && type == DATA_GENERIC_ENHANCE) {
> > >
> > > This appears in my local syzbot instance, but unfortunately it does
> > > not get any reproducer.
> >
> > Oh, it occurs in syzbot test, I guess it is possible that f2fs prints such
> > warning info after blkaddr of node/data block was fuzzed to invalid one.
> >
> > I prefer to keep WARN_ON() to catch more info of bugs found by non-fuzzed
> > type test.
> >
> > Thoughts?
>
> I am fine with both options. I can remove the WARN_ON in my local
> syzkaller instance and continue fuzzing Linux kernel.
>
> +Dmitry Vyukov how do you think? If WARN_ON is kept, this crash will
> occur on Syzbot from time to time.
WARN_ON is for kernel bugs; please refer to the documentation in
include/asm-generic/bug.h. If this is a kernel bug, then the kernel bug needs
to be fixed. Otherwise, the WARN_ON needs to be removed.
- Eric
On 2022/4/11 14:14, Eric Biggers wrote:
> On Mon, Apr 11, 2022 at 01:06:09PM +0800, Dongliang Mu wrote:
>> On Mon, Apr 11, 2022 at 11:10 AM Chao Yu <[email protected]> wrote:
>>>
>>> On 2022/4/9 14:42, Dongliang Mu wrote:
>>>> On Sat, Apr 9, 2022 at 11:46 AM Chao Yu <[email protected]> wrote:
>>>>>
>>>>> On 2022/4/9 9:34, Dongliang Mu wrote:
>>>>>> On Sat, Apr 9, 2022 at 8:27 AM Chao Yu <[email protected]> wrote:
>>>>>>>
>>>>>>> On 2022/4/8 13:22, Dongliang Mu wrote:
>>>>>>>> From: Dongliang Mu <[email protected]>
>>>>>>>>
>>>>>>>> In f2fs_is_valid_blkaddr, if type is DATA_GENERIC_ENHANCE or
>>>>>>>> DATA_GENERIC_ENHANCE_READ, it invokes WARN_ON(1) not matter
>>>>>>>> blkaddr is in the range or not.
>>>>>>>
>>>>>>> If we run into the path where we invoke WARN_ON(1) in f2fs_is_valid_blkaddr(),
>>>>>>> It means f2fs image may be broken, or there is a bug in f2fs.
>>>>>>>
>>>>>>> So, do you suffer any related issue in your environment?
>>>>>>
>>>>>> related issue? Can you explain a little?
>>>>>>
>>>>>> If you mean if this warning occurs, any other issues or crash
>>>>>
>>>>> I mean have you seen any warning info printed in the path of
>>>>> f2fs_is_valid_blkaddr() before applying this patch, and if so, w/ what
>>>>> reproducer? or you just figure out this patch from perspective of code
>>>>> review?
>>>>
>>>> Yes, I have seen both warning information from Syzbot [1] and my local
>>>> syzkaller instance.
>>>>
>>>> In f2fs_is_valid_blkaddr, if the following condition is satisfied,
>>>> i.e., blkaddr is not in the right range [2], it will directly invoke
>>>> one WARN_ON.
>>>>
>>>> if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
>>>> blkaddr < MAIN_BLKADDR(sbi))) {
>>>>
>>>> This is the case on Syzbot.
>>>>
>>>> Otherwise, it will jump into __is_bitmap_valid. And if the following
>>>> condition is satisfied [3], it will trigger another WARN_ON.
>>>>
>>>> exist = f2fs_test_bit(offset, se->cur_valid_map);
>>>> if (!exist && type == DATA_GENERIC_ENHANCE) {
>>>>
>>>> This appears in my local syzbot instance, but unfortunately it does
>>>> not get any reproducer.
>>>
>>> Oh, it occurs in syzbot test, I guess it is possible that f2fs prints such
>>> warning info after blkaddr of node/data block was fuzzed to invalid one.
>>>
>>> I prefer to keep WARN_ON() to catch more info of bugs found by non-fuzzed
>>> type test.
>>>
>>> Thoughts?
>>
>> I am fine with both options. I can remove the WARN_ON in my local
>> syzkaller instance and continue fuzzing Linux kernel.
>>
>> +Dmitry Vyukov how do you think? If WARN_ON is kept, this crash will
>> occur on Syzbot from time to time.
>
> WARN_ON is for kernel bugs; please refer to the documentation in
> include/asm-generic/bug.h. If this is a kernel bug, then the kernel bug needs
> to be fixed. Otherwise, the WARN_ON needs to be removed.
Alright, so how about using dump_stack() instead as suggested in doc?
Thanks,
>
> - Eric
On Mon, Apr 11, 2022 at 5:53 PM Chao Yu <[email protected]> wrote:
>
> On 2022/4/11 14:14, Eric Biggers wrote:
> > On Mon, Apr 11, 2022 at 01:06:09PM +0800, Dongliang Mu wrote:
> >> On Mon, Apr 11, 2022 at 11:10 AM Chao Yu <[email protected]> wrote:
> >>>
> >>> On 2022/4/9 14:42, Dongliang Mu wrote:
> >>>> On Sat, Apr 9, 2022 at 11:46 AM Chao Yu <[email protected]> wrote:
> >>>>>
> >>>>> On 2022/4/9 9:34, Dongliang Mu wrote:
> >>>>>> On Sat, Apr 9, 2022 at 8:27 AM Chao Yu <[email protected]> wrote:
> >>>>>>>
> >>>>>>> On 2022/4/8 13:22, Dongliang Mu wrote:
> >>>>>>>> From: Dongliang Mu <[email protected]>
> >>>>>>>>
> >>>>>>>> In f2fs_is_valid_blkaddr, if type is DATA_GENERIC_ENHANCE or
> >>>>>>>> DATA_GENERIC_ENHANCE_READ, it invokes WARN_ON(1) not matter
> >>>>>>>> blkaddr is in the range or not.
> >>>>>>>
> >>>>>>> If we run into the path where we invoke WARN_ON(1) in f2fs_is_valid_blkaddr(),
> >>>>>>> It means f2fs image may be broken, or there is a bug in f2fs.
> >>>>>>>
> >>>>>>> So, do you suffer any related issue in your environment?
> >>>>>>
> >>>>>> related issue? Can you explain a little?
> >>>>>>
> >>>>>> If you mean if this warning occurs, any other issues or crash
> >>>>>
> >>>>> I mean have you seen any warning info printed in the path of
> >>>>> f2fs_is_valid_blkaddr() before applying this patch, and if so, w/ what
> >>>>> reproducer? or you just figure out this patch from perspective of code
> >>>>> review?
> >>>>
> >>>> Yes, I have seen both warning information from Syzbot [1] and my local
> >>>> syzkaller instance.
> >>>>
> >>>> In f2fs_is_valid_blkaddr, if the following condition is satisfied,
> >>>> i.e., blkaddr is not in the right range [2], it will directly invoke
> >>>> one WARN_ON.
> >>>>
> >>>> if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
> >>>> blkaddr < MAIN_BLKADDR(sbi))) {
> >>>>
> >>>> This is the case on Syzbot.
> >>>>
> >>>> Otherwise, it will jump into __is_bitmap_valid. And if the following
> >>>> condition is satisfied [3], it will trigger another WARN_ON.
> >>>>
> >>>> exist = f2fs_test_bit(offset, se->cur_valid_map);
> >>>> if (!exist && type == DATA_GENERIC_ENHANCE) {
> >>>>
> >>>> This appears in my local syzbot instance, but unfortunately it does
> >>>> not get any reproducer.
> >>>
> >>> Oh, it occurs in syzbot test, I guess it is possible that f2fs prints such
> >>> warning info after blkaddr of node/data block was fuzzed to invalid one.
> >>>
> >>> I prefer to keep WARN_ON() to catch more info of bugs found by non-fuzzed
> >>> type test.
> >>>
> >>> Thoughts?
> >>
> >> I am fine with both options. I can remove the WARN_ON in my local
> >> syzkaller instance and continue fuzzing Linux kernel.
> >>
> >> +Dmitry Vyukov how do you think? If WARN_ON is kept, this crash will
> >> occur on Syzbot from time to time.
> >
> > WARN_ON is for kernel bugs; please refer to the documentation in
> > include/asm-generic/bug.h. If this is a kernel bug, then the kernel bug needs
> > to be fixed. Otherwise, the WARN_ON needs to be removed.
>
> Alright, so how about using dump_stack() instead as suggested in doc?
I agree. Let's change WARN_ON to dump_stack().
>
> Thanks,
>
> >
> > - Eric