Re: [PATCH bpf-next v2 3/3] veristat: Report max stack depth

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The MEMORY_PEAK statistic is silently truncated from the default CSV output.
- [Medium] The commit message incorrectly claims that the max stack depth 'is null for rejected programs', but the code outputs '0'.
--

commit 407a4af46d427010feeebda3a91736b7ef774219
Author: Paul Chaignon <paul.chaignon@gmail.com>

veristat: Report max stack depth

This patch adds a new "Max stack depth" field to the set of gathered statistics.
It reports the maximum combined stack depth compared to the 512 bytes limit.

>     This patch adds a new "Max stack depth" field to the set of gathered
>     statistics. This field reports the maximum combined stack depth compared
>     to the 512 bytes limit. It is null for rejected programs.

Is the last sentence accurate? The code appears to output 0 instead of "null"
for rejected programs.

When a program is rejected and the log stops early, MAX_STACK remains
zero-initialized. In prepare_value(), MAX_STACK falls back to outputting 0:

	case MAX_STACK:
	case SIZE:
	case JITED_SIZE:
	case MEMORY_PEAK:
		*val = s ? s->stats[id] : 0;
		break;

Furthermore, outputting "null" would fail in parse_stat_value() during replay
mode, as it strictly expects an integer.

> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index 5c82950e6978e..9a2275d19ee1b 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c

[ ... ]

> @@ -795,7 +796,7 @@ static const struct stat_specs default_csv_output_spec = {
>  		TOTAL_INSNS, TOTAL_STATES, PEAK_STATES,
>  		MAX_STATES_PER_INSN, MARK_READ_MAX_LEN,
>  		SIZE, JITED_SIZE, PROG_TYPE, ATTACH_TYPE,
> -		STACK, MEMORY_PEAK,
> +		STACK, MAX_STACK, MEMORY_PEAK,
>  	},
>  };

Does this change truncate the MEMORY_PEAK statistic from the default CSV output?

Looking at the full definition of default_csv_output_spec:

static const struct stat_specs default_csv_output_spec = {
	.spec_cnt = 15,
	.ids = {
		FILE_NAME, PROG_NAME, VERDICT, DURATION,
		TOTAL_INSNS, TOTAL_STATES, PEAK_STATES,
		MAX_STATES_PER_INSN, MARK_READ_MAX_LEN,
		SIZE, JITED_SIZE, PROG_TYPE, ATTACH_TYPE,
		STACK, MAX_STACK, MEMORY_PEAK,
	},
};

With MAX_STACK added, there are now 16 elements in the ids array, but spec_cnt
remains hardcoded at 15. Because the formatting loops iterate strictly up to
spec_cnt, it seems the 16th element (MEMORY_PEAK) will be silently ignored,
causing a regression in the generated statistics.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778689025.git.paul.chaignon@gmail.com?part=3