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 :)
> + * 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?
> 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.
> + 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?
> +
> + 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?
> +
> + cnt = attr->link_create.uprobe_multi.cnt;
> + if (!cnt)
> + return -EINVAL;
> +
> + uprobes = kvcalloc(cnt, sizeof(*uprobes), GFP_KERNEL);
> + if (!uprobes)
> + return -ENOMEM;
> +
[...]
