Re: [PATCH v3 bpf-next 08/10] bpf: Support ->fill_link_info for perf_event

[Yafang Shao <laoar.shao@gmail.com>]

On Tue, Jun 13, 2023 at 1:36 AM Yonghong Song <yhs@meta.com> wrote:
>
>
>
> On 6/12/23 8:16 AM, Yafang Shao 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_link_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       |  32 +++++++++++
> >   kernel/bpf/syscall.c           | 124 +++++++++++++++++++++++++++++++++++++++++
> >   tools/include/uapi/linux/bpf.h |  32 +++++++++++
> >   3 files changed, 188 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 23691ea..8d4556e 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -1056,6 +1056,16 @@ enum bpf_link_type {
> >       MAX_BPF_LINK_TYPE,
> >   };
> >
> > +enum bpf_perf_link_type {
> > +     BPF_PERF_LINK_UNSPEC = 0,
> > +     BPF_PERF_LINK_UPROBE = 1,
> > +     BPF_PERF_LINK_KPROBE = 2,
> > +     BPF_PERF_LINK_TRACEPOINT = 3,
> > +     BPF_PERF_LINK_PERF_EVENT = 4,
> > +
> > +     MAX_BPF_LINK_PERF_EVENT_TYPE,
> > +};
> > +
> >   /* cgroup-bpf attach flags used in BPF_PROG_ATTACH command
> >    *
> >    * NONE(default): No further bpf programs allowed in the subtree.
> > @@ -6443,7 +6453,29 @@ struct bpf_link_info {
> >                       __u32 count;
> >                       __u32 flags;
> >               } kprobe_multi;
> > +             struct {
> > +                     __u64 config;
> > +                     __u32 type;
> > +             } perf_event; /* BPF_LINK_PERF_EVENT_PERF_EVENT */
> > +             struct {
> > +                     __aligned_u64 file_name; /* in/out: buff ptr */
> > +                     __u32 name_len;
> > +                     __u32 offset;            /* offset from name */
> > +                     __u32 flags;
> > +             } uprobe; /* BPF_LINK_PERF_EVENT_UPROBE */
> > +             struct {
> > +                     __aligned_u64 func_name; /* in/out: buff ptr */
> > +                     __u32 name_len;
> > +                     __u32 offset;            /* offset from name */
> > +                     __u64 addr;
> > +                     __u32 flags;
> > +             } kprobe; /* BPF_LINK_PERF_EVENT_KPROBE */
> > +             struct {
> > +                     __aligned_u64 tp_name;   /* in/out: buff ptr */
> > +                     __u32 name_len;
> > +             } tracepoint; /* BPF_LINK_PERF_EVENT_TRACEPOINT */
> >       };
> > +     __u32 perf_link_type; /* enum bpf_perf_link_type */
>
> I think put perf_link_type into each indivual struct is better.
> It won't increase the bpf_link_info struct size. It will allow
> extensions for all structs in the big union (raw_tracepoint,
> tracing, cgroup, iter, ..., kprobe_multi, ...) etc.

If we put it into each individual struct, we have to choose one
specific struct to get the type before we use the real struct, for
example,
    if (info.perf_event.type == BPF_PERF_LINK_PERF_EVENT)
              goto out;
    if (info.perf_event.type == BPF_PERF_LINK_TRACEPOINT &&
               !info.tracepoint.tp_name) {
               info.tracepoint.tp_name = (unsigned long)&buf;
               info.tracepoint.name_len = sizeof(buf);
               goto again;
      }
      ...

That doesn't look perfect.

However I agree with you that the perf_link_type may disallow the
extensions for the big union.  I will think about it.

-- 
Regards
Yafang