Date:   Fri, 26 Oct 2018 18:53:28 -0700
From:   Maciej Żenczykowski <zenczykowski@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     stranche@...eaurora.org, Eric Dumazet <eric.dumazet@...il.com>,
        Linux NetDev <netdev@...r.kernel.org>,
        samanthakumar@...gle.com, Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net v2] net: udp: fix handling of CHECKSUM_COMPLETE packets

Possibly worth mentioning that without this fix you can also end up
with valid udp packets being dropped (ie. the reverse of the commit
description which talks about receiving invalid ones).

The (approximate?) requirement is:
(a) nic generates CHECKSUM_COMPLETE, but gets the actual checksum wrong
(b) udp packet is longer then 76 bytes L2 (CHECKSUM_BREAK), so we
don't calculate checksum inline prior to pulling udp header
(c) udp socket has no bpf filter, so we delay checksum until we do
copy to userspace from recvmsg()

76 bytes is 76 - 14 ether - 20 ipv4 - 8 udp header = 34 udp ipv4 payload bytes
On Wed, Oct 24, 2018 at 2:22 PM David Miller <davem@...emloft.net> wrote:
>
> From: Sean Tranchetti <stranche@...eaurora.org>
> Date: Tue, 23 Oct 2018 16:04:31 -0600
>
> > Current handling of CHECKSUM_COMPLETE packets by the UDP stack is
> > incorrect for any packet that has an incorrect checksum value.
> >
> > udp4/6_csum_init() will both make a call to
> > __skb_checksum_validate_complete() to initialize/validate the csum
> > field when receiving a CHECKSUM_COMPLETE packet. When this packet
> > fails validation, skb->csum will be overwritten with the pseudoheader
> > checksum so the packet can be fully validated by software, but the
> > skb->ip_summed value will be left as CHECKSUM_COMPLETE so that way
> > the stack can later warn the user about their hardware spewing bad
> > checksums. Unfortunately, leaving the SKB in this state can cause
> > problems later on in the checksum calculation.
> >
> > Since the the packet is still marked as CHECKSUM_COMPLETE,
> > udp_csum_pull_header() will SUBTRACT the checksum of the UDP header
> > from skb->csum instead of adding it, leaving us with a garbage value
> > in that field. Once we try to copy the packet to userspace in the
> > udp4/6_recvmsg(), we'll make a call to skb_copy_and_csum_datagram_msg()
> > to checksum the packet data and add it in the garbage skb->csum value
> > to perform our final validation check.
> >
> > Since the value we're validating is not the proper checksum, it's possible
> > that the folded value could come out to 0, causing us not to drop the
> > packet. Instead, we believe that the packet was checksummed incorrectly
> > by hardware since skb->ip_summed is still CHECKSUM_COMPLETE, and we attempt
> > to warn the user with netdev_rx_csum_fault(skb->dev);
> >
> > Unfortunately, since this is the UDP path, skb->dev has been overwritten
> > by skb->dev_scratch and is no longer a valid pointer, so we end up
> > reading invalid memory.
>
> Just want to say that it has always been complicated in this area due to
> the fact that we do this deferral of final checksum validation to when we
> copy the packet into userspace.  For example, poll() needs to do special
> things, etc.
>
> Because we have to make it seem as if we dropped the packet with a bad
> checksum from the point of view of what the user sees during recvmsg()
> and poll() calls.  But until we do that checksum validation, we don't
> know exactly what the situation is.
>
> > This patch addresses this problem in two ways:
> >       1) Do not use the dev pointer when calling netdev_rx_csum_fault()
> >          from skb_copy_and_csum_datagram_msg(). Since this gets called
> >          from the UDP path where skb->dev has been overwritten, we have
> >          no way of knowing if the pointer is still valid. Also for the
> >          sake of consistency with the other uses of
> >          netdev_rx_csum_fault(), don't attempt to call it if the
> >          packet was checksummed by software.
> >
> >       2) Add better CHECKSUM_COMPLETE handling to udp4/6_csum_init().
> >          If we receive a packet that's CHECKSUM_COMPLETE that fails
> >          verification (i.e. skb->csum_valid == 0), check who performed
> >          the calculation. It's possible that the checksum was done in
> >          software by the network stack earlier (such as Netfilter's
> >          CONNTRACK module), and if that says the checksum is bad,
> >          we can drop the packet immediately instead of waiting until
> >          we try and copy it to userspace. Otherwise, we need to
> >          mark the SKB as CHECKSUM_NONE, since the skb->csum field
> >          no longer contains the full packet checksum after the
> >          call to __skb_checksum_validate_complete().
> >
> > Fixes: e6afc8ace6dd ("udp: remove headers from UDP packets before queueing")
>
> Can't count on my hands how many regressions are a result of that change and
> it's subtle side effects. :-/
>
> > Fixes: c84d949057ca ("udp: copy skb->truesize in the first cache line")
> > Cc: Sam Kumar <samanthakumar@...gle.com>
> > Cc: Eric Dumazet <edumazet@...gle.com>
> > Signed-off-by: Sean Tranchetti <stranche@...eaurora.org>
>
> Applied and queued up for -stable, thank you.

Powered by blists - more mailing lists