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

[Viktor Malik]

On 7/1/26 20:06, Andrii Nakryiko wrote:
> On Thu, Jun 25, 2026 at 11:04 PM Viktor Malik <vmalik@xxxxxxxxxx> wrote:
>>
>> On 6/25/26 19:55, Andrii Nakryiko wrote:
>>> On Thu, Jun 25, 2026 at 4:58 AM Viktor Malik <vmalik@xxxxxxxxxx> wrote:
>>>>
>>>> On 6/24/26 19:19, Andrii Nakryiko wrote:
>>>>> On Wed, Jun 24, 2026 at 3:27 AM Viktor Malik <vmalik@xxxxxxxxxx> wrote:
>>>>>>
>>>>>> 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@xxxxxxxxxx>
>>>>>>>>>> Fixes: a68fd6a6cdd3 ("perf trace: Collect augmented data using BPF")
>>>>>>>>>> Fixes: 013eb043f37b ("perf trace: Fix BPF loading failure (-E2BIG)")
>>>>>>>>>> Cc: stable@xxxxxxxxxxxxxxx
>>>>>>>>>> ---
>>>>>>>>>>  .../bpf_skel/augmented_raw_syscalls.bpf.c     | 157 +++++++++++-------
>>>>>>>>>>  1 file changed, 96 insertions(+), 61 deletions(-)
> 
> [...]
> 
>>>>>>>>>> +  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.
>>>>>>
>>>>>
>>>>> I'd still try to adapt bpf_for(), it's a much better code structure.
>>>>> You probably need to add a bounding checking/confirming `if ()`
>>>>> condition validating that offset at which you access map_value is
>>>>> always correct. And/or you might need barrier_var() before using i,
>>>>> because bpf_for() macro does bounds checking (check the macro itself),
>>>>> but compiler often will reorder instructions leading to verifier
>>>>> complaints.
>>>>
>>>> I gave it a try but wasn't successful so far. I think that the problem
>>>> is that while it would be possible to add an upper bound condition for
>>>> `payload_offset`, the verifier tracks the value of `payload_offset` too
>>>> precisely (as map_value(..., imm=X) with a concrete offset) and never
>>>> merges states with different offsets. And since there are multiple
>>>> branches inside the loop, each incrementing `payload_offset` by a
>>>> different value, the verifier seems to fork its state on each branch,
>>>> effectively leading to the amount of states growing exponentially and
>>>> hitting the jump limit.
>>>>
>>>> To me, bpf_loop sounds like a more reliable choice in this situation.
>>>
>>> correctly verified bpf_loop would basically have to follow the same
>>> logic, so if it works with bpf_loop, it should work with bpf_for.
>>
>> Are you sure about that? My perception is that the bpf_loop callback is
>> only verified once in a single pass. On the contrary, bpf_for is a
>> normal loop, for which the verifier needs to prove that after some
>> iteration, we get to the state seen in a previous iteration (to prune
>> the state). Which never happens here because the offset to
>> beauty_payload_enter_map (the payload_offset var) is tracked precisely
>> and causes state forks on every condition inside the loop.
> 
> Hey Viktor,
> 
> Sorry for taking so long to get back.

Hey Andrii,

np, thanks for taking a look!

> Answering your question about bpf_loop() vs bpf_for() they are
> conceptually the same from verifier POV, so they are verified
> similarly. Earlier (buggier) versions of verifier did have a loophole
> where we verifier bpf_loop() in more laxed single-shot way, but that's
> not correct. We have since fixed that and it (bpf_loop) now has to
> "prove" convergence just like bpf_for().

Right, this is the piece of the information that I was missing. It now
makes much more sense.

> Anyways, the biggest issue with "normal" unrolled BPF loop is that
> people tend to write it such that there is some carry-over state
> between each iteration (like output variable which tracks advancing
> but bounded offset) which, with fixed number of iterations allows
> verifier to prove everything is bounded.
> 
> This model is really-really bad for bpf_for() because it doesn't allow
> convergence. The trick is to structure each iteration as independent
> piece of calculation where the state outside of bpf_for() loop stays
> as unspecific/imprecise as possible, which at the beginning of the
> loop you revalidate invariants, if necessary (e.g., reestablish
> map_value offset boundaries).
>
> Anyways, it needed a bit of persuasion, but here's the verification
> result and gmail-butchered diff below. The trick is in making output
> imprecise (force verifier to forget its tracked range), so it doesn't
> differ between iterations from verifier POV. That's what the global
> ZERO allows to do. (We've discussed w/ Alexei and Eduard adding
> special instruction to force scalar register into imprecise, it would
> be a cleaner solution here, alas we never got anywhere with this,
> unfortunately).

Many thanks for the patch! Looking at it, I got pretty close during my
attempts, I only missed the ZERO trick, which is obviously crucial. I
was worried I'll have to rewrite the logic much more to get rid of the
concrete carry-over state but this is really neat.

I'm wondering if we could teach the verifier to figure out that it's
tracking a value too precisely in an iterator-based loop and convert it
to a range (sort of a "widening" operation). But I guess that this part
is going to be changed quite a bit with the upcoming verifier change
that Alexei is working on.

I'll take your change and send v2 of the patch (with a fall back to
standard for loop to keep backwards compatibility).

Thanks again!
Viktor

> Processing 'augmented_raw_syscalls.bpf.o'...
> PROCESSING ./util/bpf_skel/.tmp/augmented_raw_syscalls.bpf.o/sys_enter,
> DURATION US: 1129, VERDICT: success, VERIFIER LOG:
> verification time 1129 usec
> stack depth 64
> processed 547 insns (limit 1000000) max_states_per_insn 4 total_states
> 38 peak_states 67 mark_read 0
> 
> File                          Program    Verdict  Duration (us)  Insns
>  States  Program size  Jited size
> ----------------------------  ---------  -------  -------------  -----
>  ------  ------------  ----------
> augmented_raw_syscalls.bpf.o  sys_enter  success           1129    547
>      38           172         917
> ----------------------------  ---------  -------  -------------  -----
>  ------  ------------  ----------
> 
> The diff:
> 
> 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..8436368ba203 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,17 @@ static bool pid_filter__has(struct pids_filtered
> *pids, pid_t pid)
>         return bpf_map_lookup_elem(pids, &pid) != NULL;
>  }
> 
> +u64 ZERO = 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);
> +       int i, zero = 0, index, 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 *arg;
> 
>         /* fall back to do predefined tail call */
>         if (args == NULL)
> @@ -449,7 +451,6 @@ static int augment_sys_enter(void *ctx, struct
> syscall_enter_args *args)
> 
>         /* set up payload for output */
>         payload        = bpf_map_lookup_elem(&beauty_payload_enter_map, &zero);
> -       payload_offset = (void *)&payload->aug_args;
> 
>         if (beauty_map == NULL || payload == NULL)
>                 return 1;
> @@ -466,7 +467,7 @@ static int augment_sys_enter(void *ctx, struct
> syscall_enter_args *args)
>          * 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++) {
> +       bpf_for(i, 0, 6) {
>                 arg = (void *)args->args[i];
>                 augmented = false;
>                 size = beauty_map[i];
> @@ -475,6 +476,11 @@ static int augment_sys_enter(void *ctx, struct
> syscall_enter_args *args)
>                 if (size == 0 || arg == NULL)
>                         continue;
> 
> +               if (output > sizeof(payload->aug_args) -
> sizeof(payload->aug_args[0]))
> +                       break; /* can't/shouldn't happen */
> +               barrier_var(output);
> +               void *payload_offset = (void *)&payload->aug_args + output;
> +
>                 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 */
> @@ -510,7 +516,7 @@ static int augment_sys_enter(void *ctx, struct
> syscall_enter_args *args)
> 
>                         ((struct augmented_arg *)payload_offset)->size
> = aug_size;
>                         output += written;
> -                       payload_offset += written;
> +                       output += ZERO; /* forget range */
>                         do_output = true;
>                 }
>         }
> 
> 
>>
>>> Is
>>> it possible to share your bpf_for-based code in some branch to try
>>> locally? I'm sure it can be done one way or another.
>>
>> The change is super-simple, I can as well share it here. It's just the
>> matter of using bpf_for with two additional suggested mechanisms,
>> barrier_var and a bounds check for payload_offset:
>>
>> 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..341d77a78949 100644
>> --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
>> +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
>> @@ -432,7 +432,7 @@ static bool pid_filter__has(struct pids_filtered *pids, pid_t pid)
>>  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);
>> +       int zero = 0, i, index, 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;
>> @@ -466,12 +466,16 @@ static int augment_sys_enter(void *ctx, struct syscall_enter_args *args)
>>          * 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++) {
>> +       bpf_for(i, 0, 6) {
>> +               barrier_var(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 (payload_offset + sizeof(struct augmented_arg) > (void *)payload + sizeof(struct beauty_payload_enter))
>> +                       break;
>> +
>>                 if (size == 0 || arg == NULL)
>>                         continue;
>>
>>
>> Thanks a lot for the help!
>> Viktor
>>
>