Re: [RFC/PATCH bpf-next 02/20] bpf: Add cookies support for uprobe_multi link

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Apr 26, 2023 at 12:13:20PM -0700, Andrii Nakryiko wrote:
> On Mon, Apr 24, 2023 at 9:05 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding support to specify cookies array for uprobe_multi link.
> >
> > The cookies array share indexes and length with other uprobe_multi
> > arrays (paths/offsets/ref_ctr_offsets).
> >
> > The cookies[i] value defines cookie for i-the uprobe and will be
> > returned by bpf_get_attach_cookie helper when called from ebpf
> > program hooked to that specific uprobe.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/uapi/linux/bpf.h |  1 +
> >  kernel/bpf/syscall.c     |  2 +-
> >  kernel/trace/bpf_trace.c | 46 +++++++++++++++++++++++++++++++++++++---
> >  3 files changed, 45 insertions(+), 4 deletions(-)
> >
> 
> LGTM, one nit below
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> [...]
> 
> >  static void bpf_uprobe_unregister(struct bpf_uprobe *uprobes, u32 cnt)
> > @@ -2964,6 +2982,7 @@ static int uprobe_prog_run(struct bpf_uprobe *uprobe,
> >         struct bpf_uprobe_multi_link *link = uprobe->link;
> >         struct bpf_uprobe_multi_run_ctx run_ctx = {
> >                 .entry_ip = entry_ip,
> > +               .uprobe = uprobe,
> >         };
> >         struct bpf_run_ctx *old_run_ctx;
> >         int err;
> > @@ -3005,6 +3024,16 @@ uprobe_multi_link_ret_handler(struct uprobe_consumer *con, unsigned long func, s
> >         return uprobe_prog_run(uprobe, func, regs);
> >  }
> >
> > +static u64 bpf_uprobe_multi_cookie(struct bpf_run_ctx *ctx)
> > +{
> > +       struct bpf_uprobe_multi_run_ctx *run_ctx;
> > +
> > +       if (WARN_ON_ONCE(!ctx))
> > +               return 0;
> 
> do we need this check?... seems redundant, tbh

it might be too much.. so this helper is called based on the:

  prog->expected_attach_type == BPF_TRACE_UPROBE_MULTI

so it's just screaming if there's some mismatch with the flag.. but if there is,
we probably would not get to this point or there wil be some pointer != NULL anyway,
yea, I'll remove it

there's similar one for kprobe_multi, I guess it can go as well

thanks,
jirka

> 
> > +       run_ctx = container_of(current->bpf_ctx, struct bpf_uprobe_multi_run_ctx, run_ctx);
> > +       return run_ctx->uprobe->cookie;
> > +}
> > +
> 
> [...]