Re: [PATCH bpf-next v2 3/4] bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.

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

On Fri, Mar 18, 2022 at 12:13 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Mar 15, 2022 at 05:42:30PM -0700, Kui-Feng Lee wrote:
> > Add a bpf_cookie field to attach a cookie to an instance of struct
> > bpf_link.  The cookie of a bpf_link will be installed when calling the
> > associated program to make it available to the program.
> >
> > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > ---
> >  arch/x86/net/bpf_jit_comp.c    |  4 ++--
> >  include/linux/bpf.h            |  1 +
> >  include/uapi/linux/bpf.h       |  1 +
> >  kernel/bpf/syscall.c           | 11 +++++++----
> >  kernel/trace/bpf_trace.c       | 17 +++++++++++++++++
> >  tools/include/uapi/linux/bpf.h |  1 +
> >  tools/lib/bpf/bpf.c            | 14 ++++++++++++++
> >  tools/lib/bpf/bpf.h            |  1 +
> >  tools/lib/bpf/libbpf.map       |  1 +
> >  9 files changed, 45 insertions(+), 6 deletions(-)
>
> please split kernel and libbpf changes into two different patches.
>

+1

> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > index f69ce3a01385..dbbf09c84c21 100644
> > --- a/tools/lib/bpf/bpf.c
> > +++ b/tools/lib/bpf/bpf.c
> > @@ -1133,6 +1133,20 @@ int bpf_raw_tracepoint_open(const char *name, int prog_fd)
> >       return libbpf_err_errno(fd);
> >  }
> >
> > +int bpf_raw_tracepoint_cookie_open(const char *name, int prog_fd, __u64 bpf_cookie)
>
> lets introduce opts style to raw_tp_open instead.

I remember I brought this up earlier, but I forgot the outcome. What
if don't touch BPF_RAW_TRACEPOINT_OPEN and instead allow to create all
the same links through more universal BPF_LINK_CREATE command. And
only there we add bpf_cookie? There are few advantages:

1. We can separate raw_tracepoint and trampoline-based programs more
cleanly in UAPI (it will be two separate structs: link_create.raw_tp
with raw tracepoint name vs link_create.trampoline, or whatever the
name, with cookie and stuff). Remember that raw_tp won't support
bpf_cookie for now, so it would be another advantage not to promise
cookie in UAPI.

2. libbpf can be smart enough to pick either RAW_TRACEPOINT_OPEN (and
reject it if bpf_cookie is non-zero) or BPF_LINK_CREATE, depending on
kernel support. So users would need to only use bpf_link_create()
moving forward with all the backwards compatibility preserved.


>
> > +{
> > +     union bpf_attr attr;
> > +     int fd;
> > +
> > +     memset(&attr, 0, sizeof(attr));
> > +     attr.raw_tracepoint.name = ptr_to_u64(name);
> > +     attr.raw_tracepoint.prog_fd = prog_fd;
> > +     attr.raw_tracepoint.bpf_cookie = bpf_cookie;
> > +
> > +     fd = sys_bpf_fd(BPF_RAW_TRACEPOINT_OPEN, &attr, sizeof(attr));
> > +     return libbpf_err_errno(fd);
> > +}
> > +
> >  int bpf_btf_load(const void *btf_data, size_t btf_size, const struct bpf_btf_load_opts *opts)
> >  {
> >       const size_t attr_sz = offsetofend(union bpf_attr, btf_log_level);
> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > index 5253cb4a4c0a..23bebcdaf23b 100644
> > --- a/tools/lib/bpf/bpf.h
> > +++ b/tools/lib/bpf/bpf.h
> > @@ -477,6 +477,7 @@ LIBBPF_API int bpf_prog_query(int target_fd, enum bpf_attach_type type,
> >                             __u32 query_flags, __u32 *attach_flags,
> >                             __u32 *prog_ids, __u32 *prog_cnt);
> >  LIBBPF_API int bpf_raw_tracepoint_open(const char *name, int prog_fd);
> > +LIBBPF_API int bpf_raw_tracepoint_cookie_open(const char *name, int prog_fd, __u64 bpf_cookie);
> >  LIBBPF_API int bpf_task_fd_query(int pid, int fd, __u32 flags, char *buf,
> >                                __u32 *buf_len, __u32 *prog_id, __u32 *fd_type,
> >                                __u64 *probe_offset, __u64 *probe_addr);
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index df1b947792c8..20f947a385fa 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -434,6 +434,7 @@ LIBBPF_0.7.0 {
> >               bpf_xdp_detach;
> >               bpf_xdp_query;
> >               bpf_xdp_query_id;
> > +             bpf_raw_tracepoint_cookie_open;

this (if still needed) should go into 0.8.0 section

> >               libbpf_probe_bpf_helper;
> >               libbpf_probe_bpf_map_type;
> >               libbpf_probe_bpf_prog_type;
> > --
> > 2.30.2
> >
>
> --