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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Removing the `(int)` cast in the buffer augmentation condition makes it unconditionally false, completely breaking buffer payload collection.
- [Medium] Huge buffer sizes (where the MSB is set) fail to be augmented, whereas previously they were safely clamped to `TRACE_AUG_MAX_BUF`.
- [Low] Multi-line comment does not follow the required BPF subsystem style.
--

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

perf trace: Refactor augmented_raw_syscalls using bpf_for

This commit replaces the standard loop in augment_sys_enter with a bpf_for
loop to prevent BPF verifier -E2BIG errors. It also drops some workarounds
previously added to help the verifier.

> 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 cbdd5ce19a2f6..60babc06f3812 100644
> --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c

[ ... ]

> @@ -464,11 +473,13 @@ static inline int augment_arg(struct syscall_enter_args *args, int i,
>  	} else if (size > 0 && size <= value_size) { /* struct */
>  		if (!bpf_probe_read_user(((struct augmented_arg *)payload_offset)->value, size, arg))
>  			augmented = true;
> -	} else if ((int)size < 0 && size >= -6) { /* buffer */
> +	} else if (size < 0 && size >= -6) { /* buffer */

[Severity: High]
Does removing the (int) cast make this condition unconditionally false?

Since size is an s64 assigned from an unsigned int beauty_map[i], a negative
marker (e.g. 0xFFFFFFFF) is zero-extended to a positive s64 value.
Without the (int) cast to truncate and sign-extend it back, size < 0 is
always false, breaking buffer payload collection.

>  		index = -(size + 1);
>  		barrier_var(index); // Prevent clang (noticed with v18) from removing the &= 7 trick.
>  		index &= 7;	    // Satisfy the bounds checking with the verifier in some kernels.
> -		aug_size = args->args[index] > TRACE_AUG_MAX_BUF ? TRACE_AUG_MAX_BUF : args->args[index];
> +		aug_size = args->args[index];
> +		if (aug_size > TRACE_AUG_MAX_BUF)
> +			aug_size = TRACE_AUG_MAX_BUF;

[Severity: Medium]
Will huge buffer sizes (where the most significant bit is set) fail to be
augmented here?

args->args[index] is an unsigned long. If the most significant bit is set,
assigning it to the s64 aug_size makes it negative. The subsequent
if (aug_size > TRACE_AUG_MAX_BUF) is a signed comparison and will evaluate
to false. The length remains negative, causing the later if (aug_size > 0)
check to fail, skipping buffer processing entirely instead of clamping it.

>  
>  		if (aug_size > 0) {
>  			if (!bpf_probe_read_user(((struct augmented_arg *)payload_offset)->value, aug_size, arg))

[ ... ]

> @@ -521,14 +530,29 @@ 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, 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) {
> +			written = augment_arg(args, i, beauty_map, payload, output);
> +			if (written < 0)
> +				return 1;
> +			if (written > 0) {
> +				output += written;
> +				/* guide the verifier to forget range of `output`, which
> +				 * helps to prove convergence of the loop
> +				 */

[Severity: Low]
This isn't a bug, but this multi-line comment does not follow the preferred
BPF subsystem style where the opening /* is on its own line.

> +				output += ZERO;
> +				do_output = true;
> +			}
> +		}

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