Commit 59e2e27d authored by Wedson Almeida Filho's avatar Wedson Almeida Filho Committed by Alexei Starovoitov
Browse files

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: default avatarWedson Almeida Filho <wedsonaf@google.com>
Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
Link: https://lore.kernel.org/bpf/20201121015509.3594191-1-wedsonaf@google.com
parent 607c543f

kernel/bpf/verifier.c

+95 −84
Original line number Diff line number Diff line
@@ -8047,6 +8047,11 @@ static void init_explored_state(struct bpf_verifier_env *env, int idx) 
	env->insn_aux_data[idx].prune_point = true;

}



enum {

	DONE_EXPLORING = 0,

	KEEP_EXPLORING = 1,

};



/* t, w, e - match pseudo-code above:

 * t - index of current instruction

 * w - next instruction
@@ -8059,10 +8064,10 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, 
	int *insn_state = env->cfg.insn_state;



	if (e == FALLTHROUGH && insn_state[t] >= (DISCOVERED | FALLTHROUGH))

		return 0;

		return DONE_EXPLORING;



	if (e == BRANCH && insn_state[t] >= (DISCOVERED | BRANCH))

		return 0;

		return DONE_EXPLORING;



	if (w < 0 || w >= env->prog->len) {

		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, 
		if (env->cfg.cur_stack >= env->prog->len)

			return -E2BIG;

		insn_stack[env->cfg.cur_stack++] = w;

		return 1;

		return KEEP_EXPLORING;

	} else if ((insn_state[w] & 0xF0) == DISCOVERED) {

		if (loop_ok && env->bpf_capable)

			return 0;

			return DONE_EXPLORING;

		verbose_linfo(env, t, "%d: ", t);

		verbose_linfo(env, w, "%d: ", w);

		verbose(env, "back-edge from insn %d to %d\n", t, w);
@@ -8096,74 +8101,52 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, 
		verbose(env, "insn state internal bug\n");

		return -EFAULT;

	}

	return 0;

	return DONE_EXPLORING;

}



/* non-recursive depth-first-search to detect loops in BPF program

 * loop == back-edge in directed graph

/* Visits the instruction at index t and returns one of the following:

 *  < 0 - an error occurred

 *  DONE_EXPLORING - the instruction was fully explored

 *  KEEP_EXPLORING - there is still work to be done before it is fully explored

 */

static int check_cfg(struct bpf_verifier_env *env)

static int visit_insn(int t, int insn_cnt, struct bpf_verifier_env *env)

{

	struct bpf_insn *insns = env->prog->insnsi;

	int insn_cnt = env->prog->len;

	int *insn_stack, *insn_state;

	int ret = 0;

	int i, t;



	insn_state = env->cfg.insn_state = kvcalloc(insn_cnt, sizeof(int), GFP_KERNEL);

	if (!insn_state)

		return -ENOMEM;



	insn_stack = env->cfg.insn_stack = kvcalloc(insn_cnt, sizeof(int), GFP_KERNEL);

	if (!insn_stack) {

		kvfree(insn_state);

		return -ENOMEM;

	}



	insn_state[0] = DISCOVERED; /* mark 1st insn as discovered */

	insn_stack[0] = 0; /* 0 is the first instruction */

	env->cfg.cur_stack = 1;

	int ret;



peek_stack:

	if (env->cfg.cur_stack == 0)

		goto check_state;

	t = insn_stack[env->cfg.cur_stack - 1];

	/* All non-branch instructions have a single fall-through edge. */

	if (BPF_CLASS(insns[t].code) != BPF_JMP &&

	    BPF_CLASS(insns[t].code) != BPF_JMP32)

		return push_insn(t, t + 1, FALLTHROUGH, env, false);



	if (BPF_CLASS(insns[t].code) == BPF_JMP ||

	    BPF_CLASS(insns[t].code) == BPF_JMP32) {

		u8 opcode = BPF_OP(insns[t].code);

	switch (BPF_OP(insns[t].code)) {

	case BPF_EXIT:

		return DONE_EXPLORING;



		if (opcode == BPF_EXIT) {

			goto mark_explored;

		} else if (opcode == BPF_CALL) {

	case BPF_CALL:

		ret = push_insn(t, t + 1, FALLTHROUGH, env, false);

			if (ret == 1)

				goto peek_stack;

			else if (ret < 0)

				goto err_free;

		if (ret)

			return ret;



		if (t + 1 < insn_cnt)

			init_explored_state(env, t + 1);

		if (insns[t].src_reg == BPF_PSEUDO_CALL) {

			init_explored_state(env, t);

			ret = push_insn(t, t + insns[t].imm + 1, BRANCH,

					env, false);

				if (ret == 1)

					goto peek_stack;

				else if (ret < 0)

					goto err_free;

			}

		} else if (opcode == BPF_JA) {

			if (BPF_SRC(insns[t].code) != BPF_K) {

				ret = -EINVAL;

				goto err_free;

		}

		return ret;



	case BPF_JA:

		if (BPF_SRC(insns[t].code) != BPF_K)

			return -EINVAL;



		/* unconditional jump with single edge */

			ret = push_insn(t, t + insns[t].off + 1,

					FALLTHROUGH, env, true);

			if (ret == 1)

				goto peek_stack;

			else if (ret < 0)

				goto err_free;

		ret = push_insn(t, t + insns[t].off + 1, FALLTHROUGH, env,

				true);

		if (ret)

			return ret;



		/* unconditional jmp is not a good pruning point,

		 * but it's marked, since backtracking needs

		 * to record jmp history in is_state_visited().
@@ -8174,42 +8157,70 @@ static int check_cfg(struct bpf_verifier_env *env) 
		 */

		if (t + 1 < insn_cnt)

			init_explored_state(env, t + 1);

		} else {



		return ret;



	default:

		/* conditional jump with two edges */

		init_explored_state(env, t);

		ret = push_insn(t, t + 1, FALLTHROUGH, env, true);

			if (ret == 1)

				goto peek_stack;

			else if (ret < 0)

				goto err_free;

		if (ret)

			return ret;



			ret = push_insn(t, t + insns[t].off + 1, BRANCH, env, true);

			if (ret == 1)

				goto peek_stack;

			else if (ret < 0)

				goto err_free;

		return push_insn(t, t + insns[t].off + 1, BRANCH, env, true);

	}

	} else {

		/* all other non-branch instructions with single

		 * fall-through edge

}



/* non-recursive depth-first-search to detect loops in BPF program

 * loop == back-edge in directed graph

 */

		ret = push_insn(t, t + 1, FALLTHROUGH, env, false);

		if (ret == 1)

			goto peek_stack;

		else if (ret < 0)

			goto err_free;

static int check_cfg(struct bpf_verifier_env *env)

{

	int insn_cnt = env->prog->len;

	int *insn_stack, *insn_state;

	int ret = 0;

	int i;



	insn_state = env->cfg.insn_state = kvcalloc(insn_cnt, sizeof(int), GFP_KERNEL);

	if (!insn_state)

		return -ENOMEM;



	insn_stack = env->cfg.insn_stack = kvcalloc(insn_cnt, sizeof(int), GFP_KERNEL);

	if (!insn_stack) {

		kvfree(insn_state);

		return -ENOMEM;

	}



mark_explored:

	insn_state[0] = DISCOVERED; /* mark 1st insn as discovered */

	insn_stack[0] = 0; /* 0 is the first instruction */

	env->cfg.cur_stack = 1;



	while (env->cfg.cur_stack > 0) {

		int t = insn_stack[env->cfg.cur_stack - 1];



		ret = visit_insn(t, insn_cnt, env);

		switch (ret) {

		case DONE_EXPLORING:

			insn_state[t] = EXPLORED;

	if (env->cfg.cur_stack-- <= 0) {

			env->cfg.cur_stack--;

			break;

		case KEEP_EXPLORING:

			break;

		default:

			if (ret > 0) {

				verbose(env, "visit_insn internal bug\n");

				ret = -EFAULT;

			}

			goto err_free;

		}

	}



	if (env->cfg.cur_stack < 0) {

		verbose(env, "pop stack internal bug\n");

		ret = -EFAULT;

		goto err_free;

	}

	goto peek_stack;



check_state:

	for (i = 0; i < insn_cnt; i++) {

		if (insn_state[i] != EXPLORED) {

			verbose(env, "unreachable insn %d\n", i);