Re: [PATCH v3] net: Do not include padding in TCP GRO checksum

[Alexander Duyck <alexander.duyck@gmail.com>]

On 11/15/2013 05:53 PM, Herbert Xu wrote:
> On Sat, Nov 16, 2013 at 08:47:38AM +0800, Herbert Xu wrote:
>> On Fri, Nov 15, 2013 at 03:00:34PM -0800, Alexander Duyck wrote:
>>> In some recent tests I found the TCP checksum was being treated as valid
>>> for certain frames with padding on them.  On closer inspection I found the
>>> issue was that GRO was using the skb->len instead of the length recorded in
>>> the IP/IPv6 header to determine the number of bytes to checksum.  As such
>>> padded frames that actually had invalid checksums generated by adding the
>>> padding to the checksum were being incorrectly tagged as valid.
>>>
>>> This change corrects that by using the tot_len from IPv4 headers and the
>>> payload_len from IPv6 headers to compute the total number of bytes to be
>>> included in the checksum.
>>>
>>> To address the fact that skb->csum is invalid when a padded frame is
>>> received I have updated the code to fall though to the CHECKSUM_NONE path
>>> for CHECKSUM_COMPLETE frames that contain padding.
>>>
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>> Nack.  This is needlessly complex.  As I said before, please
>> just do a pskb_trim_rcsum in inet_gro_receive and its IPv6
>> counterpart and this should all just work.
> Actually I take that back.  You are right that the preference is to
> flush and not trim, since we want to preserve the incoming packet in
> its exact form.
>
> So can you tell me a bit more about the padding? Is it added by the
> NIC or did it come from the remote side?
>
> Thanks,

The case I am addressing is padding added by the remote side. 
Specifically in my case I was seeing TCP frames without options that
were padded up to 60 bytes from a netperf TCP_RR test.  I messed up the
padding/checksum logic so it was making the same mistake in the Tx
checksum logic in the driver that I caught here in GRO.  As a result I
was seeing checksum errors errors in wireshark, but noticed they were
being accepted by the stack as valid.

The driver has been fixed, and isn't anything that has been pushed
upstream yet so no harm there.  I figured while I was at it I should
probably fix the GRO logic so that it doesn't mis-identify those types
of frames as being valid since that is something that may lead to
similar driver errors going undetected.

Thanks,

Alex