netdev - Re: [PATCH net-next v2 4/8] udp: never accept GSO

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+FuTScNjt0dTEHM8WprhDZ5G3H0Y4af4fg2Xqs+eCCrNtHwVA@mail.gmail.com>
Date:   Fri, 26 Mar 2021 14:15:32 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Alexander Lobakin <alobakin@...me>
Subject: Re: [PATCH net-next v2 4/8] udp: never accept GSO_FRAGLIST packets

On Thu, Mar 25, 2021 at 1:24 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
> Currently the UDP protocol delivers GSO_FRAGLIST packets to
> the sockets without the expected segmentation.
>
> This change addresses the issue introducing and maintaining
> a couple of new fields to explicitly accept SKB_GSO_UDP_L4
> or GSO_FRAGLIST packets. Additionally updates  udp_unexpected_gso()
> accordingly.
>
> UDP sockets enabling UDP_GRO stil keep accept_udp_fraglist
> zeroed.
>
> v1 -> v2:
>  - use 2 bits instead of a whole GSO bitmask (Willem)
>
> Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>

This looks good to me in principle, thanks for the revision.

I hadn't fully appreciated that gro_enabled implies accept_udp_l4, but
not necessarily vice versa.

It is equivalent to (accept_udp_l4 && !up->gro_receive), right?

Could the extra bit be avoided with

"
+      /* Prefer fraglist GRO unless target is a socket with UDP_GRO,
+       * which requires all but last segments to be of same gso_size,
passed in cmsg */
        if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
-                NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;
+               NAPI_GRO_CB(skb)->is_flist = sk ?
(!udp_sk(sk)->gro_enabled || udp_sk(sk)->accept_udp_fraglist) : 1;

+     /* Apply transport layer GRO if forwarding is enabled or the
flow lands at a local socket */
       if ((!sk && (skb->dev->features & NETIF_F_GRO_UDP_FWD)) ||
            (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) ||
NAPI_GRO_CB(skb)->is_flist) {
                pp = call_gro_receive(udp_gro_receive_segment, head, skb);
                return pp;
        }

+      /* Continue with tunnel GRO */
"

.. not that the extra bit matters a lot. And these two conditions with
gro_enabled are not very obvious.

Just a thought.