On Mon, Nov 20, 2023 at 3:00 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > Prior to this patch callbacks were handled as regular function calls, > execution of callback body was modeled exactly once. > This patch updates callbacks handling logic as follows: > - introduces a function push_callback_call() that schedules callback > body verification in env->head stack; > - updates prepare_func_exit() to reschedule callback body verification > upon BPF_EXIT; > - as calls to bpf_*_iter_next(), calls to callback invoking functions > are marked as checkpoints; > - is_state_visited() is updated to stop callback based iteration when > some identical parent state is found. > > Paths with callback function invoked zero times are now verified first, > which leads to necessity to modify some selftests: > - the following negative tests required adding release/unlock/drop > calls to avoid previously masked unrelated error reports: > - cb_refs.c:underflow_prog > - exceptions_fail.c:reject_rbtree_add_throw > - exceptions_fail.c:reject_with_cp_reference > - the following precision tracking selftests needed change in expected > log trace: > - verifier_subprog_precision.c:callback_result_precise > (note: r0 precision is no longer propagated inside callback and > I think this is a correct behavior) > - verifier_subprog_precision.c:parent_callee_saved_reg_precise_with_callback > - verifier_subprog_precision.c:parent_stack_slot_precise_with_callback > > Reported-by: Andrew Werner <awerner32@xxxxxxxxx> > Closes: https://lore.kernel.org/bpf/CA+vRuzPChFNXmouzGG+wsy=6eMcfr1mFG0F3g7rbg-sedGKW3w@xxxxxxxxxxxxxx/ > Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx> > --- LGTM! Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > include/linux/bpf_verifier.h | 5 + > kernel/bpf/verifier.c | 274 +++++++++++------- > tools/testing/selftests/bpf/progs/cb_refs.c | 1 + > .../selftests/bpf/progs/exceptions_fail.c | 2 + > .../bpf/progs/verifier_subprog_precision.c | 71 ++++- > 5 files changed, 240 insertions(+), 113 deletions(-) > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > index 24213a99cc79..6e21d74a64e7 100644 > --- a/include/linux/bpf_verifier.h > +++ b/include/linux/bpf_verifier.h > @@ -400,6 +400,7 @@ struct bpf_verifier_state { > struct bpf_idx_pair *jmp_history; > u32 jmp_history_cnt; > u32 dfs_depth; > + u32 cumulative_callback_depth; nit: depth and cumulative in this case doesn't make much sense to me, tbh. Cumulative is generally a sum of something. You don't seem to ever decrement this, so I guess "max_callback_depth" or "cur_callback_depth" would also make sense? "callback_unroll_depth" is another name that came to mind. But it's honestly not that important to me, the use of this field is very minimal in the code base. > }; > > #define bpf_get_spilled_reg(slot, frame, mask) \ > @@ -511,6 +512,10 @@ struct bpf_insn_aux_data { > * this instruction, regardless of any heuristics > */ > bool force_checkpoint; > + /* true if instruction is a call to a helper function that > + * accepts callback function as a parameter. > + */ > + bool calls_callback; > }; > > #define MAX_USED_MAPS 64 /* max number of maps accessed by one eBPF program */ [...]
