Skip to content

Commit 05e2249

Browse files
author
Alexei Starovoitov
committed
Merge branch 'bpf-propagate-read-precision-marks-over-state-graph-backedges'
Eduard Zingerman says: ==================== bpf: propagate read/precision marks over state graph backedges Current loop_entry-based states comparison logic does not handle the following case: .-> A --. Assume the states are visited in the order A, B, C. | | | Assume that state B reaches a state equivalent to state A. | v v At this point, state C is not processed yet, so state A '-- B C has not received any read or precision marks from C. As a result, these marks won't be propagated to B. If B has incomplete marks, it is unsafe to use it in states_equal() checks. This issue was first reported in [1]. This patch-set -------------- Here is the gist of the algorithm implemented by this patch-set: - Compute strongly connected components (SCCs) in the program CFG. - When a verifier state enters an SCC, that state is recorded as the SCC's entry point. - When a verifier state is found to be equivalent to another (e.g., B to A in the example above), it is recorded as a states-graph backedge. - Backedges are accumulated per SCC (*). - When an SCC entry state reaches `branches == 0`, propagate read and precision marks through the backedges until a fixed point is reached (e.g., from A to B, from C to A, and then again from A to B). (*) This is an oversimplification, see patch #8 for details. Unfortunately, this means that commit [2] needs to be reverted, as precision propagation requires access to jump history, and backedges represent history not belonging to `env->cur_state`. Details are provided in patch #8; a comment in `is_state_visited()` explains most of the mechanics. Patch #2 adds a `compute_scc()` function, which computes SCCs in the program CFG. This function was tested using property-based testing in [3], but it is not included in selftests. Previous attempt ---------------- A previous attempt to fix this is described in [4]: 1. Within the states loop, `states_equal(... RANGE_WITHIN)` ignores read and precision marks. 2. For states outside the loop, all registers for states within the loop are marked as read and precise. This approach led to an 86x regression on the `cond_break1` selftest. In that test, one loop was followed by another, and a certain variable was incremented in the second loop. This variable was marked as precise due to rule (2), which hindered convergence in the first loop. After some off-list discussion, it was decided that this might be a typical case and such regressions are undesirable. This patch-set avoids such eager precision markings. Alternatives ------------ Another option is to associate a mask of read/written/precise stack slots with each instruction. This mask can be populated during verifier states exploration. Upon reaching an `EXIT` instruction or an equivalent state, the accumulated masks can be used to propagate read/written/precise bits across the program's control flow graph using an analysis similar to use-def. Unfortunately, a naive implementation of this approach [5] results in a 10x regression in `veristat` for some `sched_ext` programs due to the inability to express the must-write property. This issue requires further investigation. Changes in verification performance ----------------------------------- There are some veristat regressions when comparing with master using selftests and sched_ext BPF binaries. The comparison is done using master from [6] and this patch-set from [7] where memory accounting logic is added to veristat. ========= selftests: master vs patch-set ========= File Program Insns Peak memory (KiB) --------------------- ----------------------------------- ----- ----- ---------------- ---- ----- ---------------- bpf_qdisc_fq.bpf.o bpf_fq_dequeue 1187 1645 +458 (+38.58%) 768 1240 +472 (+61.46%) dynptr_success.bpf.o test_copy_from_user_str_dynptr 208 279 +71 (+34.13%) 512 1024 +512 (+100.00%) dynptr_success.bpf.o test_copy_from_user_task_str_dynptr 205 263 +58 (+28.29%) 512 1024 +512 (+100.00%) dynptr_success.bpf.o test_probe_read_kernel_str_dynptr 686 857 +171 (+24.93%) 992 1724 +732 (+73.79%) dynptr_success.bpf.o test_probe_read_user_str_dynptr 689 860 +171 (+24.82%) 1016 1744 +728 (+71.65%) iters.bpf.o checkpoint_states_deletion 1211 1216 +5 (+0.41%) 512 1280 +768 (+150.00%) pyperf600_iter.bpf.o on_event 2591 5929 +3338 (+128.83%) 4744 11176 +6432 (+135.58%) verifier_gotol.bpf.o gotol_large_imm 40004 40004 +0 (+0.00%) 1024 1536 +512 (+50.00%) Total progs: 3725 Old success: 2157 New success: 2157 total_insns diff min: 0.00% total_insns diff max: 128.83% 0 -> value: 0 value -> 0: 0 total_insns abs max old: 837,487 total_insns abs max new: 837,487 0 .. 5 %: 3710 5 .. 15 %: 6 20 .. 30 %: 6 30 .. 40 %: 2 125 .. 130 %: 1 mem_peak diff min: -27.78% mem_peak diff max: 198.44% mem_peak abs max old: 269,312 KiB mem_peak abs max new: 269,312 KiB -30 .. -20 %: 1 -5 .. 0 %: 18 0 .. 5 %: 3568 5 .. 15 %: 4 15 .. 25 %: 3 45 .. 55 %: 4 60 .. 70 %: 1 70 .. 80 %: 2 100 .. 110 %: 3 135 .. 145 %: 1 150 .. 160 %: 1 195 .. 200 %: 1 ========= scx: master vs patch-set ========= Program Insns Peak memory (KiB) ------------------------ ----- ----- --------------- ----- ----- ----------------- arena_topology_node_init 2133 2395 +262 (+12.28%) 768 768 +0 (+0.00%) chaos_dispatch 2835 2868 +33 (+1.16%) 1972 1720 -252 (-12.78%) chaos_init 4324 5210 +886 (+20.49%) 2528 3028 +500 (+19.78%) lavd_cpu_offline 5107 5726 +619 (+12.12%) 4188 6304 +2116 (+50.53%) lavd_cpu_online 5107 5726 +619 (+12.12%) 4188 6304 +2116 (+50.53%) lavd_dispatch 41775 47601 +5826 (+13.95%) 6196 29192 +22996 (+371.14%) lavd_enqueue 20238 24188 +3950 (+19.52%) 22084 42156 +20072 (+90.89%) lavd_init 6974 7685 +711 (+10.20%) 5428 6928 +1500 (+27.63%) lavd_select_cpu 22138 26088 +3950 (+17.84%) 24448 43688 +19240 (+78.70%) layered_dispatch 17847 26581 +8734 (+48.94%) 11728 28740 +17012 (+145.05%) layered_dump 1891 2098 +207 (+10.95%) 2036 3048 +1012 (+49.71%) layered_runnable 2606 2634 +28 (+1.07%) 748 1240 +492 (+65.78%) p2dq_init 3691 4554 +863 (+23.38%) 2016 2528 +512 (+25.40%) rusty_enqueue 28853 28853 +0 (+0.00%) 2072 1824 -248 (-11.97%) rusty_init_task 31128 31128 +0 (+0.00%) 2176 2560 +384 (+17.65%) Total progs: 148 Old success: 135 New success: 135 total_insns diff min: 0.00% total_insns diff max: 48.94% 0 -> value: 0 value -> 0: 0 total_insns abs max old: 41,775 total_insns abs max new: 47,601 0 .. 5 %: 133 5 .. 15 %: 7 15 .. 25 %: 4 35 .. 45 %: 3 45 .. 50 %: 1 mem_peak diff min: -12.78% mem_peak diff max: 371.14% mem_peak abs max old: 24,448 KiB mem_peak abs max new: 43,688 KiB -15 .. -5 %: 2 -5 .. 0 %: 2 0 .. 5 %: 129 5 .. 15 %: 1 15 .. 25 %: 2 25 .. 35 %: 2 45 .. 55 %: 3 65 .. 75 %: 1 75 .. 85 %: 1 90 .. 100 %: 1 145 .. 155 %: 1 195 .. 205 %: 1 370 .. 375 %: 1 Changelog --------- v1: https://lore.kernel.org/bpf/[email protected]/ v1 -> v2: - Rebase - added mem_peak statistics (Alexei) - selftests: fixed comments and removed useless r7 assignments (Yonghong) v2: https://lore.kernel.org/bpf/[email protected]/ v2 -> v3: - Rebase Links ----- [1] https://lore.kernel.org/bpf/[email protected]/ [2] commit 96a30e4 ("bpf: use common instruction history across all states") [3] https://github.com/eddyz87/scc-test [4] https://lore.kernel.org/bpf/[email protected]/ [5] https://github.com/eddyz87/bpf/tree/propagate-read-and-precision-in-cfg [6] https://github.com/eddyz87/bpf/tree/veristat-memory-accounting [7] https://github.com/eddyz87/bpf/tree/scc-accumulate-backedges ==================== Link: https://patch.msgid.link/[email protected] Signed-off-by: Alexei Starovoitov <[email protected]>
2 parents 3b55a9e + 69afa15 commit 05e2249

File tree

3 files changed

+998
-326
lines changed

3 files changed

+998
-326
lines changed

include/linux/bpf_verifier.h

Lines changed: 52 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -344,7 +344,7 @@ struct bpf_func_state {
344344

345345
#define MAX_CALL_FRAMES 8
346346

347-
/* instruction history flags, used in bpf_insn_hist_entry.flags field */
347+
/* instruction history flags, used in bpf_jmp_history_entry.flags field */
348348
enum {
349349
/* instruction references stack slot through PTR_TO_STACK register;
350350
* we also store stack's frame number in lower 3 bits (MAX_CALL_FRAMES is 8)
@@ -366,7 +366,7 @@ enum {
366366
static_assert(INSN_F_FRAMENO_MASK + 1 >= MAX_CALL_FRAMES);
367367
static_assert(INSN_F_SPI_MASK + 1 >= MAX_BPF_STACK / 8);
368368

369-
struct bpf_insn_hist_entry {
369+
struct bpf_jmp_history_entry {
370370
u32 idx;
371371
/* insn idx can't be bigger than 1 million */
372372
u32 prev_idx : 20;
@@ -449,32 +449,20 @@ struct bpf_verifier_state {
449449
/* first and last insn idx of this verifier state */
450450
u32 first_insn_idx;
451451
u32 last_insn_idx;
452-
/* If this state is a part of states loop this field points to some
453-
* parent of this state such that:
454-
* - it is also a member of the same states loop;
455-
* - DFS states traversal starting from initial state visits loop_entry
456-
* state before this state.
457-
* Used to compute topmost loop entry for state loops.
458-
* State loops might appear because of open coded iterators logic.
459-
* See get_loop_entry() for more information.
452+
/* if this state is a backedge state then equal_state
453+
* records cached state to which this state is equal.
460454
*/
461-
struct bpf_verifier_state *loop_entry;
462-
/* Sub-range of env->insn_hist[] corresponding to this state's
463-
* instruction history.
464-
* Backtracking is using it to go from last to first.
465-
* For most states instruction history is short, 0-3 instructions.
455+
struct bpf_verifier_state *equal_state;
456+
/* jmp history recorded from first to last.
457+
* backtracking is using it to go from last to first.
458+
* For most states jmp_history_cnt is [0-3].
466459
* For loops can go up to ~40.
467460
*/
468-
u32 insn_hist_start;
469-
u32 insn_hist_end;
461+
struct bpf_jmp_history_entry *jmp_history;
462+
u32 jmp_history_cnt;
470463
u32 dfs_depth;
471464
u32 callback_unroll_depth;
472465
u32 may_goto_depth;
473-
/* If this state was ever pointed-to by other state's loop_entry field
474-
* this flag would be set to true. Used to avoid freeing such states
475-
* while they are still in use.
476-
*/
477-
u32 used_as_loop_entry;
478466
};
479467

480468
#define bpf_get_spilled_reg(slot, frame, mask) \
@@ -610,6 +598,11 @@ struct bpf_insn_aux_data {
610598
* accepts callback function as a parameter.
611599
*/
612600
bool calls_callback;
601+
/*
602+
* CFG strongly connected component this instruction belongs to,
603+
* zero if it is a singleton SCC.
604+
*/
605+
u32 scc;
613606
/* registers alive before this instruction. */
614607
u16 live_regs_before;
615608
};
@@ -719,6 +712,38 @@ struct bpf_idset {
719712
u32 ids[BPF_ID_MAP_SIZE];
720713
};
721714

715+
/* see verifier.c:compute_scc_callchain() */
716+
struct bpf_scc_callchain {
717+
/* call sites from bpf_verifier_state->frame[*]->callsite leading to this SCC */
718+
u32 callsites[MAX_CALL_FRAMES - 1];
719+
/* last frame in a chain is identified by SCC id */
720+
u32 scc;
721+
};
722+
723+
/* verifier state waiting for propagate_backedges() */
724+
struct bpf_scc_backedge {
725+
struct bpf_scc_backedge *next;
726+
struct bpf_verifier_state state;
727+
};
728+
729+
struct bpf_scc_visit {
730+
struct bpf_scc_callchain callchain;
731+
/* first state in current verification path that entered SCC
732+
* identified by the callchain
733+
*/
734+
struct bpf_verifier_state *entry_state;
735+
struct bpf_scc_backedge *backedges; /* list of backedges */
736+
u32 num_backedges;
737+
};
738+
739+
/* An array of bpf_scc_visit structs sharing tht same bpf_scc_callchain->scc
740+
* but having different bpf_scc_callchain->callsites.
741+
*/
742+
struct bpf_scc_info {
743+
u32 num_visits;
744+
struct bpf_scc_visit visits[];
745+
};
746+
722747
/* single container for all structs
723748
* one verifier_env per bpf_check() call
724749
*/
@@ -776,9 +801,7 @@ struct bpf_verifier_env {
776801
int cur_postorder;
777802
} cfg;
778803
struct backtrack_state bt;
779-
struct bpf_insn_hist_entry *insn_hist;
780-
struct bpf_insn_hist_entry *cur_hist_ent;
781-
u32 insn_hist_cap;
804+
struct bpf_jmp_history_entry *cur_hist_ent;
782805
u32 pass_cnt; /* number of times do_check() was called */
783806
u32 subprog_cnt;
784807
/* number of instructions analyzed by the verifier */
@@ -800,6 +823,7 @@ struct bpf_verifier_env {
800823
u32 longest_mark_read_walk;
801824
u32 free_list_size;
802825
u32 explored_states_size;
826+
u32 num_backedges;
803827
bpfptr_t fd_array;
804828

805829
/* bit mask to keep track of whether a register has been accessed
@@ -817,6 +841,9 @@ struct bpf_verifier_env {
817841
char tmp_str_buf[TMP_STR_BUF_LEN];
818842
struct bpf_insn insn_buf[INSN_BUF_SIZE];
819843
struct bpf_insn epilogue_buf[INSN_BUF_SIZE];
844+
/* array of pointers to bpf_scc_info indexed by SCC id */
845+
struct bpf_scc_info **scc_info;
846+
u32 scc_cnt;
820847
};
821848

822849
static inline struct bpf_func_info_aux *subprog_aux(struct bpf_verifier_env *env, int subprog)

0 commit comments

Comments
 (0)