Re: [PATCH bpf-next 4/5] bpf: Attach a cookie to a BPF program.

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

On Wed, Jan 26, 2022 at 1:48 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> Extend bpf() to attach a tracing program with a cookie by adding a
> bpf_cookie field to bpf_attr for raw tracepoints.
>
> Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> ---
>  include/linux/bpf.h            |  1 +
>  include/uapi/linux/bpf.h       |  1 +
>  kernel/bpf/syscall.c           | 12 ++++++++----
>  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 +
>  7 files changed, 27 insertions(+), 4 deletions(-)
>

We normally split out kernel, libbpf, samples/bpf, selftests/bpf,
bpftool, etc changes into separate patches, if possible. Please do
that in the next revision.

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 37353745fee5..d5196514e9bd 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1004,6 +1004,7 @@ struct bpf_prog_aux {
>                 struct work_struct work;
>                 struct rcu_head rcu;
>         };
> +       u64 cookie;
>  };
>
>  struct bpf_array_aux {
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 16a7574292a5..3fa27346ab4b 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1425,6 +1425,7 @@ union bpf_attr {
>         struct { /* anonymous struct used by BPF_RAW_TRACEPOINT_OPEN command */
>                 __u64 name;
>                 __u32 prog_fd;
> +               __u64 bpf_cookie;
>         } raw_tracepoint;
>

As an aside, Alexei, should we bite a bullet and allow attaching
raw_tp, fentry/fexit, and other tracing prog attachment through the
LINK_CREATE command? BPF_RAW_TRACEPOINT_OPEN makes little sense for
anything but raw_tp programs. Libbpf can do feature detection and
route between RAW_TRACEPOINT_OPEN and LINK_CREATE commands depending
on whether bpf_cookie and whatever other newer things we add, so that
it's compatible with old kernels. As we also add multi-attach
fentry/fexit, it would be nice to keep everything in LINK_CREATE
section of bpf_attr, similar to multi-attach kprobe case. WDYT?


>         struct { /* anonymous struct for BPF_BTF_LOAD */
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 72ce1edde950..79d057918c76 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2696,7 +2696,8 @@ static const struct bpf_link_ops bpf_tracing_link_lops = {
>
>  static int bpf_tracing_prog_attach(struct bpf_prog *prog,
>                                    int tgt_prog_fd,
> -                                  u32 btf_id)
> +                                  u32 btf_id,
> +                                  u64 bpf_cookie)
>  {
>         struct bpf_link_primer link_primer;
>         struct bpf_prog *tgt_prog = NULL;
> @@ -2832,6 +2833,8 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
>         if (err)
>                 goto out_unlock;
>
> +       prog->aux->cookie = bpf_cookie;
> +
>         err = bpf_trampoline_link_prog(prog, tr);
>         if (err) {
>                 bpf_link_cleanup(&link_primer);
> @@ -3017,7 +3020,7 @@ static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *pro
>  }
>  #endif /* CONFIG_PERF_EVENTS */
>
> -#define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD raw_tracepoint.prog_fd
> +#define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD raw_tracepoint.bpf_cookie
>
>  static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
>  {
> @@ -3052,7 +3055,7 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
>                         tp_name = prog->aux->attach_func_name;
>                         break;
>                 }
> -               err = bpf_tracing_prog_attach(prog, 0, 0);
> +               err = bpf_tracing_prog_attach(prog, 0, 0, attr->raw_tracepoint.bpf_cookie);
>                 if (err >= 0)
>                         return err;
>                 goto out_put_prog;
> @@ -4244,7 +4247,8 @@ static int tracing_bpf_link_attach(const union bpf_attr *attr, bpfptr_t uattr,
>         else if (prog->type == BPF_PROG_TYPE_EXT)
>                 return bpf_tracing_prog_attach(prog,
>                                                attr->link_create.target_fd,
> -                                              attr->link_create.target_btf_id);
> +                                              attr->link_create.target_btf_id,
> +                                              0);
>         return -EINVAL;
>  }
>
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 16a7574292a5..3fa27346ab4b 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -1425,6 +1425,7 @@ union bpf_attr {
>         struct { /* anonymous struct used by BPF_RAW_TRACEPOINT_OPEN command */
>                 __u64 name;
>                 __u32 prog_fd;
> +               __u64 bpf_cookie;
>         } raw_tracepoint;
>
>         struct { /* anonymous struct for BPF_BTF_LOAD */
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 418b259166f8..c28b017de515 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -1131,6 +1131,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)
> +{
> +       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 c2e8327010f9..c3d2c6a4cb15 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -475,6 +475,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 e10f0822845a..05af5bb8de92 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -432,6 +432,7 @@ LIBBPF_0.7.0 {
>                 bpf_xdp_detach;
>                 bpf_xdp_query;
>                 bpf_xdp_query_id;
> +               bpf_raw_tracepoint_cookie_open;
>                 libbpf_probe_bpf_helper;
>                 libbpf_probe_bpf_map_type;
>                 libbpf_probe_bpf_prog_type;
> --
> 2.30.2
>