Re: [PATCH bpf 2/4] bpf: Improve bounds when tnum has a single possible value

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

On Thu, Feb 19, 2026 at 10:32:19AM -0800, Eduard Zingerman wrote:
> On Wed, 2026-02-18 at 01:06 -0500, Harishankar Vishwanathan wrote:
> > On Tue, Feb 17, 2026 at 5:58 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > [...]
> > > > 1. The u64 range and the tnum only overlap in umin.
> > > >    u64:  ---[xxxxxx]-----
> > > >    tnum: --xx----------x-
> > > I think this hunk should be rewritten as follows:
> > > 
> > >         tnum_next = tnum_step(reg->var_off, reg->umin_value);
> > >         tnum_max = reg->var_off.value | reg->var_off.mask;
> > >         tnum_min = reg->var_off.value;
> > >         if (tnum_next > reg->umax_value) {
> > >                 /* The only overlap is umin */
> > >                 ___mark_reg_known(reg, tnum_min);
> > >         } else if (tnum_min < reg->umin_value && tnum_next == tnum_max) {
> > >                 /* The only overlap is tmax */
> > >                 ___mark_reg_known(reg, tnum_next);
> > >         } else if (tnum_next <= reg->umax_value &&
> > >                    tnum_step(reg->var_off, tnum_next) > reg->umax_value) {
> > >                 ___mark_reg_known(reg, tnum_next);
> > >         }
> > > 
> > > - At-least to me, it easier to understand this way.

Agree. That looks easier to parse to me too. Thanks!

> > 
> > We can also use tmin and tmax, like tnum_step from the previous commit does,
> > to keep things consistent.
> > 
> > > - There is no need to gate the condition `tnum_next > reg->umax_value`,
> > >   if next tnum overshoots the reg->umax_value then only tnum_min is left.
> > 
> 
> Hi Harishankar,
> 
> Sorry for delayed response.
> 
> > The guard helps avoid the case where u64 and tnum have no intersection:
> > u64:  ---[xxxxxx]-----
> > tnum: x------------x-
> > 
> > The entire condition for "only overlap of tnum is with umin" needs to check both
> > (1) if umin is a member of var_off
> > (2) if the next member of var_off after umin > umax.
> 
> Right before the hunk there is:
> 
>   reg->umin_value = max(reg->umin_value, reg->var_off.value);
> 
> Which implies that:
> 
>   tnum_min <= reg->umin_value
> 
> My mental model for register sync functions is that all ranges are
> considered valid, hence I suggested dropping the check.

I think I agree with that, especially considering the reg->umin_value
computation above. If we think we need extra checks to be on the safe
side, they should probably live in reg_bounds_sanity_check.

> If you think the check should be preserved, maybe still rewrite it like:
> 
>   tnum_min == reg->umin_value && tnum_next > reg->umax_value
> 
> That's a bit easier to parse compared to
> '(reg->umin_value & ~reg->var_off.mask) == reg->var_off.value'.
> Wdyt?
> 
> > 
> > > - Accidentally, it fixes the scx_cosmos regression
> > >   (probably, because first condition is relaxed).
> 
> I double-checked scx_cosmos with current master, patch as posted and
> patch with my changes, in all three cases I see:
> 
> File              Program             Verdict  Insns
> ----------------  ------------------  -------  -----
> scx_cosmos.bpf.o  cosmos_select_cpu   success    475
> 
> So, I'm not sure why CI flagged this as +500%.

Ok, that's reassuring. I was going crazy trying to understand how your
changes could have fixed this given the semantics should be unchanged.

I'm re-testing the patchset on Cilium just to be sure, and I'll send a
v2 afterward. Thanks for the review!