From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Duyck Subject: Re: [PATCH v3] net: Do not include padding in TCP GRO checksum Date: Fri, 15 Nov 2013 20:10:00 -0800 Message-ID: <5286F018.8060801@gmail.com> References: <20131115225856.6988.69733.stgit@ahduyck-fpga.jf.intel.com> <20131116004738.GA1491@gondor.apana.org.au> <20131116015301.GA1999@gondor.apana.org.au> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: davem@davemloft.net, netdev@vger.kernel.org, edumazet@google.com, herbert@gondor.apana.org To: Herbert Xu , Alexander Duyck Return-path: Received: from mail-pa0-f47.google.com ([209.85.220.47]:33036 "EHLO mail-pa0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752353Ab3KPEQR (ORCPT ); Fri, 15 Nov 2013 23:16:17 -0500 Received: by mail-pa0-f47.google.com with SMTP id ld10so4446754pab.20 for ; Fri, 15 Nov 2013 20:16:15 -0800 (PST) In-Reply-To: <20131116015301.GA1999@gondor.apana.org.au> Sender: netdev-owner@vger.kernel.org List-ID: 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 >> 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