Re: [PATCH bpf-next 7/7] selftests/bpf: Add kprobe multi wrapper cookie test

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Apr 23, 2024 at 05:27:22PM -0700, Andrii Nakryiko wrote:
> On Mon, Apr 22, 2024 at 5:14 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding kprobe multi session test that verifies the cookie
> > value get properly propagated from entry to return program.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> 
> Forgot to update subject (still using "wrapper" naming)

ugh, thnx

> 
> overall LGTM, see nits
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> >  tools/testing/selftests/bpf/bpf_kfuncs.h      |  1 +
> >  .../bpf/prog_tests/kprobe_multi_test.c        | 35 ++++++++++++
> >  .../bpf/progs/kprobe_multi_session_cookie.c   | 56 +++++++++++++++++++
> >  3 files changed, 92 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_session_cookie.c
> >
> > diff --git a/tools/testing/selftests/bpf/bpf_kfuncs.h b/tools/testing/selftests/bpf/bpf_kfuncs.h
> > index 180030b5d828..0281921cd654 100644
> > --- a/tools/testing/selftests/bpf/bpf_kfuncs.h
> > +++ b/tools/testing/selftests/bpf/bpf_kfuncs.h
> > @@ -77,4 +77,5 @@ extern int bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr,
> >                                       struct bpf_key *trusted_keyring) __ksym;
> >
> >  extern bool bpf_session_is_return(void) __ksym;
> > +extern __u64 *bpf_session_cookie(void) __ksym;
> 
> btw, should we use `long *` as return type to avoid relying on having
> __u64 alias be available? Long is always an 8-byte value in the BPF
> world, it should be fine.

ok, there are some __u64 in kfuncs already, but let's stay on safe side

SNIP

> > +/*
> > + * No tests in here, just to trigger 'bpf_fentry_test*'
> > + * through tracing test_run
> > + */
> > +SEC("fentry/bpf_modify_return_test")
> > +int BPF_PROG(trigger)
> > +{
> > +       return 0;
> > +}
> > +
> > +static int check_cookie(__u64 val, __u64 *result)
> > +{
> > +       if (bpf_get_current_pid_tgid() >> 32 != pid)
> > +               return 1;
> > +
> > +       __u64 *cookie = bpf_session_cookie();
> 
> we don't enforce this, but let's stick to C89 variable declaration
> style (or rather positioning in this case)?

ok, will change that

jirka