Re: [PATCH bpf-next v7 3/6] bpf: Show more useful info in stack depth stats

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The array `func_info_aux` is not updated when a subprogram is removed during dead code elimination, leading to desynchronized indices and mismatched linkage/properties output. This problem wasn't introduced by this patch, but it exposes a severe preexisting bug where subprogram metadata becomes misaligned.
--

--- Patch [3]: [PATCH bpf-next v7 3/6] bpf: Show more useful info in stack depth stats ---
commit f4f742e1d07651cd7baebbdb7ebb72f0898a6722
Author: Kumar Kartikeya Dwivedi <[email protected]>

bpf: Show more useful info in stack depth stats

This patch updates how stack depth statistics are reported in the verifier.
It keeps the combined maximum stack depth on its own line and introduces a
uniform record for each subprogram, classifying them as main, global, or
static. It also prints self and inclusive instruction counts along with stack
depth.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 9b76851dcf54b..eca30096501ff 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -18832,15 +18832,20 @@ static void print_verification_stats(struct bpf_verifier_env *env)
>  	if (env->log.level & BPF_LOG_STATS) {
>  		verbose(env, "verification time %lld usec\n",
>  			div_u64(env->verification_time, 1000));
> -		verbose(env, "stack depth %d", env->subprog_info[0].stack_depth);
> -		for (i = 1; i < subprog_cnt; i++)
> -			verbose(env, "+%d", env->subprog_info[i].stack_depth);
> -		verbose(env, " max %d\n", env->max_stack_depth);
> -		verbose(env, "insns processed %d", env->subprog_info[0].insns_total);
> -		for (i = 1; i < subprog_cnt; i++)
> -			if (bpf_subprog_is_global(env, i))
> -				verbose(env, "+%d", env->subprog_info[i].insns_total);
> -		verbose(env, "\n");
> +		verbose(env, "stack depth max %d\n", env->max_stack_depth);
> +		for (i = 0; i < subprog_cnt; i++) {
> +			const char *name = env->subprog_info[i].name;
> +			const char *kind;
> +
> +			if (!name || !name[0])
> +				name = "<unknown>";
> +			kind = i == 0 ? "main" :
> +			       bpf_subprog_is_global(env, i) ? "global" : "static";

[Severity: High]
This isn't a bug introduced by this patch, but this use of
bpf_subprog_is_global() exposes a pre-existing issue where subprogram
metadata becomes misaligned.

If a subprogram is removed during dead code elimination,
adjust_subprog_starts_after_remove() shifts func_info but fails to shift
func_info_aux:

kernel/bpf/fixups.c:adjust_subprog_starts_after_remove() {
		if (aux->func_info) {
			move = aux->func_info_cnt - j;

			memmove(aux->func_info + i,
				aux->func_info + j,
				sizeof(*aux->func_info) * move);
			aux->func_info_cnt -= j - i;
			/* func_info->insn_off is set after all code rewrites,
			 * in adjust_btf_func() - no need to adjust
			 */
		}
}

This causes func_info_aux to become permanently desynchronized with the
actual subprogram sequence.

Because bpf_subprog_is_global() relies on aux[subprog].linkage from this
un-shifted array:

kernel/bpf/verifier.c:bpf_subprog_is_global() {
	struct bpf_func_info_aux *aux = env->prog->aux->func_info_aux;

	return aux && aux[subprog].linkage == BTF_FUNC_GLOBAL;
}

Could this lead to mismatched linkage and properties output here?

More importantly, could this also allow bpf_check_attach_target() to
misread the unreliable flag and permit a freplace extension to attach to
heavily optimized static functions?

kernel/bpf/verifier.c:bpf_check_attach_target() {
    ...
		conservative = aux->func_info_aux[subprog].unreliable;
    ...
}

> +			verbose(env, "subprog %d (%s) %s insns_self %d insns_total %d stack %d\n",
> +				i, name, kind, env->subprog_info[i].insns_self,
> +				env->subprog_info[i].insns_total,
> +				env->subprog_info[i].stack_depth);
> +		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3