| 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
| ||
|
Message-ID: <CAEf4BzbRKMJRZvF2L+jfMPHo1WFmGFcQ-GPWAD9s=v+r+V3o=Q@mail.gmail.com>
Date: Wed, 5 Oct 2022 20:19:43 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Daniel Borkmann <daniel@...earbox.net>
Cc: bpf@...r.kernel.org, razor@...ckwall.org, ast@...nel.org,
andrii@...nel.org, martin.lau@...ux.dev, john.fastabend@...il.com,
joannelkoong@...il.com, memxor@...il.com, toke@...hat.com,
joe@...ium.io, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next 08/10] libbpf: Add support for BPF tc link
On Tue, Oct 4, 2022 at 4:12 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> Implement tc BPF link support for libbpf. The bpf_program__attach_fd()
> API has been refactored slightly in order to pass bpf_link_create_opts.
> A new bpf_program__attach_tc() has been added on top of this which allows
> for passing ifindex and prio parameters.
>
> New sections are tc/ingress and tc/egress which map to BPF_NET_INGRESS
> and BPF_NET_EGRESS, respectively.
>
> Co-developed-by: Nikolay Aleksandrov <razor@...ckwall.org>
> Signed-off-by: Nikolay Aleksandrov <razor@...ckwall.org>
> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
> ---
> tools/lib/bpf/bpf.c | 4 ++++
> tools/lib/bpf/bpf.h | 3 +++
> tools/lib/bpf/libbpf.c | 31 ++++++++++++++++++++++++++-----
> tools/lib/bpf/libbpf.h | 2 ++
> tools/lib/bpf/libbpf.map | 1 +
> 5 files changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index d1e338ac9a62..f73fdecbb5f8 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -752,6 +752,10 @@ int bpf_link_create(int prog_fd, int target_fd,
should we rename target_fd into more generic "target" maybe?
> if (!OPTS_ZEROED(opts, tracing))
> return libbpf_err(-EINVAL);
> break;
> + case BPF_NET_INGRESS:
> + case BPF_NET_EGRESS:
> + attr.link_create.tc.priority = OPTS_GET(opts, tc.priority, 0);
> + break;
> default:
> if (!OPTS_ZEROED(opts, flags))
> return libbpf_err(-EINVAL);
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 96de58fecdbc..937583421327 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -334,6 +334,9 @@ struct bpf_link_create_opts {
> struct {
> __u64 cookie;
> } tracing;
> + struct {
> + __u32 priority;
> + } tc;
> };
> size_t :0;
> };
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 184ce1684dcd..6eb33e4324ad 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -8474,6 +8474,8 @@ static const struct bpf_sec_def section_defs[] = {
> SEC_DEF("kretsyscall+", KPROBE, 0, SEC_NONE, attach_ksyscall),
> SEC_DEF("usdt+", KPROBE, 0, SEC_NONE, attach_usdt),
> SEC_DEF("tc", SCHED_CLS, 0, SEC_NONE),
> + SEC_DEF("tc/ingress", SCHED_CLS, BPF_NET_INGRESS, SEC_ATTACHABLE_OPT),
> + SEC_DEF("tc/egress", SCHED_CLS, BPF_NET_EGRESS, SEC_ATTACHABLE_OPT),
btw, we could implement optionally the ability to declaratively
specify priority, so that you could do SEC("tc/ingress:10") or some
syntax like that, if that seems useful in practice. If you expect that
prio is going to be dynamic most of the time, then it might not make
sense to add unnecessary parsing code
> SEC_DEF("classifier", SCHED_CLS, 0, SEC_NONE),
> SEC_DEF("action", SCHED_ACT, 0, SEC_NONE),
> SEC_DEF("tracepoint+", TRACEPOINT, 0, SEC_NONE, attach_tp),
> @@ -11238,11 +11240,10 @@ static int attach_lsm(const struct bpf_program *prog, long cookie, struct bpf_li
> }
>
> static struct bpf_link *
> -bpf_program__attach_fd(const struct bpf_program *prog, int target_fd, int btf_id,
> - const char *target_name)
> +bpf_program__attach_fd_opts(const struct bpf_program *prog,
> + const struct bpf_link_create_opts *opts,
> + int target_fd, const char *target_name)
let's move opts to be last argument or second to last before
"target_name", whichever makes more sense to you
also fd part is a lie, and whole double-underscore naming is also bad
here because this is internal helper. Let's rename this to something
like bpf_prog_create_link()?
> {
> - DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts,
> - .target_btf_id = btf_id);
> enum bpf_attach_type attach_type;
> char errmsg[STRERR_BUFSIZE];
> struct bpf_link *link;
> @@ -11260,7 +11261,7 @@ bpf_program__attach_fd(const struct bpf_program *prog, int target_fd, int btf_id
> link->detach = &bpf_link__detach_fd;
>
> attach_type = bpf_program__expected_attach_type(prog);
> - link_fd = bpf_link_create(prog_fd, target_fd, attach_type, &opts);
> + link_fd = bpf_link_create(prog_fd, target_fd, attach_type, opts);
> if (link_fd < 0) {
> link_fd = -errno;
> free(link);
> @@ -11273,6 +11274,16 @@ bpf_program__attach_fd(const struct bpf_program *prog, int target_fd, int btf_id
> return link;
> }
>
> +static struct bpf_link *
> +bpf_program__attach_fd(const struct bpf_program *prog, int target_fd, int btf_id,
> + const char *target_name)
there seems to be only one use case where we have btf_id != 0, so I
think we should just use LIBBPF_OPTS() explicitly in that one case and
for all other current uses of bpf_program__attach_fd() just use opts
variant and pass NULL
> +{
> + DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts,
> + .target_btf_id = btf_id);
> +
> + return bpf_program__attach_fd_opts(prog, &opts, target_fd, target_name);
> +}
> +
> struct bpf_link *
> bpf_program__attach_cgroup(const struct bpf_program *prog, int cgroup_fd)
> {
> @@ -11291,6 +11302,16 @@ struct bpf_link *bpf_program__attach_xdp(const struct bpf_program *prog, int ifi
> return bpf_program__attach_fd(prog, ifindex, 0, "xdp");
> }
>
> +struct bpf_link *bpf_program__attach_tc(const struct bpf_program *prog,
> + int ifindex, __u32 priority)
> +{
> + DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts,
> + .tc.priority = priority);
> +
nit: please just use shorter LIBBPF_OPTS in new code, nice and short
> + /* target_fd/target_ifindex use the same field in LINK_CREATE */
> + return bpf_program__attach_fd_opts(prog, &opts, ifindex, "tc");
> +}
> +
> struct bpf_link *bpf_program__attach_freplace(const 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 eee883f007f9..7e64cec9a1ba 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -645,6 +645,8 @@ bpf_program__attach_netns(const struct bpf_program *prog, int netns_fd);
> LIBBPF_API struct bpf_link *
> bpf_program__attach_xdp(const struct bpf_program *prog, int ifindex);
> LIBBPF_API struct bpf_link *
> +bpf_program__attach_tc(const struct bpf_program *prog, int ifindex, __u32 priority);
> +LIBBPF_API struct bpf_link *
> bpf_program__attach_freplace(const struct bpf_program *prog,
> int target_fd, const char *attach_func_name);
>
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 0c94b4862ebb..473ed71829c6 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -378,4 +378,5 @@ LIBBPF_1.1.0 {
> user_ring_buffer__reserve_blocking;
> user_ring_buffer__submit;
> bpf_prog_detach_opts;
> + bpf_program__attach_tc;
same about alphabetical order
> } LIBBPF_1.0.0;
> --
> 2.34.1
>
Powered by blists - more mailing lists