Re: [PATCH 1/1] perf bpf_skel augmented_raw_syscalls: Cap the socklen parameter using &= sizeof(saddr)

[Ian Rogers]

On Tue, Aug 22, 2023 at 11:08 AM Arnaldo Carvalho de Melo
<acme@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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