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-J=i3LtO17DQ0noEhnXxC=ebpEgBwLjAA=UM546to-BCQ@mail.gmail.com>
Date:   Mon, 17 Sep 2018 10:19:22 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     steffen.klassert@...unet.com
Cc:     Network Development <netdev@...r.kernel.org>,
        Paolo Abeni <pabeni@...hat.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 Mon, Sep 17, 2018 at 6:37 AM Steffen Klassert
<steffen.klassert@...unet.com> wrote:
>
> On Fri, Sep 14, 2018 at 01:59:40PM -0400, 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.
>
> It would be nice if we could do GRO not just for GRO configured
> sockets, but also for flows that are going to be IPsec transformed
> or directly forwarded.

I thought about that, as we have GSO. An egregious hack enables
GRO for all registered local sockets that support it and for any flow
for which no local socket is registered:

@@ -365,11 +369,13 @@ struct sk_buff *udp_gro_receive(struct list_head
*head, struct sk_buff *skb,
        rcu_read_lock();
        sk = (*lookup)(skb, uh->source, uh->dest);

-       if (sk && udp_sk(sk)->gro_receive)
-               goto unflush;
-       goto out_unlock;
+       if (!sk)
+               gro_receive_cb = udp_gro_receive_cb;
+       else if (!udp_sk(sk)->gro_receive)
+               goto out_unlock;
+       else
+               gro_receive_cb = udp_sk(sk)->gro_receive;

@@ -392,7 +398,7 @@ struct sk_buff *udp_gro_receive(struct list_head
*head, struct sk_buff *skb,

        skb_gro_pull(skb, sizeof(struct udphdr)); /* pull
encapsulating udp header */
        skb_gro_postpull_rcsum(skb, uh, sizeof(struct udphdr));
-       pp = call_gro_receive_sk(udp_sk(sk)->gro_receive, sk, head, skb);
+       pp = call_gro_receive_sk(gro_receive_cb, sk, head, skb);


But not having a local socket does not imply forwarding path, of course.

>
> Maybe in case that forwarding is enabled on the receiving device,
> inet_gro_receive() could do a route lookup and allow GRO if the
> route lookup returned at forwarding route.

That's a better solution, if the cost is acceptable. We do have to
be careful against increasing per packet cycle cost in this path
given that it's a possible vector for DoS attempts.

> For flows that are likely software segmented after that, it
> would be worth to build packet chains insted of merging the
> payload. Packets of the same flow could travel together, but
> it would save the cost of the packet merging and segmenting.

With software GSO that is faster, as it would have to allocate
all the separate segment skbs in skb_segment later. Though
there is some complexity if MTUs differ.

With hardware UDP GSO, having a single skb will be cheaper in
the forwarding path. Using napi_gro_frags, device drivers really
do only end up allocating one skb for the GSO packet.

> This could be done similar to what I proposed for the list
> receive case:
>
> https://www.spinics.net/lists/netdev/msg522706.html
>
> How GRO should be done could be even configured
> by replacing the net_offload pointer similar
> to what Paolo propsed in his pachset with
> the inet_update_offload() function.

Right. The above hack also already has to use two distinct
callback assignments.