| Message ID | 20220729044317.31975-1-liulin063@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf] bpf: Do more tight ALU bounds tracking | expand |
Hi Youlin, On Thu, Jul 28, 2022 at 9:44 PM Youlin Li <liulin063@gmail.com> wrote: > > 32bit bounds and 64bit bounds are updated separately in > adjust_scalar_min_max_vals() currently, let them learn from each other to > get more tight bounds tracking. Similar operation can be found in > reg_set_min_max(). > > 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 > > Fixes: 3f50f132d840 ("bpf: Verifier, do explicit ALU32 bounds tracking") This change looks to me like an improvement, rather than a bug fix. We probably don't need this tag. > Signed-off-by: Youlin Li <liulin063@gmail.com> > --- > kernel/bpf/verifier.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 0efbac0fd126..888aa50fbdc0 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -8934,10 +8934,13 @@ 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); > + __reg_combine_32_into_64(dst_reg); This __reg_combine_32_into_64 can be replaced with simply reg_bounds_sync, because the above zext_32_to_64 has already propagated 32 to 64. Using reg_bounds_sync would be more efficient. It turns out we can now fold reg_bounds_sync into zext_32_to_64. Can you do that and resend? IMO that will make the code slightly cleaner. > + } else { > + __reg_combine_64_into_32(dst_reg); > + } > return 0; > } > > -- > 2.25.1 >
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 0efbac0fd126..888aa50fbdc0 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -8934,10 +8934,13 @@ 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); + __reg_combine_32_into_64(dst_reg); + } else { + __reg_combine_64_into_32(dst_reg); + } return 0; }
32bit bounds and 64bit bounds are updated separately in adjust_scalar_min_max_vals() currently, let them learn from each other to get more tight bounds tracking. Similar operation can be found in reg_set_min_max(). 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 Fixes: 3f50f132d840 ("bpf: Verifier, do explicit ALU32 bounds tracking") Signed-off-by: Youlin Li <liulin063@gmail.com> --- kernel/bpf/verifier.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)