Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220408052207.209856-1-dzm91@hust.edu.cn> <f125ece9-872e-39d9-48a5-15b85508f6b6@kernel.org>
 <CAD-N9QV7RUfBreaVOmC0JUwVfieM3kOYigOav8=vpAoak6Uc6A@mail.gmail.com>
 <7e896205-027d-32e2-32b5-8951231132ac@kernel.org> <CAD-N9QXM_xds692Lh-QZc5aL01uRivLGrNnxJys41hiTUfKE5w@mail.gmail.com>
 <2f5b7291-da06-0895-8e75-c7a37251b47e@kernel.org> <CAD-N9QX7wKN_x88w7p6t1KWeTGodeF-o0i-1d0Sq4Ja1OzPFoA@mail.gmail.com>
 <YlPHWaSlk21cc32p@sol.localdomain> <a25bac9c-188b-4ab4-21e2-d34e20c92524@kernel.org>
In-Reply-To: <a25bac9c-188b-4ab4-21e2-d34e20c92524@kernel.org>
From:   Dongliang Mu <mudongliangabcd@gmail.com>
Date:   Tue, 12 Apr 2022 17:25:12 +0800
Message-ID: <CAD-N9QWCHZ46wL5zOzWSeesGRk0xzKC4oGy2f8E08FTJhE5UCA@mail.gmail.com>
Subject: Re: [f2fs-dev] [PATCH] fs: f2fs: remove WARN_ON in f2fs_is_valid_blkaddr
To:     Chao Yu <chao@kernel.org>
Cc:     Eric Biggers <ebiggers@kernel.org>,
        Dmitry Vyukov <dvyukov@google.com>,
        Jaegeuk Kim <jaegeuk@kernel.org>,
        Dongliang Mu <dzm91@hust.edu.cn>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        syzbot+763ae12a2ede1d99d4dc@syzkaller.appspotmail.com,
        linux-f2fs-devel@lists.sourceforge.net
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Apr 11, 2022 at 5:53 PM Chao Yu <chao@kernel.org> 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 <chao@kernel.org> wrote:
> >>>
> >>> On 2022/4/9 14:42, Dongliang Mu wrote:
> >>>> On Sat, Apr 9, 2022 at 11:46 AM Chao Yu <chao@kernel.org> wrote:
> >>>>>
> >>>>> On 2022/4/9 9:34, Dongliang Mu wrote:
> >>>>>> On Sat, Apr 9, 2022 at 8:27 AM Chao Yu <chao@kernel.org> wrote:
> >>>>>>>
> >>>>>>> On 2022/4/8 13:22, Dongliang Mu wrote:
> >>>>>>>> From: Dongliang Mu <mudongliangabcd@gmail.com>
> >>>>>>>>
> >>>>>>>> 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