Message-ID: <CAKgT0UedktOX1vqU=cy_5yKHMzYabyHq3je8HmO89+OKtM-_Mw@mail.gmail.com>
Date:   Sun, 6 May 2018 15:29:35 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        David Miller <davem@...emloft.net>
Subject: Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter
 to GSO segmentation

On Sun, May 6, 2018 at 10:17 AM, Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
> On Sat, May 5, 2018 at 7:39 PM, Alexander Duyck
> <alexander.duyck@...il.com> wrote:
>> On Sat, May 5, 2018 at 3:01 AM, Willem de Bruijn
>> <willemdebruijn.kernel@...il.com> wrote:
>>> On Fri, May 4, 2018 at 8:30 PM, Alexander Duyck
>>> <alexander.duyck@...il.com> wrote:
>>>> From: Alexander Duyck <alexander.h.duyck@...el.com>
>>>>
>>>> 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

Powered by blists - more mailing lists