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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
