On Wed, Jul 5, 2023 at 4:47 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> On 6/28/23 1:53 PM, 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_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 | 35 ++++++++++
> > kernel/bpf/syscall.c | 117 +++++++++++++++++++++++++++++++++
> > tools/include/uapi/linux/bpf.h | 35 ++++++++++
> > 3 files changed, 187 insertions(+)
>
> For ease of review this should be squashed with the prior one which adds
> bpf_perf_link_fill_common().
Sure. Will do it.
>
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 512ba3ba2ed3..7efe51672c15 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -1057,6 +1057,16 @@ enum bpf_link_type {
> > MAX_BPF_LINK_TYPE,
> > };
> >
> > +enum bpf_perf_event_type {
> > + BPF_PERF_EVENT_UNSPEC = 0,
> > + BPF_PERF_EVENT_UPROBE = 1,
> > + BPF_PERF_EVENT_URETPROBE = 2,
> > + BPF_PERF_EVENT_KPROBE = 3,
> > + BPF_PERF_EVENT_KRETPROBE = 4,
> > + BPF_PERF_EVENT_TRACEPOINT = 5,
> > + BPF_PERF_EVENT_EVENT = 6,
>
> Why explicitly defining the values of the enum?
With these newly introduced enums, the user can easily identify what
kind of perf_event link it is
See also the discussion:
https://lore.kernel.org/bpf/CAEf4BzYEwCZ3J51pFnUfGykEAHtdLwB8Kxi0utvUTVvewz4UCg@xxxxxxxxxxxxxx/
>
> > +};
> > +
> > /* cgroup-bpf attach flags used in BPF_PROG_ATTACH command
> > *
> > * NONE(default): No further bpf programs allowed in the subtree.
> > @@ -6444,6 +6454,31 @@ struct bpf_link_info {
> > __u32 count;
> > __u32 flags;
> > } kprobe_multi;
> > + struct {
> > + __u32 type; /* enum bpf_perf_event_type */
> > + __u32 :32;
> > + union {
> > + struct {
> > + __aligned_u64 file_name; /* in/out */
> > + __u32 name_len;
> > + __u32 offset;/* offset from file_name */
>
> nit: spacing wrt comment, also same further below
Will change it.
>
> > + } uprobe; /* BPF_PERF_EVENT_UPROBE, BPF_PERF_EVENT_URETPROBE */
> > + struct {
> > + __aligned_u64 func_name; /* in/out */
> > + __u32 name_len;
> > + __u32 offset;/* offset from func_name */
> > + __u64 addr;
> > + } kprobe; /* BPF_PERF_EVENT_KPROBE, BPF_PERF_EVENT_KRETPROBE */
> > + struct {
> > + __aligned_u64 tp_name; /* in/out */
> > + __u32 name_len;
> > + } tracepoint; /* BPF_PERF_EVENT_TRACEPOINT */
> > + struct {
> > + __u64 config;
> > + __u32 type;
> > + } event; /* BPF_PERF_EVENT_EVENT */
> > + };
> > + } perf_event;
> > };
> > } __attribute__((aligned(8)));
> >
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 72de91beabbc..05ff0a560f1a 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -3398,9 +3398,126 @@ static int bpf_perf_link_fill_common(const struct perf_event *event,
> > return 0;
> > }
> >
> > +#ifdef CONFIG_KPROBE_EVENTS
> > +static int bpf_perf_link_fill_kprobe(const struct perf_event *event,
> > + struct bpf_link_info *info)
> > +{
> > + char __user *uname;
> > + u64 addr, offset;
> > + u32 ulen, type;
> > + int err;
> > +
> > + uname = u64_to_user_ptr(info->perf_event.kprobe.func_name);
> > + ulen = info->perf_event.kprobe.name_len;
> > + err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
> > + &type);
> > + if (err)
> > + return err;
> > + if (type == BPF_FD_TYPE_KRETPROBE)
> > + info->perf_event.type = BPF_PERF_EVENT_KRETPROBE;
> > + else
> > + info->perf_event.type = BPF_PERF_EVENT_KPROBE;
> > +
> > + info->perf_event.kprobe.offset = offset;
> > + if (!kallsyms_show_value(current_cred()))
> > + addr = 0;
> > + info->perf_event.kprobe.addr = addr;
> > + return 0;
> > +}
> > +#endif
> > +
> > +#ifdef CONFIG_UPROBE_EVENTS
> > +static int bpf_perf_link_fill_uprobe(const struct perf_event *event,
> > + struct bpf_link_info *info)
> > +{
> > + char __user *uname;
> > + u64 addr, offset;
> > + u32 ulen, type;
> > + int err;
> > +
> > + uname = u64_to_user_ptr(info->perf_event.uprobe.file_name);
> > + ulen = info->perf_event.uprobe.name_len;
> > + err = bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
> > + &type);
> > + if (err)
> > + return err;
> > +
> > + if (type == BPF_FD_TYPE_URETPROBE)
> > + info->perf_event.type = BPF_PERF_EVENT_URETPROBE;
> > + else
> > + info->perf_event.type = BPF_PERF_EVENT_UPROBE;
> > + info->perf_event.uprobe.offset = offset;
> > + return 0;
> > +}
> > +#endif
> > +
> > +static int bpf_perf_link_fill_probe(const struct perf_event *event,
> > + struct bpf_link_info *info)
> > +{
> > +#ifdef CONFIG_KPROBE_EVENTS
> > + if (event->tp_event->flags & TRACE_EVENT_FL_KPROBE)
> > + return bpf_perf_link_fill_kprobe(event, info);
> > +#endif
> > +#ifdef CONFIG_UPROBE_EVENTS
> > + if (event->tp_event->flags & TRACE_EVENT_FL_UPROBE)
> > + return bpf_perf_link_fill_uprobe(event, info);
> > +#endif
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static int bpf_perf_link_fill_tracepoint(const struct perf_event *event,
> > + struct bpf_link_info *info)
> > +{
> > + char __user *uname;
> > + u64 addr, offset;
> > + u32 ulen, type;
> > +
> > + uname = u64_to_user_ptr(info->perf_event.tracepoint.tp_name);
> > + ulen = info->perf_event.tracepoint.name_len;
> > + info->perf_event.type = BPF_PERF_EVENT_TRACEPOINT;
> > + return bpf_perf_link_fill_common(event, uname, ulen, &offset, &addr,
> > + &type);
>
> Perhaps for data we don't care about in these cases, passing NULL would be
> more obvious and letting bpf_perf_link_fill_common() handle NULL inputs.
Agree. That would be better.
We should let bpf_get_perf_event_info() handle NULL inputs. As the
change in bpf_get_perf_event_info() is small, I will change it in the
same patch.
>
> > +}
> > +
> > +static int bpf_perf_link_fill_perf_event(const struct perf_event *event,
> > + struct bpf_link_info *info)
> > +{
> > + info->perf_event.event.type = event->attr.type;
> > + info->perf_event.event.config = event->attr.config;
> > + info->perf_event.type = BPF_PERF_EVENT_EVENT;
> > + return 0;
> > +}
> > +
> > +static int bpf_perf_link_fill_link_info(const struct bpf_link *link,
> > + struct bpf_link_info *info)
> > +{
> > + struct bpf_perf_link *perf_link;
> > + const struct perf_event *event;
> > +
> > + perf_link = container_of(link, struct bpf_perf_link, link);
> > + event = perf_get_event(perf_link->perf_file);
> > + if (IS_ERR(event))
> > + return PTR_ERR(event);
> > +
> > + if (!event->prog)
> > + return -EINVAL;
>
> nit: In which situations do we run into this, would ENOENT be better error code
> here given it's not an invalid arg that user passed to kernel for filling link
> info.
In practice there should be no situations. I think we can remove this
judgement directly.
--
Regards
Yafang
