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 :)
