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.
