perf trace: Refactor augmented_raw_syscalls using bpf_for
Subsystems (3)
v2 cover perf trace: Refactor augmented_raw_syscalls using bpf_for
The loop for processing syscall args in augment_raw_syscalls has a history of breaking with Clang updates. In the past, we've seen it break (i.e. stop passing the BPF verifier) between Clang 15 and 16 and 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, effectively breaking perf trace.
The reason is that the loop is quite complex and the BPF verifier often struggles to prove that it terminates.
This series fixes the issue by replacing the standard for loop by the bpf_for macro, which uses numeric BPF iterator. This should prevent future breakages of this kind since the verifier has much easier job proving that the loop terminates. Small adjustments were necessary for the loop to make it work, see the second commit message for details.
To keep perf compatible with older kernels, the first commit factors out the loop body into a function, which is then called either from bpf_for or from a standard for loop, depending on whether BPF numeric iterators are available.
Changes from v1:
- v1: https://lore.kernel.org/bpf/[email protected]/T/
- Use bpf_for instead of bpf_loop (suggested by Alexei and Andrii)
- Keep the change backwards compatible with older kernels (required by Namhyung)
Viktor Malik (2):
perf trace: Factor out BPF loop body
perf trace: Refactor augmented_raw_syscalls using bpf_for
.../bpf_skel/augmented_raw_syscalls.bpf.c | 155 +++++++++++------- 1 file changed, 98 insertions(+), 57 deletions(-)
Signature · 1 line
2.54.0
PATCH v2 1/2 perf trace: Factor out BPF loop body
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.
--- .../bpf_skel/augmented_raw_syscalls.bpf.c | 127 ++++++++++-------- 1 file changed, 72 insertions(+), 55 deletions(-)
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +72 -55
@@ -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) +{ +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;
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 1/2] perf trace: Factor out BPF loop body
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Zero-extension of map values makes the condition `size < 0` always false, resulting in dead code…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Zero-extension of map values makes the condition size < 0 always false, resulting in dead code that completely breaks tracepoint buffer augmentation. --
commit d586728f909e7c096ce4ad924848b781a274c29f
Author: Viktor Malik <[email protected]>
perf trace: Factor out BPF loop body
This commit factors out the loop body in augmented_raw_syscalls into a separate function. This is intended to simplify the code and help address problems with the BPF verifier.
Quoted 3 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +0
-0
[ ... ]
Quoted 11 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +11
-0+static inline int augment_arg (struct syscall_enter_args *args,int i, +unsigned int *beauty_map,void *payload_offset) +{ +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];
[ ... ]
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c static bool pid_filter__has(struct pids_filtered *pids, pid_t pid) · line 467
Quoted 4 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +4
-0+ }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 */
[Severity: Medium]
Will this condition evaluate correctly given that beauty_map contains unsigned integers?
When the userspace perf trace tool populates beauty_map_enter with negative values (e.g., -1) to indicate buffer arguments, they are stored as 32-bit unsigned integers (e.g., 4294967295).
Since size is an s64, reading from the map via size = beauty_map[i] zero-extends this unsigned value, making it strictly non-negative.
It appears a subsequent patch in this series removes the (int) cast to address an out-of-bounds array access caused by the size >= -6 check (which was a pre-existing issue).
However, removing that cast leaves the condition checking if size < 0, which will always be false because of the zero-extension.
Could this inadvertently turn the entire buffer augmentation branch into dead code, systematically failing to augment every buffer tracepoint argument?
Quoted 4 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +4
-0+ 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];
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Namhyung Kim
maintainerRe: [PATCH v2 1/2] perf trace: Factor out BPF loop body
In reply to Viktor MalikHello,
Quoted 20 lines · Viktor Malik
The BPF program in
augmented_raw_syscallsuses 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 <[email protected]>--- .../bpf_skel/augmented_raw_syscalls.bpf.c | 127 ++++++++++-------- 1 file changed, 72 insertions(+), 55 deletions(-)tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +1
-0@@ -429,15 +429,79 @@ static bool pid_filter__has(struct pids_filtered *pids, pid_t pid)return bpf_map_lookup_elem (pids, &pid) !=NULL ; } +/*
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c static bool pid_filter__has(struct pids_filtered *pids, pid_t pid) · line 442
Quoted 10 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +10
-0+* 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
Quoted 135 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +61
-55+{ +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;Signature · 1 line
2.54.0
Re: [PATCH v2 1/2] perf trace: Factor out BPF loop body
Quoted 22 lines · Namhyung Kim
Hello,
Quoted 19 lines · Viktor Malik
The BPF program in
augmented_raw_syscallsuses 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 <[email protected]>--- .../bpf_skel/augmented_raw_syscalls.bpf.c | 127 ++++++++++-------- 1 file changed, 72 insertions(+), 55 deletions(-)tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +0
-0@@ -429,15 +429,79 @@ static bool pid_filter__has(struct pids_filtered *pids, pid_t pid)return bpf_map_lookup_elem (pids, &pid) !=NULL ; }
Quoted 13 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c static bool pid_filter__has(struct pids_filtered *pids, pid_t pid) · line 442
Quoted 11 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +11
-0+/* +* 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
Quoted 141 lines
Thanks,
NamhyungQuoted 135 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +61
-55+{ +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;Signature · 1 line
2.54.0
PATCH v2 2/2 perf trace: Refactor augmented_raw_syscalls using bpf_for
In reply to Viktor MalikThe 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 replacing the standard for loop by the bpf_for macro, which uses numeric BPF iterator. This should prevent future breakages of this kind since the verifier has much easier job proving that the loop terminates.
Small adjustments were necessary for the loop to make it work. The main problem is that the verifier has sometimes problems with bpf_for loops that use a carry-over state, such as the payload_offset and output vars here, since the verifier tries to track their values too precisely and cannot prove loop convergence. To resolve the issue, we (1) explicitly recompute payload_offset in every iteration and (2) use a trick with adding a global zero to output to help verifier forget its precise state and use a range instead.
In exchange, this also allows to drop a few artificial checks to help the verifier, including the changes introduced by 013eb043f37b.
Finally, to keep backwards compatibility with older kernel versions which do not have bpf_for (i.e. numeric iterators), fall back to standard for loop in such a case.
--- .../bpf_skel/augmented_raw_syscalls.bpf.c | 54 +++++++++++++------ 1 file changed, 39 insertions(+), 15 deletions(-)
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +39 -15
@@ -429,6 +429,8 @@ static bool pid_filter__has(struct pids_filtered *pids, pid_t pid)return bpf_map_lookup_elem (pids, &pid) !=NULL ; } +u64 ZERO =0 ; +/* * 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
@@ -439,12 +441,13 @@ static bool pid_filter__has(struct pids_filtered *pids, pid_t pid)* 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) +unsigned int *beauty_map, +struct beauty_payload_enter *payload,u64 offset) {int index, value_size =sizeof (struct augmented_arg ) - offsetof(struct augmented_arg , value);s64 aug_size, size;bool augmented; -void *arg; +void *arg, *payload_offset; arg = (void *)args->args[i]; augmented = false;
@@ -454,6 +457,12 @@ static inline int augment_arg(struct syscall_enter_args *args, int i,if (size ==0 || arg ==NULL )return 0 ; +/* bounds check for the verifier */ +if (offset >sizeof (payload->aug_args) -sizeof (payload->aug_args[0 ])) +return -1 ; +barrier_var (offset); + payload_offset = (void *)&payload->aug_args + offset; +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 */
@@ -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 */ 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 ;if (aug_size >0 ) {if (!bpf_probe_read_user (((struct augmented_arg *)payload_offset)->value, aug_size, arg))
@@ -497,11 +508,10 @@ static inline int augment_arg(struct syscall_enter_args *args, int i,static int augment_sys_enter (void *ctx,struct syscall_enter_args *args) {bool do_output = false; -int zero =0 , written; +int i, zero =0 , written;u64 output =0 ;/* has to be u64, otherwise it won't pass the verifier */ unsigned int nr, *beauty_map;struct beauty_payload_enter *payload; -void *payload_offset;/* fall back to do predefined tail call */ if (args ==NULL )
@@ -513,7 +523,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 ;
@@ -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 +*/ + output +=ZERO ; + do_output = true; + } + } + }else { +for (i =0 ; i <6 ; i++) { + written =augment_arg (args, i, beauty_map, payload, output); +if (written <0 ) +return 1 ; +if (written >0 ) { + output += written; + do_output = true; + } } }
Signature · 1 line
2.54.0
[email protected]
AI reviewThank 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…
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 <[email protected]>
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.
Quoted 4 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +0
-0
[ ... ]
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c static inline int augment_arg(struct syscall_enter_args *args, int i, · line 476
Quoted 6 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +1
-1@@ -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.
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c static inline int augment_arg(struct syscall_enter_args *args, int i, · line 482
Quoted 7 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +3
-1index = -(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.
Quoted 3 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +0
-0if (aug_size >0 ) {if (!bpf_probe_read_user (((struct augmented_arg *)payload_offset)->value, aug_size, arg))
[ ... ]
Quoted 12 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +0
-8@@ -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;
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c static int augment_sys_enter(void *ctx, struct syscall_enter_args *args) · line 542
Quoted 10 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +10
-0+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.
Quoted 4 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +4
-0+ output +=ZERO ; + do_output = true; + } + }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Quoted 32 lines · On 7/3/26 12:45, [email protected] 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 toTRACE_AUG_MAX_BUF.
- [Low] Multi-line comment does not follow the required BPF subsystem style.
--commit
0a122bc3ab16635b5165b20566e6e1890cba855b
Author: Viktor Malik <[email protected]>perf trace: Refactor
augmented_raw_syscallsusingbpf_forThis commit replaces the standard loop in
augment_sys_enterwith abpf_forloop to prevent BPF verifier-E2BIGerrors. It also drops some workarounds previously added to help the verifier.Quoted 4 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +0
-0[ ... ]
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c static inline int augment_arg(struct syscall_enter_args *args, int i, · line 476
Quoted 6 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +1
-1@@ -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?
Quoted 4 lines
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.
Quoted 12 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c static inline int augment_arg(struct syscall_enter_args *args, int i, · line 482
Quoted 7 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +3
-1index = -(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?
Quoted 5 lines
args->args[index] is an unsigned long. If the most significant bit is set, assigning it to the s64
aug_sizemakes 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.
Quoted 24 lines
Quoted 3 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +0
-0if (aug_size >0 ) {if (!bpf_probe_read_user (((struct augmented_arg *)payload_offset)->value, aug_size, arg))[ ... ]
Quoted 18 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +6
-8@@ -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 ) {
Quoted 8 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c static int augment_sys_enter(void *ctx, struct syscall_enter_args *args) · line 542
Quoted 4 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +4
-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.
Quoted 5 lines
Quoted 4 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +4
-0+ output +=ZERO ; + do_output = true; + } + }
Namhyung Kim
maintainerQuoted 156 lines · Viktor Malik
The loop for processing syscall args in
augment_raw_syscallshas a history of breaking with Clang updates, see e.g. commit013eb043f37b("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_insn40total_states37941peak_states232mark_read0 -- 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 replacing the standard for loop by the
bpf_formacro, which uses numeric BPF iterator. This should prevent future breakages of this kind since the verifier has much easier job proving that the loop terminates.Small adjustments were necessary for the loop to make it work. The main problem is that the verifier has sometimes problems with
bpf_forloops that use a carry-over state, such as thepayload_offsetandoutputvars here, since the verifier tries to track their values too precisely and cannot prove loop convergence. To resolve the issue, we (1) explicitly recomputepayload_offsetin every iteration and (2) use a trick with adding a global zero tooutputto help verifier forget its precise state and use a range instead.In exchange, this also allows to drop a few artificial checks to help the verifier, including the changes introduced by
013eb043f37b.Finally, to keep backwards compatibility with older kernel versions which do not have
bpf_for(i.e. numeric iterators), fall back to standard for loop in such a case.Signed-off-by Viktor Malik <[email protected]>Suggested-by Andrii Nakryiko <[email protected]>Fixes a68fd6a6cdd3 ("perf trace: Collect augmented data using BPF")Fixes 013eb043f37b ("perf trace: Fix BPF loading failure (-E2BIG)")--- .../bpf_skel/augmented_raw_syscalls.bpf.c | 54 +++++++++++++------ 1 file changed, 39 insertions(+), 15 deletions(-)tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +27
-15@@ -429,6 +429,8 @@ static bool pid_filter__has(struct pids_filtered *pids, pid_t pid)return bpf_map_lookup_elem (pids, &pid) !=NULL ; } +u64 ZERO =0 ; +/* * 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 @@ -439,12 +441,13 @@ static bool pid_filter__has(struct pids_filtered *pids, pid_t pid)* 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) +unsigned int *beauty_map, +struct beauty_payload_enter *payload,u64 offset) {int index, value_size =sizeof (struct augmented_arg ) - offsetof(struct augmented_arg , value);s64 aug_size, size;bool augmented; -void *arg; +void *arg, *payload_offset; arg = (void *)args->args[i]; augmented = false;@@ -454,6 +457,12 @@ static inline int augment_arg(struct syscall_enter_args *args, int i,if (size ==0 || arg ==NULL )return 0 ; +/* bounds check for the verifier */ +if (offset >sizeof (payload->aug_args) -sizeof (payload->aug_args[0 ])) +return -1 ; +barrier_var (offset); + payload_offset = (void *)&payload->aug_args + offset; +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 */ @@ -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 */ 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 ;if (aug_size >0 ) {if (!bpf_probe_read_user (((struct augmented_arg *)payload_offset)->value, aug_size, arg))@@ -497,11 +508,10 @@ static inline int augment_arg(struct syscall_enter_args *args, int i,static int augment_sys_enter (void *ctx,struct syscall_enter_args *args) {bool do_output = false; -int zero =0 , written; +int i, zero =0 , written;u64 output =0 ;/* has to be u64, otherwise it won't pass the verifier */ unsigned int nr, *beauty_map;struct beauty_payload_enter *payload; -void *payload_offset;/* fall back to do predefined tail call */ if (args ==NULL )@@ -513,7 +523,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 ;@@ -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 +*/ + output +=ZERO ;
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c static int augment_sys_enter(void *ctx, struct syscall_enter_args *args) · line 553
Quoted 10 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +10
-0+ do_output = true; + } + } + }else { +for (i =0 ; i <6 ; i++) { + written =augment_arg (args, i, beauty_map, payload, output); +if (written <0 ) +return 1 ; +if (written >0 ) { + output += written;
Woundn't it also need '+= ZERO' here?
Thanks,
Namhyung
Quoted 8 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +2
-0+ do_output = true; + } } }Signature · 1 line
2.54.0
Quoted 161 lines · Namhyung Kim
Quoted 160 lines · Viktor Malik
The loop for processing syscall args in
augment_raw_syscallshas a history of breaking with Clang updates, see e.g. commit013eb043f37b("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_insn40total_states37941peak_states232mark_read0 -- 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 replacing the standard for loop by the
bpf_formacro, which uses numeric BPF iterator. This should prevent future breakages of this kind since the verifier has much easier job proving that the loop terminates.Small adjustments were necessary for the loop to make it work. The main problem is that the verifier has sometimes problems with
bpf_forloops that use a carry-over state, such as thepayload_offsetandoutputvars here, since the verifier tries to track their values too precisely and cannot prove loop convergence. To resolve the issue, we (1) explicitly recomputepayload_offsetin every iteration and (2) use a trick with adding a global zero tooutputto help verifier forget its precise state and use a range instead.In exchange, this also allows to drop a few artificial checks to help the verifier, including the changes introduced by
013eb043f37b.Finally, to keep backwards compatibility with older kernel versions which do not have
bpf_for(i.e. numeric iterators), fall back to standard for loop in such a case.Signed-off-by Viktor Malik <[email protected]>Suggested-by Andrii Nakryiko <[email protected]>Fixes a68fd6a6cdd3 ("perf trace: Collect augmented data using BPF")Fixes 013eb043f37b ("perf trace: Fix BPF loading failure (-E2BIG)")--- .../bpf_skel/augmented_raw_syscalls.bpf.c | 54 +++++++++++++------ 1 file changed, 39 insertions(+), 15 deletions(-)tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +31
-15@@ -429,6 +429,8 @@ static bool pid_filter__has(struct pids_filtered *pids, pid_t pid)return bpf_map_lookup_elem (pids, &pid) !=NULL ; } +u64 ZERO =0 ; +/* * 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 @@ -439,12 +441,13 @@ static bool pid_filter__has(struct pids_filtered *pids, pid_t pid)* 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) +unsigned int *beauty_map, +struct beauty_payload_enter *payload,u64 offset) {int index, value_size =sizeof (struct augmented_arg ) - offsetof(struct augmented_arg , value);s64 aug_size, size;bool augmented; -void *arg; +void *arg, *payload_offset; arg = (void *)args->args[i]; augmented = false;@@ -454,6 +457,12 @@ static inline int augment_arg(struct syscall_enter_args *args, int i,if (size ==0 || arg ==NULL )return 0 ; +/* bounds check for the verifier */ +if (offset >sizeof (payload->aug_args) -sizeof (payload->aug_args[0 ])) +return -1 ; +barrier_var (offset); + payload_offset = (void *)&payload->aug_args + offset; +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 */ @@ -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 */ 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 ;if (aug_size >0 ) {if (!bpf_probe_read_user (((struct augmented_arg *)payload_offset)->value, aug_size, arg))@@ -497,11 +508,10 @@ static inline int augment_arg(struct syscall_enter_args *args, int i,static int augment_sys_enter (void *ctx,struct syscall_enter_args *args) {bool do_output = false; -int zero =0 , written; +int i, zero =0 , written;u64 output =0 ;/* has to be u64, otherwise it won't pass the verifier */ unsigned int nr, *beauty_map;struct beauty_payload_enter *payload; -void *payload_offset;/* fall back to do predefined tail call */ if (args ==NULL )@@ -513,7 +523,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 ;@@ -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 +*/ + output +=ZERO ; + do_output = true; + } + } + }else {
Quoted 8 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c static int augment_sys_enter(void *ctx, struct syscall_enter_args *args) · line 553
Quoted 6 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +6
-0+for (i =0 ; i <6 ; i++) { + written =augment_arg (args, i, beauty_map, payload, output); +if (written <0 ) +return 1 ; +if (written >0 ) { + output += written;Woundn't it also need '+= ZERO' here?
IMHO no, the verification of standard loops doesn't suffer from the same problem as the verification of bpf_for.
Viktor
Quoted 14 lines
Thanks,
NamhyungQuoted 8 lines
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c +2
-0+ do_output = true; + } } }Signature · 1 line
2.54.0