Re: [PATCH 02/10] bpf: Add multi kprobe link

Andrii Nakryiko
2022-03-04T23:11:19Z

On Tue, Feb 22, 2022 at 9:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding new link type BPF_LINK_TYPE_KPROBE_MULTI that attaches kprobe
> program through fprobe API.
>
> The fprobe API allows to attach probe on multiple functions at once
> very fast, because it works on top of ftrace. On the other hand this
> limits the probe point to the function entry or return.
>
> The kprobe program gets the same pt_regs input ctx as when it's attached
> through the perf API.
>
> Adding new attach type BPF_TRACE_KPROBE_MULTI that allows attachment
> kprobe to multiple function with new link.
>
> User provides array of addresses or symbols with count to attach the
> kprobe program to. The new link_create uapi interface looks like:
>
> struct {
> __aligned_u64 syms;
> __aligned_u64 addrs;
> __u32 cnt;
> __u32 flags;
> } kprobe_multi;
>
> The flags field allows single BPF_TRACE_KPROBE_MULTI bit to create
> return multi kprobe.
>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

LGTM.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> include/linux/bpf_types.h | 1 +
> include/linux/trace_events.h | 6 +
> include/uapi/linux/bpf.h | 13 ++
> kernel/bpf/syscall.c | 26 +++-
> kernel/trace/bpf_trace.c | 211 +++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 13 ++
> 6 files changed, 265 insertions(+), 5 deletions(-)
>

[...]

> +
> +static int
> +kprobe_multi_resolve_syms(const void *usyms, u32 cnt,
> + unsigned long *addrs)
> +{
> + unsigned long addr, size;
> + const char **syms;
> + int err = -ENOMEM;
> + unsigned int i;
> + char *func;
> +
> + size = cnt * sizeof(*syms);
> + syms = kvzalloc(size, GFP_KERNEL);
> + if (!syms)
> + return -ENOMEM;
> +
> + func = kmalloc(KSYM_NAME_LEN, GFP_KERNEL);
> + if (!func)
> + goto error;
> +
> + if (copy_from_user(syms, usyms, size)) {
> + err = -EFAULT;
> + goto error;
> + }
> +
> + for (i = 0; i < cnt; i++) {
> + err = strncpy_from_user(func, syms[i], KSYM_NAME_LEN);
> + if (err == KSYM_NAME_LEN)
> + err = -E2BIG;
> + if (err < 0)
> + goto error;
> +
> + err = -EINVAL;
> + if (func[0] == '\0')
> + goto error;

wouldn't empty string be handled by kallsyms_lookup_name?

> + addr = kallsyms_lookup_name(func);
> + if (!addr)
> + goto error;
> + if (!kallsyms_lookup_size_offset(addr, &size, NULL))
> + size = MCOUNT_INSN_SIZE;
> + addr = ftrace_location_range(addr, addr + size - 1);
> + if (!addr)
> + goto error;
> + addrs[i] = addr;
> + }
> +
> + err = 0;
> +error:
> + kvfree(syms);
> + kfree(func);
> + return err;
> +}
> + …