Re: [PATCH v2 bpf 3/4] selftests/bpf: Test refinement of single-value tnum

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2026-02-25 at 11:21 +0100, Paul Chaignon wrote:
> On Fri, Feb 20, 2026 at 04:20:55PM -0800, Eduard Zingerman wrote:
> > On Fri, 2026-02-20 at 14:57 +0100, Paul Chaignon wrote:
> > > This patch introduces selftests to cover the new bounds refinement
> > > logic introduced in the previous patch. Without the previous patch,
> > > the first two tests fail because of the invariant violation they
> > > trigger. The last test fails because the R10 access is not detected as
> > > dead code. In addition, all tests fail because of R0 having a
> > > non-constant value in the verifier logs.
> > > 
> > > Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
> > > ---
> > 
> > Hi Paul,
> 
> Hi Eduard,
> 
> Sorry for the late answer, I've been travelling.
> 
> > 
> > I have a few nitpicks, sorry for not commenting about it in v1.
> 
> No worries. The AI bot also found a new small thing :')
> 
> I noticed you haven't acked the first patch. I think it's a bit thougher
> to follow, so is there anything we could do to ease review?

It made sense on the first pass, comments help.
Let me re-read it tomorrow, but I assume this patch works.

> > 
> > >  .../selftests/bpf/progs/verifier_bounds.c     | 91 +++++++++++++++++++
> > >  1 file changed, 91 insertions(+)
> > > 
> > > diff --git a/tools/testing/selftests/bpf/progs/verifier_bounds.c
> > > b/tools/testing/selftests/bpf/progs/verifier_bounds.c
> > > index 560531404bce..41dd249faadd 100644
> > > --- a/tools/testing/selftests/bpf/progs/verifier_bounds.c
> > > +++ b/tools/testing/selftests/bpf/progs/verifier_bounds.c
> > > @@ -1863,4 +1863,95 @@ l1_%=:	r0 = 1;				\
> > >  	: __clobber_all);
> > >  }
> > > 
> > > +/* This test covers the bounds deduction when the u64 range and the tnum
> > > + * overlap only at umax. After instruction 3, the ranges look as follows:
> > > + *
> > > + * 0    umin=0xe01     umax=0xf00                              U64_MAX
> > > + * |    [xxxxxxxxxxxxxx]                                       |
> > > + * |----------------------------|------------------------------|
> > > + * |   x               x                                       | tnum values
> > > + *
> > > + * The verifier can therefore deduce that the R0=0xf00=3840.
> > > + */
> > > +SEC("socket")
> > > +__description("bounds refinement with single-value tnum on umax")
> > > +__msg("3: (15) if r0 == 0xe00 {{.*}} R0=3840")
> > > +__success __log_level(2)
> > > +__flag(BPF_F_TEST_REG_INVARIANTS)
> > > +__naked void bounds_refinement_tnum_umax(void *ctx)
> > > +{
> > > +	asm volatile("			\
> > > +	call %[bpf_get_prandom_u32];	\
> > > +	r0 |= 0xe00;			\
> > > +	r0 &= 0xf00;			\
> > > +	if r0 == 0xe00 goto +2;		\
> > > +	if r0 == 0xf00 goto +1;		\
> > > +	r0 = 0;				\
> > 
> > Nit: make this `r10 = 0;`, just like in the last test?
> >      (and in the next test).
> > 
> > Also, the test works the same if I replace 0xe00 -> 0xe, 0xf00 -> 0xf.
> > Maybe pick the smaller constants to ease the readability?
> 
> If we switch to 0xe and 0xf, the test won't validate the changes
> anymore: it will pass regardless of the second patch. That's because
> the range would only contain two values. We can however change it to
> 0xe0 and 0xf0.

You mean that min/max logic for 'r0 == 0xe' will adjust the range, right?
Makes sense, no changes necessary then.

> 
> [...]
> 
> > Also, I think a few more tests are necessary, there are three cases:
> > 
> >          if (umin_in_tnum && tnum_next > reg->umax_value) {  // A
> > 	 	...
> >          } else if (!umin_in_tnum && tnum_next == tmax) {    // B
> > 	 	...
> >          } else if (!umin_in_tnum && tnum_next <= reg->umax_value && // C
> > 	 	...
> >          }
> > 
> > If I remove 'umin_in_tnum &&' from A no tests fail.
> 
> That would be expected. As you pointed out in the v1, this part of the
> condition is not strictly necessary because it should be implied by the
> second part. We ended up adding it just for readability.

Wellp, that's a bit embarrassing, sorry for the noise.

> > If I remove '!umin_in_tnum &&' from B or C test cases
> > 'verifier_bounds/verifier_bounds/bounds check based on reg_off + var_off + insn_off. test{1,2}'
> > fail,
> > but these seem unrelated.
> > Maybe craft a few test cases covering these conditions?
> 
> Ack. I'll include the two cases I mentioned when discussing this in the
> v1.
> 
> Thanks for the review!