Re: [RFC/PATCH bpf-next 01/20] bpf: Add multi uprobe link

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Apr 26, 2023 at 12:00:10PM -0700, Andrii Nakryiko wrote:
> On Mon, Apr 24, 2023 at 9:05 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding new multi uprobe link that allows to attach bpf program
> > to multiple uprobes.
> >
> > Uprobes to attach are specified via new link_create uprobe_multi
> > union:
> >
> >   struct {
> >           __u32           flags;
> >           __u32           cnt;
> >           __aligned_u64   paths;
> >           __aligned_u64   offsets;
> >           __aligned_u64   ref_ctr_offsets;
> >   } uprobe_multi;
> >
> > Uprobes are defined in paths/offsets/ref_ctr_offsets arrays with
> > the same 'cnt' length. Each uprobe is defined with a single index
> > in all three arrays:
> >
> >   paths[idx], offsets[idx] and/or ref_ctr_offsets[idx]
> >
> > The 'flags' supports single bit for now that marks the uprobe as
> > return probe.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/linux/trace_events.h |   6 +
> >  include/uapi/linux/bpf.h     |  14 +++
> >  kernel/bpf/syscall.c         |  16 ++-
> >  kernel/trace/bpf_trace.c     | 231 +++++++++++++++++++++++++++++++++++
> >  4 files changed, 265 insertions(+), 2 deletions(-)
> >
> 
> [...]
> 
> > @@ -4666,10 +4667,21 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
> >                 ret = bpf_perf_link_attach(attr, prog);
> >                 break;
> >         case BPF_PROG_TYPE_KPROBE:
> > +               /* Ensure that program with eBPF_TRACE_UPROBE_MULTI attach type can
> 
> eBPF_TRACE_UPROBE_MULTI :)

will fix ;-)

> 
> > +                * attach only to uprobe_multi link. It has its own runtime context
> > +                * which is specific for get_func_ip/get_attach_cookie helpers.
> > +                */
> > +               if (prog->expected_attach_type == BPF_TRACE_UPROBE_MULTI &&
> > +                   attr->link_create.attach_type != BPF_TRACE_UPROBE_MULTI) {
> > +                       ret = -EINVAL;
> > +                       goto out;
> > +               }
> 
> as Yonghong pointed out, you check this condition in
> bpf_uprobe_multi_link_attach() already, so why redundant check?

I tried to answer that in here:
  https://lore.kernel.org/bpf/ZEjU0ykZZTHMVlZt@krava/

> 
> >                 if (attr->link_create.attach_type == BPF_PERF_EVENT)
> >                         ret = bpf_perf_link_attach(attr, prog);
> > -               else
> > +               else if (attr->link_create.attach_type == BPF_TRACE_KPROBE_MULTI)
> >                         ret = bpf_kprobe_multi_link_attach(attr, prog);
> > +               else if (attr->link_create.attach_type == BPF_TRACE_UPROBE_MULTI)
> > +                       ret = bpf_uprobe_multi_link_attach(attr, prog);
> >                 break;
> >         default:
> >                 ret = -EINVAL;
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index bcf91bc7bf71..b84a7d01abf4 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -23,6 +23,7 @@
> >  #include <linux/sort.h>
> >  #include <linux/key.h>
> >  #include <linux/verification.h>
> > +#include <linux/namei.h>
> >
> >  #include <net/bpf_sk_storage.h>
> >
> > @@ -2901,3 +2902,233 @@ static u64 bpf_kprobe_multi_entry_ip(struct bpf_run_ctx *ctx)
> >         return 0;
> >  }
> >  #endif
> > +
> > +#ifdef CONFIG_UPROBES
> > +struct bpf_uprobe_multi_link;
> > +
> > +struct bpf_uprobe {
> > +       struct bpf_uprobe_multi_link *link;
> > +       struct inode *inode;
> > +       loff_t offset;
> > +       loff_t ref_ctr_offset;
> 
> you seem to need this only during link creation, so we are wasting 8
> bytes here per each instance of bpf_uprobe for no good reason? You
> should be able to easily move this out of bpf_uprobe into a temporary
> array.

right, we just need offset and inode, good catch, will fix

> 
> > +       struct uprobe_consumer consumer;
> > +};
> > +
> > +struct bpf_uprobe_multi_link {
> > +       struct bpf_link link;
> > +       u32 cnt;
> > +       struct bpf_uprobe *uprobes;
> > +};
> > +
> 
> [...]
> 
> > +       if (prog->expected_attach_type != BPF_TRACE_UPROBE_MULTI)
> > +               return -EINVAL;
> > +
> > +       flags = attr->link_create.uprobe_multi.flags;
> > +       if (flags & ~BPF_F_UPROBE_MULTI_RETURN)
> > +               return -EINVAL;
> > +
> > +       upaths = u64_to_user_ptr(attr->link_create.uprobe_multi.paths);
> > +       uoffsets = u64_to_user_ptr(attr->link_create.uprobe_multi.offsets);
> > +       if (!!upaths != !!uoffsets)
> > +               return -EINVAL;
> 
> when having these as NULL would be ok? cnt == 0? or is there some
> valid situation?

ah nope, that needs to be always != NULL, will fix

> 
> > +
> > +       uref_ctr_offsets = u64_to_user_ptr(attr->link_create.uprobe_multi.ref_ctr_offsets);
> 
> if upaths is NULL, uref_ctr_offsets should be NULL as well?

we need to fail when upaths is NULL, so that should be taken care of

thanks,
jirka