On Thu, Feb 29, 2024 at 2:16 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Wed, Feb 28, 2024 at 05:23:45PM -0800, Andrii Nakryiko wrote:
>
> SNIP
>
> > > static int
> > > kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
> > > - unsigned long entry_ip, struct pt_regs *regs)
> > > + unsigned long entry_ip, struct pt_regs *regs,
> > > + bool is_return)
> > > {
> > > struct bpf_kprobe_multi_run_ctx run_ctx = {
> > > .link = link,
> > > .entry_ip = entry_ip,
> > > + .is_return = is_return,
> > > };
> > > struct bpf_run_ctx *old_run_ctx;
> > > int err;
> > > @@ -2830,7 +2833,7 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long fentry_ip,
> > > int err;
> > >
> > > link = container_of(fp, struct bpf_kprobe_multi_link, fp);
> > > - err = kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
> > > + err = kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs, false);
> > > return link->is_wrapper ? err : 0;
> > > }
> > >
> > > @@ -2842,7 +2845,7 @@ kprobe_multi_link_exit_handler(struct fprobe *fp, unsigned long fentry_ip,
> > > struct bpf_kprobe_multi_link *link;
> > >
> > > link = container_of(fp, struct bpf_kprobe_multi_link, fp);
> > > - kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
> > > + kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs, true);
> > > }
> > >
> > > static int symbols_cmp_r(const void *a, const void *b, const void *priv)
> > > @@ -3111,6 +3114,46 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > > kvfree(cookies);
> > > return err;
> > > }
> > > +
> > > +__bpf_kfunc_start_defs();
> > > +
> > > +__bpf_kfunc bool bpf_kprobe_multi_is_return(void)
> >
> > and for uprobes we'll have bpf_uprobe_multi_is_return?...
>
> yes, but now I'm thinking maybe we could also have 'session' api and
> have single 'bpf_session_is_return' because both kprobe and uprobe
> are KPROBE program type.. and align it together with other session
> kfuncs:
>
> bpf_session_is_return
> bpf_session_set_cookie
> bpf_session_get_cookie
>
We can do that. But I was thinking more of a
u64 *bpf_session_cookie()
which would return a read/write pointer that BPF program can
manipulate. Instead of doing two calls (get_cookie + set_cookie), it
would be one call. Is there any benefit to having separate set/get
cookie calls?
> >
> > BTW, have you tried implementing a "session cookie" idea?
>
> yep, with a little fix [0] it's working on top of Masami's 'fprobe over fgraph'
> changes, you can check last 2 patches in [1] .. I did not do this on top of the
> current fprobe/rethook kernel code, because it seems it's about to go away
do you know what is the timeline for fprobe over fgraph work to be finished?
>
> I still need to implement that on top of uprobes and I will send rfc, so we can
> see all of it and discuss the interface
>
great, yeah, I think the session cookie idea should go in at the same
time, if possible, so that we can assume it is supported for new
[ku]probe.wrapper programs.
> jirka
>
>
> [0] https://lore.kernel.org/bpf/ZdyKaRiI-PnG80Q0@krava/
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git/log/?h=bpf/session_data
>
> >
> >
> > > +{
> > > + struct bpf_kprobe_multi_run_ctx *run_ctx;
> > > +
> > > + run_ctx = container_of(current->bpf_ctx, struct bpf_kprobe_multi_run_ctx, run_ctx);
> > > + return run_ctx->is_return;
> > > +}
> > > +
> > > +__bpf_kfunc_end_defs();
> > > +
> > > +BTF_KFUNCS_START(kprobe_multi_kfunc_set_ids)
> > > +BTF_ID_FLAGS(func, bpf_kprobe_multi_is_return)
> > > +BTF_KFUNCS_END(kprobe_multi_kfunc_set_ids)
> > > +
> > > +static int bpf_kprobe_multi_filter(const struct bpf_prog *prog, u32 kfunc_id)
> > > +{
> > > + if (!btf_id_set8_contains(&kprobe_multi_kfunc_set_ids, kfunc_id))
> > > + return 0;
> > > +
> > > + if (prog->expected_attach_type != BPF_TRACE_KPROBE_MULTI)
> > > + return -EACCES;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct btf_kfunc_id_set bpf_kprobe_multi_kfunc_set = {
> > > + .owner = THIS_MODULE,
> > > + .set = &kprobe_multi_kfunc_set_ids,
> > > + .filter = bpf_kprobe_multi_filter,
> > > +};
> > > +
> > > +static int __init bpf_kprobe_multi_kfuncs_init(void)
> > > +{
> > > + return register_btf_kfunc_id_set(BPF_PROG_TYPE_KPROBE, &bpf_kprobe_multi_kfunc_set);
> > > +}
> > > +
> > > +late_initcall(bpf_kprobe_multi_kfuncs_init);
> > > #else /* !CONFIG_FPROBE */
> > > int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > > {
> > > --
> > > 2.43.2
> > >
