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

[Viktor Malik <vmalik@redhat.com>]

On 7/3/26 12:45, sashiko-bot@kernel.org wrote:
> 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.

Correct, size should have been made int. Will fix that.

>>  		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.

Yeah, the original version was better, will revert to that.

>>  
>>  		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.

Will fix that, too.

I'll wait for some human reviews and will send v3 with the fixes.

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