Re: [PATCH bpf-next v3 04/17] bpf: Prune verifier diagnostics when switching paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional truncation of the diagnostic event log in `pop_stack()` destroys the diagnostic trace when the verifier encounters an error.
- [Low] The commit message inaccurately claims to add logic to record the active branch, but this logic was already introduced in a preceding patch.
--

--- Patch [4]: [PATCH bpf-next v3 04/17] bpf: Prune verifier diagnostics when switching paths ---
commit 59e4455992c872528b5e207bc7864d38d2ea193e
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Prune verifier diagnostics when switching paths
    
This commit saves the diagnostic event-log position with each verifier stack
entry and resets it when a queued state is popped or after a successful
subprogram verification. It also queues diagnostic outcomes for branches and
appends them when states are popped.

> Record branch outcomes as true or false. Later reports use those events to
> show the path that reached a verifier error. For queued branches, save the
> queued outcome with the verifier stack entry and append it when that state is
> popped; record the active branch immediately before continuing.

[Severity: Low]
This isn't a bug, but the commit message states it adds logic to "record the
active branch immediately before continuing". However, looking at the code, it
appears this logic was already introduced in a preceding patch and is not
actually part of this commit's diff in check_cond_jmp_op(). Should the commit
message be updated to reflect only the changes made here?

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -1706,8 +1710,8 @@ void bpf_free_backedges(struct bpf_scc_visit *visit)
>  	visit->backedges = NULL;
>  }
>  
> -static int pop_stack(struct bpf_verifier_env *env, int *prev_insn_idx,
> -		     int *insn_idx, bool pop_log)
> +static int pop_stack(struct bpf_verifier_env *env, int *prev_insn_idx, int *insn_idx, bool pop_log,
> +		     bool activate_diag_branch)
>  {
>  	struct bpf_verifier_state *cur = env->cur_state;
>  	struct bpf_verifier_stack_elem *elem, *head = env->head;
> @@ -1723,6 +1727,12 @@ static int pop_stack(struct bpf_verifier_env *env, int *prev_insn_idx,
>  	}
>  	if (pop_log)
>  		bpf_vlog_reset(&env->log, head->log_pos);
> +	bpf_diag_event_log_reset(env, head->diag_log_pos);

[Severity: High]
When a program fails verification, do_check() returns a negative error, causing
do_check_common() to call free_states(). 

free_states() loops over the remaining states, calling pop_stack() with false 
for both pop_log and activate_diag_branch:

kernel/bpf/verifier.c:free_states() {
    ...
    while (!pop_stack(env, NULL, NULL, false, false))
        ;
    ...
}

Because bpf_diag_event_log_reset() is called unconditionally here in pop_stack(),
won't this truncate or completely destroy the diagnostic trace during error
cleanup?

If the log is rolled back to the length it had when each unvisited state was 
queued, does this result in an empty or useless diagnostic output for users 
trying to debug verifier failures? Should this reset also be conditional?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713153910.2556007-1-memxor@gmail.com?part=4