Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation

[Alexander Duyck]

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 0x 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(>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

Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation

[Willem de Bruijn]

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 0x 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(>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?

Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation

[Alexander Duyck]

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 0x 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(>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.

Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation

[Willem de Bruijn]

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 0x 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(>check, htons(mss),
> - htons(seg->len - hdrlen - sizeof(*uh)));

Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation

[Alexander Duyck]

On Fri, May 4, 2018 at 1:19 PM, Eric Dumazet  wrote:
>
>
> On 05/04/2018 11:30 AM, 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 0x we
>> can then just add the new length in and fold that into the new result.
>>
>
>>
>> + uh = udp_hdr(segs);
>> +
>> + /* compute checksum adjustment based on old length versus new */
>> + newlen = htons(sizeof(*uh) + mss);
>> + check = ~csum_fold((__force __wsum)((__force u32)uh->check +
>> + ((__force u32)uh->len ^ 0x) +
>> + (__force u32)newlen));
>> +
>
>
> Can't this use csum_sub() instead of this ^ 0x trick ?

I could but that actually adds more instructions to all this since
csum_sub will perform the inversion across a 32b checksum when we only
need to bitflip a 16 bit value. I had considered doing (u16)(~uh->len)
but thought type casing it more than once would be a pain as well.

What I wanted to avoid is having to do the extra math to account for
the rollover. Adding 3 16 bit values will result in at most a 18 bit
value which can then be folded. Doing it this way we avoid that extra
add w/ carry logic that is needed for csum_add/sub.

Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation

[Eric Dumazet]

On 05/04/2018 11:30 AM, 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 0x we
> can then just add the new length in and fold that into the new result.
> 

>  
> + uh = udp_hdr(segs);
> +
> + /* compute checksum adjustment based on old length versus new */
> + newlen = htons(sizeof(*uh) + mss);
> + check = ~csum_fold((__force __wsum)((__force u32)uh->check +
> + ((__force u32)uh->len ^ 0x) +
> + (__force u32)newlen));
> +


Can't this use csum_sub() instead of this ^ 0x trick ?