Re: [PATCH bpf-next v2] bpf: Rewrite "rX <<= 32; rX >>= 32" into "wX = wX" to keep linked scalars

[Eduard Zingerman <eddyz87@gmail.com>]

On Sat, 2026-08-22 at 09:47 -0700, Yonghong Song wrote:

...

> > > @@ -15878,7 +15886,12 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
> > >   						insn->src_reg);
> > >   					return -EACCES;
> > >   				} else if (src_reg->type == SCALAR_VALUE) {
> > > -					if (insn->off == 0) {
> > > +					if (insn->off == 0 && insn->src_reg == insn->dst_reg &&
> > > +					    (dst_reg->id & BPF_ADD_CONST) &&
> > > +					    linked_base_fits_u32(dst_reg)) {
> > > +						dst_reg->id = (dst_reg->id & ~BPF_ADD_CONST64) |
> > > +							      BPF_ADD_CONST32;
> > This commit consists of two parts:
> > - a special case for wA = wA assignment
> > - a rewrite for `rA <<= 32; rA >>= 32;` pair
> > 
> > Could you please split it in two with separate selftest for each.
> 
> Let me explain a little bit more.
> 
> For these four insns
> 
>    3: (bf) r2 = r1               ; R1=scalar(id=1,...) R2=scalar(id=1,...)
>    4: (07) r2 += -5              ; R2=scalar(id=1-5,smin=-5,smax=0xfffffffa)
>    5: (67) r2 <<= 32             ; R2=scalar(smax=0x7fffffff00000000,...)
>    6: (77) r2 >>= 32             ; R2=scalar(smin=0,umax=0xffffffff,...)
> 
> Eventually, the above insns will be converted to
> 
>    3: (bf) r2 = r1               ; R1=scalar(id=1,...) R2=scalar(id=1,...)
>    4: (07) r2 += -5              ; R2=scalar(id=1-5,smin=-5,smax=0xfffffffa)
>    5: (bc) w2 = w2               ; R2=scalar(id=1-5,smin=0,umax=0xffffffff)
>    6: (bc) w2 = w2               ; R2=scalar(id=1-5,smin=0,umax=0xffffffff)
> 
> insn 5 and 6 are the ones to be changed.
> 
> Here:
> 
>    5: (67) r2 <<= 32             ; R2=scalar(smax=0x7fffffff00000000,...)
>    6: (77) r2 >>= 32             ; R2=scalar(smin=0,umax=0xffffffff,...)
> converted to
>    5: (bc) w2 = w2               ; R2=scalar(id=1-5,smin=0,umax=0xffffffff)
> 
> This insn
>    6: (bc) w2 = w2               ; R2=scalar(id=1-5,smin=0,umax=0xffffffff)
> is not really necessary. I put it here to maintain existing control flow graph.
> Previously for insn 6, I used "goto pc+0" which will be removed later.
> But "goto pc+0" has some impact on verification as it added yet another jump.
> See bpf_is_state_visited():
> 
>          if (env->jmps_processed - env->prev_jmps_processed >= 2 &&
>              env->insn_processed - env->prev_insn_processed >= 8)
>                  add_new_state = true;
> 
> Here "goto pc+0" may impact for some programs. I didn't investigate this
> in detail but probably I should investigate and fix the root cause.
> 
> Alternatively, we can rebuild the control flow graph by removing insn 6.
> I am just not sure whether this is worthwhile or not.
> 
> > Also, could you please comment why the special case for `wA = wA` is necessary?
> 
> The second wA = wA is not needed. We can remove it and rebuild the control flow
> for it, just not sure whether this is worthwhile or not. Probably yes.

I understand why you insert `wA = wA` twice, the question is about a
special case for `wA = wA` in check_alu_op(), why would you need
additional handling there specifically?

> > Is it because assign_scalar_id_before_mov() destroys the link:
> > 
> >    static void assign_scalar_id_before_mov(struct bpf_verifier_env *env,
> > 					  struct bpf_reg_state *src_reg)
> > 	...
> > 	if (src_reg->id & BPF_ADD_CONST)
> > 		clear_scalar_id(src_reg);
> > 
> > ?
> > 
> > If that's the only reason, is it possible to extend existing wA = wB
> > logic instead of adding a special case?

^^^
The above part is still relevant for the question.