On Wed, May 3, 2023 at 8:07 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> 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?
Patch #3 has `bt_init(bt, frame);` which sets bt->frame = frame, so
since patch #3 we maintain a real frame number, it's just that the
rest of backtrack_insn() logic makes sure we never change frame (which
is why there should be no detectable change in behavior). Only in
patch #8 I add inter-frame bits propagation and generally changing
frame number with going into/out of subprog.
So it should be all good here.
> Probably this patch alone will break something and not bi-sectable?
>
I actually painstakingly compiled kernel and selftests after each
patch to make sure I'm not regressing anything (quite time consuming
effort, but necessary), so bisectability should be preserved.
> > + 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.
It's intentional that there is no effect. It's a preparation for the
next patch where we do take advantage of having precision bits across
multiple frames. I didn't want to bundle these indentation shifting
changes with the logic changes.
