netdev - Re: [RFC] net: add gro frag support to udp tunnel api

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: <20200130100159.GI27973@gauss3.secunet.de>
Date:   Thu, 30 Jan 2020 11:01:59 +0100
From:   Steffen Klassert <steffen.klassert@...unet.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
CC:     "Jason A. Donenfeld" <Jason@...c4.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [RFC] net: add gro frag support to udp tunnel api

On Mon, Jan 27, 2020 at 10:40:55AM -0500, Willem de Bruijn wrote:
> On Mon, Jan 27, 2020 at 10:25 AM Jason A. Donenfeld <Jason@...c4.com> wrote:
> >
> > Hi Steffen,
> >
> > This is very much a "RFC", in that the code here is fodder for
> > discussion more than something I'm seriously proposing at the moment.
> > I'm writing to you specifically because I recall us having discussed
> > something like this a while ago and you being interested.
> >
> > WireGuard would benefit from getting lists of SKBs all together in a
> > bunch on the receive side. At the moment, encap_rcv splits them apart
> > one by one before giving them to the API. This patch proposes a way to
> > opt-in to receiving them before they've been split. The solution
> > involves adding an encap_type flag that enables calling the encap_rcv
> > function earlier before the skbs have been split apart.
> >
> > I worry that it's not this straight forward, however, because of this
> > function below called, "udp_unexpected_gso". It looks like there's a
> > fast path for the case when it doesn't need to be split apart, and that
> > if it is already split apart, that's expected, whereas splitting it
> > apart would be "unexpected." I'm not too familiar with this code. Do you
> > know off hand why this would be unexpected?
> 
> This is for delivery to local sockets.
> 
> UDP GRO packets need to be split back up before delivery, unless the
> socket has set socket option UDP_GRO.
> 
> This is checked in the GRO layer by checking udp_sk(sk)->gro_enabled.
> 
> There is a race condition between this check and the packet arriving
> at the socket. Hence the unexpected.

Actually, if fraglist GRO is enabled this codepath is not that
unexpected anymore. Maybe we should remove the branch predictor
on !udp_unexpected_gso.

> 
> Note that UDP GSO can take two forms, the fraglist approach by Steffen
> or the earlier implementation that builds a single skb with frags.
> 
> >  static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> >  {
> > +       int (*encap_rcv_gro)(struct sock *sk, struct sk_buff *skb);
> >         struct sk_buff *next, *segs;
> >         int ret;
> >
> > +       if (static_branch_unlikely(&udp_encap_needed_key) &&
> > +           up->encap_type & UDP_ENCAP_SUPPORT_GRO_FRAGS &&
> > +           (encap_rcv_gro = READ_ONCE(up->encap_rcv))) {
> > +               //XXX: deal with checksum?
> > +               ret = encap_rcv(sk, skb);
> > +               if (ret <= 0) //XXX: deal with incrementing udp error stats?
> > +                       return -ret;
> > +       }
> 
> I think it'll be sufficient to just set optionally
> udp_sk(sk)->gro_enabled on encap sockets and let it takes the default
> path, below?

I think Jason wants to have the fraglist GRO version.
Just setting udp_sk(sk)->gro_enabled would generate
standard GRO packets. The UDP payload from wireguard
is encrypted, so merging into a single datagram does
not work that well here.

> 
> > +
> >         if (likely(!udp_unexpected_gso(sk, skb)))
> >                 return udp_queue_rcv_one_skb(sk, skb);
> >