| 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, 22 Apr 2020 18:19:03 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: David Ahern <dsahern@...nel.org>
Cc: Networking <netdev@...r.kernel.org>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Prashant Bhole <prashantbhole.linux@...il.com>,
Jason Wang <jasowang@...hat.com>,
Jesper Dangaard Brouer <brouer@...hat.com>,
Toke Høiland-Jørgensen <toke@...hat.com>,
Toshiaki Makita <toshiaki.makita1@...il.com>,
Daniel Borkmann <daniel@...earbox.net>,
john fastabend <john.fastabend@...il.com>,
Alexei Starovoitov <ast@...nel.org>, Martin Lau <kafai@...com>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
Andrii Nakryiko <andriin@...com>,
David Ahern <dsahern@...il.com>,
David Ahern <dahern@...italocean.com>
Subject: Re: [PATCH bpf-next 12/16] libbpf: Add egress XDP support
On Mon, Apr 20, 2020 at 1:02 PM David Ahern <dsahern@...nel.org> wrote:
>
> From: David Ahern <dahern@...italocean.com>
>
> Patch adds egress XDP support in libbpf.
>
> New section name hint, xdp_egress, is added to set expected attach
> type at program load. Programs can use xdp_egress as the prefix in
> the SEC statement to load the program with the BPF_XDP_EGRESS
> attach type set.
>
> egress is added to bpf_xdp_set_link_opts to specify egress type for
> use with bpf_set_link_xdp_fd_opts. Update library side to check
> for flag and set nla_type to IFLA_XDP_EGRESS.
>
> Add egress version of bpf_get_link_xdp* info and id apis with core
> code refactored to handle both rx and tx paths.
>
> Signed-off-by: Prashant Bhole <prashantbhole.linux@...il.com>
> Co-developed-by: David Ahern <dahern@...italocean.com>
> Signed-off-by: David Ahern <dahern@...italocean.com>
> ---
> tools/lib/bpf/libbpf.c | 2 ++
> tools/lib/bpf/libbpf.h | 9 +++++-
> tools/lib/bpf/libbpf.map | 2 ++
> tools/lib/bpf/netlink.c | 63 +++++++++++++++++++++++++++++++++++-----
> 4 files changed, 67 insertions(+), 9 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 8f480e29a6b0..32fc970495d9 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -6366,6 +6366,8 @@ static const struct bpf_sec_def section_defs[] = {
> .is_attach_btf = true,
> .expected_attach_type = BPF_LSM_MAC,
> .attach_fn = attach_lsm),
> + BPF_EAPROG_SEC("xdp_egress", BPF_PROG_TYPE_XDP,
> + BPF_XDP_EGRESS),
> BPF_PROG_SEC("xdp", BPF_PROG_TYPE_XDP),
> BPF_PROG_SEC("perf_event", BPF_PROG_TYPE_PERF_EVENT),
> BPF_PROG_SEC("lwt_in", BPF_PROG_TYPE_LWT_IN),
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index f1dacecb1619..3feb1242f78e 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -453,14 +453,16 @@ struct xdp_link_info {
> __u32 drv_prog_id;
> __u32 hw_prog_id;
> __u32 skb_prog_id;
> + __u32 egress_core_prog_id;
This changes layout of struct xdp_link_info in ABI-breaking way. New
fields have to be added at the end.
> __u8 attach_mode;
> };
>
> struct bpf_xdp_set_link_opts {
> size_t sz;
> int old_fd;
> + __u8 egress;
Is this a true/false field? If yes, why not bool then?
> };
> -#define bpf_xdp_set_link_opts__last_field old_fd
> +#define bpf_xdp_set_link_opts__last_field egress
>
> LIBBPF_API int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags);
> LIBBPF_API int bpf_set_link_xdp_fd_opts(int ifindex, int fd, __u32 flags,
> @@ -468,6 +470,11 @@ LIBBPF_API int bpf_set_link_xdp_fd_opts(int ifindex, int fd, __u32 flags,
> LIBBPF_API int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags);
> LIBBPF_API int bpf_get_link_xdp_info(int ifindex, struct xdp_link_info *info,
> size_t info_size, __u32 flags);
> +LIBBPF_API int bpf_get_link_xdp_egress_id(int ifindex, __u32 *prog_id,
> + __u32 flags);
> +LIBBPF_API int bpf_get_link_xdp_egress_info(int ifindex,
> + struct xdp_link_info *info,
> + size_t info_size, __u32 flags);
>
> struct perf_buffer;
>
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index bb8831605b25..51576c8a02fe 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -253,4 +253,6 @@ LIBBPF_0.0.8 {
> bpf_program__set_attach_target;
> bpf_program__set_lsm;
> bpf_set_link_xdp_fd_opts;
> + bpf_get_link_xdp_egress_id;
> + bpf_get_link_xdp_egress_info;
This should go into 0.0.9 section, 0.0.8 is sealed now.
> } LIBBPF_0.0.7;
[...]
> @@ -203,6 +204,7 @@ static int __bpf_set_link_xdp_fd_replace(int ifindex, int fd, int old_fd,
> int bpf_set_link_xdp_fd_opts(int ifindex, int fd, __u32 flags,
> const struct bpf_xdp_set_link_opts *opts)
> {
> + __u16 nla_type = IFLA_XDP;
> int old_fd = -1;
>
> if (!OPTS_VALID(opts, bpf_xdp_set_link_opts))
> @@ -213,14 +215,22 @@ int bpf_set_link_xdp_fd_opts(int ifindex, int fd, __u32 flags,
> flags |= XDP_FLAGS_REPLACE;
> }
>
> + if (OPTS_HAS(opts, egress)) {
I don't think you need to check OPTS_HAS here, just OPTS_GET with
proper default would work.
> + __u8 egress = OPTS_GET(opts, egress, 0);
> +
> + if (egress)
> + nla_type = IFLA_XDP_EGRESS;
> + }
> +
> return __bpf_set_link_xdp_fd_replace(ifindex, fd,
> old_fd,
> - flags);
> + flags,
nit: old_fd, flags fit on the same line as ifindex, fd, not sure why
there are so many lines for this?
> + nla_type);
> }
>
> int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags)
> {
> - return __bpf_set_link_xdp_fd_replace(ifindex, fd, 0, flags);
> + return __bpf_set_link_xdp_fd_replace(ifindex, fd, 0, flags, IFLA_XDP);
> }
>
[...]
>
> +int bpf_get_link_xdp_info(int ifindex, struct xdp_link_info *info,
> + size_t info_size, __u32 flags)
> +{
> + return __bpf_get_link_xdp_info(ifindex, info, info_size, flags,
> + IFLA_XDP);
> +}
> +
> +int bpf_get_link_xdp_egress_info(int ifindex, struct xdp_link_info *info,
> + size_t info_size, __u32 flags)
> +{
> + return __bpf_get_link_xdp_info(ifindex, info, info_size, flags,
> + IFLA_XDP_EGRESS);
> +}
> +
> static __u32 get_xdp_id(struct xdp_link_info *info, __u32 flags)
> {
> if (info->attach_mode != XDP_ATTACHED_MULTI && !flags)
> @@ -345,6 +376,22 @@ int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
> return ret;
> }
>
> +int bpf_get_link_xdp_egress_id(int ifindex, __u32 *prog_id, __u32 flags)
Is bpf_get_link_xdp_egress_id() even needed? This is a special case of
bpf_get_link_xdp_egress_info(), I don't think we have to add another
API to support it specifically.
Also, just curious, would it be better to have a generalized
XDP/XDP_EGRESS xdp_info() functions instead of two separate ones?
Could there be some third variant of XDP program later, whatever that
might be?
> +{
> + struct xdp_link_info info;
> + int ret;
> +
> + /* egress path does not support SKB, DRV or HW mode */
> + if (flags & XDP_FLAGS_MODES)
> + return -EINVAL;
> +
> + ret = bpf_get_link_xdp_egress_info(ifindex, &info, sizeof(info), flags);
> + if (!ret)
> + *prog_id = get_xdp_id(&info, flags);
> +
> + return ret;
> +}
> +
> int libbpf_nl_get_link(int sock, unsigned int nl_pid,
> libbpf_dump_nlmsg_t dump_link_nlmsg, void *cookie)
> {
> --
> 2.21.1 (Apple Git-122.3)
>
Powered by blists - more mailing lists