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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The new stack depth verifier log format breaks stack metrics parsing in the `veristat` tool.
--

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

bpf: Show more useful info in stack depth stats

This commit changes the output format of stack depth statistics in the BPF
verifier log. It replaces the old subprogram index-based format with a clearer
name and depth format, avoiding hard-to-read sequences.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8d0635ee48c7e..51ac0a9ae3394 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -18703,10 +18703,18 @@ 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, "stack depth");
> +		for (i = 0; i < subprog_cnt; i++) {
> +			const char *name = env->subprog_info[i].name;
> +
> +			if (name && name[0])
> +				verbose(env, " %s=%d", name,
> +					env->subprog_info[i].stack_depth);
> +			else
> +				verbose(env, " subprog[%d]=%d", i,
> +					env->subprog_info[i].stack_depth);
> +		}
> +		verbose(env, " max=%d\n", env->max_stack_depth);

[Severity: Medium]
Does this change break the veristat tool's parsing of stack depths?

Looking at tools/testing/selftests/bpf/veristat.c in parse_verif_log(), it
appears to rely on the old format using a hardcoded string and splitting by
the plus sign:

tools/testing/selftests/bpf/veristat.c:parse_verif_log() {
    ...
    if (2 == sscanf(cur, "stack depth %511s max %ld", stack, &s->stats[MAX_STACK]))
    ...
    while ((token = strtok_r(cnt++ ? NULL : stack, "+", &state))) {
        if (sscanf(token, "%d", &sub_stack) == 0)
            break;
    ...
}

Since the verifier now outputs max=%ld instead of max %ld, and the individual
depths are formatted as name=depth rather than numbers separated by a plus
sign, will veristat silently fail to parse STACK and MAX_STACK metrics when
processing BPF objects?

[ ... ]

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