Re: [PATCH v3 2/2] perf trace: Refactor augmented_raw_syscalls using bpf_for

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The fallback standard `for` loop will fail to load with `-E2BIG` on older kernels when compiled with Clang 15-18, breaking backward compatibility.
--

commit be9035b8b528902be67522e81fa232a2f597b164
Author: Viktor Malik <vmalik@redhat.com>

perf trace: Refactor augmented_raw_syscalls using bpf_for

The commit replaces the standard `for` loop in `augment_sys_enter()` with
`bpf_for` to fix BPF verifier `-E2BIG` errors on newer Clang versions. It
drops artificial verifier workarounds in `augment_arg()` that were previously
added to fix these errors. A fallback standard `for` loop is kept for backward
compatibility with older kernels lacking `bpf_for`.

> diff --git a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> index bc036a348079d..215516df5d2a3 100644
> --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> @@ -440,10 +442,11 @@ static inline int augment_arg(struct syscall_enter_args *args, int i,
>   */
>  static inline int augment_arg(struct syscall_enter_args *args, int i,
>  			      unsigned int *beauty_map,
> -			      struct augmented_arg *payload_offset)
> +			      struct beauty_payload_enter *payload, u64 offset)
>  {
> -	int index, value_size = sizeof(struct augmented_arg) - offsetof(struct augmented_arg, value);
> -	s64 aug_size, size;
> +	int index, size, value_size = sizeof(struct augmented_arg) - offsetof(struct augmented_arg, value);
> +	struct augmented_arg *payload_offset;
> +	s64 aug_size;

[ ... ]

> -	} else if ((int)size < 0 && size >= -6) { /* buffer */
> +	} else if (size < 0 && size >= -6) { /* buffer */

[ ... ]

> @@ -522,14 +529,30 @@ static int augment_sys_enter(void *ctx, struct syscall_enter_args *args)
> 	/* copy the sys_enter header, which has the syscall_nr */
> 	__builtin_memcpy(&payload->args, args, sizeof(struct syscall_enter_args));
>  
> -	for (int i = 0; i < 6; i++) {
> -		written = augment_arg(args, i, beauty_map, (struct augmented_arg *)payload_offset);
> -		if (written < 0)
> -			return 1;
> -		if (written > 0) {
> -			output += written;
> -			payload_offset += written;
> -			do_output = true;
> +	if (bpf_ksym_exists(bpf_iter_num_new)) {
> +		bpf_for(i, 0, 6) {

[ ... ]

> +	} else {
> +		for (i = 0; i < 6; i++) {
> +			written = augment_arg(args, i, beauty_map, payload, output);

[Severity: Medium]
Will this fallback standard `for` loop fail to load with `-E2BIG` on older
kernels when compiled with Clang 15-18?

Since the `(int)size < 0` cast and `s64 size` verifier workarounds were removed
from `augment_arg()`, it appears the unrolled loop in this fallback path will
once again fail the BPF verifier checks when evaluated on kernels older than
6.4 (where `bpf_ksym_exists(bpf_iter_num_new)` is false). Could this break
backward compatibility for `perf trace` on those older kernels?

> +			if (written < 0)
> +				return 1;
> +			if (written > 0) {
> +				output += written;
> +				do_output = true;
> +			}
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783339165.git.vmalik@redhat.com?part=2