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

On Tue Aug 4, 2026 at 10:22 AM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issu=
e(s) to consider:
> - [Medium] Unconditionally printing the instruction count for all subprog=
rams outputs `insns 0` for all static (non-global) subprograms, defeating t=
he stated goal of showing "where verifier work is spent".
> - [Medium] The new verifier stack depth log format silently breaks the `v=
eristat` tool, causing it to fail to parse both `MAX_STACK` and `STACK` sta=
tistics.
> --
>
> --- Patch [1]: [PATCH bpf-next v5 1/3] bpf: Show more useful info in stac=
k depth stats ---
> commit a5f11407b8ca52d1698171e4ae37a0d5f859eb2f
> Author: Kumar Kartikeya Dwivedi <[email protected]>
>
> bpf: Show more useful info in stack depth stats
>
> 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 bp=
f_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_processe=
d);
>> -		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 use=
s
> 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 f=
ail.
> This would cause sscanf to return 1, silently leaving the MAX_STACK and S=
TACK
> 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 th=
e
> main program, as static subprograms are verified inline and their cost is
> attributed to their callers. By removing the bpf_subprog_is_global() chec=
k,
> are we now unconditionally outputting "insns 0" for all static (non-globa=
l)
> subprograms?
>

This is legit, but it is based on how we account insns processed. It is don=
e for
main subprogs and global subprogs. It might not even make sense to do per s=
tatic
subprog accounting, given it can be called in various ways that affect the =
insns
processed of the CFG rooted at it throughout the program.

So we can leave it as insns 0, or do something different, but I don't have
better ideas on what that would be.

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