netdev - Re: [PATCH net-next RFC 7/8] udp: gro behind static key

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: <CAF=yD-LeY-cLDg=ecK_rETvOMFiChxgZ3--gtSpMfea=HOhMHA@mail.gmail.com>
Date:   Sun, 16 Sep 2018 14:10:39 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
Cc:     Network Development <netdev@...r.kernel.org>,
        Paolo Abeni <pabeni@...hat.com>, steffen.klassert@...unet.com,
        David Miller <davem@...emloft.net>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net-next RFC 7/8] udp: gro behind static key

On Fri, Sep 14, 2018 at 11:37 PM Subash Abhinov Kasiviswanathan
<subashab@...eaurora.org> wrote:
>
> On 2018-09-14 11:59, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@...gle.com>
> >
> > Avoid the socket lookup cost in udp_gro_receive if no socket has a
> > gro callback configured.
> >
> > Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> > index 4f6aa95a9b12..f44fe328aa0f 100644
> > --- a/net/ipv4/udp_offload.c
> > +++ b/net/ipv4/udp_offload.c
> > @@ -405,7 +405,7 @@ static struct sk_buff *udp4_gro_receive(struct
> > list_head *head,
> >  {
> >       struct udphdr *uh = udp_gro_udphdr(skb);
> >
> > -     if (unlikely(!uh))
> > +     if (unlikely(!uh) ||
> > !static_branch_unlikely(&udp_encap_needed_key))
> >               goto flush;
> >
>
> Hi Willem
>
> Does this need to be
>
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 6dd3f0a..fcd5589 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -407,7 +407,7 @@ static struct sk_buff *udp4_gro_receive(struct
> list_head *head,
>   {
>          struct udphdr *uh = udp_gro_udphdr(skb);
>
> -       if (unlikely(!uh) ||
> !static_branch_unlikely(&udp_encap_needed_key))
> +       if (unlikely(!uh) ||
> static_branch_unlikely(&udp_encap_needed_key))
>                  goto flush;
>
>          /* Don't bother verifying checksum if we're going to flush
> anyway. */
>
> I tried setting UDP_GRO socket option and I had to make this change to
> exercise the udp_gro_receive_cb code path.

Thanks for trying it out, Subash.

The static_branch logic is correct as is. It skips the gro logic if
the static key is not enabled.

But there is indeed a bug. the UDP_GRO setsockopt has to enable
the static key. A full patch is more complete, but this should fix it for
now:

--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2506,6 +2506,7 @@ int udp_lib_setsockopt(struct sock *sk, int
level, int optname,
                        if (!udp_sk(sk)->gro_receive) {
                                udp_sk(sk)->gro_complete = udp_gro_complete_cb;
                                udp_sk(sk)->gro_receive = udp_gro_receive_cb;
+                               udp_encap_enable();

sorry about that. I had tested the two patches independently, but
apparently not on top of each other.

Independent of udp gro, I tested the static key by running udp_rr
with perf record -a -g and looking for __udp4_lib_lookup. With the
patch, all calls are from __udp4_lib_rcv else udp_gro_receive also
shows up. I had to stress the porttable by opening ~32K ports.

Also, the simple patch to udpgso_bench_rx.c that I used to test
gro:

diff --git a/tools/testing/selftests/net/udpgso_bench_rx.c
b/tools/testing/selftests/net/udpgso_bench_rx.c
index 727cf67a3f75..35ba8567fc56 100644
--- a/tools/testing/selftests/net/udpgso_bench_rx.c
+++ b/tools/testing/selftests/net/udpgso_bench_rx.c
@@ -31,6 +31,11 @@
 #include <sys/wait.h>
 #include <unistd.h>

+#ifndef UDP_GRO
+#define UDP_GRO                104
+#endif
+
+static bool cfg_do_gro;
 static int  cfg_port           = 8000;
 static bool cfg_tcp;
 static bool cfg_verify;
@@ -89,6 +94,11 @@ static int do_socket(bool do_tcp)
        if (setsockopt(fd, SOL_SOCKET, SO_REUSEPORT, &val, sizeof(val)))
                error(1, errno, "setsockopt reuseport");

+       if (cfg_do_gro) {
+               if (setsockopt(fd, SOL_UDP, UDP_GRO, &val, sizeof(val)))
+                       error(1, errno, "setsockopt gro");
+       }
+
        addr.sin6_family =      PF_INET6;
        addr.sin6_port =        htons(cfg_port);
        addr.sin6_addr =        in6addr_any;
@@ -199,8 +209,11 @@ static void parse_opts(int argc, char **argv)
 {
        int c;

-       while ((c = getopt(argc, argv, "ptv")) != -1) {
+       while ((c = getopt(argc, argv, "gptv")) != -1) {
                switch (c) {
+               case 'g':
+                       cfg_do_gro = true;
+                       break;
                case 'p':
                        cfg_port = htons(strtoul(optarg, NULL, 0));
                        break;