From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Duyck Subject: [PATCH v3] net: Do not include padding in TCP GRO checksum Date: Fri, 15 Nov 2013 15:00:34 -0800 Message-ID: <20131115225856.6988.69733.stgit@ahduyck-fpga.jf.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, edumazet@google.com, herbert@gondor.apana.org To: davem@davemloft.net Return-path: Received: from mga14.intel.com ([143.182.124.37]:14896 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752476Ab3KOXJt (ORCPT ); Fri, 15 Nov 2013 18:09:49 -0500 Sender: netdev-owner@vger.kernel.org List-ID: 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 --- v2: Update byte ordering of tot_len and payload_len so it is in host order. Updated CHECKSUM_COMPLETE path so it falls back through CHECKSUM_NONE for padded frames since this is how it is handled in ip_rcv. I have tested and verified the CHECKSUM_NONE path works, but I don't have any adapters that generate CHECKSUM_COMPLETE to test with. v3: Added check to handle case where length is greater than skb_gro_len. net/ipv4/tcp_offload.c | 30 +++++++++++++++++++++--------- net/ipv6/tcpv6_offload.c | 31 +++++++++++++++++++++---------- 2 files changed, 42 insertions(+), 19 deletions(-) diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c index a2b68a1..b32f6c3 100644 --- a/net/ipv4/tcp_offload.c +++ b/net/ipv4/tcp_offload.c @@ -273,26 +273,38 @@ static int tcp_v4_gso_send_check(struct sk_buff *skb) static struct sk_buff **tcp4_gro_receive(struct sk_buff **head, struct sk_buff *skb) { const struct iphdr *iph = skb_gro_network_header(skb); + int length = ntohs(iph->tot_len); __wsum wsum; __sum16 sum; + /* adjust for any offsets */ + length += skb_network_offset(skb) - skb_gro_offset(skb); + + /* verify the entire packet is here */ + if (length > skb_gro_len(skb)) + goto flush; + switch (skb->ip_summed) { case CHECKSUM_COMPLETE: - if (!tcp_v4_check(skb_gro_len(skb), iph->saddr, iph->daddr, - skb->csum)) { - skb->ip_summed = CHECKSUM_UNNECESSARY; - break; - } + if (length == skb_gro_len(skb)) { + if (!tcp_v4_check(length, iph->saddr, iph->daddr, + skb->csum)) { + skb->ip_summed = CHECKSUM_UNNECESSARY; + break; + } flush: - NAPI_GRO_CB(skb)->flush = 1; - return NULL; + NAPI_GRO_CB(skb)->flush = 1; + return NULL; + } + /* skb->csum is invalid if frame is padded */ + skb->ip_summed = CHECKSUM_NONE; case CHECKSUM_NONE: wsum = csum_tcpudp_nofold(iph->saddr, iph->daddr, - skb_gro_len(skb), IPPROTO_TCP, 0); + length, IPPROTO_TCP, 0); sum = csum_fold(skb_checksum(skb, skb_gro_offset(skb), - skb_gro_len(skb), + length, wsum)); if (sum) goto flush; diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c index c1097c7..f6047cc 100644 --- a/net/ipv6/tcpv6_offload.c +++ b/net/ipv6/tcpv6_offload.c @@ -36,27 +36,38 @@ static struct sk_buff **tcp6_gro_receive(struct sk_buff **head, struct sk_buff *skb) { const struct ipv6hdr *iph = skb_gro_network_header(skb); + int length = ntohs(iph->payload_len); __wsum wsum; __sum16 sum; + /* adjust for any offset due to extension headers */ + length += skb_transport_offset(skb) - skb_gro_offset(skb); + + /* verify the entire packet is here */ + if (length > skb_gro_len(skb)) + goto flush; + switch (skb->ip_summed) { case CHECKSUM_COMPLETE: - if (!tcp_v6_check(skb_gro_len(skb), &iph->saddr, &iph->daddr, - skb->csum)) { - skb->ip_summed = CHECKSUM_UNNECESSARY; - break; - } + if (length == skb_gro_len(skb)) { + if (!tcp_v6_check(length, &iph->saddr, &iph->daddr, + skb->csum)) { + skb->ip_summed = CHECKSUM_UNNECESSARY; + break; + } flush: - NAPI_GRO_CB(skb)->flush = 1; - return NULL; + NAPI_GRO_CB(skb)->flush = 1; + return NULL; + } + /* skb->csum is invalid if frame is padded */ + skb->ip_summed = CHECKSUM_NONE; case CHECKSUM_NONE: wsum = ~csum_unfold(csum_ipv6_magic(&iph->saddr, &iph->daddr, - skb_gro_len(skb), - IPPROTO_TCP, 0)); + length, IPPROTO_TCP, 0)); sum = csum_fold(skb_checksum(skb, skb_gro_offset(skb), - skb_gro_len(skb), + length, wsum)); if (sum) goto flush;