Re: [PATCH] perf trace: Refactor augmented_raw_syscalls using bpf_loop

[Viktor Malik <vmalik@redhat.com>]

On 6/24/26 08:47, Viktor Malik wrote:
> On 6/23/26 19:10, Namhyung Kim wrote:
>> Hello,
>>
>> On Tue, Jun 23, 2026 at 08:27:39AM -0700, Alexei Starovoitov wrote:
>>> On Tue Jun 23, 2026 at 4:25 AM PDT, Viktor Malik wrote:
>>>> The loop for processing syscall args in augment_raw_syscalls has a
>>>> history of breaking with Clang updates, see e.g. commit 013eb043f37b
>>>> ("perf trace: Fix BPF loading failure (-E2BIG)") from Clang 15 to 16.
>>>>
>>>> Now, a similar thing happened between Clang 21 and 22. While the issue
>>>> is mitigated on the main line by a recent verifier update, it remains
>>>> broken on the 6.12 and 6.18 stable branches:
>>>>
>>>>     [linux-6.18.y]# sudo perf trace true
>>>>     libbpf: prog 'sys_enter': BPF program load failed: -E2BIG
>>>>     libbpf: prog 'sys_enter': -- BEGIN PROG LOAD LOG --
>>>>     [...]
>>>>     BPF program is too large. Processed 1000001 insn
>>>>     processed 1000001 insns (limit 1000000) max_states_per_insn 40 total_states 37941 peak_states 232 mark_read 0
>>>>     -- END PROG LOAD LOG --
>>>>     libbpf: prog 'sys_enter': failed to load: -E2BIG
>>>>     libbpf: failed to load object 'augmented_raw_syscalls_bpf'
>>>>     libbpf: failed to load BPF skeleton 'augmented_raw_syscalls_bpf': -E2BIG
>>>>     Error: failed to get syscall or beauty map fd
>>>>     [...]
>>>>
>>>> The reason is that the loop is quite complex and the BPF verifier often
>>>> struggles to prove that it terminates.
>>>>
>>>> Fix the issue by refactoring the loop body into a callback function and
>>>> calling the bpf_loop helper. This should prevent future breakages of
>>>> this kind since the callback function has no loops. It also allows to
>>>> drop a few artificial checks to help the verifier, including the changes
>>>> introduced by 013eb043f37b.
>>
>> Thanks for working on this.  I encountered this issue before and never
>> found time to take a deeper look yet.
>>
>>>>
>>>> Signed-off-by: Viktor Malik <vmalik@redhat.com>
>>>> Fixes: a68fd6a6cdd3 ("perf trace: Collect augmented data using BPF")
>>>> Fixes: 013eb043f37b ("perf trace: Fix BPF loading failure (-E2BIG)")
>>>> Cc: stable@vger.kernel.org
>>>> ---
>>>>  .../bpf_skel/augmented_raw_syscalls.bpf.c     | 157 +++++++++++-------
>>>>  1 file changed, 96 insertions(+), 61 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..6d553ed3ac23 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,96 @@ static bool pid_filter__has(struct pids_filtered *pids, pid_t pid)
>>>>  	return bpf_map_lookup_elem(pids, &pid) != NULL;
>>>>  }
>>>>  
>>>> +struct args_loop_ctx {
>>>> +	struct syscall_enter_args *args;
>>>> +	unsigned int *beauty_map;
>>>> +	void *payload_offset;
>>>> +	int value_size;
>>>> +	u64 *output;
>>>> +	bool *do_output;
>>>> +};
>>>> +
>>>> +static long process_arg_cb(u64 i, void *ctx)
>>>> +{
>>>> +	/*
>>>> +	 * 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)
>>>> +	 */
>>>> +	struct augmented_arg *augmented_arg;
>>>> +	struct args_loop_ctx *loop_ctx;
>>>> +	int aug_size, size, index;
>>>> +	bool augmented;
>>>> +	void *arg;
>>>> +
>>>> +	/* Bounds check for the below map access to help the verifier */
>>>> +	if (i < 0 || i >= 6)
>>>> +		return 1;
>>>> +
>>>> +	loop_ctx = (struct args_loop_ctx *)ctx;
>>>> +	arg = (void *)loop_ctx->args->args[i];
>>>> +	augmented = false;
>>>> +	size = loop_ctx->beauty_map[i];
>>>> +	aug_size = size; /* size of the augmented data read from user space */
>>>> +	augmented_arg = (struct augmented_arg *)loop_ctx->payload_offset;
>>>> +
>>>> +	if (size == 0 || arg == NULL)
>>>> +		return 0; /* continue */
>>>> +
>>>> +	if (size == 1) { /* string */
>>>> +		aug_size = bpf_probe_read_user_str(augmented_arg->value, loop_ctx->value_size, arg);
>>>> +		augmented = true;
>>>> +	} else if (size > 0 && size <= loop_ctx->value_size) { /* struct */
>>>> +		if (!bpf_probe_read_user(augmented_arg->value, size, arg))
>>>> +			augmented = true;
>>>> +	} else if (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 = loop_ctx->args->args[index];
>>>> +
>>>> +		if (aug_size > TRACE_AUG_MAX_BUF)
>>>> +			aug_size = TRACE_AUG_MAX_BUF;
>>>> +
>>>> +		if (aug_size > 0) {
>>>> +			if (!bpf_probe_read_user(augmented_arg->value, aug_size, arg))
>>>> +				augmented = true;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	/* Augmented data size is limited to sizeof(augmented_arg->unnamed union with value field) */
>>>> +	if (aug_size > loop_ctx->value_size)
>>>> +		aug_size = loop_ctx->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; /* break */
>>>> +
>>>> +		augmented_arg->size = aug_size;
>>>> +		*loop_ctx->output += written;
>>>> +		loop_ctx->payload_offset += written;
>>>> +		*loop_ctx->do_output = true;
>>>> +	}
>>>> +
>>>> +	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, value_size = sizeof(struct augmented_arg) - offsetof(struct augmented_arg, value);
>>>>  	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;
>>>> +	long iters;
>>>>  
>>>>  	/* fall back to do predefined tail call */
>>>>  	if (args == NULL)
>>>> @@ -457,63 +538,17 @@ 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;
>>>> -			output += written;
>>>> -			payload_offset += written;
>>>> -			do_output = true;
>>>> -		}
>>>> -	}
>>>> +	struct args_loop_ctx loop_ctx = {
>>>> +		.args = args,
>>>> +		.beauty_map = beauty_map,
>>>> +		.payload_offset = payload_offset,
>>>> +		.value_size = value_size,
>>>> +		.output = &output,
>>>> +		.do_output = &do_output
>>>> +	};
>>>> +	iters = bpf_loop(6, process_arg_cb, &loop_ctx, 0);
>>>
>>> bpf_loop() is old and generally not recommended.
>>> Please use bpf_for() then the diff will be one line change and
>>> can scale to any number of args. Not just 6.
> 
> Thanks Alexei, I didn't know about this preference.
> 
>> One thing we should take care is to support old kernels.  The oldest
>> LTS kernel in the kernel.org is 5.10 and bpf_loop() was introduced in
>> 5.17 and bpf_for (bpf_iter_num) was 6.4.
> 
> The problematic loop was introduced in 6.12 by a68fd6a6cdd3 ("perf
> trace: Collect augmented data using BPF") so we should be good using
> bpf_for. Or is perf from 7.2 supposed to work on 5.10 LTS kernels?
> 
> I'll refactor with bpf_for and will send v2. 

Or I won't. It turns out that just swapping the for loop for bpf_for
leads to -E2BIG from the verifier again. Looking at the verifier log, it
fails to find equivalence between states at the loop head:

    [...]
    78: (85) call bpf_iter_num_next#84922 [...]
fp-56=map_value(map=beauty_payload_,ks=4,vs=24688,imm=112)
    [...]
    78: (85) call bpf_iter_num_next#84922 [...]
fp-56=map_value(map=beauty_payload_,ks=4,vs=24688,imm=120)
    [...]

IMHO, the reason is that payload_offset, which points to the
beauty_payload_enter_map entry, gets updated in every iteration.

This could be probably fixed on the perf side by reworking how augmented
args are stored but at this point, bpf_loop sounds like an easier and
more reliable approach.

Let me know if anyone has objections, otherwise I'll send v2 of the
bpf_loop approach, with suggestions from Sashiko incorporated.

Thanks,
Viktor

> It should be then
> backported to stable kernels down to 6.12 LTS.
> 
> Viktor
> 
>>
>> Maybe we can factor out the loop body and call it from different
>> mechanisms like open-coded loop, bpf_loop or bpf_for depending on the
>> kernel version.  But not sure it'd fix the verifier issue though.
>>
>> Thanks,
>> Namhyung
>>