From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Duyck Subject: Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation Date: Sun, 6 May 2018 15:29:35 -0700 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: Willem de Bruijn Return-path: Received: from mail-oi0-f45.google.com ([209.85.218.45]:36385 "EHLO mail-oi0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751854AbeEFW3g (ORCPT ); Sun, 6 May 2018 18:29:36 -0400 Received: by mail-oi0-f45.google.com with SMTP id v2-v6so23534691oif.3 for ; Sun, 06 May 2018 15:29:36 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Sun, May 6, 2018 at 10:17 AM, Willem de Bruijn wrote: > 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? Actually it looks like the math I was applying to test this was off. Basically the part I wasn't a fan of is the fact that we account for the UDP header in the first calculation but not in the next. I guess in the grand scheme of things though you are just dropping it from both the value being removed and the value being added so it works out do to the fact that the checksum can be associative. I guess I was just being too literal in my thinking. Still an expensive way of doing this though. I'll update the patch description. Thanks. - Alex