netdev - Re: [PATCH net] net: Fix gro aggregation for udp encaps with zero csum

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]

Date:   Sat, 27 Feb 2021 09:16:38 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        John Fastabend <john.fastabend@...il.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Tom Herbert <tom@...bertland.com>
Subject: Re: [PATCH net] net: Fix gro aggregation for udp encaps with zero
 csum

Willem de Bruijn wrote:
> On Fri, Feb 26, 2021 at 4:23 PM Daniel Borkmann <daniel@...earbox.net> wrote:
> >
> > We noticed a GRO issue for UDP-based encaps such as vxlan/geneve when the
> > csum for the UDP header itself is 0. In that case, GRO aggregation does
> > not take place on the phys dev, but instead is deferred to the vxlan/geneve
> > driver (see trace below).
> >
> > The reason is essentially that GRO aggregation bails out in udp_gro_receive()
> > for such case when drivers marked the skb with CHECKSUM_UNNECESSARY (ice, i40e,
> > others) where for non-zero csums 2abb7cdc0dc8 ("udp: Add support for doing
> > checksum unnecessary conversion") promotes those skbs to CHECKSUM_COMPLETE
> > and napi context has csum_valid set. This is however not the case for zero
> > UDP csum (here: csum_cnt is still 0 and csum_valid continues to be false).
> >
> > At the same time 57c67ff4bd92 ("udp: additional GRO support") added matches
> > on !uh->check ^ !uh2->check as part to determine candidates for aggregation,
> > so it certainly is expected to handle zero csums in udp_gro_receive(). The
> > purpose of the check added via 662880f44203 ("net: Allow GRO to use and set
> > levels of checksum unnecessary") seems to catch bad csum and stop aggregation
> > right away.
> >
> > One way to fix aggregation in the zero case is to only perform the !csum_valid
> > check in udp_gro_receive() if uh->check is infact non-zero.
> >

[...]

> We cannot do checksum conversion with zero field, but that does not
> have to limit coalescing.
> 
> CHECKSUM_COMPLETE with a checksum validated by
> skb_gro_checksum_validate_zero_check implies csum_valid.
> 
> So the test
> 
> >             (skb->ip_summed != CHECKSUM_PARTIAL &&
> >              NAPI_GRO_CB(skb)->csum_cnt == 0 &&
> >              !NAPI_GRO_CB(skb)->csum_valid) ||
> 
> Basically matches
> 
> - CHECKSUM_NONE
> - CHECKSUM_UNNECESSARY which has already used up its valid state on a
> prior header
> - CHECKSUM_COMPLETE with bad checksum.
> 
> This change just refines to not drop for in the first two cases on a
> zero checksum field.

+1

> 
> Making this explicit in case anyone sees holes in the logic. Else,
> 
> Acked-by: Willem de Bruijn <willemb@...gle.com>

LGTM,

Acked-by: John Fastabend <john.fastabend@...il.com>