From mboxrd@z Thu Jan 1 00:00:00 1970 From: Willem de Bruijn Subject: Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation Date: Sun, 6 May 2018 19:17:29 +0200 Message-ID: References: <20180504182537.5194.72775.stgit@localhost.localdomain> <20180504182958.5194.62398.stgit@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: Network Development , Willem de Bruijn , David Miller To: Alexander Duyck Return-path: Received: from mail-ua0-f181.google.com ([209.85.217.181]:40814 "EHLO mail-ua0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751793AbeEFRSL (ORCPT ); Sun, 6 May 2018 13:18:11 -0400 Received: by mail-ua0-f181.google.com with SMTP id g9so16973254uak.7 for ; Sun, 06 May 2018 10:18:11 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Sat, May 5, 2018 at 7:39 PM, Alexander Duyck wrote: > On Sat, May 5, 2018 at 3:01 AM, Willem de Bruijn > wrote: >> On Fri, May 4, 2018 at 8:30 PM, Alexander Duyck >> wrote: >>> From: Alexander Duyck >>> >>> This patch is meant to allow us to avoid having to recompute the checksum >>> from scratch and have it passed as a parameter. >>> >>> Instead of taking that approach we can take advantage of the fact that the >>> length that was used to compute the existing checksum is included in the >>> UDP header. If we cancel that out by adding the value XOR with 0xFFFF we >>> can then just add the new length in and fold that into the new result. >>> >>> I think this may be fixing a checksum bug in the original code as well >>> since the checksum that was passed included the UDP header in the checksum >>> computation, but then excluded it for the adjustment on the last frame. I >>> believe this may have an effect on things in the cases where the two differ >>> by bits that would result in things crossing the byte boundaries. >> >> The replacement code, below, subtracts original payload size then adds >> the new payload size. mss here excludes the udp header size. >> >>> /* last packet can be partial gso_size */ >>> - if (!seg->next) >>> - csum_replace2(&uh->check, htons(mss), >>> - htons(seg->len - hdrlen - sizeof(*uh))); > > That is my point. When you calculated your checksum you included the > UDP header in the calculation. > > - return __udp_gso_segment(gso_skb, features, > - udp_v4_check(sizeof(struct udphdr) + mss, > - iph->saddr, iph->daddr, 0)); > > Basically the problem is in one spot you are adding the sizeof(struct > udphdr) + mss and then in another you are cancelling it out as mss and > trying to account for it by also dropping the UDP header from the > payload length of the value you are adding. That works in the cases > where the effect doesn't cause any issues with the byte ordering, > however I think when mss + 8 crosses a byte boundary it can lead to > issues since the calculation is done on a byte swapped value. Do you mean that the issue is that the arithmetic operations on a __be16 in csum_replace2 may be incorrect if they exceed the least significant byte? csum_replace2 is used in many locations in the stack to adjust a network byte order csum when the payload length changes (e.g., iph->tot_len in inet_gro_complete). Or am I missing something specific about the udphdr calculations?