On Sat, Jun 17, 2023 at 4:36 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Jun 13, 2023 at 7:46 PM Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
> >
> > On Wed, Jun 14, 2023 at 10:34 AM Kui-Feng Lee <sinquersw@xxxxxxxxx> wrote:
> > >
> > >
> > >
> > > On 6/12/23 19:47, Yafang Shao wrote:
> > > > On Tue, Jun 13, 2023 at 1:36 AM Yonghong Song <yhs@xxxxxxxx> 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@xxxxxxxxx>
> > > >>> ---
> > > >>> 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
>
> > 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
