netdev - Re: [PATCH net-next v2 2/8] udp: skip L4 aggregation for UDP tunnel packets

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+FuTSfZnmz7pWQxOtnAEj9Dq1SQiTvpUoz8ySj_Tagmh6HtAQ@mail.gmail.com>
Date:   Fri, 26 Mar 2021 14:23:35 -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 2/8] udp: skip L4 aggregation for UDP tunnel packets

On Thu, Mar 25, 2021 at 1:24 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
> If NETIF_F_GRO_FRAGLIST or NETIF_F_GRO_UDP_FWD are enabled, and there
> are UDP tunnels available in the system, udp_gro_receive() could end-up
> doing L4 aggregation (either SKB_GSO_UDP_L4 or SKB_GSO_FRAGLIST) at
> the outer UDP tunnel level for packets effectively carrying and UDP
> tunnel header.
>
> That could cause inner protocol corruption. If e.g. the relevant
> packets carry a vxlan header, different vxlan ids will be ignored/
> aggregated to the same GSO packet. Inner headers will be ignored, too,
> so that e.g. TCP over vxlan push packets will be held in the GRO
> engine till the next flush, etc.
>
> Just skip the SKB_GSO_UDP_L4 and SKB_GSO_FRAGLIST code path if the
> current packet could land in a UDP tunnel, and let udp_gro_receive()
> do GRO via udp_sk(sk)->gro_receive.
>
> The check implemented in this patch is broader than what is strictly
> needed, as the existing UDP tunnel could be e.g. configured on top of
> a different device: we could end-up skipping GRO at-all for some packets.
>
> Anyhow, the latter is a very thin corner case and covering it would add
> quite a bit of complexity.
>
> v1 -> v2:
>  - hopefully clarify the commit message
>
> Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
> Fixes: 36707061d6ba ("udp: allow forwarding of plain (non-fraglisted) UDP GRO packets")
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>

Reviewed-by: Willem de Bruijn <willemb@...gle.com>

Key is that udp tunnel GRO must take precedence over transport GRO,
but the way the code is structured, the latter is tried first.