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