| Message ID | 20220827135711.21507-1-liulin063@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf,v2,1/2] bpf: Do more tight ALU bounds tracking | expand |
Hi Youlin, On Sat, Aug 27, 2022 at 6:57 AM Youlin Li <liulin063@gmail.com> wrote: > > In adjust_scalar_min_max_vals(), let 32bit bounds learn from 64bit bounds > to get more tight bounds tracking. Similar operation can be found in > reg_set_min_max(). > > Note that we cannot simply add a call to __reg_combine_64_into_32(). In > previous versions of the code, when __reg_combine_64_into_32() was > called, the 32bit boundary was completely deduced from the 64bit > boundary, so there was a call to __mark_reg32_unbounded() in > __reg_combine_64_into_32(). But in adjust_scalar_min_max_vals(), the 32bit > bounds are already calculated to some extent, and __mark_reg32_unbounded() > will eliminate these information. > > Simply copying a code without __mark_reg32_unbounded() should work. > > Also, we can now fold reg_bounds_sync() into zext_32_to_64(). > > Before: > > func#0 @0 > 0: R1=ctx(off=0,imm=0) R10=fp0 > 0: (b7) r0 = 0 ; R0_w=0 > 1: (b7) r1 = 0 ; R1_w=0 > 2: (87) r1 = -r1 ; R1_w=scalar() > 3: (87) r1 = -r1 ; R1_w=scalar() > 4: (c7) r1 s>>= 63 ; R1_w=scalar(smin=-1,smax=0) > 5: (07) r1 += 2 ; R1_w=scalar(umin=1,umax=2,var_off=(0x0; 0xffffffff)) <--- [*] > 6: (95) exit > > It can be seen that even if the 64bit bounds is clear here, the 32bit > bounds is still in the state of 'UNKNOWN'. > > After: > > func#0 @0 > 0: R1=ctx(off=0,imm=0) R10=fp0 > 0: (b7) r0 = 0 ; R0_w=0 > 1: (b7) r1 = 0 ; R1_w=0 > 2: (87) r1 = -r1 ; R1_w=scalar() > 3: (87) r1 = -r1 ; R1_w=scalar() > 4: (c7) r1 s>>= 63 ; R1_w=scalar(smin=-1,smax=0) > 5: (07) r1 += 2 ; R1_w=scalar(umin=1,umax=2,var_off=(0x0; 0x3)) <--- [*] > 6: (95) exit > > Signed-off-by: Youlin Li <liulin063@gmail.com> > --- It might be better to put the code that performs the actual bounds deduction into a helper function. It avoids code duplication. But the current version looks fine to me. Thanks for the patch! Acked-by: Hao Luo <haoluo@google.com>
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 3eadb14e090b..b7403773e834 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4383,6 +4383,7 @@ static void zext_32_to_64(struct bpf_reg_state *reg) { reg->var_off = tnum_subreg(reg->var_off); __reg_assign_32_into_64(reg); + reg_bounds_sync(reg); } /* truncate register to smaller size (in bytes) @@ -9010,10 +9011,22 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, break; } - /* ALU32 ops are zero extended into 64bit register */ - if (alu32) + if (alu32) { + /* ALU32 ops are zero extended into 64bit register */ zext_32_to_64(dst_reg); - reg_bounds_sync(dst_reg); + } else { + if (__reg64_bound_s32(dst_reg->smin_value) && + __reg64_bound_s32(dst_reg->smax_value)) { + dst_reg->s32_min_value = (s32)dst_reg->smin_value; + dst_reg->s32_max_value = (s32)dst_reg->smax_value; + } + if (__reg64_bound_u32(dst_reg->umin_value) && + __reg64_bound_u32(dst_reg->umax_value)) { + dst_reg->u32_min_value = (u32)dst_reg->umin_value; + dst_reg->u32_max_value = (u32)dst_reg->umax_value; + } + reg_bounds_sync(dst_reg); + } return 0; } @@ -9202,7 +9215,6 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn) insn->dst_reg); } zext_32_to_64(dst_reg); - reg_bounds_sync(dst_reg); } } else { /* case: R = imm
In adjust_scalar_min_max_vals(), let 32bit bounds learn from 64bit bounds to get more tight bounds tracking. Similar operation can be found in reg_set_min_max(). Note that we cannot simply add a call to __reg_combine_64_into_32(). In previous versions of the code, when __reg_combine_64_into_32() was called, the 32bit boundary was completely deduced from the 64bit boundary, so there was a call to __mark_reg32_unbounded() in __reg_combine_64_into_32(). But in adjust_scalar_min_max_vals(), the 32bit bounds are already calculated to some extent, and __mark_reg32_unbounded() will eliminate these information. Simply copying a code without __mark_reg32_unbounded() should work. Also, we can now fold reg_bounds_sync() into zext_32_to_64(). Before: func#0 @0 0: R1=ctx(off=0,imm=0) R10=fp0 0: (b7) r0 = 0 ; R0_w=0 1: (b7) r1 = 0 ; R1_w=0 2: (87) r1 = -r1 ; R1_w=scalar() 3: (87) r1 = -r1 ; R1_w=scalar() 4: (c7) r1 s>>= 63 ; R1_w=scalar(smin=-1,smax=0) 5: (07) r1 += 2 ; R1_w=scalar(umin=1,umax=2,var_off=(0x0; 0xffffffff)) <--- [*] 6: (95) exit It can be seen that even if the 64bit bounds is clear here, the 32bit bounds is still in the state of 'UNKNOWN'. After: func#0 @0 0: R1=ctx(off=0,imm=0) R10=fp0 0: (b7) r0 = 0 ; R0_w=0 1: (b7) r1 = 0 ; R1_w=0 2: (87) r1 = -r1 ; R1_w=scalar() 3: (87) r1 = -r1 ; R1_w=scalar() 4: (c7) r1 s>>= 63 ; R1_w=scalar(smin=-1,smax=0) 5: (07) r1 += 2 ; R1_w=scalar(umin=1,umax=2,var_off=(0x0; 0x3)) <--- [*] 6: (95) exit Signed-off-by: Youlin Li <liulin063@gmail.com> --- v1 -> v2: Replaced the call to __reg_combine_64_into_32() with the code in __reg_combine_64_into_32(), and removed the call to __mark_reg32_unbounded(). Sorry for the delay, I've been busy looking for a job recently :) kernel/bpf/verifier.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)