diff mbox series

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

Message ID 20220126214809.3868787-5-kuifeng@fb.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series Attach a cookie to a tracing program. | expand

Checks

Context Check Description
bpf/vmtest-bpf-next fail VM_Test
bpf/vmtest-bpf-next-PR fail PR summary
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1808 this patch: 1808
netdev/cc_maintainers warning 7 maintainers not CCed: joe@cilium.io kpsingh@kernel.org john.fastabend@gmail.com kafai@fb.com songliubraving@fb.com yhs@fb.com netdev@vger.kernel.org
netdev/build_clang success Errors and warnings before: 218 this patch: 218
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1825 this patch: 1825
netdev/checkpatch warning WARNING: line length of 83 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns WARNING: line length of 95 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 8 this patch: 8
netdev/source_inline success Was 0 now: 0

Commit Message

Kui-Feng Lee Jan. 26, 2022, 9:48 p.m. UTC
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(-)

Comments

Andrii Nakryiko Feb. 1, 2022, 6:46 a.m. UTC | #1
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
>
Alexei Starovoitov Feb. 1, 2022, 8:17 p.m. UTC | #2
On Mon, Jan 31, 2022 at 10:47 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> >  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.

raw_tp_open is used for raw_tp, tp_btf, lsm, fentry.
iirc it's creating a normal bpf_link underneath.
link_create doesn't exist for raw_tp and friends,
so this is the best place to add a cookie.
We can add an alias cmd (instead of raw_tp_open)
to make it a bit cleaner from uapi naming pov.
We can allow link_create to do the attach in all those cases as well,
but it's a different discussion.
link_create.perf_event.bpf_cookie isn't the best name.
That name was a cause of confusion for me.
I thought it applies to perf_event only,
but it's for kuprobe too.
So plenty of bikeshedding to do if we decide to do
link_create for raw_tp. Hence, for now, I'd add a cookie to
raw_tp/tp_btf/lsm/fentry like this patch is doing.
Andrii Nakryiko Feb. 2, 2022, 1:24 a.m. UTC | #3
On Tue, Feb 1, 2022 at 12:17 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Jan 31, 2022 at 10:47 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > >  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.
>
> raw_tp_open is used for raw_tp, tp_btf, lsm, fentry.
> iirc it's creating a normal bpf_link underneath.
> link_create doesn't exist for raw_tp and friends,
> so this is the best place to add a cookie.
> We can add an alias cmd (instead of raw_tp_open)
> to make it a bit cleaner from uapi naming pov.
> We can allow link_create to do the attach in all those cases as well,
> but it's a different discussion.

I was actually proposing exactly the latter: to allow LINK_CREATE to
create all the programs that RAW_TRACEPOINT_OPEN allows. It's already
confusing because bpf_iter programs are created using LINK_CREATE
(realized that when I saw your recent patches). Also extension
programs are attached through LINK_CREATE. So while we can't get rid
of BPF_RAW_TRACEPOINT_OPEN, I hoped we can add lsm and fentry support
as well (I don't mind raw_tp/tp_btf there as well for completeness),
so at least in the future it would be we all just a universal
LINK_CREATE command.

> link_create.perf_event.bpf_cookie isn't the best name.
> That name was a cause of confusion for me.
> I thought it applies to perf_event only,
> but it's for kuprobe too.

Yeah, not great, but given it was "attach to perf_event FD" command,
it seemed like the most accurate name at the time :) I still don't
know what I'd call it today, apart from having separate (and
duplicate) link_create.kprobe.bpf_cookie,
link_create.uprobe.bpf_cookie, etc. At least libbpf is hiding it
behind bpf_program__attach_kprobe and bpf_program__attach_uprobe,
though.


> So plenty of bikeshedding to do if we decide to do
> link_create for raw_tp. Hence, for now, I'd add a cookie to
> raw_tp/tp_btf/lsm/fentry like this patch is doing.

Sure, that's fine, it was a long shot anyway. But I'd like to get back
to this discussion when we are going to multi-attach fentry/fexit :)
diff mbox series

Patch

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;
 
 	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;