On Sat, Jun 17, 2023 at 4:36 AM Andrii Nakryiko
[off-list ref] wrote:

On Tue, Jun 13, 2023 at 7:46 PM Yafang Shao [off-list ref] wrote:

quoted

On Wed, Jun 14, 2023 at 10:34 AM Kui-Feng Lee [off-list ref] wrote:

quoted

On 6/12/23 19:47, Yafang Shao wrote:

quoted

On Tue, Jun 13, 2023 at 1:36 AM Yonghong Song [off-list ref] wrote:

quoted

On 6/12/23 8:16 AM, Yafang Shao wrote:

quoted

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 <redacted>
---
   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.

I perfer below struct,

+1, we should do it this way

quoted

                struct {
                        __u32 type; /* enum bpf_perf_link_type */
                        union {
                                struct {
                                        __u64 config;
                                        __u32 type;
                                } perf_event; /* BPF_PERF_LINK_PERF_EVENT */
                                struct {
                                        __aligned_u64 file_name; /* in/out */
                                        __u32 name_len;
                                        __u32 offset;/* offset from file_name */
                                        __u32 flags;
                                } uprobe; /* BPF_PERF_LINK_UPROBE */
                                struct {
                                        __aligned_u64 func_name; /* in/out */
                                        __u32 name_len;
                                        __u32 offset;/* offset from func_name */
                                        __u64 addr;
                                        __u32 flags;
                                } kprobe; /* BPF_PERF_LINK_KPROBE */
                                struct {
                                        __aligned_u64 tp_name;   /* in/out */
                                        __u32 name_len;
                                } tracepoint; /* BPF_PERF_LINK_TRACEPOINT */
                        };
                } perf_link;

this should be named "perf_event" to match BPF_LINK_TYPE_PERF_EVENT

and "perf_event" above probably could be just "event" then? Similarly
we can s/BPF_PERF_LINK_PERF_EVENT/BPF_PERF_LINK_EVENT/?

Agree. Will change it.

-- 
Regards
Yafang