On Sat, May 5, 2018 at 7:39 PM, Alexander Duyck <alexander.du...@gmail.com> wrote: > On Sat, May 5, 2018 at 3:01 AM, Willem de Bruijn > <willemdebruijn.ker...@gmail.com> wrote: >> On Fri, May 4, 2018 at 8:30 PM, Alexander Duyck >> <alexander.du...@gmail.com> wrote: >>> From: Alexander Duyck <alexander.h.du...@intel.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?
