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

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

On Fri, May 16, 2025 at 09:14:40AM -0700, Andrii Nakryiko wrote:
> On Fri, May 16, 2025 at 2:34 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 v4:
> >   - Evaluate condition once and stringify it, as suggested by Alexei.
> >   - Use verifier_bug_if instead of verifier_bug where it can help
> >     disambiguate the callsite or shorten the message.
> >   - Add newline character in verifier_bug_if directly.
> > Changes in v3:
> >   - Introduce and use verifier_bug_if, as suggested by Andrii.
> > 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.
> >
> >  include/linux/bpf.h          |   6 ++
> >  include/linux/bpf_verifier.h |  11 +++
> >  kernel/bpf/btf.c             |   4 +-
> >  kernel/bpf/verifier.c        | 140 +++++++++++++++--------------------
> >  4 files changed, 77 insertions(+), 84 deletions(-)
> >
> 
> LGTM overall, left a few comments below, please take a look
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>

Thanks a lot for the detailed review!

> 
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 83c56f40842b..5b25d278409b 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -346,6 +346,12 @@ static inline const char *btf_field_type_name(enum btf_field_type type)
> >         }
> >  }
> >
> > +#if IS_ENABLED(CONFIG_DEBUG_KERNEL)
> > +#define BPF_WARN_ONCE(cond, format...) WARN_ONCE(cond, format)
> > +#else
> > +#define BPF_WARN_ONCE(cond, format...) BUILD_BUG_ON_INVALID(cond)
> > +#endif
> > +
> >  static inline u32 btf_field_type_size(enum btf_field_type type)
> >  {
> >         switch (type) {
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index cedd66867ecf..7edb15830132 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -839,6 +839,17 @@ __printf(3, 4) void verbose_linfo(struct bpf_verifier_env *env,
> >                                   u32 insn_off,
> >                                   const char *prefix_fmt, ...);
> >
> > +#define verifier_bug_if(cond, env, fmt, args...)                                               \
> > +       ({                                                                                      \
> > +               bool __cond = unlikely(cond);                                                   \
> > +               if (__cond) {                                                                   \
> 
> this might be equivalent in terms of code generation, but I'd expect
> unlikely() to be inside the if()
> 
> bool __cond = (cond);
> if (unlikely(__cond)) { ... }

I was worried the compiler may not take the unlikely into account when
doing if (verifier_bug_if(...)). I checked with a small example
involving a similar macro and the generated code is indeed the exact
same. I'll stick to the usual style, as suggested.

[...]

> > +                                                    bt_reg_mask(bt));
> >                                         return -EFAULT;
> >                                 }
> >                                 /* global subprog always sets R0 */
> > @@ -4299,16 +4295,16 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
> >                                  * the current frame should be zero by now
> >                                  */
> >                                 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, "unexpected precise regs %x",
> 
> "static subprog unexpected regs %x"
> 
> (note, user is not expected to really make sense out of this, it's
> just for reporting to us and our debugging, so let's make messages
> distinct, but they don't necessarily need to be precise, IMO)

That makes sense. Considering this, I went back over the four identical
"No program starts at insn" error messages and tweaked them a bit based
on context. All error messages should now be unique.

[...]