netdev - Re: [bpf-next PATCH 2/2] samples/bpf: xdp_rxq_info action XDP

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAPhsuW74ZzHawToa3YXxzT6ojk+ScdCvcO3KyT3EBcCxqPHDXg@mail.gmail.com>
Date:   Tue, 26 Jun 2018 17:09:01 -0700
From:   Song Liu <liu.song.a23@...il.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     Networking <netdev@...r.kernel.org>,
        Daniel Borkmann <borkmann@...earbox.net>,
        Toke Høiland-Jørgensen <toke@...e.dk>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Subject: Re: [bpf-next PATCH 2/2] samples/bpf: xdp_rxq_info action XDP_TX must
 adjust MAC-addrs

On Mon, Jun 25, 2018 at 7:27 AM, Jesper Dangaard Brouer
<brouer@...hat.com> wrote:
> XDP_TX requires also changing the MAC-addrs, else some hardware
> may drop the TX packet before reaching the wire.  This was
> observed with driver mlx5.
>
> If xdp_rxq_info select --action XDP_TX the swapmac functionality
> is activated.  It is also possible to manually enable via cmdline
> option --swapmac.  This is practical if wanting to measure the
> overhead of writing/updating payload for other action types.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@...e.dk>
> ---
>  samples/bpf/xdp_rxq_info_kern.c |   26 +++++++++++++++++++++++++-
>  samples/bpf/xdp_rxq_info_user.c |   11 +++++++++++
>  2 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/samples/bpf/xdp_rxq_info_kern.c b/samples/bpf/xdp_rxq_info_kern.c
> index 61af6210df2f..222a83eed1cb 100644
> --- a/samples/bpf/xdp_rxq_info_kern.c
> +++ b/samples/bpf/xdp_rxq_info_kern.c
> @@ -21,6 +21,7 @@ struct config {
>  enum cfg_options_flags {
>         NO_TOUCH = 0x0U,
>         READ_MEM = 0x1U,
> +       SWAP_MAC = 0x2U,
>  };
>  struct bpf_map_def SEC("maps") config_map = {
>         .type           = BPF_MAP_TYPE_ARRAY,
> @@ -52,6 +53,23 @@ struct bpf_map_def SEC("maps") rx_queue_index_map = {
>         .max_entries    = MAX_RXQs + 1,
>  };
>
> +static __always_inline
> +void swap_src_dst_mac(void *data)
> +{
> +       unsigned short *p = data;
> +       unsigned short dst[3];
> +
> +       dst[0] = p[0];
> +       dst[1] = p[1];
> +       dst[2] = p[2];
> +       p[0] = p[3];
> +       p[1] = p[4];
> +       p[2] = p[5];
> +       p[3] = dst[0];
> +       p[4] = dst[1];
> +       p[5] = dst[2];
> +}
> +
>  SEC("xdp_prog0")
>  int  xdp_prognum0(struct xdp_md *ctx)
>  {
> @@ -98,7 +116,7 @@ int  xdp_prognum0(struct xdp_md *ctx)
>                 rxq_rec->issue++;
>
>         /* Default: Don't touch packet data, only count packets */
> -       if (unlikely(config->options & READ_MEM)) {
> +       if (unlikely(config->options & (READ_MEM|SWAP_MAC))) {
>                 struct ethhdr *eth = data;
>
>                 if (eth + 1 > data_end)
> @@ -107,6 +125,12 @@ int  xdp_prognum0(struct xdp_md *ctx)
>                 /* Avoid compiler removing this: Drop non 802.3 Ethertypes */
>                 if (ntohs(eth->h_proto) < ETH_P_802_3_MIN)
>                         return XDP_ABORTED;
> +
> +               /* XDP_TX requires changing MAC-addrs, else HW may drop.
> +                * Can also be enabled with --swapmac (for test purposes)
> +                */
> +               if (unlikely(config->options & SWAP_MAC))
> +                       swap_src_dst_mac(data);
>         }
>
>         return config->action;
> diff --git a/samples/bpf/xdp_rxq_info_user.c b/samples/bpf/xdp_rxq_info_user.c
> index 435485d4f49e..248a7eab9531 100644
> --- a/samples/bpf/xdp_rxq_info_user.c
> +++ b/samples/bpf/xdp_rxq_info_user.c
> @@ -51,6 +51,7 @@ static const struct option long_options[] = {
>         {"no-separators", no_argument,          NULL, 'z' },
>         {"action",      required_argument,      NULL, 'a' },
>         {"readmem",     no_argument,            NULL, 'r' },
> +       {"swapmac",     no_argument,            NULL, 'm' },
>         {0, 0, NULL,  0 }
>  };
>
> @@ -72,6 +73,7 @@ struct config {
>  enum cfg_options_flags {
>         NO_TOUCH = 0x0U,
>         READ_MEM = 0x1U,
> +       SWAP_MAC = 0x2U,
>  };
>  #define XDP_ACTION_MAX (XDP_TX + 1)
>  #define XDP_ACTION_MAX_STRLEN 11
> @@ -119,6 +121,8 @@ static char* options2str(enum cfg_options_flags flag)
>  {
>         if (flag == NO_TOUCH)
>                 return "no_touch";
> +       if (flag & SWAP_MAC)
> +               return "swapmac";
>         if (flag & READ_MEM)
>                 return "read";

I guess SWAP_MAC also reads the memory, so it "includes" READ_MEM?
It is OK for now. We may need to refactor this part when adding other
flags in the
future.

Thanks,
Song


>         fprintf(stderr, "ERR: Unknown config option flags");
> @@ -517,6 +521,9 @@ int main(int argc, char **argv)
>                 case 'r':
>                         cfg_options |= READ_MEM;
>                         break;
> +               case 'm':
> +                       cfg_options |= SWAP_MAC;
> +                       break;
>                 case 'h':
>                 error:
>                 default:
> @@ -543,6 +550,10 @@ int main(int argc, char **argv)
>                 }
>         }
>         cfg.action = action;
> +
> +       /* XDP_TX requires changing MAC-addrs, else HW may drop */
> +       if (action == XDP_TX)
> +               cfg_options |= SWAP_MAC;
>         cfg.options = cfg_options;
>
>         /* Trick to pretty printf with thousands separators use %' */
>