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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
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.
[...]
