* [PATCH 1/1] perf bpf_skel augmented_raw_syscalls: Cap the socklen parameter using &= sizeof(saddr)
@ 2023-08-16 21:48 Arnaldo Carvalho de Melo
2023-08-16 22:10 ` Ian Rogers
0 siblings, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-08-16 21:48 UTC (permalink / raw)
To: Ian Rogers
Cc: Adrian Hunter, Jiri Olsa, Namhyung Kim, Linux Kernel Mailing List
This works with:
$ clang -v
clang version 14.0.5 (Fedora 14.0.5-2.fc36)
$
But not with:
$ clang -v
clang version 16.0.6 (Fedora 16.0.6-2.fc38)
$
[root@quaco ~]# perf trace -e connect*,sendto* ping -c 10 localhost
libbpf: prog 'sys_enter_sendto': BPF program load failed: Permission denied
libbpf: prog 'sys_enter_sendto': -- BEGIN PROG LOAD LOG --
reg type unsupported for arg#0 function sys_enter_sendto#59
0: R1=ctx(off=0,imm=0) R10=fp0
; int sys_enter_sendto(struct syscall_enter_args *args)
0: (bf) r6 = r1 ; R1=ctx(off=0,imm=0) R6_w=ctx(off=0,imm=0)
1: (b7) r1 = 0 ; R1_w=0
; int key = 0;
2: (63) *(u32 *)(r10 -4) = r1 ; R1_w=0 R10=fp0 fp-8=0000????
3: (bf) r2 = r10 ; R2_w=fp0 R10=fp0
;
4: (07) r2 += -4 ; R2_w=fp-4
; return bpf_map_lookup_elem(&augmented_args_tmp, &key);
5: (18) r1 = 0xffff8de5a5b8bc00 ; R1_w=map_ptr(off=0,ks=4,vs=8272,imm=0)
7: (85) call bpf_map_lookup_elem#1 ; R0_w=map_value_or_null(id=1,off=0,ks=4,vs=8272,imm=0)
8: (bf) r7 = r0 ; R0_w=map_value_or_null(id=1,off=0,ks=4,vs=8272,imm=0) R7_w=map_value_or_null(id=1,off=0,ks=4,vs=8272,imm=0)
9: (b7) r0 = 1 ; R0_w=1
; if (augmented_args == NULL)
10: (15) if r7 == 0x0 goto pc+25 ; R7_w=map_value(off=0,ks=4,vs=8272,imm=0)
; unsigned int socklen = args->args[5];
11: (79) r1 = *(u64 *)(r6 +56) ; R1_w=scalar() R6_w=ctx(off=0,imm=0)
;
12: (bf) r2 = r1 ; R1_w=scalar(id=2) R2_w=scalar(id=2)
13: (67) r2 <<= 32 ; R2_w=scalar(smax=9223372032559808512,umax=18446744069414584320,var_off=(0x0; 0xffffffff00000000),s32_min=0,s32_max=0,u32_max=0)
14: (77) r2 >>= 32 ; R2_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff))
15: (b7) r8 = 128 ; R8=128
; if (socklen > sizeof(augmented_args->saddr))
16: (25) if r2 > 0x80 goto pc+1 ; R2=scalar(umax=128,var_off=(0x0; 0xff))
17: (bf) r8 = r1 ; R1=scalar(id=2) R8_w=scalar(id=2)
; const void *sockaddr_arg = (const void *)args->args[4];
18: (79) r3 = *(u64 *)(r6 +48) ; R3_w=scalar() R6=ctx(off=0,imm=0)
; bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
19: (bf) r1 = r7 ; R1_w=map_value(off=0,ks=4,vs=8272,imm=0) R7=map_value(off=0,ks=4,vs=8272,imm=0)
20: (07) r1 += 64 ; R1_w=map_value(off=64,ks=4,vs=8272,imm=0)
; bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
21: (bf) r2 = r8 ; R2_w=scalar(id=2) R8_w=scalar(id=2)
22: (85) call bpf_probe_read#4
R2 min value is negative, either use unsigned or 'var &= const'
processed 22 insns (limit 1000000) max_states_per_insn 0 total_states 1 peak_states 1 mark_read 1
-- END PROG LOAD LOG --
libbpf: prog 'sys_enter_sendto': failed to load: -13
libbpf: failed to load object 'augmented_raw_syscalls_bpf'
libbpf: failed to load BPF skeleton 'augmented_raw_syscalls_bpf': -13
So use the suggested &= variant since sizeof(saddr) == 128 bytes.
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c | 6 ++----
1 file changed, 2 insertions(+), 4 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 0586c4118656d3e4..9c1d0b271b20f693 100644
--- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
+++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
@@ -187,8 +187,7 @@ int sys_enter_connect(struct syscall_enter_args *args)
if (augmented_args == NULL)
return 1; /* Failure: don't filter */
- if (socklen > sizeof(augmented_args->saddr))
- socklen = sizeof(augmented_args->saddr);
+ socklen &= sizeof(augmented_args->saddr) - 1;
bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
@@ -206,8 +205,7 @@ int sys_enter_sendto(struct syscall_enter_args *args)
if (augmented_args == NULL)
return 1; /* Failure: don't filter */
- if (socklen > sizeof(augmented_args->saddr))
- socklen = sizeof(augmented_args->saddr);
+ socklen &= sizeof(augmented_args->saddr) - 1;
bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
--
2.41.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] perf bpf_skel augmented_raw_syscalls: Cap the socklen parameter using &= sizeof(saddr)
2023-08-16 21:48 [PATCH 1/1] perf bpf_skel augmented_raw_syscalls: Cap the socklen parameter using &= sizeof(saddr) Arnaldo Carvalho de Melo
@ 2023-08-16 22:10 ` Ian Rogers
2023-08-21 13:40 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 6+ messages in thread
From: Ian Rogers @ 2023-08-16 22:10 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Adrian Hunter, Jiri Olsa, Namhyung Kim, Linux Kernel Mailing List
On Wed, Aug 16, 2023 at 2:48 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> This works with:
>
> $ clang -v
> clang version 14.0.5 (Fedora 14.0.5-2.fc36)
> $
>
> But not with:
>
> $ clang -v
> clang version 16.0.6 (Fedora 16.0.6-2.fc38)
> $
>
> [root@quaco ~]# perf trace -e connect*,sendto* ping -c 10 localhost
> libbpf: prog 'sys_enter_sendto': BPF program load failed: Permission denied
> libbpf: prog 'sys_enter_sendto': -- BEGIN PROG LOAD LOG --
> reg type unsupported for arg#0 function sys_enter_sendto#59
> 0: R1=ctx(off=0,imm=0) R10=fp0
> ; int sys_enter_sendto(struct syscall_enter_args *args)
> 0: (bf) r6 = r1 ; R1=ctx(off=0,imm=0) R6_w=ctx(off=0,imm=0)
> 1: (b7) r1 = 0 ; R1_w=0
> ; int key = 0;
> 2: (63) *(u32 *)(r10 -4) = r1 ; R1_w=0 R10=fp0 fp-8=0000????
> 3: (bf) r2 = r10 ; R2_w=fp0 R10=fp0
> ;
> 4: (07) r2 += -4 ; R2_w=fp-4
> ; return bpf_map_lookup_elem(&augmented_args_tmp, &key);
> 5: (18) r1 = 0xffff8de5a5b8bc00 ; R1_w=map_ptr(off=0,ks=4,vs=8272,imm=0)
> 7: (85) call bpf_map_lookup_elem#1 ; R0_w=map_value_or_null(id=1,off=0,ks=4,vs=8272,imm=0)
> 8: (bf) r7 = r0 ; R0_w=map_value_or_null(id=1,off=0,ks=4,vs=8272,imm=0) R7_w=map_value_or_null(id=1,off=0,ks=4,vs=8272,imm=0)
> 9: (b7) r0 = 1 ; R0_w=1
> ; if (augmented_args == NULL)
> 10: (15) if r7 == 0x0 goto pc+25 ; R7_w=map_value(off=0,ks=4,vs=8272,imm=0)
> ; unsigned int socklen = args->args[5];
> 11: (79) r1 = *(u64 *)(r6 +56) ; R1_w=scalar() R6_w=ctx(off=0,imm=0)
> ;
> 12: (bf) r2 = r1 ; R1_w=scalar(id=2) R2_w=scalar(id=2)
> 13: (67) r2 <<= 32 ; R2_w=scalar(smax=9223372032559808512,umax=18446744069414584320,var_off=(0x0; 0xffffffff00000000),s32_min=0,s32_max=0,u32_max=0)
> 14: (77) r2 >>= 32 ; R2_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff))
> 15: (b7) r8 = 128 ; R8=128
> ; if (socklen > sizeof(augmented_args->saddr))
> 16: (25) if r2 > 0x80 goto pc+1 ; R2=scalar(umax=128,var_off=(0x0; 0xff))
> 17: (bf) r8 = r1 ; R1=scalar(id=2) R8_w=scalar(id=2)
> ; const void *sockaddr_arg = (const void *)args->args[4];
> 18: (79) r3 = *(u64 *)(r6 +48) ; R3_w=scalar() R6=ctx(off=0,imm=0)
> ; bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
> 19: (bf) r1 = r7 ; R1_w=map_value(off=0,ks=4,vs=8272,imm=0) R7=map_value(off=0,ks=4,vs=8272,imm=0)
> 20: (07) r1 += 64 ; R1_w=map_value(off=64,ks=4,vs=8272,imm=0)
> ; bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
> 21: (bf) r2 = r8 ; R2_w=scalar(id=2) R8_w=scalar(id=2)
> 22: (85) call bpf_probe_read#4
> R2 min value is negative, either use unsigned or 'var &= const'
> processed 22 insns (limit 1000000) max_states_per_insn 0 total_states 1 peak_states 1 mark_read 1
> -- END PROG LOAD LOG --
> libbpf: prog 'sys_enter_sendto': failed to load: -13
> libbpf: failed to load object 'augmented_raw_syscalls_bpf'
> libbpf: failed to load BPF skeleton 'augmented_raw_syscalls_bpf': -13
>
> So use the suggested &= variant since sizeof(saddr) == 128 bytes.
Could this be an assert?
Thanks,
Ian
>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Ian Rogers <irogers@google.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
> tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 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 0586c4118656d3e4..9c1d0b271b20f693 100644
> --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> @@ -187,8 +187,7 @@ int sys_enter_connect(struct syscall_enter_args *args)
> if (augmented_args == NULL)
> return 1; /* Failure: don't filter */
>
> - if (socklen > sizeof(augmented_args->saddr))
> - socklen = sizeof(augmented_args->saddr);
> + socklen &= sizeof(augmented_args->saddr) - 1;
>
> bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
>
> @@ -206,8 +205,7 @@ int sys_enter_sendto(struct syscall_enter_args *args)
> if (augmented_args == NULL)
> return 1; /* Failure: don't filter */
>
> - if (socklen > sizeof(augmented_args->saddr))
> - socklen = sizeof(augmented_args->saddr);
> + socklen &= sizeof(augmented_args->saddr) - 1;
>
> bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
>
> --
> 2.41.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] perf bpf_skel augmented_raw_syscalls: Cap the socklen parameter using &= sizeof(saddr)
2023-08-16 22:10 ` Ian Rogers
@ 2023-08-21 13:40 ` Arnaldo Carvalho de Melo
2023-08-21 14:52 ` Ian Rogers
0 siblings, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-08-21 13:40 UTC (permalink / raw)
To: Ian Rogers
Cc: Adrian Hunter, Jiri Olsa, Namhyung Kim, Linux Kernel Mailing List
Em Wed, Aug 16, 2023 at 03:10:00PM -0700, Ian Rogers escreveu:
> On Wed, Aug 16, 2023 at 2:48 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > R2 min value is negative, either use unsigned or 'var &= const'
> > processed 22 insns (limit 1000000) max_states_per_insn 0 total_states 1 peak_states 1 mark_read 1
> > -- END PROG LOAD LOG --
> > libbpf: prog 'sys_enter_sendto': failed to load: -13
> > libbpf: failed to load object 'augmented_raw_syscalls_bpf'
> > libbpf: failed to load BPF skeleton 'augmented_raw_syscalls_bpf': -13
> >
> > So use the suggested &= variant since sizeof(saddr) == 128 bytes.
>
> Could this be an assert?
you mean (removing the change to saddr to make it trigger):
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 9c1d0b271b20f693..521ce2d7357d983c 100644
--- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
+++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
@@ -10,6 +10,16 @@
#include <bpf/bpf_helpers.h>
#include <linux/limits.h>
+/**
+ * is_power_of_2() - check if a value is a power of two
+ * @n: the value to check
+ *
+ * Determine whether some value is a power of two, where zero is *not*
+ * considered a power of two. Return: true if @n is a power of 2, otherwise
+ * false.
+ */
+#define is_power_of_2(n) (n != 0 && ((n & (n - 1)) == 0))
+
#define MAX_CPUS 4096
// FIXME: These should come from system headers
@@ -112,7 +122,10 @@ struct augmented_args_payload {
struct {
struct augmented_arg arg, arg2;
};
- struct sockaddr_storage saddr;
+ struct {
+ struct sockaddr_storage real_saddr;
+ char padding;
+ } saddr;
char __data[sizeof(struct augmented_arg)];
};
};
@@ -187,6 +200,7 @@ int sys_enter_connect(struct syscall_enter_args *args)
if (augmented_args == NULL)
return 1; /* Failure: don't filter */
+ _Static_assert(is_power_of_2(sizeof(augmented_args->saddr)), "sizeof(augmented_args->saddr) needs to be a power of two");
socklen &= sizeof(augmented_args->saddr) - 1;
bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
--------------------------------
CLANG /tmp/build/perf-tools-next/util/bpf_skel/.tmp/augmented_raw_syscalls.bpf.o
util/bpf_skel/augmented_raw_syscalls.bpf.c:203:2: error: static_assert failed due to requirement 'sizeof (augmented_args->saddr) != 0 && ((sizeof (augmented_args->saddr) & (sizeof (augmented_args->saddr) - 1)) == 0)' "sizeof(augmented_args->saddr) needs to be a power of two"
_Static_assert(is_power_of_2(sizeof(augmented_args->saddr)), "sizeof(augmented_args->saddr) needs to be a power of two");
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
make[2]: *** [Makefile.perf:1104: /tmp/build/perf-tools-next/util/bpf_skel/.tmp/augmented_raw_syscalls.bpf.o] Error 1
make[1]: *** [Makefile.perf:238: sub-make] Error 2
make: *** [Makefile:113: install-bin] Error 2
make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'
And here the assert being satisfied:
⬢[acme@toolbox perf-tools-next]$ git 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 9c1d0b271b20f693..43549b63b433d81e 100644
--- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
+++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
@@ -10,6 +10,16 @@
#include <bpf/bpf_helpers.h>
#include <linux/limits.h>
+/**
+ * is_power_of_2() - check if a value is a power of two
+ * @n: the value to check
+ *
+ * Determine whether some value is a power of two, where zero is *not*
+ * considered a power of two. Return: true if @n is a power of 2, otherwise
+ * false.
+ */
+#define is_power_of_2(n) (n != 0 && ((n & (n - 1)) == 0))
+
#define MAX_CPUS 4096
// FIXME: These should come from system headers
@@ -187,6 +197,7 @@ int sys_enter_connect(struct syscall_enter_args *args)
if (augmented_args == NULL)
return 1; /* Failure: don't filter */
+ _Static_assert(is_power_of_2(sizeof(augmented_args->saddr)), "sizeof(augmented_args->saddr) needs to be a power of two");
socklen &= sizeof(augmented_args->saddr) - 1;
bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
⬢[acme@toolbox perf-tools-next]$ m
make: Entering directory '/var/home/acme/git/perf-tools-next/tools/perf'
BUILD: Doing 'make -j32' parallel build
INSTALL libsubcmd_headers
INSTALL libperf_headers
INSTALL libapi_headers
INSTALL libbpf_headers
CLANG /tmp/build/perf-tools-next/util/bpf_skel/.tmp/augmented_raw_syscalls.bpf.o
GENSKEL /tmp/build/perf-tools-next/util/bpf_skel/augmented_raw_syscalls.skel.h
INSTALL libsymbol_headers
CC /tmp/build/perf-tools-next/builtin-trace.o
LD /tmp/build/perf-tools-next/perf-in.o
LINK /tmp/build/perf-tools-next/perf
INSTALL binaries
INSTALL tests
INSTALL libperf-jvmti.so
INSTALL libexec
INSTALL perf-archive
INSTALL perf-iostat
INSTALL strace/groups
INSTALL perl-scripts
INSTALL python-scripts
INSTALL dlfilters
INSTALL perf_completion-script
INSTALL perf-tip
make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'
⬢[acme@toolbox perf-tools-next]$
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] perf bpf_skel augmented_raw_syscalls: Cap the socklen parameter using &= sizeof(saddr)
2023-08-21 13:40 ` Arnaldo Carvalho de Melo
@ 2023-08-21 14:52 ` Ian Rogers
2023-08-22 18:08 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 6+ messages in thread
From: Ian Rogers @ 2023-08-21 14:52 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Adrian Hunter, Jiri Olsa, Namhyung Kim, Linux Kernel Mailing List
On Mon, Aug 21, 2023 at 6:40 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Wed, Aug 16, 2023 at 03:10:00PM -0700, Ian Rogers escreveu:
> > On Wed, Aug 16, 2023 at 2:48 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > R2 min value is negative, either use unsigned or 'var &= const'
> > > processed 22 insns (limit 1000000) max_states_per_insn 0 total_states 1 peak_states 1 mark_read 1
> > > -- END PROG LOAD LOG --
> > > libbpf: prog 'sys_enter_sendto': failed to load: -13
> > > libbpf: failed to load object 'augmented_raw_syscalls_bpf'
> > > libbpf: failed to load BPF skeleton 'augmented_raw_syscalls_bpf': -13
> > >
> > > So use the suggested &= variant since sizeof(saddr) == 128 bytes.
> >
> > Could this be an assert?
>
> you mean (removing the change to saddr to make it trigger):
I was more meaning that doing sizeof(x) -1 and assuming that x is a
power of two should really have an assert that sizeof(x) is a power of
2. Using 127 and asserting sizeof(x)==128 also works.
Thanks,
Ian
> 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 9c1d0b271b20f693..521ce2d7357d983c 100644
> --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> @@ -10,6 +10,16 @@
> #include <bpf/bpf_helpers.h>
> #include <linux/limits.h>
>
> +/**
> + * is_power_of_2() - check if a value is a power of two
> + * @n: the value to check
> + *
> + * Determine whether some value is a power of two, where zero is *not*
> + * considered a power of two. Return: true if @n is a power of 2, otherwise
> + * false.
> + */
> +#define is_power_of_2(n) (n != 0 && ((n & (n - 1)) == 0))
> +
> #define MAX_CPUS 4096
>
> // FIXME: These should come from system headers
> @@ -112,7 +122,10 @@ struct augmented_args_payload {
> struct {
> struct augmented_arg arg, arg2;
> };
> - struct sockaddr_storage saddr;
> + struct {
> + struct sockaddr_storage real_saddr;
> + char padding;
> + } saddr;
> char __data[sizeof(struct augmented_arg)];
> };
> };
> @@ -187,6 +200,7 @@ int sys_enter_connect(struct syscall_enter_args *args)
> if (augmented_args == NULL)
> return 1; /* Failure: don't filter */
>
> + _Static_assert(is_power_of_2(sizeof(augmented_args->saddr)), "sizeof(augmented_args->saddr) needs to be a power of two");
> socklen &= sizeof(augmented_args->saddr) - 1;
>
> bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
>
>
> --------------------------------
>
> CLANG /tmp/build/perf-tools-next/util/bpf_skel/.tmp/augmented_raw_syscalls.bpf.o
> util/bpf_skel/augmented_raw_syscalls.bpf.c:203:2: error: static_assert failed due to requirement 'sizeof (augmented_args->saddr) != 0 && ((sizeof (augmented_args->saddr) & (sizeof (augmented_args->saddr) - 1)) == 0)' "sizeof(augmented_args->saddr) needs to be a power of two"
> _Static_assert(is_power_of_2(sizeof(augmented_args->saddr)), "sizeof(augmented_args->saddr) needs to be a power of two");
> ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 error generated.
> make[2]: *** [Makefile.perf:1104: /tmp/build/perf-tools-next/util/bpf_skel/.tmp/augmented_raw_syscalls.bpf.o] Error 1
> make[1]: *** [Makefile.perf:238: sub-make] Error 2
> make: *** [Makefile:113: install-bin] Error 2
> make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'
>
> And here the assert being satisfied:
>
> ⬢[acme@toolbox perf-tools-next]$ git 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 9c1d0b271b20f693..43549b63b433d81e 100644
> --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> @@ -10,6 +10,16 @@
> #include <bpf/bpf_helpers.h>
> #include <linux/limits.h>
>
> +/**
> + * is_power_of_2() - check if a value is a power of two
> + * @n: the value to check
> + *
> + * Determine whether some value is a power of two, where zero is *not*
> + * considered a power of two. Return: true if @n is a power of 2, otherwise
> + * false.
> + */
> +#define is_power_of_2(n) (n != 0 && ((n & (n - 1)) == 0))
> +
> #define MAX_CPUS 4096
>
> // FIXME: These should come from system headers
> @@ -187,6 +197,7 @@ int sys_enter_connect(struct syscall_enter_args *args)
> if (augmented_args == NULL)
> return 1; /* Failure: don't filter */
>
> + _Static_assert(is_power_of_2(sizeof(augmented_args->saddr)), "sizeof(augmented_args->saddr) needs to be a power of two");
> socklen &= sizeof(augmented_args->saddr) - 1;
>
> bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
> ⬢[acme@toolbox perf-tools-next]$ m
> make: Entering directory '/var/home/acme/git/perf-tools-next/tools/perf'
> BUILD: Doing 'make -j32' parallel build
> INSTALL libsubcmd_headers
> INSTALL libperf_headers
> INSTALL libapi_headers
> INSTALL libbpf_headers
> CLANG /tmp/build/perf-tools-next/util/bpf_skel/.tmp/augmented_raw_syscalls.bpf.o
> GENSKEL /tmp/build/perf-tools-next/util/bpf_skel/augmented_raw_syscalls.skel.h
> INSTALL libsymbol_headers
> CC /tmp/build/perf-tools-next/builtin-trace.o
> LD /tmp/build/perf-tools-next/perf-in.o
> LINK /tmp/build/perf-tools-next/perf
> INSTALL binaries
> INSTALL tests
> INSTALL libperf-jvmti.so
> INSTALL libexec
> INSTALL perf-archive
> INSTALL perf-iostat
> INSTALL strace/groups
> INSTALL perl-scripts
> INSTALL python-scripts
> INSTALL dlfilters
> INSTALL perf_completion-script
> INSTALL perf-tip
> make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'
> ⬢[acme@toolbox perf-tools-next]$
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] perf bpf_skel augmented_raw_syscalls: Cap the socklen parameter using &= sizeof(saddr)
2023-08-21 14:52 ` Ian Rogers
@ 2023-08-22 18:08 ` Arnaldo Carvalho de Melo
2023-08-22 18:16 ` Ian Rogers
0 siblings, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-08-22 18:08 UTC (permalink / raw)
To: Ian Rogers
Cc: Adrian Hunter, Jiri Olsa, Namhyung Kim, Linux Kernel Mailing List
Em Mon, Aug 21, 2023 at 07:52:46AM -0700, Ian Rogers escreveu:
> On Mon, Aug 21, 2023 at 6:40 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Wed, Aug 16, 2023 at 03:10:00PM -0700, Ian Rogers escreveu:
> > > On Wed, Aug 16, 2023 at 2:48 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > > R2 min value is negative, either use unsigned or 'var &= const'
> > > > processed 22 insns (limit 1000000) max_states_per_insn 0 total_states 1 peak_states 1 mark_read 1
> > > > -- END PROG LOAD LOG --
> > > > libbpf: prog 'sys_enter_sendto': failed to load: -13
> > > > libbpf: failed to load object 'augmented_raw_syscalls_bpf'
> > > > libbpf: failed to load BPF skeleton 'augmented_raw_syscalls_bpf': -13
> > > >
> > > > So use the suggested &= variant since sizeof(saddr) == 128 bytes.
> > >
> > > Could this be an assert?
> >
> > you mean (removing the change to saddr to make it trigger):
>
> I was more meaning that doing sizeof(x) -1 and assuming that x is a
> power of two should really have an assert that sizeof(x) is a power of
Right, that was my understanding, and that is what:
_Static_assert(is_power_of_2(sizeof(augmented_args->saddr)), "sizeof(augmented_args->saddr) needs to be a power of two")
Does, right?
- Arnaldo
> 2. Using 127 and asserting sizeof(x)==128 also works.
> Thanks,
> Ian
>
> > 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 9c1d0b271b20f693..521ce2d7357d983c 100644
> > --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> > +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> > @@ -10,6 +10,16 @@
> > #include <bpf/bpf_helpers.h>
> > #include <linux/limits.h>
> >
> > +/**
> > + * is_power_of_2() - check if a value is a power of two
> > + * @n: the value to check
> > + *
> > + * Determine whether some value is a power of two, where zero is *not*
> > + * considered a power of two. Return: true if @n is a power of 2, otherwise
> > + * false.
> > + */
> > +#define is_power_of_2(n) (n != 0 && ((n & (n - 1)) == 0))
> > +
> > #define MAX_CPUS 4096
> >
> > // FIXME: These should come from system headers
> > @@ -112,7 +122,10 @@ struct augmented_args_payload {
> > struct {
> > struct augmented_arg arg, arg2;
> > };
> > - struct sockaddr_storage saddr;
> > + struct {
> > + struct sockaddr_storage real_saddr;
> > + char padding;
> > + } saddr;
> > char __data[sizeof(struct augmented_arg)];
> > };
> > };
> > @@ -187,6 +200,7 @@ int sys_enter_connect(struct syscall_enter_args *args)
> > if (augmented_args == NULL)
> > return 1; /* Failure: don't filter */
> >
> > + _Static_assert(is_power_of_2(sizeof(augmented_args->saddr)), "sizeof(augmented_args->saddr) needs to be a power of two");
> > socklen &= sizeof(augmented_args->saddr) - 1;
> >
> > bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
> >
> >
> > --------------------------------
> >
> > CLANG /tmp/build/perf-tools-next/util/bpf_skel/.tmp/augmented_raw_syscalls.bpf.o
> > util/bpf_skel/augmented_raw_syscalls.bpf.c:203:2: error: static_assert failed due to requirement 'sizeof (augmented_args->saddr) != 0 && ((sizeof (augmented_args->saddr) & (sizeof (augmented_args->saddr) - 1)) == 0)' "sizeof(augmented_args->saddr) needs to be a power of two"
> > _Static_assert(is_power_of_2(sizeof(augmented_args->saddr)), "sizeof(augmented_args->saddr) needs to be a power of two");
> > ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 1 error generated.
> > make[2]: *** [Makefile.perf:1104: /tmp/build/perf-tools-next/util/bpf_skel/.tmp/augmented_raw_syscalls.bpf.o] Error 1
> > make[1]: *** [Makefile.perf:238: sub-make] Error 2
> > make: *** [Makefile:113: install-bin] Error 2
> > make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'
> >
> > And here the assert being satisfied:
> >
> > ⬢[acme@toolbox perf-tools-next]$ git 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 9c1d0b271b20f693..43549b63b433d81e 100644
> > --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> > +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> > @@ -10,6 +10,16 @@
> > #include <bpf/bpf_helpers.h>
> > #include <linux/limits.h>
> >
> > +/**
> > + * is_power_of_2() - check if a value is a power of two
> > + * @n: the value to check
> > + *
> > + * Determine whether some value is a power of two, where zero is *not*
> > + * considered a power of two. Return: true if @n is a power of 2, otherwise
> > + * false.
> > + */
> > +#define is_power_of_2(n) (n != 0 && ((n & (n - 1)) == 0))
> > +
> > #define MAX_CPUS 4096
> >
> > // FIXME: These should come from system headers
> > @@ -187,6 +197,7 @@ int sys_enter_connect(struct syscall_enter_args *args)
> > if (augmented_args == NULL)
> > return 1; /* Failure: don't filter */
> >
> > + _Static_assert(is_power_of_2(sizeof(augmented_args->saddr)), "sizeof(augmented_args->saddr) needs to be a power of two");
> > socklen &= sizeof(augmented_args->saddr) - 1;
> >
> > bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
> > ⬢[acme@toolbox perf-tools-next]$ m
> > make: Entering directory '/var/home/acme/git/perf-tools-next/tools/perf'
> > BUILD: Doing 'make -j32' parallel build
> > INSTALL libsubcmd_headers
> > INSTALL libperf_headers
> > INSTALL libapi_headers
> > INSTALL libbpf_headers
> > CLANG /tmp/build/perf-tools-next/util/bpf_skel/.tmp/augmented_raw_syscalls.bpf.o
> > GENSKEL /tmp/build/perf-tools-next/util/bpf_skel/augmented_raw_syscalls.skel.h
> > INSTALL libsymbol_headers
> > CC /tmp/build/perf-tools-next/builtin-trace.o
> > LD /tmp/build/perf-tools-next/perf-in.o
> > LINK /tmp/build/perf-tools-next/perf
> > INSTALL binaries
> > INSTALL tests
> > INSTALL libperf-jvmti.so
> > INSTALL libexec
> > INSTALL perf-archive
> > INSTALL perf-iostat
> > INSTALL strace/groups
> > INSTALL perl-scripts
> > INSTALL python-scripts
> > INSTALL dlfilters
> > INSTALL perf_completion-script
> > INSTALL perf-tip
> > make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'
> > ⬢[acme@toolbox perf-tools-next]$
> >
> >
> >
> >
--
- Arnaldo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] perf bpf_skel augmented_raw_syscalls: Cap the socklen parameter using &= sizeof(saddr)
2023-08-22 18:08 ` Arnaldo Carvalho de Melo
@ 2023-08-22 18:16 ` Ian Rogers
0 siblings, 0 replies; 6+ messages in thread
From: Ian Rogers @ 2023-08-22 18:16 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Adrian Hunter, Jiri Olsa, Namhyung Kim, Linux Kernel Mailing List
On Tue, Aug 22, 2023 at 11:08 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Mon, Aug 21, 2023 at 07:52:46AM -0700, Ian Rogers escreveu:
> > On Mon, Aug 21, 2023 at 6:40 AM Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > > Em Wed, Aug 16, 2023 at 03:10:00PM -0700, Ian Rogers escreveu:
> > > > On Wed, Aug 16, 2023 at 2:48 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > > > R2 min value is negative, either use unsigned or 'var &= const'
> > > > > processed 22 insns (limit 1000000) max_states_per_insn 0 total_states 1 peak_states 1 mark_read 1
> > > > > -- END PROG LOAD LOG --
> > > > > libbpf: prog 'sys_enter_sendto': failed to load: -13
> > > > > libbpf: failed to load object 'augmented_raw_syscalls_bpf'
> > > > > libbpf: failed to load BPF skeleton 'augmented_raw_syscalls_bpf': -13
> > > > >
> > > > > So use the suggested &= variant since sizeof(saddr) == 128 bytes.
> > > >
> > > > Could this be an assert?
> > >
> > > you mean (removing the change to saddr to make it trigger):
> >
> > I was more meaning that doing sizeof(x) -1 and assuming that x is a
> > power of two should really have an assert that sizeof(x) is a power of
>
> Right, that was my understanding, and that is what:
>
> _Static_assert(is_power_of_2(sizeof(augmented_args->saddr)), "sizeof(augmented_args->saddr) needs to be a power of two")
>
> Does, right?
Yep, I was reading your emails in order :-)
Thanks,
Ian
> - Arnaldo
>
>
> > 2. Using 127 and asserting sizeof(x)==128 also works.
>
>
> > Thanks,
> > Ian
> >
> > > 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 9c1d0b271b20f693..521ce2d7357d983c 100644
> > > --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> > > +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> > > @@ -10,6 +10,16 @@
> > > #include <bpf/bpf_helpers.h>
> > > #include <linux/limits.h>
> > >
> > > +/**
> > > + * is_power_of_2() - check if a value is a power of two
> > > + * @n: the value to check
> > > + *
> > > + * Determine whether some value is a power of two, where zero is *not*
> > > + * considered a power of two. Return: true if @n is a power of 2, otherwise
> > > + * false.
> > > + */
> > > +#define is_power_of_2(n) (n != 0 && ((n & (n - 1)) == 0))
> > > +
> > > #define MAX_CPUS 4096
> > >
> > > // FIXME: These should come from system headers
> > > @@ -112,7 +122,10 @@ struct augmented_args_payload {
> > > struct {
> > > struct augmented_arg arg, arg2;
> > > };
> > > - struct sockaddr_storage saddr;
> > > + struct {
> > > + struct sockaddr_storage real_saddr;
> > > + char padding;
> > > + } saddr;
> > > char __data[sizeof(struct augmented_arg)];
> > > };
> > > };
> > > @@ -187,6 +200,7 @@ int sys_enter_connect(struct syscall_enter_args *args)
> > > if (augmented_args == NULL)
> > > return 1; /* Failure: don't filter */
> > >
> > > + _Static_assert(is_power_of_2(sizeof(augmented_args->saddr)), "sizeof(augmented_args->saddr) needs to be a power of two");
> > > socklen &= sizeof(augmented_args->saddr) - 1;
> > >
> > > bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
> > >
> > >
> > > --------------------------------
> > >
> > > CLANG /tmp/build/perf-tools-next/util/bpf_skel/.tmp/augmented_raw_syscalls.bpf.o
> > > util/bpf_skel/augmented_raw_syscalls.bpf.c:203:2: error: static_assert failed due to requirement 'sizeof (augmented_args->saddr) != 0 && ((sizeof (augmented_args->saddr) & (sizeof (augmented_args->saddr) - 1)) == 0)' "sizeof(augmented_args->saddr) needs to be a power of two"
> > > _Static_assert(is_power_of_2(sizeof(augmented_args->saddr)), "sizeof(augmented_args->saddr) needs to be a power of two");
> > > ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > 1 error generated.
> > > make[2]: *** [Makefile.perf:1104: /tmp/build/perf-tools-next/util/bpf_skel/.tmp/augmented_raw_syscalls.bpf.o] Error 1
> > > make[1]: *** [Makefile.perf:238: sub-make] Error 2
> > > make: *** [Makefile:113: install-bin] Error 2
> > > make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'
> > >
> > > And here the assert being satisfied:
> > >
> > > ⬢[acme@toolbox perf-tools-next]$ git 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 9c1d0b271b20f693..43549b63b433d81e 100644
> > > --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> > > +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> > > @@ -10,6 +10,16 @@
> > > #include <bpf/bpf_helpers.h>
> > > #include <linux/limits.h>
> > >
> > > +/**
> > > + * is_power_of_2() - check if a value is a power of two
> > > + * @n: the value to check
> > > + *
> > > + * Determine whether some value is a power of two, where zero is *not*
> > > + * considered a power of two. Return: true if @n is a power of 2, otherwise
> > > + * false.
> > > + */
> > > +#define is_power_of_2(n) (n != 0 && ((n & (n - 1)) == 0))
> > > +
> > > #define MAX_CPUS 4096
> > >
> > > // FIXME: These should come from system headers
> > > @@ -187,6 +197,7 @@ int sys_enter_connect(struct syscall_enter_args *args)
> > > if (augmented_args == NULL)
> > > return 1; /* Failure: don't filter */
> > >
> > > + _Static_assert(is_power_of_2(sizeof(augmented_args->saddr)), "sizeof(augmented_args->saddr) needs to be a power of two");
> > > socklen &= sizeof(augmented_args->saddr) - 1;
> > >
> > > bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
> > > ⬢[acme@toolbox perf-tools-next]$ m
> > > make: Entering directory '/var/home/acme/git/perf-tools-next/tools/perf'
> > > BUILD: Doing 'make -j32' parallel build
> > > INSTALL libsubcmd_headers
> > > INSTALL libperf_headers
> > > INSTALL libapi_headers
> > > INSTALL libbpf_headers
> > > CLANG /tmp/build/perf-tools-next/util/bpf_skel/.tmp/augmented_raw_syscalls.bpf.o
> > > GENSKEL /tmp/build/perf-tools-next/util/bpf_skel/augmented_raw_syscalls.skel.h
> > > INSTALL libsymbol_headers
> > > CC /tmp/build/perf-tools-next/builtin-trace.o
> > > LD /tmp/build/perf-tools-next/perf-in.o
> > > LINK /tmp/build/perf-tools-next/perf
> > > INSTALL binaries
> > > INSTALL tests
> > > INSTALL libperf-jvmti.so
> > > INSTALL libexec
> > > INSTALL perf-archive
> > > INSTALL perf-iostat
> > > INSTALL strace/groups
> > > INSTALL perl-scripts
> > > INSTALL python-scripts
> > > INSTALL dlfilters
> > > INSTALL perf_completion-script
> > > INSTALL perf-tip
> > > make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'
> > > ⬢[acme@toolbox perf-tools-next]$
> > >
> > >
> > >
> > >
>
> --
>
> - Arnaldo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-08-22 18:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-16 21:48 [PATCH 1/1] perf bpf_skel augmented_raw_syscalls: Cap the socklen parameter using &= sizeof(saddr) Arnaldo Carvalho de Melo
2023-08-16 22:10 ` Ian Rogers
2023-08-21 13:40 ` Arnaldo Carvalho de Melo
2023-08-21 14:52 ` Ian Rogers
2023-08-22 18:08 ` Arnaldo Carvalho de Melo
2023-08-22 18:16 ` Ian Rogers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox