On Tue, Apr 25, 2023 at 04:49:06PM -0700, Andrii Nakryiko wrote: > Teach __mark_chain_precision logic to maintain register/stack masks > across all active frames when going from child state to parent state. > Currently this should be mostly no-op, as precision backtracking usually > bails out when encountering subprog entry/exit. > > It's not very apparent from the diff due to increased indentation, but > the logic remains the same, except everything is done on specific `fr` > frame index. Calls to bt_clear_reg() and bt_clear_slot() are replaced > with frame-specific bt_clear_frame_reg() and bt_clear_frame_slot(), > where frame index is passed explicitly, instead of using current frame > number. > > We also adjust logging to emit affected frame number. And we also add > better logging of human-readable register and stack slot masks, similar > to previous patch. > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > --- > kernel/bpf/verifier.c | 101 ++++++++++-------- > .../testing/selftests/bpf/verifier/precise.c | 18 ++-- > 2 files changed, 63 insertions(+), 56 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 8faf9170acf0..0b19b3d9af65 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -3703,7 +3703,7 @@ static int __mark_chain_precision(struct bpf_verifier_env *env, int frame, int r > struct bpf_func_state *func; > struct bpf_reg_state *reg; > bool skip_first = true; > - int i, err; > + int i, fr, err; > > if (!env->bpf_capable) > return 0; > @@ -3812,56 +3812,63 @@ static int __mark_chain_precision(struct bpf_verifier_env *env, int frame, int r > if (!st) > break; > > - func = st->frame[frame]; > - bitmap_from_u64(mask, bt_reg_mask(bt)); > - for_each_set_bit(i, mask, 32) { > - reg = &func->regs[i]; > - if (reg->type != SCALAR_VALUE) { > - bt_clear_reg(bt, i); > - continue; > + for (fr = bt->frame; fr >= 0; fr--) { I'm lost. 'frame' arg is now unused and the next patch passes -1 into it anyway? Probably this patch alone will break something and not bi-sectable? > + func = st->frame[fr]; > + bitmap_from_u64(mask, bt_frame_reg_mask(bt, fr)); .. > diff --git a/tools/testing/selftests/bpf/verifier/precise.c b/tools/testing/selftests/bpf/verifier/precise.c > index fce831667b06..ac9be4c576d6 100644 > --- a/tools/testing/selftests/bpf/verifier/precise.c > +++ b/tools/testing/selftests/bpf/verifier/precise.c > @@ -44,7 +44,7 @@ > mark_precise: frame0: regs(0x4)=r2 stack(0x0)= before 23\ > mark_precise: frame0: regs(0x4)=r2 stack(0x0)= before 22\ > mark_precise: frame0: regs(0x4)=r2 stack(0x0)= before 20\ > - parent didn't have regs=4 stack=0 marks:\ > + mark_precise: frame0: parent state regs(0x4)=r2 stack(0x0)=:\ > mark_precise: frame0: last_idx 19 first_idx 10\ > mark_precise: frame0: regs(0x4)=r2 stack(0x0)= before 19\ > mark_precise: frame0: regs(0x200)=r9 stack(0x0)= before 18\ > @@ -55,7 +55,7 @@ > mark_precise: frame0: regs(0x200)=r9 stack(0x0)= before 12\ > mark_precise: frame0: regs(0x200)=r9 stack(0x0)= before 11\ > mark_precise: frame0: regs(0x200)=r9 stack(0x0)= before 10\ > - parent already had regs=0 stack=0 marks:", > + mark_precise: frame0: parent state regs(0x0)= stack(0x0)=:", The effect of the patch looks minor, so it might be correct, just super confusing.