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