Re: [PATCH bpf-next v3 03/17] bpf: Add verifier diagnostic event log

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2026-07-13 at 17:38 +0200, Kumar Kartikeya Dwivedi wrote:

...

> diff --git a/kernel/bpf/diagnostics.c b/kernel/bpf/diagnostics.c
> index 49106115621d..ed19220aca9a 100644
> --- a/kernel/bpf/diagnostics.c
> +++ b/kernel/bpf/diagnostics.c
> @@ -36,6 +36,22 @@
>  #define BPF_DIAG_SCRATCH_STR_LEN 256
>  #define BPF_DIAG_TEXT_LEN 160
>  
> +enum bpf_diag_history_kind {
> +	BPF_DIAG_HISTORY_BRANCH,
> +};
> +
> +struct bpf_diag_history_event {
> +	u32 insn_idx;
> +	u8 kind;

In the patch #5 you have this as:

	u32 insn_idx:24;
	u8 kind:8;

Let's keep it like that from the beginning.

> +	union {
> +		struct {
> +			bool cond_true;
> +		} branch;
> +	};
> +};

...

> @@ -144,6 +168,51 @@ const char *bpf_diag_scratch_printf(struct bpf_verifier_env *env, unsigned int s
>  	return buf;
>  }
>  
> +static void diag_write(struct bpf_verifier_env *env, const char *fmt, ...)

This supersedes diag_log(), let's not add diag_log in a first place.

> +{
> +	va_list args;
> +
> +	if (!bpf_diag_enabled(env))
> +		return;
> +
> +	va_start(args, fmt);
> +	bpf_verifier_vlog(&env->log, fmt, args);
> +	va_end(args);
> +}

...

> @@ -619,3 +723,51 @@ void bpf_diag_report_source(struct bpf_verifier_env *env, u32 insn_idx, const ch
>  out_free_msg:
>  	kfree(msg);
>  }
> +
> +int bpf_diag_record_branch(struct bpf_verifier_env *env, u32 insn_idx, bool cond_true)
> +{
> +	struct bpf_diag_history_event event = {
> +		.insn_idx = insn_idx,
> +		.kind = BPF_DIAG_HISTORY_BRANCH,
> +		.branch.cond_true = cond_true,
> +	};
> +
> +	return diag_append_history(env, &event);
> +}
> +
> +static void diag_print_history(struct bpf_verifier_env *env)
> +{
> +	const struct bpf_diag_history_event *event;
> +	const struct bpf_diag_log *log;
> +	bool printed = false;
> +	u32 i;
> +
> +	diag_report_section(env, "Causal path");
> +
> +	if (!env->diag) {
> +		diag_write(env, "  no recorded diagnostic events on this path\n");
> +		return;
> +	}
> +	log = &env->diag->log;
> +
> +	for (i = 0; i < log->cnt; i++) {
             ^^^^^
As far as I understand, when the complete patch-set is applied
the start index for branch history events is zero as well.
Do we want to cap the amount of printed events by e.g. 10/50/100
or some reasonable amount?

> +		event = diag_history_event(log, i);
> +
> +		switch (event->kind) {
> +		case BPF_DIAG_HISTORY_BRANCH:
> +			if (printed)
> +				diag_write(env, "\n");
> +			bpf_diag_report_source(env, event->insn_idx, "branch",
> +					       "took the %s branch of this conditional, goto %s",
> +					       event->branch.cond_true ? "true" : "false",
> +					       event->branch.cond_true ? "followed" : "not followed");
> +			printed = true;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	if (!printed)
> +		diag_write(env, "  no retained diagnostic events on this path\n");
> +}

...

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 4f127c8db134..d0d399b879e5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -16135,6 +16135,9 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
>  		}
>  		if (env->log.level & BPF_LOG_LEVEL)
>  			print_insn_state(env, this_branch, this_branch->curframe);
> +		err = bpf_diag_record_branch(env, *insn_idx, true);
> +		if (err)
> +			return err;

I'd try to avoid having spreading these calls across the code-base.
How about checking prev_insn_idx and insn_idx in the do_check()
in the same block where the following happens:

		if (bpf_is_jmp_point(env, env->insn_idx)) {
			err = bpf_push_jmp_history(env, state, 0, 0, 0, 0);
			if (err)
				return err;
		}

?          

>  		*insn_idx += insn->off;
>  		return 0;
>  	} else if (pred == 0) {

...

> @@ -17437,6 +17443,10 @@ static int do_check(struct bpf_verifier_env *env)
>  		/* reset current history entry on each new instruction */
>  		env->cur_hist_ent = NULL;
>  
> +		err = bpf_diag_error(env);
> +		if (err)
> +			return err;
> +

Let's not do this and propagate errors properly instead,
would that lead to a big diff?

>  		env->prev_insn_idx = prev_insn_idx;
>  		if (env->insn_idx >= insn_cnt) {
>  			verbose(env, "invalid insn idx %d insn_cnt %d\n",

...