Re: [PATCH bpf-next 2/2] bpf: Keep one basic block in the level 1 log and print the way to it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] bpf: preserve flow->from across subprogram calls in
  bpf_flow_call()
- [Low] bpf: clear env->prev_log_pos when resetting level 1 log in
  do_check()
--

commit 436e80edd288c8e9b5661dd9e9cfd3aca7f99381
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Keep one basic block in the level 1 log and print the way to it

This commit rewinds the level 1 verifier log before each basic block and
reconstructs and prints the path of basic blocks leading to a rejected block
on verification failure.

> diff --git a/kernel/bpf/flow.c b/kernel/bpf/flow.c
> index e6d1817ffa57d..c9458b3ad114a 100644
> --- a/kernel/bpf/flow.c
> +++ b/kernel/bpf/flow.c
[ ... ]
> @@ -2883,6 +2886,7 @@ static struct flow_state *flow_new_state(struct bpf_verifier_env *env, int prev_
>  		return NULL;
>  	s->prev_insn_idx = prev_insn_idx;
>  	s->seq = ++flow->seq;
> +	s->from = flow->from;
>  	env->total_states++;
>  	flow->live_states++;
>  	env->peak_states = max(env->peak_states, flow->live_states);
> @@ -3308,8 +3312,11 @@ int bpf_flow_next(struct bpf_verifier_env *env, int *prev_insn_idx)
>  
>  	env->insn_idx = s->st.insn_idx;
>  	*prev_insn_idx = s->prev_insn_idx;
> -	if (test_bit(s->st.insn_idx, flow->keep))
> +	if (test_bit(s->st.insn_idx, flow->keep)) {
> +		flow->from = s->seq;
>  		return bpf_copy_verifier_state(env->cur_state, &s->st);
> +	}
> +	flow->from = s->from;
>  	*env->cur_state = s->st;
>  	memset(&s->st, 0, sizeof(s->st));
>  	s->st.insn_idx = env->insn_idx;
> @@ -3317,3 +3324,89 @@ int bpf_flow_next(struct bpf_verifier_env *env, int *prev_insn_idx)
>  	flow_free_state(env, s);
>  	return 0;
>  }
> +
> +/* A basic block on the way to the block that is rejected, up to the insn that leads on */
> +struct flow_step {
> +	const struct flow_state *s;	/* the state at its head, if one is kept */
> +	int head, last;
> +};
> +
> +/*
> + * The state at @head with that @seq. If it is gone a state that came later took its place:
> + * the last one then.
> + */
> +static const struct flow_state *flow_find_state(const struct bpf_flow *flow, int head, u32 seq)
> +{
> +	const struct list_head *list = &flow->states[head].list;
> +	const struct flow_state *s;
> +
> +	if (!test_bit(head, flow->keep))
> +		return NULL;
> +	list_for_each_entry(s, list, node)
> +		if (s->seq == seq)
> +			return s;
> +	return list_empty(list) ? NULL : list_last_entry(list, struct flow_state, node);
> +}

[Severity: Medium]
Does flow_find_state() look up the caller's basic block with a callee sequence
number after a call to a static subprogram that is verified on its own?

When bpf_flow_call() parks the waiting caller state w, it does not save
w->from = flow->from.  Verifying the callee then overwrites flow->from with
the callee's kept state sequence numbers, and flow_apply_exit() creates the
resumed caller state at call_idx + 1 (or call_idx) via flow_new_state() using
that overwritten flow->from:

kernel/bpf/flow.c:flow_apply_exit() {
	...
	if (!err)
		err = bpf_flow_deliver(env, &st, call_idx + 1, call_idx);
	bpf_free_verifier_state(&st, false);
	if (err != 1)
		return err;
	...
	s = flow_new_state(env, w->st.last_insn_idx);
	...
}

If verification later fails in the caller, bpf_flow_print_way() steps back to
call_idx carrying the callee's seq in from.  When multiple states were kept at
the caller's basic block, s->seq == seq never matches in flow_find_state(),
so it falls back to list_last_entry() and prints the wrong state and incoming
edge for the caller.  Saving flow->from in w->from in bpf_flow_call() and
restoring flow->from = w->from in flow_apply_exit() would preserve the
caller's sequence number across the call.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index fd333b1063c3d..20b1b9ff574c7 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -22049,7 +22057,15 @@ static int do_check(struct bpf_verifier_env *env)
>  			break;
>  		/* What is known about how the state came to be is not kept with it. */
>  		bpf_diag_event_log_restore(env, 0);
> +		/* Neither is the text: the level 1 log has one basic block at a time. */
> +		if (level1) {
> +			bpf_vlog_reset(&env->log, log_pos);
> +			env->prev_linfo = NULL;
> +		}

[Severity: Low]
Should this also reset env->prev_log_pos = 0 when rewinding env->log?

When a previous function verification pass leaves a small env->prev_log_pos
that happens to match log_pos after do_check_common() prints the "Validating
<func>() func#<n>..." header line for a global subprogram, print_insn_state()
on the first instruction of the basic block sees
env->prev_log_pos == env->log.end_pos:

kernel/bpf/log.c:print_insn_state() {
	if (env->prev_log_pos && env->prev_log_pos == env->log.end_pos) {
		/* remove new line character */
		bpf_vlog_reset(&env->log, env->prev_log_pos - 1);
		verbose(env, "%*c;", bpf_vlog_alignment(env->prev_insn_print_pos), ' ');
	} else {
		verbose(env, "%d:", env->insn_idx);
	}
	...
}

That strips the trailing newline from the "Validating ..." header line and
appends the initial state inline with a semicolon instead of printing
"<insn_idx>:".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108053937.4134138-1-alexei.starovoitov@gmail.com?part=2