| 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 |
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 >
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.
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 --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;
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(-)