| Message ID | 20210604063116.234316-7-memxor@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add bpf_link based TC-BPF API | expand |
On Fri, Jun 04, 2021 at 12:01:15PM +0530, Kumar Kartikeya Dwivedi wrote: > +/* TC bpf_link related API */ > +struct bpf_tc_hook; > + > +struct bpf_tc_link_opts { > + size_t sz; > + __u32 handle; > + __u32 priority; > + __u32 gen_flags; > + size_t :0; > +}; Did you think of a way to share struct bpf_tc_opts with above? Or use bpf_tc_link_opts inside bpf_tc_opts? Some other way? gen_flags here and flags there are the same?
On Fri, Jun 04, 2021 at 11:31:57PM IST, Alexei Starovoitov wrote: > On Fri, Jun 04, 2021 at 12:01:15PM +0530, Kumar Kartikeya Dwivedi wrote: > > +/* TC bpf_link related API */ > > +struct bpf_tc_hook; > > + > > +struct bpf_tc_link_opts { > > + size_t sz; > > + __u32 handle; > > + __u32 priority; > > + __u32 gen_flags; > > + size_t :0; > > +}; > > Did you think of a way to share struct bpf_tc_opts with above? > Or use bpf_tc_link_opts inside bpf_tc_opts? A couple of fields in bpf_tc_opts aren't really relevant here (prog_fd, prog_id) and will always be unused, so I thought it would be cleaner to give this its own opts struct. It still reuses the hook abstraction that was added, though. > Some other way? > gen_flags here and flags there are the same? No, it was an oversight that I missed adding gen_flags there, I'll send a patch separately with some other assorted things. It's used when offloading to HW. We don't really support any other flags (e.g. BPF_TC_F_REPLACE) for this. -- Kartikeya
On 6/3/21 11:31 PM, Kumar Kartikeya Dwivedi wrote: > This adds userspace TC-BPF management API based on bpf_link. > > Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>. > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > tools/lib/bpf/bpf.c | 8 +++++- > tools/lib/bpf/bpf.h | 8 +++++- > tools/lib/bpf/libbpf.c | 59 ++++++++++++++++++++++++++++++++++++++-- > tools/lib/bpf/libbpf.h | 17 ++++++++++++ > tools/lib/bpf/libbpf.map | 1 + > tools/lib/bpf/netlink.c | 5 ++-- > tools/lib/bpf/netlink.h | 8 ++++++ > 7 files changed, 100 insertions(+), 6 deletions(-) > create mode 100644 tools/lib/bpf/netlink.h > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > index 86dcac44f32f..bebccea9bfd7 100644 > --- a/tools/lib/bpf/bpf.c > +++ b/tools/lib/bpf/bpf.c > @@ -28,6 +28,7 @@ > #include <asm/unistd.h> > #include <errno.h> > #include <linux/bpf.h> > +#include <arpa/inet.h> > #include "bpf.h" > #include "libbpf.h" > #include "libbpf_internal.h" > @@ -693,7 +694,12 @@ int bpf_link_create(int prog_fd, int target_fd, > attr.link_create.attach_type = attach_type; > attr.link_create.flags = OPTS_GET(opts, flags, 0); > > - if (iter_info_len) { > + if (attach_type == BPF_TC) { > + attr.link_create.tc.parent = OPTS_GET(opts, tc.parent, 0); > + attr.link_create.tc.handle = OPTS_GET(opts, tc.handle, 0); > + attr.link_create.tc.priority = OPTS_GET(opts, tc.priority, 0); > + attr.link_create.tc.gen_flags = OPTS_GET(opts, tc.gen_flags, 0); > + } else if (iter_info_len) { > attr.link_create.iter_info = > ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0)); > attr.link_create.iter_info_len = iter_info_len; > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h > index 4f758f8f50cd..f2178309e9ea 100644 > --- a/tools/lib/bpf/bpf.h > +++ b/tools/lib/bpf/bpf.h > @@ -177,8 +177,14 @@ struct bpf_link_create_opts { > union bpf_iter_link_info *iter_info; > __u32 iter_info_len; > __u32 target_btf_id; > + struct { > + __u32 parent; > + __u32 handle; > + __u32 priority; > + __u32 gen_flags; > + } tc; > }; > -#define bpf_link_create_opts__last_field target_btf_id > +#define bpf_link_create_opts__last_field tc.gen_flags > > 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 1c4e20e75237..7809536980b1 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -55,6 +55,7 @@ > #include "libbpf_internal.h" > #include "hashmap.h" > #include "bpf_gen_internal.h" > +#include "netlink.h" > > #ifndef BPF_FS_MAGIC > #define BPF_FS_MAGIC 0xcafe4a11 > @@ -7185,7 +7186,7 @@ static int bpf_object__collect_relos(struct bpf_object *obj) > > for (i = 0; i < obj->nr_programs; i++) { > struct bpf_program *p = &obj->programs[i]; > - > + > if (!p->nr_reloc) > continue; > > @@ -10005,7 +10006,7 @@ struct bpf_link { > int bpf_link__update_program(struct bpf_link *link, struct bpf_program *prog) > { > int ret; > - > + > ret = bpf_link_update(bpf_link__fd(link), bpf_program__fd(prog), NULL); > return libbpf_err_errno(ret); > } > @@ -10613,6 +10614,60 @@ struct bpf_link *bpf_program__attach_xdp(struct bpf_program *prog, int ifindex) > return bpf_program__attach_fd(prog, ifindex, 0, "xdp"); > } > > +struct bpf_link *bpf_program__attach_tc(struct bpf_program *prog, > + const struct bpf_tc_hook *hook, > + const struct bpf_tc_link_opts *opts) > +{ > + DECLARE_LIBBPF_OPTS(bpf_link_create_opts, lopts, 0); > + char errmsg[STRERR_BUFSIZE]; > + int prog_fd, link_fd, ret; > + struct bpf_link *link; > + __u32 parent; > + > + if (!hook || !OPTS_VALID(hook, bpf_tc_hook) || > + !OPTS_VALID(opts, bpf_tc_link_opts)) > + return ERR_PTR(-EINVAL); For libbpf API functions, the libbpf proposed 1.0 change https://lore.kernel.org/bpf/20210525035935.1461796-5-andrii@kernel.org will return NULL and set errno properly. So the above code probably should be return errno = EINVAL, NULL; > + > + if (OPTS_GET(hook, ifindex, 0) <= 0 || > + OPTS_GET(opts, priority, 0) > UINT16_MAX) > + return ERR_PTR(-EINVAL); > + > + parent = OPTS_GET(hook, parent, 0); > + > + ret = tc_get_tcm_parent(OPTS_GET(hook, attach_point, 0), > + &parent); > + if (ret < 0) > + return ERR_PTR(ret); > + > + lopts.tc.parent = parent; > + lopts.tc.handle = OPTS_GET(opts, handle, 0); > + lopts.tc.priority = OPTS_GET(opts, priority, 0); > + lopts.tc.gen_flags = OPTS_GET(opts, gen_flags, 0); > + > + prog_fd = bpf_program__fd(prog); > + if (prog_fd < 0) { > + pr_warn("prog '%s': can't attach before loaded\n", prog->name); > + return ERR_PTR(-EINVAL); > + } > + > + link = calloc(1, sizeof(*link)); > + if (!link) > + return ERR_PTR(-ENOMEM); > + link->detach = &bpf_link__detach_fd; > + > + link_fd = bpf_link_create(prog_fd, OPTS_GET(hook, ifindex, 0), BPF_TC, &lopts); > + if (link_fd < 0) { > + link_fd = -errno; > + free(link); > + pr_warn("prog '%s': failed to attach tc filter: %s\n", > + prog->name, libbpf_strerror_r(link_fd, errmsg, sizeof(errmsg))); > + return ERR_PTR(link_fd); > + } > + link->fd = link_fd; > + > + return link; > +} > + [...]
On Fri, Jun 4, 2021 at 9:52 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Fri, Jun 04, 2021 at 11:31:57PM IST, Alexei Starovoitov wrote: > > On Fri, Jun 04, 2021 at 12:01:15PM +0530, Kumar Kartikeya Dwivedi wrote: > > > +/* TC bpf_link related API */ > > > +struct bpf_tc_hook; > > > + > > > +struct bpf_tc_link_opts { > > > + size_t sz; > > > + __u32 handle; > > > + __u32 priority; > > > + __u32 gen_flags; > > > + size_t :0; > > > +}; > > > > Did you think of a way to share struct bpf_tc_opts with above? > > Or use bpf_tc_link_opts inside bpf_tc_opts? > > A couple of fields in bpf_tc_opts aren't really relevant here (prog_fd, prog_id) > and will always be unused, so I thought it would be cleaner to give this its own > opts struct. It still reuses the hook abstraction that was added, though. Overall probably it will be less confusing to have one _opts struct across both APIs, even if some of the fields are not used. Just enforce that they are always NULL (and document in a comment that for bpf_link-based API it's not expected). > > > Some other way? > > gen_flags here and flags there are the same? > > No, it was an oversight that I missed adding gen_flags there, I'll send a patch > separately with some other assorted things. It's used when offloading to HW. > > We don't really support any other flags (e.g. BPF_TC_F_REPLACE) for this. > > -- > Kartikeya
On Thu, Jun 3, 2021 at 11:32 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > This adds userspace TC-BPF management API based on bpf_link. > > Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>. > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > tools/lib/bpf/bpf.c | 8 +++++- > tools/lib/bpf/bpf.h | 8 +++++- > tools/lib/bpf/libbpf.c | 59 ++++++++++++++++++++++++++++++++++++++-- > tools/lib/bpf/libbpf.h | 17 ++++++++++++ > tools/lib/bpf/libbpf.map | 1 + > tools/lib/bpf/netlink.c | 5 ++-- > tools/lib/bpf/netlink.h | 8 ++++++ > 7 files changed, 100 insertions(+), 6 deletions(-) > create mode 100644 tools/lib/bpf/netlink.h > [...] > + link = calloc(1, sizeof(*link)); > + if (!link) > + return ERR_PTR(-ENOMEM); > + link->detach = &bpf_link__detach_fd; > + > + link_fd = bpf_link_create(prog_fd, OPTS_GET(hook, ifindex, 0), BPF_TC, &lopts); > + if (link_fd < 0) { > + link_fd = -errno; > + free(link); > + pr_warn("prog '%s': failed to attach tc filter: %s\n", > + prog->name, libbpf_strerror_r(link_fd, errmsg, sizeof(errmsg))); > + return ERR_PTR(link_fd); like Yonghong mentioned, all these error returns have to be either `return libbpf_err_ptr(err);` or, given it's a new API, we can do just direct `return errno = err, NULL;`. I'd probably use libbpf_err_ptr() for now for consistency. > + } > + link->fd = link_fd; > + > + return link; > +} > + > struct bpf_link *bpf_program__attach_freplace(struct bpf_program *prog, > int target_fd, > const char *attach_func_name) [...] > diff --git a/tools/lib/bpf/netlink.h b/tools/lib/bpf/netlink.h > new file mode 100644 > index 000000000000..c89133d56eb4 > --- /dev/null > +++ b/tools/lib/bpf/netlink.h > @@ -0,0 +1,8 @@ > +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) > +#pragma once we don't use "#pragma once" in libbpf. And I saw some kernel discussions discouraging its use. So please just stick to the classic #ifdef/#define/#endif combo. > + > +#include <linux/types.h> > +#include "libbpf.h" > + > +int tc_get_tcm_parent(enum bpf_tc_attach_point attach_point, > + __u32 *parent); > -- > 2.31.1 >
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c index 86dcac44f32f..bebccea9bfd7 100644 --- a/tools/lib/bpf/bpf.c +++ b/tools/lib/bpf/bpf.c @@ -28,6 +28,7 @@ #include <asm/unistd.h> #include <errno.h> #include <linux/bpf.h> +#include <arpa/inet.h> #include "bpf.h" #include "libbpf.h" #include "libbpf_internal.h" @@ -693,7 +694,12 @@ int bpf_link_create(int prog_fd, int target_fd, attr.link_create.attach_type = attach_type; attr.link_create.flags = OPTS_GET(opts, flags, 0); - if (iter_info_len) { + if (attach_type == BPF_TC) { + attr.link_create.tc.parent = OPTS_GET(opts, tc.parent, 0); + attr.link_create.tc.handle = OPTS_GET(opts, tc.handle, 0); + attr.link_create.tc.priority = OPTS_GET(opts, tc.priority, 0); + attr.link_create.tc.gen_flags = OPTS_GET(opts, tc.gen_flags, 0); + } else if (iter_info_len) { attr.link_create.iter_info = ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0)); attr.link_create.iter_info_len = iter_info_len; diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h index 4f758f8f50cd..f2178309e9ea 100644 --- a/tools/lib/bpf/bpf.h +++ b/tools/lib/bpf/bpf.h @@ -177,8 +177,14 @@ struct bpf_link_create_opts { union bpf_iter_link_info *iter_info; __u32 iter_info_len; __u32 target_btf_id; + struct { + __u32 parent; + __u32 handle; + __u32 priority; + __u32 gen_flags; + } tc; }; -#define bpf_link_create_opts__last_field target_btf_id +#define bpf_link_create_opts__last_field tc.gen_flags 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 1c4e20e75237..7809536980b1 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -55,6 +55,7 @@ #include "libbpf_internal.h" #include "hashmap.h" #include "bpf_gen_internal.h" +#include "netlink.h" #ifndef BPF_FS_MAGIC #define BPF_FS_MAGIC 0xcafe4a11 @@ -7185,7 +7186,7 @@ static int bpf_object__collect_relos(struct bpf_object *obj) for (i = 0; i < obj->nr_programs; i++) { struct bpf_program *p = &obj->programs[i]; - + if (!p->nr_reloc) continue; @@ -10005,7 +10006,7 @@ struct bpf_link { int bpf_link__update_program(struct bpf_link *link, struct bpf_program *prog) { int ret; - + ret = bpf_link_update(bpf_link__fd(link), bpf_program__fd(prog), NULL); return libbpf_err_errno(ret); } @@ -10613,6 +10614,60 @@ struct bpf_link *bpf_program__attach_xdp(struct bpf_program *prog, int ifindex) return bpf_program__attach_fd(prog, ifindex, 0, "xdp"); } +struct bpf_link *bpf_program__attach_tc(struct bpf_program *prog, + const struct bpf_tc_hook *hook, + const struct bpf_tc_link_opts *opts) +{ + DECLARE_LIBBPF_OPTS(bpf_link_create_opts, lopts, 0); + char errmsg[STRERR_BUFSIZE]; + int prog_fd, link_fd, ret; + struct bpf_link *link; + __u32 parent; + + if (!hook || !OPTS_VALID(hook, bpf_tc_hook) || + !OPTS_VALID(opts, bpf_tc_link_opts)) + return ERR_PTR(-EINVAL); + + if (OPTS_GET(hook, ifindex, 0) <= 0 || + OPTS_GET(opts, priority, 0) > UINT16_MAX) + return ERR_PTR(-EINVAL); + + parent = OPTS_GET(hook, parent, 0); + + ret = tc_get_tcm_parent(OPTS_GET(hook, attach_point, 0), + &parent); + if (ret < 0) + return ERR_PTR(ret); + + lopts.tc.parent = parent; + lopts.tc.handle = OPTS_GET(opts, handle, 0); + lopts.tc.priority = OPTS_GET(opts, priority, 0); + lopts.tc.gen_flags = OPTS_GET(opts, gen_flags, 0); + + prog_fd = bpf_program__fd(prog); + if (prog_fd < 0) { + pr_warn("prog '%s': can't attach before loaded\n", prog->name); + return ERR_PTR(-EINVAL); + } + + link = calloc(1, sizeof(*link)); + if (!link) + return ERR_PTR(-ENOMEM); + link->detach = &bpf_link__detach_fd; + + link_fd = bpf_link_create(prog_fd, OPTS_GET(hook, ifindex, 0), BPF_TC, &lopts); + if (link_fd < 0) { + link_fd = -errno; + free(link); + pr_warn("prog '%s': failed to attach tc filter: %s\n", + prog->name, libbpf_strerror_r(link_fd, errmsg, sizeof(errmsg))); + return ERR_PTR(link_fd); + } + link->fd = link_fd; + + return link; +} + struct bpf_link *bpf_program__attach_freplace(struct bpf_program *prog, int target_fd, const char *attach_func_name) diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 6e61342ba56c..284a446c6513 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -282,6 +282,23 @@ LIBBPF_API struct bpf_link * bpf_program__attach_iter(struct bpf_program *prog, const struct bpf_iter_attach_opts *opts); +/* TC bpf_link related API */ +struct bpf_tc_hook; + +struct bpf_tc_link_opts { + size_t sz; + __u32 handle; + __u32 priority; + __u32 gen_flags; + size_t :0; +}; +#define bpf_tc_link_opts__last_field gen_flags + +LIBBPF_API struct bpf_link * +bpf_program__attach_tc(struct bpf_program *prog, + const struct bpf_tc_hook *hook, + const struct bpf_tc_link_opts *opts); + struct bpf_insn; /* diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index 944c99d1ded3..5aa2e62b9fc2 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -373,5 +373,6 @@ LIBBPF_0.5.0 { bpf_map__initial_value; bpf_map_lookup_and_delete_elem_flags; bpf_object__gen_loader; + bpf_program__attach_tc; libbpf_set_strict_mode; } LIBBPF_0.4.0; diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c index d743c8721aa7..b7ac36fc9c1a 100644 --- a/tools/lib/bpf/netlink.c +++ b/tools/lib/bpf/netlink.c @@ -17,6 +17,7 @@ #include "libbpf.h" #include "libbpf_internal.h" #include "nlattr.h" +#include "netlink.h" #ifndef SOL_NETLINK #define SOL_NETLINK 270 @@ -405,8 +406,8 @@ static int attach_point_to_config(struct bpf_tc_hook *hook, } } -static int tc_get_tcm_parent(enum bpf_tc_attach_point attach_point, - __u32 *parent) +int tc_get_tcm_parent(enum bpf_tc_attach_point attach_point, + __u32 *parent) { switch (attach_point) { case BPF_TC_INGRESS: diff --git a/tools/lib/bpf/netlink.h b/tools/lib/bpf/netlink.h new file mode 100644 index 000000000000..c89133d56eb4 --- /dev/null +++ b/tools/lib/bpf/netlink.h @@ -0,0 +1,8 @@ +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) +#pragma once + +#include <linux/types.h> +#include "libbpf.h" + +int tc_get_tcm_parent(enum bpf_tc_attach_point attach_point, + __u32 *parent);