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

[Kui-Feng Lee <sinquersw@gmail.com>]

On 6/12/23 19:47, Yafang Shao wrote:
> 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.

How about adding a common struct?

  struct {
        __u32 type;
  } perf_common;

Then you check info.perf_common.type.


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