Re: [PATCH v2 1/2] perf trace: Factor out BPF loop body

[Viktor Malik <vmalik@redhat.com>]

On 7/4/26 01:49, Namhyung Kim wrote:
> Hello,
> 
> On Fri, Jul 03, 2026 at 12:32:14PM +0200, Viktor Malik wrote:
>> The BPF program in augmented_raw_syscalls uses a for loop to iterate all
>> syscall arguments. The loop body is quite complex and often poses
>> problems for the BPF verifier. As a preparation step for addressing this
>> issue, factor out the loop body into a separate function.
>>
>> Signed-off-by: Viktor Malik <vmalik@redhat.com>
>> Cc: stable@vger.kernel.org
>> ---
>>  .../bpf_skel/augmented_raw_syscalls.bpf.c     | 127 ++++++++++--------
>>  1 file changed, 72 insertions(+), 55 deletions(-)
>>
>> 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 2a6e61864ee0..cbdd5ce19a2f 100644
>> --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
>> +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
>> @@ -429,15 +429,79 @@ static bool pid_filter__has(struct pids_filtered *pids, pid_t pid)
>>  	return bpf_map_lookup_elem(pids, &pid) != NULL;
>>  }
>>  
>> +/*
>> + * Determine what type of argument and how many bytes to read from user space, using the
>> + * value in the beauty_map. This is the relation of parameter type and its corresponding
>> + * value in the beauty map, and how many bytes we read eventually:
>> + *
>> + * string: 1			      -> size of string
>> + * struct: size of struct	      -> size of struct
>> + * buffer: -1 * (index of paired len) -> value of paired len (maximum: TRACE_AUG_MAX_BUF)
>> + */
>> +static inline int augment_arg(struct syscall_enter_args *args, int i,
>> +			      unsigned int *beauty_map, void *payload_offset)
> 
> Can we make it 'struct augmented_arg *payload_offset' instead?

Sure, good idea, that will allow to drop a few casts from the function
body.

Viktor

> 
> Thanks,
> Namhyung
> 
> 
>> +{
>> +	int index, value_size = sizeof(struct augmented_arg) - offsetof(struct augmented_arg, value);
>> +	s64 aug_size, size;
>> +	bool augmented;
>> +	void *arg;
>> +
>> +	arg = (void *)args->args[i];
>> +	augmented = false;
>> +	size = beauty_map[i];
>> +	aug_size = size; /* size of the augmented data read from user space */
>> +
>> +	if (size == 0 || arg == NULL)
>> +		return 0;
>> +
>> +	if (size == 1) { /* string */
>> +		aug_size = bpf_probe_read_user_str(((struct augmented_arg *)payload_offset)->value, value_size, arg);
>> +		/* minimum of 0 to pass the verifier */
>> +		if (aug_size < 0)
>> +			aug_size = 0;
>> +
>> +		augmented = true;
>> +	} 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 */
>> +		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];
>> +
>> +		if (aug_size > 0) {
>> +			if (!bpf_probe_read_user(((struct augmented_arg *)payload_offset)->value, aug_size, arg))
>> +				augmented = true;
>> +		}
>> +	}
>> +
>> +	/* Augmented data size is limited to sizeof(augmented_arg->unnamed union with value field) */
>> +	if (aug_size > value_size)
>> +		aug_size = value_size;
>> +
>> +	/* write data to payload */
>> +	if (augmented) {
>> +		int written = offsetof(struct augmented_arg, value) + aug_size;
>> +
>> +		if (written < 0 || written > sizeof(struct augmented_arg))
>> +			return -1;
>> +
>> +		((struct augmented_arg *)payload_offset)->size = aug_size;
>> +		return written;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int augment_sys_enter(void *ctx, struct syscall_enter_args *args)
>>  {
>> -	bool augmented, do_output = false;
>> -	int zero = 0, index, value_size = sizeof(struct augmented_arg) - offsetof(struct augmented_arg, value);
>> +	bool do_output = false;
>> +	int zero = 0, written;
>>  	u64 output = 0; /* has to be u64, otherwise it won't pass the verifier */
>> -	s64 aug_size, size;
>>  	unsigned int nr, *beauty_map;
>>  	struct beauty_payload_enter *payload;
>> -	void *arg, *payload_offset;
>> +	void *payload_offset;
>>  
>>  	/* fall back to do predefined tail call */
>>  	if (args == NULL)
>> @@ -457,58 +521,11 @@ 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));
>>  
>> -	/*
>> -	 * Determine what type of argument and how many bytes to read from user space, using the
>> -	 * value in the beauty_map. This is the relation of parameter type and its corresponding
>> -	 * value in the beauty map, and how many bytes we read eventually:
>> -	 *
>> -	 * string: 1			      -> size of string
>> -	 * struct: size of struct	      -> size of struct
>> -	 * buffer: -1 * (index of paired len) -> value of paired len (maximum: TRACE_AUG_MAX_BUF)
>> -	 */
>>  	for (int i = 0; i < 6; i++) {
>> -		arg = (void *)args->args[i];
>> -		augmented = false;
>> -		size = beauty_map[i];
>> -		aug_size = size; /* size of the augmented data read from user space */
>> -
>> -		if (size == 0 || arg == NULL)
>> -			continue;
>> -
>> -		if (size == 1) { /* string */
>> -			aug_size = bpf_probe_read_user_str(((struct augmented_arg *)payload_offset)->value, value_size, arg);
>> -			/* minimum of 0 to pass the verifier */
>> -			if (aug_size < 0)
>> -				aug_size = 0;
>> -
>> -			augmented = true;
>> -		} 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 */
>> -			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];
>> -
>> -			if (aug_size > 0) {
>> -				if (!bpf_probe_read_user(((struct augmented_arg *)payload_offset)->value, aug_size, arg))
>> -					augmented = true;
>> -			}
>> -		}
>> -
>> -		/* Augmented data size is limited to sizeof(augmented_arg->unnamed union with value field) */
>> -		if (aug_size > value_size)
>> -			aug_size = value_size;
>> -
>> -		/* write data to payload */
>> -		if (augmented) {
>> -			int written = offsetof(struct augmented_arg, value) + aug_size;
>> -
>> -			if (written < 0 || written > sizeof(struct augmented_arg))
>> -				return 1;
>> -
>> -			((struct augmented_arg *)payload_offset)->size = aug_size;
>> +		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;
>> -- 
>> 2.54.0
>>
>