Skip to content

Commit acb12c8

Browse files
author
Alexei Starovoitov
committedNov 21, 2023
Merge branch 'verify-callbacks-as-if-they-are-called-unknown-number-of-times'
Eduard Zingerman says: ==================== verify callbacks as if they are called unknown number of times This series updates verifier logic for callback functions handling. Current master simulates callback body execution exactly once, which leads to verifier not detecting unsafe programs like below: static int unsafe_on_zero_iter_cb(__u32 idx, struct num_context *ctx) { ctx->i = 0; return 0; } SEC("?raw_tp") int unsafe_on_zero_iter(void *unused) { struct num_context loop_ctx = { .i = 32 }; __u8 choice_arr[2] = { 0, 1 }; bpf_loop(100, unsafe_on_zero_iter_cb, &loop_ctx, 0); return choice_arr[loop_ctx.i]; } This was reported previously in [0]. The basic idea of the fix is to schedule callback entry state for verification in env->head until some identical, previously visited state in current DFS state traversal is found. Same logic as with open coded iterators, and builds on top recent fixes [1] for those. The series is structured as follows: - patches #1,2,3 update strobemeta, xdp_synproxy selftests and bpf_loop_bench benchmark to allow convergence of the bpf_loop callback states; - patches #4,5 just shuffle the code a bit; - patch #6 is the main part of the series; - patch #7 adds test cases for #6; - patch #8 extend patch #6 with same speculative scalar widening logic, as used for open coded iterators; - patch #9 adds test cases for #8; - patch #10 extends patch #6 to track maximal number of callback executions specifically for bpf_loop(); - patch #11 adds test cases for #10. Veristat results comparing this series to master+patches #1,2,3 using selftests show the following difference: File Program States (A) States (B) States (DIFF) ------------------------- ------------- ---------- ---------- ------------- bpf_loop_bench.bpf.o benchmark 1 2 +1 (+100.00%) pyperf600_bpf_loop.bpf.o on_event 322 407 +85 (+26.40%) strobemeta_bpf_loop.bpf.o on_event 113 151 +38 (+33.63%) xdp_synproxy_kern.bpf.o syncookie_tc 341 291 -50 (-14.66%) xdp_synproxy_kern.bpf.o syncookie_xdp 344 301 -43 (-12.50%) Veristat results comparing this series to master using Tetragon BPF files [2] also show some differences. States diff varies from +2% to +15% on 23 programs out of 186, no new failures. Changelog: - V3 [5] -> V4, changes suggested by Andrii: - validate mark_chain_precision() result in patch #10; - renaming s/cumulative_callback_depth/callback_unroll_depth/. - V2 [4] -> V3: - fixes in expected log messages for test cases: - callback_result_precise; - parent_callee_saved_reg_precise_with_callback; - parent_stack_slot_precise_with_callback; - renamings (suggested by Alexei): - s/callback_iter_depth/cumulative_callback_depth/ - s/is_callback_iter_next/calls_callback/ - s/mark_callback_iter_next/mark_calls_callback/ - prepare_func_exit() updated to exit with -EFAULT when callee->in_callback_fn is true but calls_callback() is not true for callsite; - test case 'bpf_loop_iter_limit_nested' rewritten to use return value check instead of verifier log message checks (suggested by Alexei). - V1 [3] -> V2, changes suggested by Andrii: - small changes for error handling code in __check_func_call(); - callback body processing log is now matched in relevant verifier_subprog_precision.c tests; - R1 passed to bpf_loop() is now always marked as precise; - log level 2 message for bpf_loop() iteration termination instead of iteration depth messages; - __no_msg macro removed; - bpf_loop_iter_limit_nested updated to avoid using __no_msg; - commit message for patch #3 updated according to Alexei's request. [0] https://lore.kernel.org/bpf/CA+vRuzPChFNXmouzGG+wsy=6eMcfr1mFG0F3g7rbg-sedGKW3w@mail.gmail.com/ [1] https://lore.kernel.org/bpf/[email protected]/ [2] [email protected]:cilium/tetragon.git [3] https://lore.kernel.org/bpf/[email protected]/T/#t [4] https://lore.kernel.org/bpf/[email protected]/T/#t [5] https://lore.kernel.org/bpf/[email protected]/T/#t ==================== Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Alexei Starovoitov <[email protected]>
2 parents fcb905d + 57e2a52 commit acb12c8

File tree

10 files changed

+709
-217
lines changed

10 files changed

+709
-217
lines changed
 

‎include/linux/bpf_verifier.h

+16
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,17 @@ struct bpf_func_state {
301301
struct tnum callback_ret_range;
302302
bool in_async_callback_fn;
303303
bool in_exception_callback_fn;
304+
/* For callback calling functions that limit number of possible
305+
* callback executions (e.g. bpf_loop) keeps track of current
306+
* simulated iteration number.
307+
* Value in frame N refers to number of times callback with frame
308+
* N+1 was simulated, e.g. for the following call:
309+
*
310+
* bpf_loop(..., fn, ...); | suppose current frame is N
311+
* | fn would be simulated in frame N+1
312+
* | number of simulations is tracked in frame N
313+
*/
314+
u32 callback_depth;
304315

305316
/* The following fields should be last. See copy_func_state() */
306317
int acquired_refs;
@@ -400,6 +411,7 @@ struct bpf_verifier_state {
400411
struct bpf_idx_pair *jmp_history;
401412
u32 jmp_history_cnt;
402413
u32 dfs_depth;
414+
u32 callback_unroll_depth;
403415
};
404416

405417
#define bpf_get_spilled_reg(slot, frame, mask) \
@@ -511,6 +523,10 @@ struct bpf_insn_aux_data {
511523
* this instruction, regardless of any heuristics
512524
*/
513525
bool force_checkpoint;
526+
/* true if instruction is a call to a helper function that
527+
* accepts callback function as a parameter.
528+
*/
529+
bool calls_callback;
514530
};
515531

516532
#define MAX_USED_MAPS 64 /* max number of maps accessed by one eBPF program */

0 commit comments

Comments
 (0)
Please sign in to comment.