Re: [PATCH v2 bpf-next 08/11] bpf: Support ->fill_link_info for perf_event

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

On Fri, Jun 9, 2023 at 2:57 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Fri, Jun 9, 2023 at 5:53 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > On Fri, Jun 9, 2023 at 7:12 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Thu, Jun 8, 2023 at 3:35 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 set to 2, the
> > > > probed address will not be exposed, maintaining security measures.
> > > >
> > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > > ---
> > > >  include/uapi/linux/bpf.h       | 22 ++++++++++
> > > >  kernel/bpf/syscall.c           | 98 ++++++++++++++++++++++++++++++++++++++++++
> > > >  tools/include/uapi/linux/bpf.h | 22 ++++++++++
> > > >  3 files changed, 142 insertions(+)
> > > >
> > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > index d99cc16..c3b821d 100644
> > > > --- a/include/uapi/linux/bpf.h
> > > > +++ b/include/uapi/linux/bpf.h
> > > > @@ -6443,6 +6443,28 @@ struct bpf_link_info {
> > > >                         __u32 count;
> > > >                         __u8  retprobe;
> > > >                 } kprobe_multi;
> > > > +               union {
> > > > +                       struct {
> > > > +                               /* The name is:
> > > > +                                * a) uprobe: file name
> > > > +                                * b) kprobe: kernel function
> > > > +                                */
> > > > +                               __aligned_u64 name; /* in/out: name buffer ptr */
> > > > +                               __u32 name_len;
> > > > +                               __u32 offset;   /* offset from the name */
> > > > +                               __u64 addr;
> > > > +                               __u8 retprobe;
> > > > +                       } probe; /* uprobe, kprobe */
> > > > +                       struct {
> > > > +                               /* in/out: tracepoint name buffer ptr */
> > > > +                               __aligned_u64 tp_name;
> > > > +                               __u32 name_len;
> > > > +                       } tp; /* tracepoint */
> > > > +                       struct {
> > > > +                               __u64 config;
> > > > +                               __u32 type;
> > > > +                       } event; /* generic perf event */
> > >
> > > how should the user know which of those structs are relevant? we need
> > > some enum to specify what kind of perf_event link it is?
> > >
> >
> > Do you mean that we add a new field 'type' into the union perf_event,
> > as follows ?
> >     union {
> >         __u32 type;
> >         struct {} probe;  /* BPF_LINK_TYPE_PERF_EVENT_PROBE */
> >         struct {} tp; /* BPF_LINK_TYPE_PERF_EVENT_TP */
> >         struct {} event; /* BPF_LINK_TYPE_PERF_EVENT_EVENT */
> >     };
> >
>
> Correct it:
>
> struct {
>     __u32 type;
>     union {
>          struct {} probe;  /* BPF_LINK_TYPE_PERF_EVENT_PROBE */
>          struct {} tp; /* BPF_LINK_TYPE_PERF_EVENT_TP */
>          struct {} event; /* BPF_LINK_TYPE_PERF_EVENT_EVENT */
>      };
> } perf_event;

yes, something like this. Unless we want to leave  perf_event {} to
mean really perf event only, while kprobe/uprobe/tracepoint should be
their own separate sections at the same level of nestedness as
perf_Event and other cases. Not sure.

>
> --
> Regards
> Yafang