Re: [PATCH bpf-next 1/2] bpf: Use tnums for JEQ/JNE is_branch_taken logic

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

On Thu, Aug 14, 2025 at 08:55:22PM +0800, Shung-Hsi Yu wrote:
> On Wed, Aug 13, 2025 at 05:34:08PM +0200, Paul Chaignon wrote:

Thanks for the review Shung-Hsi!

[...]

> > +bool tnum_agree(struct tnum a, struct tnum b)
> > +{
> > +	u64 mu;
> > +
> > +	mu = ~a.mask & ~b.mask;
> > +	return (a.value & mu) == (b.value & mu);
> > +}
> 
> Nit: I finding the naming a bit unconventional compared to other tnum
> helpers we have, with are either usually named after a BPF instruction
> or set operation. tnum_overlap() would be my choice for the name of such
> new helper.

Agree suggested name is better. Thanks!

> 
> One more comment below.
> 
> >  /* Note that if a and b disagree - i.e. one has a 'known 1' where the other has
> >   * a 'known 0' - this will return a 'known 1' for that bit.
> >   */
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 3a3982fe20d4..fa86833254e3 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -15891,6 +15891,8 @@ static int is_scalar_branch_taken(struct bpf_reg_state *reg1, struct bpf_reg_sta
> >  			return 0;
> >  		if (smin1 > smax2 || smax1 < smin2)
> >  			return 0;
> > +		if (!tnum_agree(t1, t2))
> > +			return 0;
> 
> Could we reuse tnum_xor() here instead?
> 
> If xor of two register cannot be 0, then the two can never hold the same
> value. Also we can use the tnum_xor() result in place of tnum_is_const()
> checks.
> 
> case BPF_JEQ:
>     t = tnum_xor(t1, t2);
>     if (!t.mask) /* Equvalent of tnum_is_const(t1) && tnum_is_const(t2) */
>         return t.value == 0;
>     if (umin1 > umax2 || umax1 < umin2)
>         return 0;
>     if (smin1 > smax2 || smax1 < smin2)
>         return 0;
>     if (!t.value) /* Equvalent of !tnum_agree(t1, t2) */
>       return 0;
>     ...
> case BPF_JNE:
>     t = tnum_xor(t1, t2);
>     if (!t.mask) /* Equvalent of tnum_is_const(t1) && tnum_is_const(t2) */
>         return t.value != 0;
>     /* non-overlapping ranges */
>     if (umin1 > umax2 || umax1 < umin2)
>         return 1;
>     if (smin1 > smax2 || smax1 < smin2)
>         return 1;
>     if (!t.value) /* Equvalent of !tnum_agree(t1, t2) */
>         return 1;
> 
> Looks slighly less readable though.

Nice find on the xor-based equivalent version! I lean a bit toward
keeping the slightly more readable version though. The xor version is
probably a bit more efficient, but I'm unsure that matters much for this
piece of code, whereas I think readability matters a lot here. That
said, if others prefer the xor version, I don't mind much :)