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

[Namhyung Kim <namhyung@kernel.org>]

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?

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
>