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

["Kumar Kartikeya Dwivedi" <memxor@gmail.com>]

On Tue Aug 4, 2026 at 10:22 AM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Unconditionally printing the instruction count for all subprograms 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 `veristat` tool, causing it to fail to parse both `MAX_STACK` and `STACK` statistics.
> --
>
> --- Patch [1]: [PATCH bpf-next v5 1/3] bpf: Show more useful info in stack depth stats ---
> commit a5f11407b8ca52d1698171e4ae37a0d5f859eb2f
> Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>
> 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 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].insn_processed);
>> -		for (i = 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
> sscanf to parse the legacy format:
>
>     if (2 == 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 fail.
> This would cause sscanf to return 1, silently leaving the MAX_STACK and STACK
> counters unpopulated (reporting 0) for all analyzed programs.
>
>> +		for (i = 0; i < subprog_cnt; i++) {
>> +			const char *name = env->subprog_info[i].name;
>> +
>> +			if (!name || !name[0])
>> +				name = "<unknown>";
>> +			if (i == 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?
>

This is legit, but it is based on how we account insns processed. It is done for
main subprogs and global subprogs. It might not even make sense to do per static
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 "