On Thu, May 04, 2023 at 01:50:29PM -0700, Andrii Nakryiko wrote:
> 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.
Got it, but then remove 'frame' arg in this patch too, since it's unused
here and anything passed into it the later patches is also ignored.
