Skip to content

Commit 59e2e27

Browse files
wedsonafAlexei Starovoitov
authored andcommitted
bpf: Refactor check_cfg to use a structured loop.
The current implementation uses a number of gotos to implement a loop and different paths within the loop, which makes the code less readable than it would be with an explicit while-loop. This patch also replaces a chain of if/if-elses keyed on the same expression with a switch statement. No change in behaviour is intended. Signed-off-by: Wedson Almeida Filho <[email protected]> Signed-off-by: Alexei Starovoitov <[email protected]> Link: https://lore.kernel.org/bpf/[email protected]
1 parent 607c543 commit 59e2e27

File tree

1 file changed

+95
-84
lines changed

1 file changed

+95
-84
lines changed

kernel/bpf/verifier.c

Lines changed: 95 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -8047,6 +8047,11 @@ static void init_explored_state(struct bpf_verifier_env *env, int idx)
80478047
env->insn_aux_data[idx].prune_point = true;
80488048
}
80498049

8050+
enum {
8051+
DONE_EXPLORING = 0,
8052+
KEEP_EXPLORING = 1,
8053+
};
8054+
80508055
/* t, w, e - match pseudo-code above:
80518056
* t - index of current instruction
80528057
* w - next instruction
@@ -8059,10 +8064,10 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env,
80598064
int *insn_state = env->cfg.insn_state;
80608065

80618066
if (e == FALLTHROUGH && insn_state[t] >= (DISCOVERED | FALLTHROUGH))
8062-
return 0;
8067+
return DONE_EXPLORING;
80638068

80648069
if (e == BRANCH && insn_state[t] >= (DISCOVERED | BRANCH))
8065-
return 0;
8070+
return DONE_EXPLORING;
80668071

80678072
if (w < 0 || w >= env->prog->len) {
80688073
verbose_linfo(env, t, "%d: ", t);
@@ -8081,10 +8086,10 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env,
80818086
if (env->cfg.cur_stack >= env->prog->len)
80828087
return -E2BIG;
80838088
insn_stack[env->cfg.cur_stack++] = w;
8084-
return 1;
8089+
return KEEP_EXPLORING;
80858090
} else if ((insn_state[w] & 0xF0) == DISCOVERED) {
80868091
if (loop_ok && env->bpf_capable)
8087-
return 0;
8092+
return DONE_EXPLORING;
80888093
verbose_linfo(env, t, "%d: ", t);
80898094
verbose_linfo(env, w, "%d: ", w);
80908095
verbose(env, "back-edge from insn %d to %d\n", t, w);
@@ -8096,19 +8101,85 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env,
80968101
verbose(env, "insn state internal bug\n");
80978102
return -EFAULT;
80988103
}
8099-
return 0;
8104+
return DONE_EXPLORING;
8105+
}
8106+
8107+
/* Visits the instruction at index t and returns one of the following:
8108+
* < 0 - an error occurred
8109+
* DONE_EXPLORING - the instruction was fully explored
8110+
* KEEP_EXPLORING - there is still work to be done before it is fully explored
8111+
*/
8112+
static int visit_insn(int t, int insn_cnt, struct bpf_verifier_env *env)
8113+
{
8114+
struct bpf_insn *insns = env->prog->insnsi;
8115+
int ret;
8116+
8117+
/* All non-branch instructions have a single fall-through edge. */
8118+
if (BPF_CLASS(insns[t].code) != BPF_JMP &&
8119+
BPF_CLASS(insns[t].code) != BPF_JMP32)
8120+
return push_insn(t, t + 1, FALLTHROUGH, env, false);
8121+
8122+
switch (BPF_OP(insns[t].code)) {
8123+
case BPF_EXIT:
8124+
return DONE_EXPLORING;
8125+
8126+
case BPF_CALL:
8127+
ret = push_insn(t, t + 1, FALLTHROUGH, env, false);
8128+
if (ret)
8129+
return ret;
8130+
8131+
if (t + 1 < insn_cnt)
8132+
init_explored_state(env, t + 1);
8133+
if (insns[t].src_reg == BPF_PSEUDO_CALL) {
8134+
init_explored_state(env, t);
8135+
ret = push_insn(t, t + insns[t].imm + 1, BRANCH,
8136+
env, false);
8137+
}
8138+
return ret;
8139+
8140+
case BPF_JA:
8141+
if (BPF_SRC(insns[t].code) != BPF_K)
8142+
return -EINVAL;
8143+
8144+
/* unconditional jump with single edge */
8145+
ret = push_insn(t, t + insns[t].off + 1, FALLTHROUGH, env,
8146+
true);
8147+
if (ret)
8148+
return ret;
8149+
8150+
/* unconditional jmp is not a good pruning point,
8151+
* but it's marked, since backtracking needs
8152+
* to record jmp history in is_state_visited().
8153+
*/
8154+
init_explored_state(env, t + insns[t].off + 1);
8155+
/* tell verifier to check for equivalent states
8156+
* after every call and jump
8157+
*/
8158+
if (t + 1 < insn_cnt)
8159+
init_explored_state(env, t + 1);
8160+
8161+
return ret;
8162+
8163+
default:
8164+
/* conditional jump with two edges */
8165+
init_explored_state(env, t);
8166+
ret = push_insn(t, t + 1, FALLTHROUGH, env, true);
8167+
if (ret)
8168+
return ret;
8169+
8170+
return push_insn(t, t + insns[t].off + 1, BRANCH, env, true);
8171+
}
81008172
}
81018173

81028174
/* non-recursive depth-first-search to detect loops in BPF program
81038175
* loop == back-edge in directed graph
81048176
*/
81058177
static int check_cfg(struct bpf_verifier_env *env)
81068178
{
8107-
struct bpf_insn *insns = env->prog->insnsi;
81088179
int insn_cnt = env->prog->len;
81098180
int *insn_stack, *insn_state;
81108181
int ret = 0;
8111-
int i, t;
8182+
int i;
81128183

81138184
insn_state = env->cfg.insn_state = kvcalloc(insn_cnt, sizeof(int), GFP_KERNEL);
81148185
if (!insn_state)
@@ -8124,92 +8195,32 @@ static int check_cfg(struct bpf_verifier_env *env)
81248195
insn_stack[0] = 0; /* 0 is the first instruction */
81258196
env->cfg.cur_stack = 1;
81268197

8127-
peek_stack:
8128-
if (env->cfg.cur_stack == 0)
8129-
goto check_state;
8130-
t = insn_stack[env->cfg.cur_stack - 1];
8131-
8132-
if (BPF_CLASS(insns[t].code) == BPF_JMP ||
8133-
BPF_CLASS(insns[t].code) == BPF_JMP32) {
8134-
u8 opcode = BPF_OP(insns[t].code);
8135-
8136-
if (opcode == BPF_EXIT) {
8137-
goto mark_explored;
8138-
} else if (opcode == BPF_CALL) {
8139-
ret = push_insn(t, t + 1, FALLTHROUGH, env, false);
8140-
if (ret == 1)
8141-
goto peek_stack;
8142-
else if (ret < 0)
8143-
goto err_free;
8144-
if (t + 1 < insn_cnt)
8145-
init_explored_state(env, t + 1);
8146-
if (insns[t].src_reg == BPF_PSEUDO_CALL) {
8147-
init_explored_state(env, t);
8148-
ret = push_insn(t, t + insns[t].imm + 1, BRANCH,
8149-
env, false);
8150-
if (ret == 1)
8151-
goto peek_stack;
8152-
else if (ret < 0)
8153-
goto err_free;
8154-
}
8155-
} else if (opcode == BPF_JA) {
8156-
if (BPF_SRC(insns[t].code) != BPF_K) {
8157-
ret = -EINVAL;
8158-
goto err_free;
8159-
}
8160-
/* unconditional jump with single edge */
8161-
ret = push_insn(t, t + insns[t].off + 1,
8162-
FALLTHROUGH, env, true);
8163-
if (ret == 1)
8164-
goto peek_stack;
8165-
else if (ret < 0)
8166-
goto err_free;
8167-
/* unconditional jmp is not a good pruning point,
8168-
* but it's marked, since backtracking needs
8169-
* to record jmp history in is_state_visited().
8170-
*/
8171-
init_explored_state(env, t + insns[t].off + 1);
8172-
/* tell verifier to check for equivalent states
8173-
* after every call and jump
8174-
*/
8175-
if (t + 1 < insn_cnt)
8176-
init_explored_state(env, t + 1);
8177-
} else {
8178-
/* conditional jump with two edges */
8179-
init_explored_state(env, t);
8180-
ret = push_insn(t, t + 1, FALLTHROUGH, env, true);
8181-
if (ret == 1)
8182-
goto peek_stack;
8183-
else if (ret < 0)
8184-
goto err_free;
8198+
while (env->cfg.cur_stack > 0) {
8199+
int t = insn_stack[env->cfg.cur_stack - 1];
81858200

8186-
ret = push_insn(t, t + insns[t].off + 1, BRANCH, env, true);
8187-
if (ret == 1)
8188-
goto peek_stack;
8189-
else if (ret < 0)
8190-
goto err_free;
8191-
}
8192-
} else {
8193-
/* all other non-branch instructions with single
8194-
* fall-through edge
8195-
*/
8196-
ret = push_insn(t, t + 1, FALLTHROUGH, env, false);
8197-
if (ret == 1)
8198-
goto peek_stack;
8199-
else if (ret < 0)
8201+
ret = visit_insn(t, insn_cnt, env);
8202+
switch (ret) {
8203+
case DONE_EXPLORING:
8204+
insn_state[t] = EXPLORED;
8205+
env->cfg.cur_stack--;
8206+
break;
8207+
case KEEP_EXPLORING:
8208+
break;
8209+
default:
8210+
if (ret > 0) {
8211+
verbose(env, "visit_insn internal bug\n");
8212+
ret = -EFAULT;
8213+
}
82008214
goto err_free;
8215+
}
82018216
}
82028217

8203-
mark_explored:
8204-
insn_state[t] = EXPLORED;
8205-
if (env->cfg.cur_stack-- <= 0) {
8218+
if (env->cfg.cur_stack < 0) {
82068219
verbose(env, "pop stack internal bug\n");
82078220
ret = -EFAULT;
82088221
goto err_free;
82098222
}
8210-
goto peek_stack;
82118223

8212-
check_state:
82138224
for (i = 0; i < insn_cnt; i++) {
82148225
if (insn_state[i] != EXPLORED) {
82158226
verbose(env, "unreachable insn %d\n", i);

0 commit comments

Comments
 (0)