| Message ID | 160017006352.98230.621859348254499900.stgit@toke.dk |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Tue, Sep 15, 2020 at 5:50 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > From: Toke Høiland-Jørgensen <toke@redhat.com> > > This adds support for supplying a target btf ID for the bpf_link_create() > operation, and adds a new bpf_program__attach_freplace() high-level API for > attaching freplace functions with a target. > > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- > tools/lib/bpf/bpf.c | 1 + > tools/lib/bpf/bpf.h | 3 ++- > tools/lib/bpf/libbpf.c | 24 ++++++++++++++++++------ > tools/lib/bpf/libbpf.h | 3 +++ > tools/lib/bpf/libbpf.map | 1 + > 5 files changed, 25 insertions(+), 7 deletions(-) > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > index 82b983ff6569..e928456c0dd6 100644 > --- a/tools/lib/bpf/bpf.c > +++ b/tools/lib/bpf/bpf.c > @@ -599,6 +599,7 @@ int bpf_link_create(int prog_fd, int target_fd, > attr.link_create.iter_info = > ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0)); > attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0); > + attr.link_create.target_btf_id = OPTS_GET(opts, target_btf_id, 0); > > return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr)); > } > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h > index 015d13f25fcc..f8dbf666b62b 100644 > --- a/tools/lib/bpf/bpf.h > +++ b/tools/lib/bpf/bpf.h > @@ -174,8 +174,9 @@ struct bpf_link_create_opts { > __u32 flags; > union bpf_iter_link_info *iter_info; > __u32 iter_info_len; > + __u32 target_btf_id; > }; > -#define bpf_link_create_opts__last_field iter_info_len > +#define bpf_link_create_opts__last_field target_btf_id > > LIBBPF_API int bpf_link_create(int prog_fd, int target_fd, > enum bpf_attach_type attach_type, > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 550950eb1860..165131c73f40 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -9322,12 +9322,14 @@ static struct bpf_link *attach_iter(const struct bpf_sec_def *sec, > > static struct bpf_link * > bpf_program__attach_fd(struct bpf_program *prog, int target_fd, > - const char *target_name) > + int target_btf_id, const char *target_name) > { > enum bpf_attach_type attach_type; > char errmsg[STRERR_BUFSIZE]; > struct bpf_link *link; > int prog_fd, link_fd; > + DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts, > + .target_btf_id = target_btf_id); > > prog_fd = bpf_program__fd(prog); > if (prog_fd < 0) { > @@ -9340,8 +9342,12 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd, > return ERR_PTR(-ENOMEM); > link->detach = &bpf_link__detach_fd; > > - attach_type = bpf_program__get_expected_attach_type(prog); > - link_fd = bpf_link_create(prog_fd, target_fd, attach_type, NULL); > + if (bpf_program__get_type(prog) == BPF_PROG_TYPE_EXT) > + attach_type = BPF_TRACE_FREPLACE; doing this unconditionally will break an old-style freplace without target_fd/btf_id on older kernels. Safe and simple way would be to continue using raw_tracepoint_open when there is no target_fd/btf_id, and use LINK_CREATE for newer stuff. Alternatively, you'd need to do feature detection, but it's still would be nice to handle old-style attach through raw_tracepoint_open for bpf_program__attach_freplace. so I suggest leaving bpf_program__attach_fd() as is and to create a custom bpf_program__attach_freplace() implementation. > + else > + attach_type = bpf_program__get_expected_attach_type(prog); > + > + link_fd = bpf_link_create(prog_fd, target_fd, attach_type, &opts); > if (link_fd < 0) { > link_fd = -errno; > free(link); > @@ -9357,19 +9363,25 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd, > struct bpf_link * > bpf_program__attach_cgroup(struct bpf_program *prog, int cgroup_fd) > { > - return bpf_program__attach_fd(prog, cgroup_fd, "cgroup"); > + return bpf_program__attach_fd(prog, cgroup_fd, 0, "cgroup"); > } > > struct bpf_link * > bpf_program__attach_netns(struct bpf_program *prog, int netns_fd) > { > - return bpf_program__attach_fd(prog, netns_fd, "netns"); > + return bpf_program__attach_fd(prog, netns_fd, 0, "netns"); > } > > struct bpf_link *bpf_program__attach_xdp(struct bpf_program *prog, int ifindex) > { > /* target_fd/target_ifindex use the same field in LINK_CREATE */ > - return bpf_program__attach_fd(prog, ifindex, "xdp"); > + return bpf_program__attach_fd(prog, ifindex, 0, "xdp"); > +} > + > +struct bpf_link *bpf_program__attach_freplace(struct bpf_program *prog, > + int target_fd, int target_btf_id) > +{ > + return bpf_program__attach_fd(prog, target_fd, target_btf_id, "freplace"); > } > > struct bpf_link * > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index a750f67a23f6..ce5add9b9203 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -261,6 +261,9 @@ LIBBPF_API struct bpf_link * > bpf_program__attach_netns(struct bpf_program *prog, int netns_fd); > LIBBPF_API struct bpf_link * > bpf_program__attach_xdp(struct bpf_program *prog, int ifindex); > +LIBBPF_API struct bpf_link * > +bpf_program__attach_freplace(struct bpf_program *prog, > + int target_fd, int target_btf_id); maybe a const char * function name instead of target_btf_id would be a nicer API? Users won't have to deal with fetching target prog's BTF, searching it, etc. That's all pretty straightforward for libbpf to do, leaving users with more natural and simpler API. > > struct bpf_map; > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > index 92ceb48a5ca2..3cc2c42f435d 100644 > --- a/tools/lib/bpf/libbpf.map > +++ b/tools/lib/bpf/libbpf.map > @@ -302,6 +302,7 @@ LIBBPF_0.1.0 { > > LIBBPF_0.2.0 { > global: > + bpf_program__attach_freplace; > bpf_program__section_name; > perf_buffer__buffer_cnt; > perf_buffer__buffer_fd; >
On Wed, Sep 16, 2020 at 1:37 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Sep 15, 2020 at 5:50 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > > > From: Toke Høiland-Jørgensen <toke@redhat.com> > > > > This adds support for supplying a target btf ID for the bpf_link_create() > > operation, and adds a new bpf_program__attach_freplace() high-level API for > > attaching freplace functions with a target. > > > > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > > --- > > tools/lib/bpf/bpf.c | 1 + > > tools/lib/bpf/bpf.h | 3 ++- > > tools/lib/bpf/libbpf.c | 24 ++++++++++++++++++------ > > tools/lib/bpf/libbpf.h | 3 +++ > > tools/lib/bpf/libbpf.map | 1 + > > 5 files changed, 25 insertions(+), 7 deletions(-) > > > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > > index 82b983ff6569..e928456c0dd6 100644 > > --- a/tools/lib/bpf/bpf.c > > +++ b/tools/lib/bpf/bpf.c > > @@ -599,6 +599,7 @@ int bpf_link_create(int prog_fd, int target_fd, > > attr.link_create.iter_info = > > ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0)); > > attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0); > > + attr.link_create.target_btf_id = OPTS_GET(opts, target_btf_id, 0); > > > > return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr)); > > } > > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h > > index 015d13f25fcc..f8dbf666b62b 100644 > > --- a/tools/lib/bpf/bpf.h > > +++ b/tools/lib/bpf/bpf.h > > @@ -174,8 +174,9 @@ struct bpf_link_create_opts { > > __u32 flags; > > union bpf_iter_link_info *iter_info; > > __u32 iter_info_len; > > + __u32 target_btf_id; > > }; > > -#define bpf_link_create_opts__last_field iter_info_len > > +#define bpf_link_create_opts__last_field target_btf_id > > > > LIBBPF_API int bpf_link_create(int prog_fd, int target_fd, > > enum bpf_attach_type attach_type, > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index 550950eb1860..165131c73f40 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -9322,12 +9322,14 @@ static struct bpf_link *attach_iter(const struct bpf_sec_def *sec, > > > > static struct bpf_link * > > bpf_program__attach_fd(struct bpf_program *prog, int target_fd, > > - const char *target_name) > > + int target_btf_id, const char *target_name) > > { > > enum bpf_attach_type attach_type; > > char errmsg[STRERR_BUFSIZE]; > > struct bpf_link *link; > > int prog_fd, link_fd; > > + DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts, > > + .target_btf_id = target_btf_id); > > > > prog_fd = bpf_program__fd(prog); > > if (prog_fd < 0) { > > @@ -9340,8 +9342,12 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd, > > return ERR_PTR(-ENOMEM); > > link->detach = &bpf_link__detach_fd; > > > > - attach_type = bpf_program__get_expected_attach_type(prog); > > - link_fd = bpf_link_create(prog_fd, target_fd, attach_type, NULL); > > + if (bpf_program__get_type(prog) == BPF_PROG_TYPE_EXT) > > + attach_type = BPF_TRACE_FREPLACE; > > doing this unconditionally will break an old-style freplace without > target_fd/btf_id on older kernels. Safe and simple way would be to > continue using raw_tracepoint_open when there is no target_fd/btf_id, > and use LINK_CREATE for newer stuff. Alternatively, you'd need to do > feature detection, but it's still would be nice to handle old-style > attach through raw_tracepoint_open for bpf_program__attach_freplace. > > so I suggest leaving bpf_program__attach_fd() as is and to create a > custom bpf_program__attach_freplace() implementation. > > > + else > > + attach_type = bpf_program__get_expected_attach_type(prog); > > + > > + link_fd = bpf_link_create(prog_fd, target_fd, attach_type, &opts); > > if (link_fd < 0) { > > link_fd = -errno; > > free(link); > > @@ -9357,19 +9363,25 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd, > > struct bpf_link * > > bpf_program__attach_cgroup(struct bpf_program *prog, int cgroup_fd) > > { > > - return bpf_program__attach_fd(prog, cgroup_fd, "cgroup"); > > + return bpf_program__attach_fd(prog, cgroup_fd, 0, "cgroup"); > > } > > > > struct bpf_link * > > bpf_program__attach_netns(struct bpf_program *prog, int netns_fd) > > { > > - return bpf_program__attach_fd(prog, netns_fd, "netns"); > > + return bpf_program__attach_fd(prog, netns_fd, 0, "netns"); > > } > > > > struct bpf_link *bpf_program__attach_xdp(struct bpf_program *prog, int ifindex) > > { > > /* target_fd/target_ifindex use the same field in LINK_CREATE */ > > - return bpf_program__attach_fd(prog, ifindex, "xdp"); > > + return bpf_program__attach_fd(prog, ifindex, 0, "xdp"); > > +} > > + > > +struct bpf_link *bpf_program__attach_freplace(struct bpf_program *prog, > > + int target_fd, int target_btf_id) > > +{ > > + return bpf_program__attach_fd(prog, target_fd, target_btf_id, "freplace"); > > } > > > > struct bpf_link * > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > > index a750f67a23f6..ce5add9b9203 100644 > > --- a/tools/lib/bpf/libbpf.h > > +++ b/tools/lib/bpf/libbpf.h > > @@ -261,6 +261,9 @@ LIBBPF_API struct bpf_link * > > bpf_program__attach_netns(struct bpf_program *prog, int netns_fd); > > LIBBPF_API struct bpf_link * > > bpf_program__attach_xdp(struct bpf_program *prog, int ifindex); > > +LIBBPF_API struct bpf_link * > > +bpf_program__attach_freplace(struct bpf_program *prog, > > + int target_fd, int target_btf_id); > > maybe a const char * function name instead of target_btf_id would be a > nicer API? Users won't have to deal with fetching target prog's BTF, > searching it, etc. That's all pretty straightforward for libbpf to do, > leaving users with more natural and simpler API. > bpf_program__set_attach_target() uses string name for target functions, we should definitely be consistent here > > > > struct bpf_map; > > > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > > index 92ceb48a5ca2..3cc2c42f435d 100644 > > --- a/tools/lib/bpf/libbpf.map > > +++ b/tools/lib/bpf/libbpf.map > > @@ -302,6 +302,7 @@ LIBBPF_0.1.0 { > > > > LIBBPF_0.2.0 { > > global: > > + bpf_program__attach_freplace; > > bpf_program__section_name; > > perf_buffer__buffer_cnt; > > perf_buffer__buffer_fd; > >
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Wed, Sep 16, 2020 at 1:37 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: >> >> On Tue, Sep 15, 2020 at 5:50 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> > >> > From: Toke Høiland-Jørgensen <toke@redhat.com> >> > >> > This adds support for supplying a target btf ID for the bpf_link_create() >> > operation, and adds a new bpf_program__attach_freplace() high-level API for >> > attaching freplace functions with a target. >> > >> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >> > --- >> > tools/lib/bpf/bpf.c | 1 + >> > tools/lib/bpf/bpf.h | 3 ++- >> > tools/lib/bpf/libbpf.c | 24 ++++++++++++++++++------ >> > tools/lib/bpf/libbpf.h | 3 +++ >> > tools/lib/bpf/libbpf.map | 1 + >> > 5 files changed, 25 insertions(+), 7 deletions(-) >> > >> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c >> > index 82b983ff6569..e928456c0dd6 100644 >> > --- a/tools/lib/bpf/bpf.c >> > +++ b/tools/lib/bpf/bpf.c >> > @@ -599,6 +599,7 @@ int bpf_link_create(int prog_fd, int target_fd, >> > attr.link_create.iter_info = >> > ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0)); >> > attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0); >> > + attr.link_create.target_btf_id = OPTS_GET(opts, target_btf_id, 0); >> > >> > return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr)); >> > } >> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h >> > index 015d13f25fcc..f8dbf666b62b 100644 >> > --- a/tools/lib/bpf/bpf.h >> > +++ b/tools/lib/bpf/bpf.h >> > @@ -174,8 +174,9 @@ struct bpf_link_create_opts { >> > __u32 flags; >> > union bpf_iter_link_info *iter_info; >> > __u32 iter_info_len; >> > + __u32 target_btf_id; >> > }; >> > -#define bpf_link_create_opts__last_field iter_info_len >> > +#define bpf_link_create_opts__last_field target_btf_id >> > >> > LIBBPF_API int bpf_link_create(int prog_fd, int target_fd, >> > enum bpf_attach_type attach_type, >> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> > index 550950eb1860..165131c73f40 100644 >> > --- a/tools/lib/bpf/libbpf.c >> > +++ b/tools/lib/bpf/libbpf.c >> > @@ -9322,12 +9322,14 @@ static struct bpf_link *attach_iter(const struct bpf_sec_def *sec, >> > >> > static struct bpf_link * >> > bpf_program__attach_fd(struct bpf_program *prog, int target_fd, >> > - const char *target_name) >> > + int target_btf_id, const char *target_name) >> > { >> > enum bpf_attach_type attach_type; >> > char errmsg[STRERR_BUFSIZE]; >> > struct bpf_link *link; >> > int prog_fd, link_fd; >> > + DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts, >> > + .target_btf_id = target_btf_id); >> > >> > prog_fd = bpf_program__fd(prog); >> > if (prog_fd < 0) { >> > @@ -9340,8 +9342,12 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd, >> > return ERR_PTR(-ENOMEM); >> > link->detach = &bpf_link__detach_fd; >> > >> > - attach_type = bpf_program__get_expected_attach_type(prog); >> > - link_fd = bpf_link_create(prog_fd, target_fd, attach_type, NULL); >> > + if (bpf_program__get_type(prog) == BPF_PROG_TYPE_EXT) >> > + attach_type = BPF_TRACE_FREPLACE; >> >> doing this unconditionally will break an old-style freplace without >> target_fd/btf_id on older kernels. Safe and simple way would be to >> continue using raw_tracepoint_open when there is no target_fd/btf_id, >> and use LINK_CREATE for newer stuff. Alternatively, you'd need to do >> feature detection, but it's still would be nice to handle old-style >> attach through raw_tracepoint_open for bpf_program__attach_freplace. >> >> so I suggest leaving bpf_program__attach_fd() as is and to create a >> custom bpf_program__attach_freplace() implementation. Sure, I'll take another pass at this. Not sure how useful feature detection in libbpf is; if the caller passes a target, libbpf can't really do much if the kernel doesn't support it... >> > + else >> > + attach_type = bpf_program__get_expected_attach_type(prog); >> > + >> > + link_fd = bpf_link_create(prog_fd, target_fd, attach_type, &opts); >> > if (link_fd < 0) { >> > link_fd = -errno; >> > free(link); >> > @@ -9357,19 +9363,25 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd, >> > struct bpf_link * >> > bpf_program__attach_cgroup(struct bpf_program *prog, int cgroup_fd) >> > { >> > - return bpf_program__attach_fd(prog, cgroup_fd, "cgroup"); >> > + return bpf_program__attach_fd(prog, cgroup_fd, 0, "cgroup"); >> > } >> > >> > struct bpf_link * >> > bpf_program__attach_netns(struct bpf_program *prog, int netns_fd) >> > { >> > - return bpf_program__attach_fd(prog, netns_fd, "netns"); >> > + return bpf_program__attach_fd(prog, netns_fd, 0, "netns"); >> > } >> > >> > struct bpf_link *bpf_program__attach_xdp(struct bpf_program *prog, int ifindex) >> > { >> > /* target_fd/target_ifindex use the same field in LINK_CREATE */ >> > - return bpf_program__attach_fd(prog, ifindex, "xdp"); >> > + return bpf_program__attach_fd(prog, ifindex, 0, "xdp"); >> > +} >> > + >> > +struct bpf_link *bpf_program__attach_freplace(struct bpf_program *prog, >> > + int target_fd, int target_btf_id) >> > +{ >> > + return bpf_program__attach_fd(prog, target_fd, target_btf_id, "freplace"); >> > } >> > >> > struct bpf_link * >> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h >> > index a750f67a23f6..ce5add9b9203 100644 >> > --- a/tools/lib/bpf/libbpf.h >> > +++ b/tools/lib/bpf/libbpf.h >> > @@ -261,6 +261,9 @@ LIBBPF_API struct bpf_link * >> > bpf_program__attach_netns(struct bpf_program *prog, int netns_fd); >> > LIBBPF_API struct bpf_link * >> > bpf_program__attach_xdp(struct bpf_program *prog, int ifindex); >> > +LIBBPF_API struct bpf_link * >> > +bpf_program__attach_freplace(struct bpf_program *prog, >> > + int target_fd, int target_btf_id); >> >> maybe a const char * function name instead of target_btf_id would be a >> nicer API? Users won't have to deal with fetching target prog's BTF, >> searching it, etc. That's all pretty straightforward for libbpf to do, >> leaving users with more natural and simpler API. >> > > bpf_program__set_attach_target() uses string name for target > functions, we should definitely be consistent here All right, fair enough :) -Toke
On Wed, Sep 16, 2020 at 2:21 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Wed, Sep 16, 2020 at 1:37 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > >> > >> On Tue, Sep 15, 2020 at 5:50 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> > > >> > From: Toke Høiland-Jørgensen <toke@redhat.com> > >> > > >> > This adds support for supplying a target btf ID for the bpf_link_create() > >> > operation, and adds a new bpf_program__attach_freplace() high-level API for > >> > attaching freplace functions with a target. > >> > > >> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > >> > --- > >> > tools/lib/bpf/bpf.c | 1 + > >> > tools/lib/bpf/bpf.h | 3 ++- > >> > tools/lib/bpf/libbpf.c | 24 ++++++++++++++++++------ > >> > tools/lib/bpf/libbpf.h | 3 +++ > >> > tools/lib/bpf/libbpf.map | 1 + > >> > 5 files changed, 25 insertions(+), 7 deletions(-) > >> > > >> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > >> > index 82b983ff6569..e928456c0dd6 100644 > >> > --- a/tools/lib/bpf/bpf.c > >> > +++ b/tools/lib/bpf/bpf.c > >> > @@ -599,6 +599,7 @@ int bpf_link_create(int prog_fd, int target_fd, > >> > attr.link_create.iter_info = > >> > ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0)); > >> > attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0); > >> > + attr.link_create.target_btf_id = OPTS_GET(opts, target_btf_id, 0); > >> > > >> > return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr)); > >> > } > >> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h > >> > index 015d13f25fcc..f8dbf666b62b 100644 > >> > --- a/tools/lib/bpf/bpf.h > >> > +++ b/tools/lib/bpf/bpf.h > >> > @@ -174,8 +174,9 @@ struct bpf_link_create_opts { > >> > __u32 flags; > >> > union bpf_iter_link_info *iter_info; > >> > __u32 iter_info_len; > >> > + __u32 target_btf_id; > >> > }; > >> > -#define bpf_link_create_opts__last_field iter_info_len > >> > +#define bpf_link_create_opts__last_field target_btf_id > >> > > >> > LIBBPF_API int bpf_link_create(int prog_fd, int target_fd, > >> > enum bpf_attach_type attach_type, > >> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >> > index 550950eb1860..165131c73f40 100644 > >> > --- a/tools/lib/bpf/libbpf.c > >> > +++ b/tools/lib/bpf/libbpf.c > >> > @@ -9322,12 +9322,14 @@ static struct bpf_link *attach_iter(const struct bpf_sec_def *sec, > >> > > >> > static struct bpf_link * > >> > bpf_program__attach_fd(struct bpf_program *prog, int target_fd, > >> > - const char *target_name) > >> > + int target_btf_id, const char *target_name) > >> > { > >> > enum bpf_attach_type attach_type; > >> > char errmsg[STRERR_BUFSIZE]; > >> > struct bpf_link *link; > >> > int prog_fd, link_fd; > >> > + DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts, > >> > + .target_btf_id = target_btf_id); > >> > > >> > prog_fd = bpf_program__fd(prog); > >> > if (prog_fd < 0) { > >> > @@ -9340,8 +9342,12 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd, > >> > return ERR_PTR(-ENOMEM); > >> > link->detach = &bpf_link__detach_fd; > >> > > >> > - attach_type = bpf_program__get_expected_attach_type(prog); > >> > - link_fd = bpf_link_create(prog_fd, target_fd, attach_type, NULL); > >> > + if (bpf_program__get_type(prog) == BPF_PROG_TYPE_EXT) > >> > + attach_type = BPF_TRACE_FREPLACE; > >> > >> doing this unconditionally will break an old-style freplace without > >> target_fd/btf_id on older kernels. Safe and simple way would be to > >> continue using raw_tracepoint_open when there is no target_fd/btf_id, > >> and use LINK_CREATE for newer stuff. Alternatively, you'd need to do > >> feature detection, but it's still would be nice to handle old-style > >> attach through raw_tracepoint_open for bpf_program__attach_freplace. > >> > >> so I suggest leaving bpf_program__attach_fd() as is and to create a > >> custom bpf_program__attach_freplace() implementation. > > Sure, I'll take another pass at this. Not sure how useful feature > detection in libbpf is; if the caller passes a target, libbpf can't > really do much if the kernel doesn't support it... I was thinking about bpf_program__attach_freplace(prog, 0, NULL) doing bpf_raw_tracepoint_open(). It would be nice to support this, for API uniformity, no? > > >> > + else > >> > + attach_type = bpf_program__get_expected_attach_type(prog); > >> > + > >> > + link_fd = bpf_link_create(prog_fd, target_fd, attach_type, &opts); > >> > if (link_fd < 0) { > >> > link_fd = -errno; > >> > free(link); > >> > @@ -9357,19 +9363,25 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd, > >> > struct bpf_link * > >> > bpf_program__attach_cgroup(struct bpf_program *prog, int cgroup_fd) > >> > { > >> > - return bpf_program__attach_fd(prog, cgroup_fd, "cgroup"); > >> > + return bpf_program__attach_fd(prog, cgroup_fd, 0, "cgroup"); > >> > } > >> > > >> > struct bpf_link * > >> > bpf_program__attach_netns(struct bpf_program *prog, int netns_fd) > >> > { > >> > - return bpf_program__attach_fd(prog, netns_fd, "netns"); > >> > + return bpf_program__attach_fd(prog, netns_fd, 0, "netns"); > >> > } > >> > > >> > struct bpf_link *bpf_program__attach_xdp(struct bpf_program *prog, int ifindex) > >> > { > >> > /* target_fd/target_ifindex use the same field in LINK_CREATE */ > >> > - return bpf_program__attach_fd(prog, ifindex, "xdp"); > >> > + return bpf_program__attach_fd(prog, ifindex, 0, "xdp"); > >> > +} > >> > + > >> > +struct bpf_link *bpf_program__attach_freplace(struct bpf_program *prog, > >> > + int target_fd, int target_btf_id) > >> > +{ > >> > + return bpf_program__attach_fd(prog, target_fd, target_btf_id, "freplace"); > >> > } > >> > > >> > struct bpf_link * > >> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > >> > index a750f67a23f6..ce5add9b9203 100644 > >> > --- a/tools/lib/bpf/libbpf.h > >> > +++ b/tools/lib/bpf/libbpf.h > >> > @@ -261,6 +261,9 @@ LIBBPF_API struct bpf_link * > >> > bpf_program__attach_netns(struct bpf_program *prog, int netns_fd); > >> > LIBBPF_API struct bpf_link * > >> > bpf_program__attach_xdp(struct bpf_program *prog, int ifindex); > >> > +LIBBPF_API struct bpf_link * > >> > +bpf_program__attach_freplace(struct bpf_program *prog, > >> > + int target_fd, int target_btf_id); > >> > >> maybe a const char * function name instead of target_btf_id would be a > >> nicer API? Users won't have to deal with fetching target prog's BTF, > >> searching it, etc. That's all pretty straightforward for libbpf to do, > >> leaving users with more natural and simpler API. > >> > > > > bpf_program__set_attach_target() uses string name for target > > functions, we should definitely be consistent here > > All right, fair enough :) > > -Toke >
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Wed, Sep 16, 2020 at 2:21 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> >> > On Wed, Sep 16, 2020 at 1:37 PM Andrii Nakryiko >> > <andrii.nakryiko@gmail.com> wrote: >> >> >> >> On Tue, Sep 15, 2020 at 5:50 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> > >> >> > From: Toke Høiland-Jørgensen <toke@redhat.com> >> >> > >> >> > This adds support for supplying a target btf ID for the bpf_link_create() >> >> > operation, and adds a new bpf_program__attach_freplace() high-level API for >> >> > attaching freplace functions with a target. >> >> > >> >> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >> >> > --- >> >> > tools/lib/bpf/bpf.c | 1 + >> >> > tools/lib/bpf/bpf.h | 3 ++- >> >> > tools/lib/bpf/libbpf.c | 24 ++++++++++++++++++------ >> >> > tools/lib/bpf/libbpf.h | 3 +++ >> >> > tools/lib/bpf/libbpf.map | 1 + >> >> > 5 files changed, 25 insertions(+), 7 deletions(-) >> >> > >> >> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c >> >> > index 82b983ff6569..e928456c0dd6 100644 >> >> > --- a/tools/lib/bpf/bpf.c >> >> > +++ b/tools/lib/bpf/bpf.c >> >> > @@ -599,6 +599,7 @@ int bpf_link_create(int prog_fd, int target_fd, >> >> > attr.link_create.iter_info = >> >> > ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0)); >> >> > attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0); >> >> > + attr.link_create.target_btf_id = OPTS_GET(opts, target_btf_id, 0); >> >> > >> >> > return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr)); >> >> > } >> >> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h >> >> > index 015d13f25fcc..f8dbf666b62b 100644 >> >> > --- a/tools/lib/bpf/bpf.h >> >> > +++ b/tools/lib/bpf/bpf.h >> >> > @@ -174,8 +174,9 @@ struct bpf_link_create_opts { >> >> > __u32 flags; >> >> > union bpf_iter_link_info *iter_info; >> >> > __u32 iter_info_len; >> >> > + __u32 target_btf_id; >> >> > }; >> >> > -#define bpf_link_create_opts__last_field iter_info_len >> >> > +#define bpf_link_create_opts__last_field target_btf_id >> >> > >> >> > LIBBPF_API int bpf_link_create(int prog_fd, int target_fd, >> >> > enum bpf_attach_type attach_type, >> >> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> >> > index 550950eb1860..165131c73f40 100644 >> >> > --- a/tools/lib/bpf/libbpf.c >> >> > +++ b/tools/lib/bpf/libbpf.c >> >> > @@ -9322,12 +9322,14 @@ static struct bpf_link *attach_iter(const struct bpf_sec_def *sec, >> >> > >> >> > static struct bpf_link * >> >> > bpf_program__attach_fd(struct bpf_program *prog, int target_fd, >> >> > - const char *target_name) >> >> > + int target_btf_id, const char *target_name) >> >> > { >> >> > enum bpf_attach_type attach_type; >> >> > char errmsg[STRERR_BUFSIZE]; >> >> > struct bpf_link *link; >> >> > int prog_fd, link_fd; >> >> > + DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts, >> >> > + .target_btf_id = target_btf_id); >> >> > >> >> > prog_fd = bpf_program__fd(prog); >> >> > if (prog_fd < 0) { >> >> > @@ -9340,8 +9342,12 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd, >> >> > return ERR_PTR(-ENOMEM); >> >> > link->detach = &bpf_link__detach_fd; >> >> > >> >> > - attach_type = bpf_program__get_expected_attach_type(prog); >> >> > - link_fd = bpf_link_create(prog_fd, target_fd, attach_type, NULL); >> >> > + if (bpf_program__get_type(prog) == BPF_PROG_TYPE_EXT) >> >> > + attach_type = BPF_TRACE_FREPLACE; >> >> >> >> doing this unconditionally will break an old-style freplace without >> >> target_fd/btf_id on older kernels. Safe and simple way would be to >> >> continue using raw_tracepoint_open when there is no target_fd/btf_id, >> >> and use LINK_CREATE for newer stuff. Alternatively, you'd need to do >> >> feature detection, but it's still would be nice to handle old-style >> >> attach through raw_tracepoint_open for bpf_program__attach_freplace. >> >> >> >> so I suggest leaving bpf_program__attach_fd() as is and to create a >> >> custom bpf_program__attach_freplace() implementation. >> >> Sure, I'll take another pass at this. Not sure how useful feature >> detection in libbpf is; if the caller passes a target, libbpf can't >> really do much if the kernel doesn't support it... > > I was thinking about bpf_program__attach_freplace(prog, 0, NULL) doing > bpf_raw_tracepoint_open(). It would be nice to support this, for API > uniformity, no? Yeah, sure, that we can do :) -Toke
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c index 82b983ff6569..e928456c0dd6 100644 --- a/tools/lib/bpf/bpf.c +++ b/tools/lib/bpf/bpf.c @@ -599,6 +599,7 @@ int bpf_link_create(int prog_fd, int target_fd, attr.link_create.iter_info = ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0)); attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0); + attr.link_create.target_btf_id = OPTS_GET(opts, target_btf_id, 0); return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr)); } diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h index 015d13f25fcc..f8dbf666b62b 100644 --- a/tools/lib/bpf/bpf.h +++ b/tools/lib/bpf/bpf.h @@ -174,8 +174,9 @@ struct bpf_link_create_opts { __u32 flags; union bpf_iter_link_info *iter_info; __u32 iter_info_len; + __u32 target_btf_id; }; -#define bpf_link_create_opts__last_field iter_info_len +#define bpf_link_create_opts__last_field target_btf_id LIBBPF_API int bpf_link_create(int prog_fd, int target_fd, enum bpf_attach_type attach_type, diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 550950eb1860..165131c73f40 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -9322,12 +9322,14 @@ static struct bpf_link *attach_iter(const struct bpf_sec_def *sec, static struct bpf_link * bpf_program__attach_fd(struct bpf_program *prog, int target_fd, - const char *target_name) + int target_btf_id, const char *target_name) { enum bpf_attach_type attach_type; char errmsg[STRERR_BUFSIZE]; struct bpf_link *link; int prog_fd, link_fd; + DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts, + .target_btf_id = target_btf_id); prog_fd = bpf_program__fd(prog); if (prog_fd < 0) { @@ -9340,8 +9342,12 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd, return ERR_PTR(-ENOMEM); link->detach = &bpf_link__detach_fd; - attach_type = bpf_program__get_expected_attach_type(prog); - link_fd = bpf_link_create(prog_fd, target_fd, attach_type, NULL); + if (bpf_program__get_type(prog) == BPF_PROG_TYPE_EXT) + attach_type = BPF_TRACE_FREPLACE; + else + attach_type = bpf_program__get_expected_attach_type(prog); + + link_fd = bpf_link_create(prog_fd, target_fd, attach_type, &opts); if (link_fd < 0) { link_fd = -errno; free(link); @@ -9357,19 +9363,25 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd, struct bpf_link * bpf_program__attach_cgroup(struct bpf_program *prog, int cgroup_fd) { - return bpf_program__attach_fd(prog, cgroup_fd, "cgroup"); + return bpf_program__attach_fd(prog, cgroup_fd, 0, "cgroup"); } struct bpf_link * bpf_program__attach_netns(struct bpf_program *prog, int netns_fd) { - return bpf_program__attach_fd(prog, netns_fd, "netns"); + return bpf_program__attach_fd(prog, netns_fd, 0, "netns"); } struct bpf_link *bpf_program__attach_xdp(struct bpf_program *prog, int ifindex) { /* target_fd/target_ifindex use the same field in LINK_CREATE */ - return bpf_program__attach_fd(prog, ifindex, "xdp"); + return bpf_program__attach_fd(prog, ifindex, 0, "xdp"); +} + +struct bpf_link *bpf_program__attach_freplace(struct bpf_program *prog, + int target_fd, int target_btf_id) +{ + return bpf_program__attach_fd(prog, target_fd, target_btf_id, "freplace"); } struct bpf_link * diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index a750f67a23f6..ce5add9b9203 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -261,6 +261,9 @@ LIBBPF_API struct bpf_link * bpf_program__attach_netns(struct bpf_program *prog, int netns_fd); LIBBPF_API struct bpf_link * bpf_program__attach_xdp(struct bpf_program *prog, int ifindex); +LIBBPF_API struct bpf_link * +bpf_program__attach_freplace(struct bpf_program *prog, + int target_fd, int target_btf_id); struct bpf_map; diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index 92ceb48a5ca2..3cc2c42f435d 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -302,6 +302,7 @@ LIBBPF_0.1.0 { LIBBPF_0.2.0 { global: + bpf_program__attach_freplace; bpf_program__section_name; perf_buffer__buffer_cnt; perf_buffer__buffer_fd;