| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 5 May 2021 23:42:01 +0200
From: Daniel Borkmann <daniel@...earbox.net>
To: Kumar Kartikeya Dwivedi <memxor@...il.com>, bpf@...r.kernel.org
Cc: Toke Høiland-Jørgensen <toke@...hat.com>,
Alexei Starovoitov <ast@...nel.org>,
Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Jesper Dangaard Brouer <brouer@...hat.com>,
Shaun Crampton <shaun@...era.io>, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v6 2/3] libbpf: add low level TC-BPF API
On 5/4/21 2:50 AM, Kumar Kartikeya Dwivedi wrote:
> This adds functions that wrap the netlink API used for adding,
> manipulating, and removing traffic control filters.
>
> An API summary:
Looks better, few minor comments below:
> A bpf_tc_hook represents a location where a TC-BPF filter can be
> attached. This means that creating a hook leads to creation of the
> backing qdisc, while destruction either removes all filters attached to
> a hook, or destroys qdisc if requested explicitly (as discussed below).
>
> The TC-BPF API functions operate on this bpf_tc_hook to attach, replace,
> query, and detach tc filters.
>
> All functions return 0 on success, and a negative error code on failure.
>
> bpf_tc_hook_create - Create a hook
> Parameters:
> @hook - Cannot be NULL, ifindex > 0, attach_point must be set to
> proper enum constant. Note that parent must be unset when
> attach_point is one of BPF_TC_INGRESS or BPF_TC_EGRESS. Note
> that as an exception BPF_TC_INGRESS|BPF_TC_EGRESS is also a
> valid value for attach_point.
>
> Returns -EOPNOTSUPP when hook has attach_point as BPF_TC_CUSTOM.
>
> hook's flags member can be BPF_TC_F_REPLACE, which
> creates qdisc in non-exclusive mode (i.e. an existing
> qdisc will be replaced instead of this function failing
> with -EEXIST).
Why supporting BPF_TC_F_REPLACE here? It's not changing any qdisc parameters
given clsact doesn't have any, no? Iow, what effect are you expecting on this
with BPF_TC_F_REPLACE & why supporting it? I'd probably just require flags to
be 0 here, and if hook exists return sth like -EEXIST.
> bpf_tc_hook_destroy - Destroy the hook
> Parameters:
> @hook - Cannot be NULL. The behaviour depends on value of
> attach_point.
>
> If BPF_TC_INGRESS, all filters attached to the ingress
> hook will be detached.
> If BPF_TC_EGRESS, all filters attached to the egress hook
> will be detached.
> If BPF_TC_INGRESS|BPF_TC_EGRESS, the clsact qdisc will be
> deleted, also detaching all filters.
>
> As before, parent must be unset for these attach_points,
> and set for BPF_TC_CUSTOM. flags must also be unset.
>
> It is advised that if the qdisc is operated on by many programs,
> then the program at least check that there are no other existing
> filters before deleting the clsact qdisc. An example is shown
> below:
>
> DECLARE_LIBBPF_OPTS(bpf_tc_hook, .ifindex = if_nametoindex("lo"),
> .attach_point = BPF_TC_INGRESS);
> /* set opts as NULL, as we're not really interested in
> * getting any info for a particular filter, but just
> * detecting its presence.
> */
> r = bpf_tc_query(&hook, NULL);
> if (r == -ENOENT) {
> /* no filters */
> hook.attach_point = BPF_TC_INGRESS|BPF_TC_EGREESS;
> return bpf_tc_hook_destroy(&hook);
> } else {
> /* failed or r == 0, the latter means filters do exist */
> return r;
> }
>
> Note that there is a small race between checking for no
> filters and deleting the qdisc. This is currently unavoidable.
>
> Returns -EOPNOTSUPP when hook has attach_point as BPF_TC_CUSTOM.
>
> bpf_tc_attach - Attach a filter to a hook
> Parameters:
> @hook - Cannot be NULL. Represents the hook the filter will be
> attached to. Requirements for ifindex and attach_point are
> same as described in bpf_tc_hook_create, but BPF_TC_CUSTOM
> is also supported. In that case, parent must be set to the
> handle where the filter will be attached (using TC_H_MAKE).
> flags member must be unset.
>
> E.g. To set parent to 1:16 like in tc command line,
> the equivalent would be TC_H_MAKE(1 << 16, 16)
Small nit: I wonder whether from libbpf side we should just support a more
user friendly TC_H_MAKE, so you'd have: BPF_TC_CUSTOM + BPF_TC_PARENT(1, 16).
> @opts - Cannot be NULL.
>
> The following opts are optional:
> handle - The handle of the filter
> priority - The priority of the filter
> Must be >= 0 and <= UINT16_MAX
It should probably be mentioned that if they are not specified, then they
are auto-allocated from kernel.
> The following opts must be set:
> prog_fd - The fd of the loaded SCHED_CLS prog
> The following opts must be unset:
> prog_id - The ID of the BPF prog
> The following opts are optional:
> flags - Currently only BPF_TC_F_REPLACE is
> allowed. It allows replacing an existing
> filter instead of failing with -EEXIST.
>
> The following opts will be filled by bpf_tc_attach on a
> successful attach operation if they are unset:
> handle - The handle of the attached filter
> priority - The priority of the attached filter
> prog_id - The ID of the attached SCHED_CLS prog
>
> This way, the user can know what the auto allocated
> values for optional opts like handle and priority are
> for the newly attached filter, if they were unset.
>
> Note that some other attributes are set to some default
> values listed below (this holds for all bpf_tc_* APIs):
> protocol - ETH_P_ALL
> mode - direct action
> chain index - 0
> class ID - 0 (this can be set by writing to the
> skb->tc_classid field from the BPF program)
>
> bpf_tc_detach
> Parameters:
> @hook: Cannot be NULL. Represents the hook the filter will be
> detached from. Requirements are same as described above
> in bpf_tc_attach.
>
> @opts: Cannot be NULL.
>
> The following opts must be set:
> handle
> priority
> The following opts must be unset:
> prog_fd
> prog_id
> flags
>
> bpf_tc_query
> Parameters:
> @hook: Cannot be NULL. Represents the hook where the filter
> lookup will be performed. Requires are same as described
> above in bpf_tc_attach.
>
> @opts: Can be NULL.
Shouldn't it be: Cannot be NULL?
> The following opts are optional:
> handle
> priority
> prog_fd
> prog_id
What is the use case to set prog_fd here?
> The following opts must be unset:
> flags
>
> However, only one of prog_fd and prog_id must be
> set. Setting both leads to an error. Setting none is
> allowed.
>
> The following fields will be filled by bpf_tc_query on a
> successful lookup if they are unset:
> handle
> priority
> prog_id
>
> Based on the specified optional parameters, the matching
> data for the first matching filter is filled in and 0 is
> returned. When setting prog_fd, the prog_id will be
> matched against prog_id of the loaded SCHED_CLS prog
> represented by prog_fd.
>
> To uniquely identify a filter, e.g. to detect its presence,
> it is recommended to set both handle and priority fields.
What if prog_id is not unique, but part of multiple instances? Do we need
to support this case?
Why not just bpf_tc_query() with non-NULL hook and non-NULL opts where
handle and priority is required to be set, and rest must be 0?
> Some usage examples (using bpf skeleton infrastructure):
>
> BPF program (test_tc_bpf.c):
>
> #include <linux/bpf.h>
> #include <bpf/bpf_helpers.h>
>
> SEC("classifier")
> int cls(struct __sk_buff *skb)
> {
> return 0;
> }
>
> Userspace loader:
>
> DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, 0);
> struct test_tc_bpf *skel = NULL;
> int fd, r;
>
> skel = test_tc_bpf__open_and_load();
> if (!skel)
> return -ENOMEM;
>
> fd = bpf_program__fd(skel->progs.cls);
>
> DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex =
> if_nametoindex("lo"), .attach_point =
> BPF_TC_INGRESS);
> /* Create clsact qdisc */
> r = bpf_tc_hook_create(&hook);
> if (r < 0)
> goto end;
>
> DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .prog_fd = fd);
Given we had DECLARE_LIBBPF_OPTS earlier, can't we just set:
opts.prog_fd = fd here?
> r = bpf_tc_attach(&hook, &opts);
> if (r < 0)
> goto end;
> /* Print the auto allocated handle and priority */
> printf("Handle=%u", opts.handle);
> printf("Priority=%u", opts.priority);
>
> opts.prog_fd = opts.prog_id = 0;
> bpf_tc_detach(&hook, &opts);
Here we detach ...
> end:
> test_tc_bpf__destroy(skel);
>
> This is equivalent to doing the following using tc command line:
> # tc qdisc add dev lo clsact
> # tc filter add dev lo ingress bpf obj foo.o sec classifier da
... so this is not equivalent to your tc cmdline description.
> Another example replacing a filter (extending prior example):
>
> /* We can also choose both (or one), let's try replacing an
> * existing filter.
> */
> DECLARE_LIBBPF_OPTS(bpf_tc_opts, replace_opts, .handle =
> opts.handle, .priority = opts.priority,
> .prog_fd = fd);
> r = bpf_tc_attach(&hook, &replace_opts);
> if (r == -EEXIST) {
> /* Expected, now use BPF_TC_F_REPLACE to replace it */
> replace_opts.flags = BPF_TC_F_REPLACE;
> return bpf_tc_attach(&hook, &replace_opts);
> } else if (r < 0) {
> return r;
> }
> /* There must be no existing filter with these
> * attributes, so cleanup and return an error.
> */
> replace_opts.flags = replace_opts.prog_fd = replace_opts.prog_id = 0;
> bpf_tc_detach(&hook, &replace_opts);
> return -1;
>
> To obtain info of a particular filter:
>
> /* Find info for filter with handle 1 and priority 50 */
> DECLARE_LIBBPF_OPTS(bpf_tc_opts, info_opts, .handle = 1,
> .priority = 50);
> r = bpf_tc_query(&hook, &info_opts);
> if (r == -ENOENT)
> printf("Filter not found");
> else if (r < 0)
> return r;
> printf("Prog ID: %u", info_opts.prog_id);
> return 0;
>
> We can also match using prog_id to find the same filter:
>
> DECLARE_LIBBPF_OPTS(bpf_tc_opts, info_opts2, .prog_id =
> info_opts.prog_id);
> r = bpf_tc_query(&hook, &info_opts2);
> if (r == -ENOENT)
> printf("Filter not found");
> else if (r < 0)
> return r;
> /* If we know there's only one filter for this loaded prog,
> * it is safe to assert that the handle and priority are
> * as expected.
> */
> assert(info_opts2.handle == 1);
> assert(info_opts2.priority == 50);
What if a given prog_id is attached to multiple instances?
> return 0;
>
> Reviewed-by: Toke Høiland-Jørgensen <toke@...hat.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@...il.com>
> ---
> tools/lib/bpf/libbpf.h | 42 ++++
> tools/lib/bpf/libbpf.map | 5 +
> tools/lib/bpf/netlink.c | 473 ++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 519 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index bec4e6a6e31d..09d1a4fb10f9 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -775,6 +775,48 @@ LIBBPF_API int bpf_linker__add_file(struct bpf_linker *linker, const char *filen
> LIBBPF_API int bpf_linker__finalize(struct bpf_linker *linker);
> LIBBPF_API void bpf_linker__free(struct bpf_linker *linker);
>
> +enum bpf_tc_attach_point {
> + BPF_TC_INGRESS = 1 << 0,
> + BPF_TC_EGRESS = 1 << 1,
> + BPF_TC_CUSTOM = 1 << 2,
> +};
> +
> +enum bpf_tc_flags {
> + BPF_TC_F_REPLACE = 1 << 0,
> +};
> +
> +struct bpf_tc_hook {
> + size_t sz;
> + int ifindex;
> + int flags;
nit: __u32 flags; (or rather dropping as discussed)
> + enum bpf_tc_attach_point attach_point;
> + __u32 parent;
> + size_t :0;
> +};
> +
> +#define bpf_tc_hook__last_field parent
> +
> +struct bpf_tc_opts {
> + size_t sz;
> + int prog_fd;
> + int flags;
nit: __u32 flags;
> + __u32 prog_id;
> + __u32 handle;
> + __u32 priority;
> + size_t :0;
> +};
> +
> +#define bpf_tc_opts__last_field priority
> +
> +LIBBPF_API int bpf_tc_hook_create(struct bpf_tc_hook *hook);
> +LIBBPF_API int bpf_tc_hook_destroy(struct bpf_tc_hook *hook);
> +LIBBPF_API int bpf_tc_attach(const struct bpf_tc_hook *hook,
> + struct bpf_tc_opts *opts);
> +LIBBPF_API int bpf_tc_detach(const struct bpf_tc_hook *hook,
> + const struct bpf_tc_opts *opts);
> +LIBBPF_API int bpf_tc_query(const struct bpf_tc_hook *hook,
> + struct bpf_tc_opts *opts);
> +
> #ifdef __cplusplus
> } /* extern "C" */
> #endif
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index b9b29baf1df8..6c96729050dc 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -361,4 +361,9 @@ LIBBPF_0.4.0 {
> bpf_linker__new;
> bpf_map__inner_map;
> bpf_object__set_kversion;
> + bpf_tc_attach;
> + bpf_tc_detach;
> + bpf_tc_hook_create;
> + bpf_tc_hook_destroy;
> + bpf_tc_query;
> } LIBBPF_0.3.0;
> diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c
> index 8a01d9eed5f9..95c87f87a178 100644
> --- a/tools/lib/bpf/netlink.c
> +++ b/tools/lib/bpf/netlink.c
> @@ -4,7 +4,10 @@
> #include <stdlib.h>
> #include <memory.h>
> #include <unistd.h>
> +#include <arpa/inet.h>
> #include <linux/bpf.h>
> +#include <linux/if_ether.h>
> +#include <linux/pkt_cls.h>
> #include <linux/rtnetlink.h>
> #include <sys/socket.h>
> #include <errno.h>
> @@ -73,6 +76,12 @@ static int libbpf_netlink_open(__u32 *nl_pid)
> return ret;
> }
>
> +enum {
> + BPF_NL_CONT,
> + BPF_NL_NEXT,
> + BPF_NL_DONE,
nit: I don't think we need BPF_ prefix here given it's not specific to BPF.
> +};
> +
> static int bpf_netlink_recv(int sock, __u32 nl_pid, int seq,
> __dump_nlmsg_t _fn, libbpf_dump_nlmsg_t fn,
> void *cookie)
> @@ -84,6 +93,7 @@ static int bpf_netlink_recv(int sock, __u32 nl_pid, int seq,
> int len, ret;
>
> while (multipart) {
> +start:
> multipart = false;
> len = recv(sock, buf, sizeof(buf), 0);
> if (len < 0) {
[...]
Powered by blists - more mailing lists