Re: [PATCH v5 bpf-next 09/11] bpf: Support ->fill_link_info for perf_event

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Jun 23, 2023 at 7:16 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> By introducing support for ->fill_link_info to the perf_event link, users
> gain the ability to inspect it using `bpftool link show`. While the current
> approach involves accessing this information via `bpftool perf show`,
> consolidating link information for all link types in one place offers
> greater convenience. Additionally, this patch extends support to the
> generic perf event, which is not currently accommodated by
> `bpftool perf show`. While only the perf type and config are exposed to
> userspace, other attributes such as sample_period and sample_freq are
> ignored. It's important to note that if kptr_restrict is not permitted, the
> probed address will not be exposed, maintaining security measures.
>
> A new enum bpf_perf_event_type is introduced to help the user understand
> which struct is relevant.
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  include/uapi/linux/bpf.h       |  35 +++++++++++++
>  kernel/bpf/syscall.c           | 115 +++++++++++++++++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h |  35 +++++++++++++
>  3 files changed, 185 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 23691ea..1c579d5 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1056,6 +1056,14 @@ enum bpf_link_type {
>         MAX_BPF_LINK_TYPE,
>  };
>
> +enum bpf_perf_event_type {
> +       BPF_PERF_EVENT_UNSPEC = 0,
> +       BPF_PERF_EVENT_UPROBE = 1,
> +       BPF_PERF_EVENT_KPROBE = 2,
> +       BPF_PERF_EVENT_TRACEPOINT = 3,
> +       BPF_PERF_EVENT_EVENT = 4,
> +};
> +
>  /* cgroup-bpf attach flags used in BPF_PROG_ATTACH command
>   *
>   * NONE(default): No further bpf programs allowed in the subtree.
> @@ -6443,6 +6451,33 @@ struct bpf_link_info {
>                         __u32 count;
>                         __u32 flags;
>                 } kprobe_multi;
> +               struct {
> +                       __u32 type; /* enum bpf_perf_event_type */
> +                       __u32 :32;
> +                       union {
> +                               struct {
> +                                       __aligned_u64 file_name; /* in/out */
> +                                       __u32 name_len;
> +                                       __u32 offset;/* offset from file_name */
> +                                       __u32 flags;
> +                               } uprobe; /* BPF_PERF_EVENT_UPROBE */
> +                               struct {
> +                                       __aligned_u64 func_name; /* in/out */
> +                                       __u32 name_len;
> +                                       __u32 offset;/* offset from func_name */
> +                                       __u64 addr;
> +                                       __u32 flags;
> +                               } kprobe; /* BPF_PERF_EVENT_KPROBE */
> +                               struct {
> +                                       __aligned_u64 tp_name;   /* in/out */
> +                                       __u32 name_len;
> +                               } tracepoint; /* BPF_PERF_EVENT_TRACEPOINT */
> +                               struct {
> +                                       __u64 config;
> +                                       __u32 type;
> +                               } event; /* BPF_PERF_EVENT_EVENT */
> +                       };
> +               } perf_event;
>         };
>  } __attribute__((aligned(8)));
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index c863d39..02dad3c 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3394,9 +3394,124 @@ static int bpf_perf_link_fill_common(const struct perf_event *event,
>         return 0;
>  }
>
> +#ifdef CONFIG_KPROBE_EVENTS
> +static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
> +                                    struct bpf_link_info *info)
> +{
> +       char __user *uname;
> +       u64 addr, offset;
> +       u32 ulen, type;
> +       int err;
> +
> +       uname = u64_to_user_ptr(info->perf_event.kprobe.func_name);
> +       ulen = info->perf_event.kprobe.name_len;
> +       info->perf_event.type = BPF_PERF_EVENT_KPROBE;
> +       err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
> +                                       &type);
> +       if (err)
> +               return err;
> +
> +       info->perf_event.kprobe.offset = offset;
> +       if (type == BPF_FD_TYPE_KRETPROBE)
> +               info->perf_event.kprobe.flags = 1;

hm... ok, sorry, I didn't realize that these flags are not part of
UAPI. I don't think just randomly defining 1 to mean retprobe is a
good approach. Let's drop flags if there are actually no flags.

How about in addition to BPF_PERF_EVENT_UPROBE add
BPF_PERF_EVENT_URETPROBE, and for BPF_PERF_EVENT_KPROBE add also
BPF_PERF_EVENT_KRETPROBE. They will share respective perf_event.uprobe
and perf_event.kprobe sections in bpf_link_info.

It seems consistent with what we did for bpf_task_fd_type enum.

> +       if (!kallsyms_show_value(current_cred()))
> +               return 0;
> +       info->perf_event.kprobe.addr = addr;
> +       return 0;
> +}
> +#endif
> +

[...]