Netdev Archive on lore.kernel.org
 help / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>
Cc: Networking <netdev@vger.kernel.org>,
	Jesper Dangaard Brouer <brouer@redhat.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Alexei Starovoitov <ast@kernel.org>,
	David Miller <davem@davemloft.net>,
	Jonathan Lemon <jonathan.lemon@gmail.com>
Subject: Re: [PATCH bpf-next v4 2/3] bpf_xdp_redirect_map: Perform map lookup in eBPF helper
Date: Fri, 14 Jun 2019 16:07:08 -0700
Message-ID: <CAEf4BzZq4FBoFaaUCDnA8p=gRkUtzrDHOuYGibq1_98sPqaRUQ@mail.gmail.com> (raw)
In-Reply-To: <156042464155.25684.9001494922674130772.stgit@alrua-x1>

On Thu, Jun 13, 2019 at 8:31 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> The bpf_redirect_map() helper used by XDP programs doesn't return any
> indication of whether it can successfully redirect to the map index it was
> given. Instead, BPF programs have to track this themselves, leading to
> programs using duplicate maps to track which entries are populated in the
> devmap.
>
> This patch fixes this by moving the map lookup into the bpf_redirect_map()
> helper, which makes it possible to return failure to the eBPF program. The
> lower bits of the flags argument is used as the return code, which means
> that existing users who pass a '0' flag argument will get XDP_ABORTED.

I see that we have absolutely no documentation for
bpf_xdp_redirect_map. Can you please add it to
include/uapi/linux/bpf.h? Don't forget to mention this handling of
lower bits of flags. Thanks!

>
> With this, a BPF program can check the return code from the helper call and
> react by, for instance, substituting a different redirect. This works for
> any type of map used for redirect.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  include/linux/filter.h |    1 +
>  net/core/filter.c      |   27 +++++++++++++--------------
>  2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 43b45d6db36d..f31ae8b9035a 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -580,6 +580,7 @@ struct bpf_skb_data_end {
>  struct bpf_redirect_info {
>         u32 ifindex;
>         u32 flags;
> +       void *item;

This is so generic name that some short comment describing what that
item is would help a lot.

>         struct bpf_map *map;
>         struct bpf_map *map_to_flush;
>         u32 kern_flags;
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 7a996887c500..7d742ea61e2d 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3608,17 +3608,13 @@ static int xdp_do_redirect_map(struct net_device *dev, struct xdp_buff *xdp,
>                                struct bpf_redirect_info *ri)
>  {
>         u32 index = ri->ifindex;
> -       void *fwd = NULL;
> +       void *fwd = ri->item;
>         int err;
>
>         ri->ifindex = 0;
> +       ri->item = NULL;
>         WRITE_ONCE(ri->map, NULL);
>
> -       fwd = __xdp_map_lookup_elem(map, index);
> -       if (unlikely(!fwd)) {
> -               err = -EINVAL;
> -               goto err;
> -       }
>         if (ri->map_to_flush && unlikely(ri->map_to_flush != map))
>                 xdp_do_flush_map();
>
> @@ -3655,18 +3651,13 @@ static int xdp_do_generic_redirect_map(struct net_device *dev,
>  {
>         struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
>         u32 index = ri->ifindex;
> -       void *fwd = NULL;
> +       void *fwd = ri->item;
>         int err = 0;
>
>         ri->ifindex = 0;
> +       ri->item = NULL;
>         WRITE_ONCE(ri->map, NULL);
>
> -       fwd = __xdp_map_lookup_elem(map, index);
> -       if (unlikely(!fwd)) {
> -               err = -EINVAL;
> -               goto err;
> -       }
> -
>         if (map->map_type == BPF_MAP_TYPE_DEVMAP) {
>                 struct bpf_dtab_netdev *dst = fwd;
>
> @@ -3735,6 +3726,7 @@ BPF_CALL_2(bpf_xdp_redirect, u32, ifindex, u64, flags)
>
>         ri->ifindex = ifindex;
>         ri->flags = flags;
> +       ri->item = NULL;
>         WRITE_ONCE(ri->map, NULL);
>
>         return XDP_REDIRECT;
> @@ -3753,9 +3745,16 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct bpf_map *, map, u32, ifindex,
>  {
>         struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
>
> -       if (unlikely(flags))
> +       /* Lower bits of the flags are used as return code on lookup failure */
> +       if (unlikely(flags > XDP_TX))
>                 return XDP_ABORTED;
>
> +       ri->item = __xdp_map_lookup_elem(map, ifindex);
> +       if (unlikely(!ri->item)) {
> +               WRITE_ONCE(ri->map, NULL);
> +               return flags;
> +       }
> +
>         ri->ifindex = ifindex;
>         ri->flags = flags;
>         WRITE_ONCE(ri->map, map);
>

  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-13 11:17 [PATCH bpf-next v4 0/3] xdp: Allow lookup into devmaps before redirect Toke Høiland-Jørgensen
2019-06-13 11:17 ` [PATCH bpf-next v4 3/3] devmap: Allow map lookups from eBPF Toke Høiland-Jørgensen
2019-06-15  0:08   ` Andrii Nakryiko
2019-06-13 11:17 ` [PATCH bpf-next v4 2/3] bpf_xdp_redirect_map: Perform map lookup in eBPF helper Toke Høiland-Jørgensen
2019-06-14 23:07   ` Andrii Nakryiko [this message]
2019-06-15 10:11     ` Toke Høiland-Jørgensen
2019-06-13 11:17 ` [PATCH bpf-next v4 1/3] devmap/cpumap: Use flush list instead of bitmap Toke Høiland-Jørgensen
2019-06-13 18:36   ` Andrii Nakryiko
2019-06-13 18:40 ` [PATCH bpf-next v4 0/3] xdp: Allow lookup into devmaps before redirect Jonathan Lemon

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEf4BzZq4FBoFaaUCDnA8p=gRkUtzrDHOuYGibq1_98sPqaRUQ@mail.gmail.com' \
    --to=andrii.nakryiko@gmail.com \
    --cc=ast@kernel.org \
    --cc=brouer@redhat.com \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=jonathan.lemon@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=toke@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox