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

Thank you for your contribution! Sashiko AI review found 2 potential issue(=
s) to consider:
- [Medium] Unconditionally printing the instruction count for all subprogra=
ms outputs `insns 0` for all static (non-global) subprograms, defeating the=
 stated goal of showing "where verifier work is spent".
- [Medium] The new verifier stack depth log format silently breaks the `ver=
istat` tool, causing it to fail to parse both `MAX_STACK` and `STACK` stati=
stics.
--

--- Patch [1]: [PATCH bpf-next v5 1/3] bpf: Show more useful info in stack =
depth stats ---
commit a5f11407b8ca52d1698171e4ae37a0d5f859eb2f
Author: Kumar Kartikeya Dwivedi <[email protected]>

bpf: Show more useful info in stack depth stats
   =20
This commit updates the verifier log to print stack depth statistics
in a new format, listing the main program and each subprogram on
separate lines with their name, instruction count, and stack depth.
It also updates the BPF selftests to match the new output format.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -18777,15 +18777,21 @@ 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 =3D 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].insn_processed=
);
> -		for (i =3D 1; i < subprog_cnt; i++)
> -			if (bpf_subprog_is_global(env, i))
> -				verbose(env, "+%d", env->subprog_info[i].insn_processed);
> -		verbose(env, "\n");
> +		verbose(env, "stack depth max %d\n", env->max_stack_depth);

[Severity: Medium]
Does this new output format break the veristat tool?

In tools/testing/selftests/bpf/veristat.c:parse_verif_log(), the tool uses=
=20
sscanf to parse the legacy format:

    if (2 =3D=3D sscanf(cur, "stack depth %511s max %ld", stack,
                    &s->stats[MAX_STACK]))

When presented with the new format "stack depth max %d", the %511s token
greedily matches the string "max", causing the literal " max " check to fai=
l.
This would cause sscanf to return 1, silently leaving the MAX_STACK and STA=
CK
counters unpopulated (reporting 0) for all analyzed programs.

> +		for (i =3D 0; i < subprog_cnt; i++) {
> +			const char *name =3D env->subprog_info[i].name;
> +
> +			if (!name || !name[0])
> +				name =3D "<unknown>";
> +			if (i =3D=3D 0)
> +				verbose(env, "prog (%s) insns %d stack %d\n", name,
> +					env->subprog_info[i].insn_processed,
> +					env->subprog_info[i].stack_depth);
> +			else
> +				verbose(env, "subprog %d (%s) insns %d stack %d\n", i, name,
> +					env->subprog_info[i].insn_processed,
> +					env->subprog_info[i].stack_depth);

[Severity: Medium]
Since the bpf_subprog_is_global(env, i) check was removed here, does this
print misleading instruction counts for static subprograms?

The verifier only calculates insn_processed for global subprograms and the
main program, as static subprograms are verified inline and their cost is
attributed to their callers. By removing the bpf_subprog_is_global() check,
are we now unconditionally outputting "insns 0" for all static (non-global)
subprograms?

> +		}
>  	}
>  	verbose(env, "processed %d insns (limit %d) max_states_per_insn %d "

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260804081114.3871=
[email protected]?part=3D1