Re: [PATCHv2 bpf-next 6/7] selftests/bpf: Add kprobe session test

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Apr 30, 2024 at 10:29:05AM -0700, Andrii Nakryiko wrote:
> On Tue, Apr 30, 2024 at 4:29 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding kprobe session test and testing that the entry program
> > return value controls execution of the return probe program.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/testing/selftests/bpf/bpf_kfuncs.h      |  2 +
> >  .../bpf/prog_tests/kprobe_multi_test.c        | 39 ++++++++++
> >  .../bpf/progs/kprobe_multi_session.c          | 78 +++++++++++++++++++
> >  3 files changed, 119 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_session.c
> >
> 
> Given the things I mentioned below were the only "problems" I found, I
> applied the patch and fixed those issues up while applying. Thanks a
> lot for working on this! Excited about this feature, it's been asked
> by our internal customers for a while as well. Looking forward to
> uprobe session program type!

great, I'll send it soon

> 
> > diff --git a/tools/testing/selftests/bpf/bpf_kfuncs.h b/tools/testing/selftests/bpf/bpf_kfuncs.h
> > index 14ebe7d9e1a3..180030b5d828 100644
> > --- a/tools/testing/selftests/bpf/bpf_kfuncs.h
> > +++ b/tools/testing/selftests/bpf/bpf_kfuncs.h
> > @@ -75,4 +75,6 @@ extern void bpf_key_put(struct bpf_key *key) __ksym;
> >  extern int bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr,
> >                                       struct bpf_dynptr *sig_ptr,
> >                                       struct bpf_key *trusted_keyring) __ksym;
> > +
> > +extern bool bpf_session_is_return(void) __ksym;
> 
> should be __weak, always make it __weak. vmlinux.h with kfuncs is coming
> 
> same for another kfunc in next patch

ok

> 
> >  #endif
> 
> [...]
> 
> > +static const void *kfuncs[8] = {
> > +       &bpf_fentry_test1,
> > +       &bpf_fentry_test2,
> > +       &bpf_fentry_test3,
> > +       &bpf_fentry_test4,
> > +       &bpf_fentry_test5,
> > +       &bpf_fentry_test6,
> > +       &bpf_fentry_test7,
> > +       &bpf_fentry_test8,
> > +};
> > +
> 
> this is not supposed to work :) I don't think libbpf support this kind
> of relocations in data section.

aah, nice ;-) should we make it work (or make sure it works) ? seems useful

> 
> The only reason it works in practice is because compiler completely
> inlines access to this array and so code just has unrolled loop
> (thanks to "static const" and -O2).
> 
> This is a bit fragile, though. It might keep working, of course
> (though I'm not sure if -O1 would still work), but I'd feel a bit more
> comfortable if you define and initialize this array inside the
> function (then it will be guaranteed to work with libbpf logic)

thanks,
jirka