Re: [PATCH v2 bpf-next 04/10] bpf: add support for BPF_OBJ_GET_INFO_BY_FD for bpf_link

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

On Tue, Apr 28, 2020 at 2:46 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andriin@fb.com> writes:
>
> > Add ability to fetch bpf_link details through BPF_OBJ_GET_INFO_BY_FD command.
> > Also enhance show_fdinfo to potentially include bpf_link type-specific
> > information (similarly to obj_info).
> >
> > Also introduce enum bpf_link_type stored in bpf_link itself and expose it in
> > UAPI. bpf_link_tracing also now will store and return bpf_attach_type.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >  include/linux/bpf-cgroup.h     |   2 -
> >  include/linux/bpf.h            |  10 +-
> >  include/linux/bpf_types.h      |   6 ++
> >  include/uapi/linux/bpf.h       |  28 ++++++
> >  kernel/bpf/btf.c               |   2 +
> >  kernel/bpf/cgroup.c            |  45 ++++++++-
> >  kernel/bpf/syscall.c           | 164 +++++++++++++++++++++++++++++----
> >  kernel/bpf/verifier.c          |   2 +
> >  tools/include/uapi/linux/bpf.h |  31 +++++++
> >  9 files changed, 266 insertions(+), 24 deletions(-)
> >
> > diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> > index d2d969669564..ab95824a1d99 100644
> > --- a/include/linux/bpf-cgroup.h
> > +++ b/include/linux/bpf-cgroup.h
> > @@ -57,8 +57,6 @@ struct bpf_cgroup_link {
> >       enum bpf_attach_type type;
> >  };
> >
> > -extern const struct bpf_link_ops bpf_cgroup_link_lops;
> > -
> >  struct bpf_prog_list {
> >       struct list_head node;
> >       struct bpf_prog *prog;
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 875d1f0af803..701c4387c711 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1026,9 +1026,11 @@ extern const struct file_operations bpf_prog_fops;
> >       extern const struct bpf_verifier_ops _name ## _verifier_ops;
> >  #define BPF_MAP_TYPE(_id, _ops) \
> >       extern const struct bpf_map_ops _ops;
> > +#define BPF_LINK_TYPE(_id, _name)
> >  #include <linux/bpf_types.h>
> >  #undef BPF_PROG_TYPE
> >  #undef BPF_MAP_TYPE
> > +#undef BPF_LINK_TYPE
> >
> >  extern const struct bpf_prog_ops bpf_offload_prog_ops;
> >  extern const struct bpf_verifier_ops tc_cls_act_analyzer_ops;
> > @@ -1086,6 +1088,7 @@ int bpf_prog_new_fd(struct bpf_prog *prog);
> >  struct bpf_link {
> >       atomic64_t refcnt;
> >       u32 id;
> > +     enum bpf_link_type type;
> >       const struct bpf_link_ops *ops;
> >       struct bpf_prog *prog;
> >       struct work_struct work;
> > @@ -1103,9 +1106,14 @@ struct bpf_link_ops {
> >       void (*dealloc)(struct bpf_link *link);
> >       int (*update_prog)(struct bpf_link *link, struct bpf_prog *new_prog,
> >                          struct bpf_prog *old_prog);
> > +     void (*show_fdinfo)(const struct bpf_link *link, struct seq_file *seq);
> > +     int (*fill_link_info)(const struct bpf_link *link,
> > +                           struct bpf_link_info *info,
> > +                           const struct bpf_link_info *uinfo,
> > +                           u32 info_len);
> >  };
> >
> > -void bpf_link_init(struct bpf_link *link,
> > +void bpf_link_init(struct bpf_link *link, enum bpf_link_type type,
> >                  const struct bpf_link_ops *ops, struct bpf_prog *prog);
> >  int bpf_link_prime(struct bpf_link *link, struct bpf_link_primer *primer);
> >  int bpf_link_settle(struct bpf_link_primer *primer);
> > diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> > index ba0c2d56f8a3..8345cdf553b8 100644
> > --- a/include/linux/bpf_types.h
> > +++ b/include/linux/bpf_types.h
> > @@ -118,3 +118,9 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_STACK, stack_map_ops)
> >  #if defined(CONFIG_BPF_JIT)
> >  BPF_MAP_TYPE(BPF_MAP_TYPE_STRUCT_OPS, bpf_struct_ops_map_ops)
> >  #endif
> > +
> > +BPF_LINK_TYPE(BPF_LINK_TYPE_RAW_TRACEPOINT, raw_tracepoint)
> > +BPF_LINK_TYPE(BPF_LINK_TYPE_TRACING, tracing)
> > +#ifdef CONFIG_CGROUP_BPF
> > +BPF_LINK_TYPE(BPF_LINK_TYPE_CGROUP, cgroup)
> > +#endif
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 7e6541fceade..0eccafae55bb 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -222,6 +222,15 @@ enum bpf_attach_type {
> >
> >  #define MAX_BPF_ATTACH_TYPE __MAX_BPF_ATTACH_TYPE
> >
> > +enum bpf_link_type {
> > +     BPF_LINK_TYPE_UNSPEC = 0,
> > +     BPF_LINK_TYPE_RAW_TRACEPOINT = 1,
> > +     BPF_LINK_TYPE_TRACING = 2,
> > +     BPF_LINK_TYPE_CGROUP = 3,
> > +
> > +     MAX_BPF_LINK_TYPE,
> > +};
> > +
> >  /* cgroup-bpf attach flags used in BPF_PROG_ATTACH command
> >   *
> >   * NONE(default): No further bpf programs allowed in the subtree.
> > @@ -3612,6 +3621,25 @@ struct bpf_btf_info {
> >       __u32 id;
> >  } __attribute__((aligned(8)));
> >
> > +struct bpf_link_info {
> > +     __u32 type;
> > +     __u32 id;
> > +     __u32 prog_id;
> > +     union {
> > +             struct {
> > +                     __aligned_u64 tp_name; /* in/out: tp_name buffer ptr */
> > +                     __u32 tp_name_len;     /* in/out: tp_name buffer len */
> > +             } raw_tracepoint;
> > +             struct {
> > +                     __u32 attach_type;
> > +             } tracing;
>
> On the RFC I asked whether we could get the attach target here as well.
> You said you'd look into it; what happened to that? :)
>

Right, sorry, forgot to mention this. I did take a look, but tracing
links are quite diverse, so I didn't see one clear way to structure
such "target" information, so I'd say we should push it into a
separate patch/discussion. E.g., fentry/fexit/fmod_exit are attached
to kernel functions (how do we represent that), freplace are attached
to another BPF program (this is a bit clearer how to represent, but
how do we combine that with fentry/fexit info?). LSM is also attached
to kernel function, but who knows, maybe we want slightly more
extended semantics for it. Either way, I don't see one best way to
structure this information and would want to avoid blocking on this
for this series. Also bpf_link_info is extensible, so it's not a
problem to extend it in follow up patches.

Does it make sense?

> -Toke
>