Re: [PATCH bpf-next v2] bpf: WARN_ONCE on verifier bugs

[Paul Chaignon <paul.chaignon@gmail.com>]

On Wed, May 14, 2025 at 09:28:11AM -0700, Andrii Nakryiko wrote:
> On Wed, May 14, 2025 at 4:35 AM Paul Chaignon <paul.chaignon@gmail.com> wrote:
> >
> > Throughout the verifier's logic, there are multiple checks for
> > inconsistent states that should never happen and would indicate a
> > verifier bug. These bugs are typically logged in the verifier logs and
> > sometimes preceded by a WARN_ONCE.
> >
> > This patch reworks these checks to consistently emit a verifier log AND
> > a warning when CONFIG_DEBUG_KERNEL is enabled. The consistent use of
> > WARN_ONCE should help fuzzers (ex. syzkaller) expose any situation
> > where they are actually able to reach one of those buggy verifier
> > states.
> >
> > Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
> > ---
> > Changes in v2:
> >   - Introduce a new BPF_WARN_ONCE macro, with WARN_ONCE conditioned on
> >     CONFIG_DEBUG_KERNEL, as per reviews.
> >   - Use the new helper function for verifier bugs missed in v1,
> >     particularly around backtracking.
> >

[...]

> >                                 /* r1-r5 are invalidated after subprog call,
> >                                  * so for global func call it shouldn't be set
> >                                  * anymore
> >                                  */
> >                                 if (bt_reg_mask(bt) & BPF_REGMASK_ARGS) {
> > -                                       verbose(env, "BUG regs %x\n", bt_reg_mask(bt));
> > -                                       WARN_ONCE(1, "verifier backtracking bug");
> > +                                       verifier_bug(env, "scratch reg set: regs %x\n",
> > +                                                    bt_reg_mask(bt));
> >                                         return -EFAULT;
> 
> 
> but please don't go overboard with verifier_buf_if() for cases like
> this, I think this should use plain verifier_bug() as you did, even if
> it *can* be expressed with verifier_buf_if() check

Where would you set the bar on these cases? Is it mostly a matter of
readability?

I'm asking because, with Alexei's suggestion to stringify the condition
in verifier_bug_if() (cf. v3), we would gain from using verifier_bug_if
more often.

[...]