[PATCH bpf-next 0/3] Refactor verifier prune and jump point handling

[PATCH bpf-next 0/3] Refactor verifier prune and jump point handling

[Andrii Nakryiko]

Disentangle prune and jump points in BPF verifier code. They are conceptually
independent but currently coupled together. This small patch set refactors
related code and make it possible to have some instruction marked as pruning
or jump point independently.

Besides just conceptual cleanliness, this allows to remove unnecessary jump
points (saving a tiny bit of performance and memory usage, potentially), and
even more importantly it allows for clean extension of special pruning points,
similarly to how it's done for BPF_FUNC_timer_set_callback. This will be used
by future patches implementing open-coded BPF iterators.

Andrii Nakryiko (3):
  bpf: decouple prune and jump points
  bpf: mostly decouple jump history management from is_state_visited()
  bpf: remove unnecessary prune and jump points

 include/linux/bpf_verifier.h |  1 +
 kernel/bpf/verifier.c        | 98 +++++++++++++++++++++---------------
 2 files changed, 58 insertions(+), 41 deletions(-)

-- 
2.30.2

[PATCH bpf-next 1/3] bpf: decouple prune and jump points

[Andrii Nakryiko]

BPF verifier marks some instructions as prune points. Currently these
prune points serve two purposes.

It's a point where verifier tries to find previously verified state and
check current state's equivalence to short circuit verification for
current code path.

But also currently it's a point where jump history, used for precision
backtracking, is updated. This is done so that non-linear flow of
execution could be properly backtracked.

Such coupling is coincidental and unnecessary. Some prune points are not
part of some non-linear jump path, so don't need update of jump history.
On the other hand, not all instructions which have to be recorded in
jump history necessarily are good prune points.

This patch splits prune and jump points into independent flags.
Currently all prune points are marked as jump points to minimize amount
of changes in this patch, but next patch will perform some optimization
of prune vs jmp point placement.

No functional changes are intended.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/linux/bpf_verifier.h |  1 +
 kernel/bpf/verifier.c        | 57 +++++++++++++++++++++++++++---------
 2 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index b5090e89cb3f..9870d1d0df01 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -452,6 +452,7 @@ struct bpf_insn_aux_data {
 	/* below fields are initialized once */
 	unsigned int orig_idx; /* original instruction index */
 	bool prune_point;
+	bool jmp_point;
 };
 
 #define MAX_USED_MAPS 64 /* max number of maps accessed by one eBPF program */
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d0ecc0b18b20..2dec4ca47bb5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2525,6 +2525,16 @@ static int check_reg_arg(struct bpf_verifier_env *env, u32 regno,
 	return 0;
 }
 
+static void mark_jmp_point(struct bpf_verifier_env *env, int idx)
+{
+	env->insn_aux_data[idx].jmp_point = true;
+}
+
+static bool is_jmp_point(struct bpf_verifier_env *env, int insn_idx)
+{
+	return env->insn_aux_data[insn_idx].jmp_point;
+}
+
 /* for any branch, call, exit record the history of jmps in the given state */
 static int push_jmp_history(struct bpf_verifier_env *env,
 			    struct bpf_verifier_state *cur)
@@ -2533,6 +2543,9 @@ static int push_jmp_history(struct bpf_verifier_env *env,
 	struct bpf_idx_pair *p;
 	size_t alloc_size;
 
+	if (!is_jmp_point(env, env->insn_idx))
+		return 0;
+
 	cnt++;
 	alloc_size = kmalloc_size_roundup(size_mul(cnt, sizeof(*p)));
 	p = krealloc(cur->jmp_history, alloc_size, GFP_USER);
@@ -12116,11 +12129,16 @@ static struct bpf_verifier_state_list **explored_state(
 	return &env->explored_states[(idx ^ state->callsite) % state_htab_size(env)];
 }
 
-static void init_explored_state(struct bpf_verifier_env *env, int idx)
+static void mark_prune_point(struct bpf_verifier_env *env, int idx)
 {
 	env->insn_aux_data[idx].prune_point = true;
 }
 
+static bool is_prune_point(struct bpf_verifier_env *env, int insn_idx)
+{
+	return env->insn_aux_data[insn_idx].prune_point;
+}
+
 enum {
 	DONE_EXPLORING = 0,
 	KEEP_EXPLORING = 1,
@@ -12149,9 +12167,11 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env,
 		return -EINVAL;
 	}
 
-	if (e == BRANCH)
+	if (e == BRANCH) {
 		/* mark branch target for state pruning */
-		init_explored_state(env, w);
+		mark_prune_point(env, w);
+		mark_jmp_point(env, w);
+	}
 
 	if (insn_state[w] == 0) {
 		/* tree-edge */
@@ -12189,10 +12209,13 @@ static int visit_func_call_insn(int t, int insn_cnt,
 	if (ret)
 		return ret;
 
-	if (t + 1 < insn_cnt)
-		init_explored_state(env, t + 1);
+	if (t + 1 < insn_cnt) {
+		mark_prune_point(env, t + 1);
+		mark_jmp_point(env, t + 1);
+	}
 	if (visit_callee) {
-		init_explored_state(env, t);
+		mark_prune_point(env, t);
+		mark_jmp_point(env, t);
 		ret = push_insn(t, t + insns[t].imm + 1, BRANCH, env,
 				/* It's ok to allow recursion from CFG point of
 				 * view. __check_func_call() will do the actual
@@ -12226,13 +12249,15 @@ static int visit_insn(int t, int insn_cnt, struct bpf_verifier_env *env)
 		return DONE_EXPLORING;
 
 	case BPF_CALL:
-		if (insns[t].imm == BPF_FUNC_timer_set_callback)
+		if (insns[t].imm == BPF_FUNC_timer_set_callback) {
 			/* Mark this call insn to trigger is_state_visited() check
 			 * before call itself is processed by __check_func_call().
 			 * Otherwise new async state will be pushed for further
 			 * exploration.
 			 */
-			init_explored_state(env, t);
+			mark_prune_point(env, t);
+			mark_jmp_point(env, t);
+		}
 		return visit_func_call_insn(t, insn_cnt, insns, env,
 					    insns[t].src_reg == BPF_PSEUDO_CALL);
 
@@ -12250,18 +12275,22 @@ static int visit_insn(int t, int insn_cnt, struct bpf_verifier_env *env)
 		 * but it's marked, since backtracking needs
 		 * to record jmp history in is_state_visited().
 		 */
-		init_explored_state(env, t + insns[t].off + 1);
+		mark_prune_point(env, t + insns[t].off + 1);
+		mark_jmp_point(env, t + insns[t].off + 1);
 		/* tell verifier to check for equivalent states
 		 * after every call and jump
 		 */
-		if (t + 1 < insn_cnt)
-			init_explored_state(env, t + 1);
+		if (t + 1 < insn_cnt) {
+			mark_prune_point(env, t + 1);
+			mark_jmp_point(env, t + 1);
+		}
 
 		return ret;
 
 	default:
 		/* conditional jump with two edges */
-		init_explored_state(env, t);
+		mark_prune_point(env, t);
+		mark_jmp_point(env, t);
 		ret = push_insn(t, t + 1, FALLTHROUGH, env, true);
 		if (ret)
 			return ret;
@@ -13296,11 +13325,11 @@ static int is_state_visited(struct bpf_verifier_env *env, int insn_idx)
 	bool add_new_state = env->test_state_freq ? true : false;
 
 	cur->last_insn_idx = env->prev_insn_idx;
-	if (!env->insn_aux_data[insn_idx].prune_point)
+	if (!is_prune_point(env, insn_idx))
 		/* this 'insn_idx' instruction wasn't marked, so we will not
 		 * be doing state search here
 		 */
-		return 0;
+		return push_jmp_history(env, cur);
 
 	/* bpf progs typically have pruning point every 4 instructions
 	 * http://vger.kernel.org/bpfconf2019.html#session-1
-- 
2.30.2

[PATCH bpf-next 2/3] bpf: mostly decouple jump history management from is_state_visited()

[Andrii Nakryiko]

Jump history updating and state equivalence checks are conceptually
independent, so move push_jmp_history() out of is_state_visited(). Also
make a decision whether to perform state equivalence checks or not one
layer higher in do_check(), keeping is_state_visited() unconditionally
performing state checks.

push_jmp_history() should be performed after state checks. There is just
one small non-uniformity. When is_state_visited() finds already
validated equivalent state, it propagates precision marks to current
state's parent chain. For this to work correctly, jump history has to be
updated, so is_state_visited() is doing that internally.

But if no equivalent verified state is found, jump history has to be
updated in a newly cloned child state, so is_jmp_point()
+ push_jmp_history() is performed after is_state_visited() exited with
zero result, which means "proceed with validation".

This change has no functional changes. It's not strictly necessary, but
feels right to decouple these two processes.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/verifier.c | 49 +++++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2dec4ca47bb5..75a56ded5aca 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -13324,13 +13324,6 @@ static int is_state_visited(struct bpf_verifier_env *env, int insn_idx)
 	int i, j, err, states_cnt = 0;
 	bool add_new_state = env->test_state_freq ? true : false;
 
-	cur->last_insn_idx = env->prev_insn_idx;
-	if (!is_prune_point(env, insn_idx))
-		/* this 'insn_idx' instruction wasn't marked, so we will not
-		 * be doing state search here
-		 */
-		return push_jmp_history(env, cur);
-
 	/* bpf progs typically have pruning point every 4 instructions
 	 * http://vger.kernel.org/bpfconf2019.html#session-1
 	 * Do not add new state for future pruning if the verifier hasn't seen
@@ -13465,10 +13458,10 @@ static int is_state_visited(struct bpf_verifier_env *env, int insn_idx)
 		env->max_states_per_insn = states_cnt;
 
 	if (!env->bpf_capable && states_cnt > BPF_COMPLEXITY_LIMIT_STATES)
-		return push_jmp_history(env, cur);
+		return 0;
 
 	if (!add_new_state)
-		return push_jmp_history(env, cur);
+		return 0;
 
 	/* There were no equivalent states, remember the current one.
 	 * Technically the current state is not proven to be safe yet,
@@ -13608,21 +13601,31 @@ static int do_check(struct bpf_verifier_env *env)
 			return -E2BIG;
 		}
 
-		err = is_state_visited(env, env->insn_idx);
-		if (err < 0)
-			return err;
-		if (err == 1) {
-			/* found equivalent state, can prune the search */
-			if (env->log.level & BPF_LOG_LEVEL) {
-				if (do_print_state)
-					verbose(env, "\nfrom %d to %d%s: safe\n",
-						env->prev_insn_idx, env->insn_idx,
-						env->cur_state->speculative ?
-						" (speculative execution)" : "");
-				else
-					verbose(env, "%d: safe\n", env->insn_idx);
+		state->last_insn_idx = env->prev_insn_idx;
+
+		if (is_prune_point(env, env->insn_idx)) {
+			err = is_state_visited(env, env->insn_idx);
+			if (err < 0)
+				return err;
+			if (err == 1) {
+				/* found equivalent state, can prune the search */
+				if (env->log.level & BPF_LOG_LEVEL) {
+					if (do_print_state)
+						verbose(env, "\nfrom %d to %d%s: safe\n",
+							env->prev_insn_idx, env->insn_idx,
+							env->cur_state->speculative ?
+							" (speculative execution)" : "");
+					else
+						verbose(env, "%d: safe\n", env->insn_idx);
+				}
+				goto process_bpf_exit;
 			}
-			goto process_bpf_exit;
+		}
+
+		if (is_jmp_point(env, env->insn_idx)) {
+			err = push_jmp_history(env, state);
+			if (err)
+				return err;
 		}
 
 		if (signal_pending(current))
-- 
2.30.2

[PATCH bpf-next 3/3] bpf: remove unnecessary prune and jump points

[Andrii Nakryiko]

Don't mark some instructions as jump points when there are actually no
jumps and instructions are just processed sequentially. Such case is
handled naturally by precision backtracking logic without the need to
update jump history.

Also remove both jump and prune point marking for instruction right
after unconditional jumps, as program flow can get to the instruction
right after unconditional jump instruction only if there is a jump to
that instruction from somewhere else in the program. In such case we'll
mark such instruction as prune/jump point because it's a destination of
a jump.

This change has no changes in terms of number of instructions or states
processes across Cilium and selftests programs.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/verifier.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 75a56ded5aca..03c2cc116292 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -12209,13 +12209,10 @@ static int visit_func_call_insn(int t, int insn_cnt,
 	if (ret)
 		return ret;
 
-	if (t + 1 < insn_cnt) {
-		mark_prune_point(env, t + 1);
-		mark_jmp_point(env, t + 1);
-	}
+	mark_prune_point(env, t + 1);
+
 	if (visit_callee) {
 		mark_prune_point(env, t);
-		mark_jmp_point(env, t);
 		ret = push_insn(t, t + insns[t].imm + 1, BRANCH, env,
 				/* It's ok to allow recursion from CFG point of
 				 * view. __check_func_call() will do the actual
@@ -12249,15 +12246,13 @@ static int visit_insn(int t, int insn_cnt, struct bpf_verifier_env *env)
 		return DONE_EXPLORING;
 
 	case BPF_CALL:
-		if (insns[t].imm == BPF_FUNC_timer_set_callback) {
+		if (insns[t].imm == BPF_FUNC_timer_set_callback)
 			/* Mark this call insn to trigger is_state_visited() check
 			 * before call itself is processed by __check_func_call().
 			 * Otherwise new async state will be pushed for further
 			 * exploration.
 			 */
 			mark_prune_point(env, t);
-			mark_jmp_point(env, t);
-		}
 		return visit_func_call_insn(t, insn_cnt, insns, env,
 					    insns[t].src_reg == BPF_PSEUDO_CALL);
 
@@ -12271,26 +12266,15 @@ static int visit_insn(int t, int insn_cnt, struct bpf_verifier_env *env)
 		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().
-		 */
 		mark_prune_point(env, t + insns[t].off + 1);
 		mark_jmp_point(env, t + insns[t].off + 1);
-		/* tell verifier to check for equivalent states
-		 * after every call and jump
-		 */
-		if (t + 1 < insn_cnt) {
-			mark_prune_point(env, t + 1);
-			mark_jmp_point(env, t + 1);
-		}
 
 		return ret;
 
 	default:
 		/* conditional jump with two edges */
 		mark_prune_point(env, t);
-		mark_jmp_point(env, t);
+
 		ret = push_insn(t, t + 1, FALLTHROUGH, env, true);
 		if (ret)
 			return ret;
-- 
2.30.2

RE: [PATCH bpf-next 1/3] bpf: decouple prune and jump points

[John Fastabend]

Andrii Nakryiko wrote:
> BPF verifier marks some instructions as prune points. Currently these
> prune points serve two purposes.
> 
> It's a point where verifier tries to find previously verified state and
> check current state's equivalence to short circuit verification for
> current code path.
> 
> But also currently it's a point where jump history, used for precision
> backtracking, is updated. This is done so that non-linear flow of
> execution could be properly backtracked.
> 
> Such coupling is coincidental and unnecessary. Some prune points are not
> part of some non-linear jump path, so don't need update of jump history.
> On the other hand, not all instructions which have to be recorded in
> jump history necessarily are good prune points.
> 
> This patch splits prune and jump points into independent flags.
> Currently all prune points are marked as jump points to minimize amount
> of changes in this patch, but next patch will perform some optimization
> of prune vs jmp point placement.
> 
> No functional changes are intended.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Acked-by: John Fastabend <john.fastabend@gmail.com>

> ---
>  include/linux/bpf_verifier.h |  1 +
>  kernel/bpf/verifier.c        | 57 +++++++++++++++++++++++++++---------
>  2 files changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index b5090e89cb3f..9870d1d0df01 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -452,6 +452,7 @@ struct bpf_insn_aux_data {
>  	/* below fields are initialized once */
>  	unsigned int orig_idx; /* original instruction index */
>  	bool prune_point;
> +	bool jmp_point;

Random thought we might want to make these flags in the future so you
can have,

   type = BPF_PRUNE_POINT | BPF_JMP_POINT | BPF_ITERATOR_POINT

and so on without a bunch of bools.

>  };
>  

RE: [PATCH bpf-next 2/3] bpf: mostly decouple jump history management from is_state_visited()

[John Fastabend]

Andrii Nakryiko wrote:
> Jump history updating and state equivalence checks are conceptually
> independent, so move push_jmp_history() out of is_state_visited(). Also
> make a decision whether to perform state equivalence checks or not one
> layer higher in do_check(), keeping is_state_visited() unconditionally
> performing state checks.
> 
> push_jmp_history() should be performed after state checks. There is just
> one small non-uniformity. When is_state_visited() finds already
> validated equivalent state, it propagates precision marks to current
> state's parent chain. For this to work correctly, jump history has to be
> updated, so is_state_visited() is doing that internally.
> 
> But if no equivalent verified state is found, jump history has to be
> updated in a newly cloned child state, so is_jmp_point()
> + push_jmp_history() is performed after is_state_visited() exited with
> zero result, which means "proceed with validation".
> 
> This change has no functional changes. It's not strictly necessary, but
> feels right to decouple these two processes.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

Acked-by: John Fastabend <john.fastabend@gmail.com>

RE: [PATCH bpf-next 3/3] bpf: remove unnecessary prune and jump points

[John Fastabend]

Andrii Nakryiko wrote:
> Don't mark some instructions as jump points when there are actually no
> jumps and instructions are just processed sequentially. Such case is
> handled naturally by precision backtracking logic without the need to
> update jump history.
> 

Sorry having trouble matching up commit message with code below.

> Also remove both jump and prune point marking for instruction right
> after unconditional jumps, as program flow can get to the instruction
> right after unconditional jump instruction only if there is a jump to
> that instruction from somewhere else in the program. In such case we'll
> mark such instruction as prune/jump point because it's a destination of
> a jump.
> 
> This change has no changes in terms of number of instructions or states
> processes across Cilium and selftests programs.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  kernel/bpf/verifier.c | 24 ++++--------------------
>  1 file changed, 4 insertions(+), 20 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 75a56ded5aca..03c2cc116292 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -12209,13 +12209,10 @@ static int visit_func_call_insn(int t, int insn_cnt,
>  	if (ret)
>  		return ret;
>  
> -	if (t + 1 < insn_cnt) {
> -		mark_prune_point(env, t + 1);
> -		mark_jmp_point(env, t + 1);
> -	}
> +	mark_prune_point(env, t + 1);
> +
>  	if (visit_callee) {
>  		mark_prune_point(env, t);
> -		mark_jmp_point(env, t);
>  		ret = push_insn(t, t + insns[t].imm + 1, BRANCH, env,
>  				/* It's ok to allow recursion from CFG point of
>  				 * view. __check_func_call() will do the actual
> @@ -12249,15 +12246,13 @@ static int visit_insn(int t, int insn_cnt, struct bpf_verifier_env *env)
>  		return DONE_EXPLORING;
>  
>  	case BPF_CALL:
> -		if (insns[t].imm == BPF_FUNC_timer_set_callback) {
> +		if (insns[t].imm == BPF_FUNC_timer_set_callback)
>  			/* Mark this call insn to trigger is_state_visited() check

maybe fix the comment here?

>  			 * before call itself is processed by __check_func_call().
>  			 * Otherwise new async state will be pushed for further
>  			 * exploration.
>  			 */
>  			mark_prune_point(env, t);
> -			mark_jmp_point(env, t);
> -		}
>  		return visit_func_call_insn(t, insn_cnt, insns, env,
>  					    insns[t].src_reg == BPF_PSEUDO_CALL);
>  
> @@ -12271,26 +12266,15 @@ static int visit_insn(int t, int insn_cnt, struct bpf_verifier_env *env)
>  		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().
> -		 */
>  		mark_prune_point(env, t + insns[t].off + 1);
>  		mark_jmp_point(env, t + insns[t].off + 1);
> -		/* tell verifier to check for equivalent states
> -		 * after every call and jump
> -		 */
> -		if (t + 1 < insn_cnt) {
> -			mark_prune_point(env, t + 1);
> -			mark_jmp_point(env, t + 1);

This makes sense to me its unconditional jmp. So no need to
add jmp point.

> -		}
>  
>  		return ret;
>  
>  	default:
>  		/* conditional jump with two edges */
>  		mark_prune_point(env, t);
> -		mark_jmp_point(env, t);

                 ^^^^^^^^^^^^^^^^^^^^^^^

Specifically, try to see why we dropped this jmp_point?

> +
>  		ret = push_insn(t, t + 1, FALLTHROUGH, env, true);
>  		if (ret)
>  			return ret;
> -- 
> 2.30.2
> 

Re: [PATCH bpf-next 1/3] bpf: decouple prune and jump points

[Andrii Nakryiko]

On Tue, Dec 6, 2022 at 1:42 PM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Andrii Nakryiko wrote:
> > BPF verifier marks some instructions as prune points. Currently these
> > prune points serve two purposes.
> >
> > It's a point where verifier tries to find previously verified state and
> > check current state's equivalence to short circuit verification for
> > current code path.
> >
> > But also currently it's a point where jump history, used for precision
> > backtracking, is updated. This is done so that non-linear flow of
> > execution could be properly backtracked.
> >
> > Such coupling is coincidental and unnecessary. Some prune points are not
> > part of some non-linear jump path, so don't need update of jump history.
> > On the other hand, not all instructions which have to be recorded in
> > jump history necessarily are good prune points.
> >
> > This patch splits prune and jump points into independent flags.
> > Currently all prune points are marked as jump points to minimize amount
> > of changes in this patch, but next patch will perform some optimization
> > of prune vs jmp point placement.
> >
> > No functional changes are intended.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>
> Acked-by: John Fastabend <john.fastabend@gmail.com>
>
> > ---
> >  include/linux/bpf_verifier.h |  1 +
> >  kernel/bpf/verifier.c        | 57 +++++++++++++++++++++++++++---------
> >  2 files changed, 44 insertions(+), 14 deletions(-)
> >
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index b5090e89cb3f..9870d1d0df01 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -452,6 +452,7 @@ struct bpf_insn_aux_data {
> >       /* below fields are initialized once */
> >       unsigned int orig_idx; /* original instruction index */
> >       bool prune_point;
> > +     bool jmp_point;
>
> Random thought we might want to make these flags in the future so you
> can have,
>
>    type = BPF_PRUNE_POINT | BPF_JMP_POINT | BPF_ITERATOR_POINT
>
> and so on without a bunch of bools.

Bunch of bools are nice because they are explicit :) As long as we
have up to 8 of them, we should be fine, I think. I don't envision
adding BPF_ITERATOR_POINT, I just need prune_point on a few special
helpers.

>
> >  };
> >

Re: [PATCH bpf-next 3/3] bpf: remove unnecessary prune and jump points

[Andrii Nakryiko]

On Tue, Dec 6, 2022 at 2:19 PM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Andrii Nakryiko wrote:
> > Don't mark some instructions as jump points when there are actually no
> > jumps and instructions are just processed sequentially. Such case is
> > handled naturally by precision backtracking logic without the need to
> > update jump history.
> >
>
> Sorry having trouble matching up commit message with code below.

Sorry, not clear which part? I'm referring to the logic in
get_prev_insn_idx() here, where we go linearly one instruction
backwards, unless last jmp_history entry matches current instruction.
In the latter case we go to the st->jmp_history[cnt - 1].prev_idx
(non-linear case).

>
> > Also remove both jump and prune point marking for instruction right
> > after unconditional jumps, as program flow can get to the instruction
> > right after unconditional jump instruction only if there is a jump to
> > that instruction from somewhere else in the program. In such case we'll
> > mark such instruction as prune/jump point because it's a destination of
> > a jump.
> >
> > This change has no changes in terms of number of instructions or states
> > processes across Cilium and selftests programs.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  kernel/bpf/verifier.c | 24 ++++--------------------
> >  1 file changed, 4 insertions(+), 20 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 75a56ded5aca..03c2cc116292 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -12209,13 +12209,10 @@ static int visit_func_call_insn(int t, int insn_cnt,
> >       if (ret)
> >               return ret;
> >
> > -     if (t + 1 < insn_cnt) {
> > -             mark_prune_point(env, t + 1);
> > -             mark_jmp_point(env, t + 1);
> > -     }
> > +     mark_prune_point(env, t + 1);

I'm now thinking that maybe here we actually need to keep jmp_point,
as we don't record a jump point for the target of EXIT instruction in
subprog. I'll add this back.

> > +
> >       if (visit_callee) {
> >               mark_prune_point(env, t);
> > -             mark_jmp_point(env, t);
> >               ret = push_insn(t, t + insns[t].imm + 1, BRANCH, env,
> >                               /* It's ok to allow recursion from CFG point of
> >                                * view. __check_func_call() will do the actual
> > @@ -12249,15 +12246,13 @@ static int visit_insn(int t, int insn_cnt, struct bpf_verifier_env *env)
> >               return DONE_EXPLORING;
> >
> >       case BPF_CALL:
> > -             if (insns[t].imm == BPF_FUNC_timer_set_callback) {
> > +             if (insns[t].imm == BPF_FUNC_timer_set_callback)
> >                       /* Mark this call insn to trigger is_state_visited() check
>
> maybe fix the comment here?

It's not broken, but would you like me to explicitly state "Mark this
call insn as a prune point to trigger..."?

>
> >                        * before call itself is processed by __check_func_call().
> >                        * Otherwise new async state will be pushed for further
> >                        * exploration.
> >                        */
> >                       mark_prune_point(env, t);
> > -                     mark_jmp_point(env, t);
> > -             }
> >               return visit_func_call_insn(t, insn_cnt, insns, env,
> >                                           insns[t].src_reg == BPF_PSEUDO_CALL);
> >
> > @@ -12271,26 +12266,15 @@ static int visit_insn(int t, int insn_cnt, struct bpf_verifier_env *env)
> >               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().
> > -              */
> >               mark_prune_point(env, t + insns[t].off + 1);
> >               mark_jmp_point(env, t + insns[t].off + 1);
> > -             /* tell verifier to check for equivalent states
> > -              * after every call and jump
> > -              */
> > -             if (t + 1 < insn_cnt) {
> > -                     mark_prune_point(env, t + 1);
> > -                     mark_jmp_point(env, t + 1);
>
> This makes sense to me its unconditional jmp. So no need to
> add jmp point.
>

yep

> > -             }
> >
> >               return ret;
> >
> >       default:
> >               /* conditional jump with two edges */
> >               mark_prune_point(env, t);
> > -             mark_jmp_point(env, t);
>
>                  ^^^^^^^^^^^^^^^^^^^^^^^
>
> Specifically, try to see why we dropped this jmp_point?

Because there is nothing special about current instruction (which is a
conditional jump instruction). If we got here sequentially, no need to
record jmp_history. If we jumped here from somewhere non-linearly, we
should have already marked this instruction as jmp_point in push_insn
(with e == BRANCH).

So jmp_point is completely irrelevant, the goal here is to force state
checkpointing before we process jump instruction.

Also keep in mind that we mark *destination* of non-linear jump as a
jump point. So target of function call, jump, etc, but not call
instruction or jump instruction itself.


>
> > +
> >               ret = push_insn(t, t + 1, FALLTHROUGH, env, true);
> >               if (ret)
> >                       return ret;
> > --
> > 2.30.2
> >
>
>