From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Duyck Subject: Re: [PATCH] net: Do not include padding in TCP GRO checksum Date: Thu, 14 Nov 2013 22:00:00 -0800 Message-ID: <5285B860.1060800@gmail.com> References: <20131115011413.7090.9349.stgit@ahduyck-fpga.jf.intel.com> <20131115042057.GA24470@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-pb0-f45.google.com ([209.85.160.45]:62019 "EHLO mail-pb0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932104Ab3KOFGD (ORCPT ); Fri, 15 Nov 2013 00:06:03 -0500 Received: by mail-pb0-f45.google.com with SMTP id mc8so3059426pbc.18 for ; Thu, 14 Nov 2013 21:06:02 -0800 (PST) In-Reply-To: <20131115042057.GA24470@gondor.apana.org.au> Sender: netdev-owner@vger.kernel.org List-ID: On 11/14/2013 08:20 PM, Herbert Xu wrote: > On Thu, Nov 14, 2013 at 05:18:18PM -0800, Alexander Duyck wrote: >> In some recent tests where I was generating invalid frames I found that >> the checksum was being treated as valid for certain frames that computed >> the checksum with padding included. 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. >> >> Signed-off-by: Alexander Duyck > > Good catch. I wouldn't have caught it except for I had introduced just the right packet to see this when I was debugging some FPGA hardware. > >> + /* adjust for any offsets */ >> + length += skb_network_offset(skb) - skb_gro_offset(skb); > > Since skb->csum includes your padding, you'll need to adjust that > as well. Also this is not the only place where we use skb_gro_len > to measure the packet length. So rather than changing each one > of them, I think we could just do a pskb_trim_rcsum at the point > where we obtain the network packet length, i.e., in ipv4/ipv6. I thought about doing it there, but it seemed like the preference was to flush instead of trim. Both the inet_gro_receive call and the ipv6_gro_receive perform a check for length, and if it differs they set the flush bit. > We should then fix pskb_trim_rcsum to adjust CHECKSUM_COMPLETE > checksums as otherwise your NIC's RX checksum offload feature > will be useless. > > Thanks, If that is the case why aren't we doing this for ipv4 right now? I think what I will do for now is just detect the case where we are padded for CHECKSUM_COMPLETE and simply fall back to the CHECKSUM_NONE case. Thanks, Alex