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: <CA+FuTSecc8ZzNL+8RvYUj4n_iTWCy4-vV46eCWtsHenT9u96QQ@mail.gmail.com>
Date:   Mon, 27 Jan 2020 10:40:55 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     "Jason A. Donenfeld" <Jason@...c4.com>,
        Paolo Abeni <pabeni@...hat.com>
Cc:     Steffen Klassert <steffen.klassert@...unet.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: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.

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?

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