| Message ID | 56901235.1010800@solarflare.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Edward Cree > Sent: 08 January 2016 19:47 ... > + if (skb->ip_summed == CHECKSUM_PARTIAL) { > + csum = csum_fold(lco_csum(skb)); > + if (csum == 0) > + csum = CSUM_MANGLED_0; > + return csum; > + } else { > + return csum_fold(skb_checksum(skb, 0, skb->len, 0)); > + } You see to be worried about csum_fold() returning 0 in one path, but not in the other. I'm guessing that 0 can only happen if all the bytes that have been checksummed are zero. This is almost certainly never true if any bytes have been summed, and might be better avoided by initialising any such checksum to 0xffff (instead of 0) much earlier on. David
On 11/01/16 10:09, David Laight wrote: > From: Edward Cree >> Sent: 08 January 2016 19:47 > ... >> + if (skb->ip_summed == CHECKSUM_PARTIAL) { >> + csum = csum_fold(lco_csum(skb)); >> + if (csum == 0) >> + csum = CSUM_MANGLED_0; >> + return csum; >> + } else { >> + return csum_fold(skb_checksum(skb, 0, skb->len, 0)); >> + } > You see to be worried about csum_fold() returning 0 in one > path, but not in the other. > I'm guessing that 0 can only happen if all the bytes that have > been checksummed are zero. csum_fold complements, so if the sum comes to (say) 0x1fffe, it will return ~0xffff which is 0. Next version of patch will mangle 0 for both branches. -ed
On Mon, Jan 11, 2016 at 5:21 AM, Edward Cree <ecree@solarflare.com> wrote: > On 11/01/16 10:09, David Laight wrote: >> From: Edward Cree >>> Sent: 08 January 2016 19:47 >> ... >>> + if (skb->ip_summed == CHECKSUM_PARTIAL) { >>> + csum = csum_fold(lco_csum(skb)); >>> + if (csum == 0) >>> + csum = CSUM_MANGLED_0; >>> + return csum; >>> + } else { >>> + return csum_fold(skb_checksum(skb, 0, skb->len, 0)); >>> + } >> You see to be worried about csum_fold() returning 0 in one >> path, but not in the other. >> I'm guessing that 0 can only happen if all the bytes that have >> been checksummed are zero. > csum_fold complements, so if the sum comes to (say) 0x1fffe, it will return ~0xffff which is 0. > > Next version of patch will mangle 0 for both branches. Actually you may want to go the other way on that. If they weren't flipping the checksum value for GRE before why should we start doing that now? I'm pretty sure the checksum mangling is a very UDP centric thing. There is no need to introduce it to other protocols. - Alex
On 11/01/16 18:39, Alexander Duyck wrote: > On Mon, Jan 11, 2016 at 5:21 AM, Edward Cree <ecree@solarflare.com> wrote: >> csum_fold complements, so if the sum comes to (say) 0x1fffe, it will return ~0xffff which is 0. >> >> Next version of patch will mangle 0 for both branches. > Actually you may want to go the other way on that. If they weren't > flipping the checksum value for GRE before why should we start doing > that now? I'm pretty sure the checksum mangling is a very UDP centric > thing. There is no need to introduce it to other protocols. Good catch, you're quite right. -ed
Alexander Duyck <alexander.duyck@gmail.com> wrote: > Actually you may want to go the other way on that. If they weren't > flipping the checksum value for GRE before why should we start doing > that now? I'm pretty sure the checksum mangling is a very UDP centric > thing. There is no need to introduce it to other protocols. If different checksum representations are needed, then there really should be an explicit indication of whether it is a UDP-style checksum or other in the skb I would think rather than guessing it based on the offset. Of course it would be convenient if all the protocols that use a one's complement checksum would tolerate the UDP representation. I have a long (and now old) history working with real one's complement machines, and so I would want to believe that any correct implementation would tolerate it, but I don't know for a fact that they do. -- Mark Rustad, Networking Division, Intel Corporation
On Wed, Jan 20, 2016 at 11:11 AM, Rustad, Mark D <mark.d.rustad@intel.com> wrote: > Alexander Duyck <alexander.duyck@gmail.com> wrote: > >> Actually you may want to go the other way on that. If they weren't >> flipping the checksum value for GRE before why should we start doing >> that now? I'm pretty sure the checksum mangling is a very UDP centric >> thing. There is no need to introduce it to other protocols. > > > If different checksum representations are needed, then there really should > be an explicit indication of whether it is a UDP-style checksum or other in > the skb I would think rather than guessing it based on the offset. Of course > it would be convenient if all the protocols that use a one's complement > checksum would tolerate the UDP representation. I have a long (and now old) > history working with real one's complement machines, and so I would want to > believe that any correct implementation would tolerate it, but I don't know > for a fact that they do. The only reason why UDP does the bit flip is because it has reserved a checksum of 0 as a special value. For the checksum math itself either 0xFFFF or 0 are interchangeable. The only time they would make any difference would be if we had a value of 0 that we were checksumming, but since that is not the case the values will always end up converging back onto 0xFFFF as the final result in the case of a correct checksum. - Alex
On Wed, Jan 20, 2016 at 11:35 AM, Alexander Duyck <alexander.duyck@gmail.com> wrote: > On Wed, Jan 20, 2016 at 11:11 AM, Rustad, Mark D > <mark.d.rustad@intel.com> wrote: >> Alexander Duyck <alexander.duyck@gmail.com> wrote: >> >>> Actually you may want to go the other way on that. If they weren't >>> flipping the checksum value for GRE before why should we start doing >>> that now? I'm pretty sure the checksum mangling is a very UDP centric >>> thing. There is no need to introduce it to other protocols. >> >> >> If different checksum representations are needed, then there really should >> be an explicit indication of whether it is a UDP-style checksum or other in >> the skb I would think rather than guessing it based on the offset. Of course >> it would be convenient if all the protocols that use a one's complement >> checksum would tolerate the UDP representation. I have a long (and now old) >> history working with real one's complement machines, and so I would want to >> believe that any correct implementation would tolerate it, but I don't know >> for a fact that they do. > > The only reason why UDP does the bit flip is because it has reserved a > checksum of 0 as a special value. For the checksum math itself either > 0xFFFF or 0 are interchangeable. The only time they would make any > difference would be if we had a value of 0 that we were checksumming, > but since that is not the case the values will always end up > converging back onto 0xFFFF as the final result in the case of a > correct checksum. > 0xffff is mathematically equivalent to 0x0 for checksum. I would rather always flip 0 to 0xffff in LCO rather than adding an explicit indication (i.e. another flag) in SKB that it has a UDP checksum. Tom > - Alex
On Wed, Jan 20, 2016 at 11:58 AM, Tom Herbert <tom@herbertland.com> wrote: > On Wed, Jan 20, 2016 at 11:35 AM, Alexander Duyck > <alexander.duyck@gmail.com> wrote: >> On Wed, Jan 20, 2016 at 11:11 AM, Rustad, Mark D >> <mark.d.rustad@intel.com> wrote: >>> Alexander Duyck <alexander.duyck@gmail.com> wrote: >>> >>>> Actually you may want to go the other way on that. If they weren't >>>> flipping the checksum value for GRE before why should we start doing >>>> that now? I'm pretty sure the checksum mangling is a very UDP centric >>>> thing. There is no need to introduce it to other protocols. >>> >>> >>> If different checksum representations are needed, then there really should >>> be an explicit indication of whether it is a UDP-style checksum or other in >>> the skb I would think rather than guessing it based on the offset. Of course >>> it would be convenient if all the protocols that use a one's complement >>> checksum would tolerate the UDP representation. I have a long (and now old) >>> history working with real one's complement machines, and so I would want to >>> believe that any correct implementation would tolerate it, but I don't know >>> for a fact that they do. >> >> The only reason why UDP does the bit flip is because it has reserved a >> checksum of 0 as a special value. For the checksum math itself either >> 0xFFFF or 0 are interchangeable. The only time they would make any >> difference would be if we had a value of 0 that we were checksumming, >> but since that is not the case the values will always end up >> converging back onto 0xFFFF as the final result in the case of a >> correct checksum. >> > 0xffff is mathematically equivalent to 0x0 for checksum. I would > rather always flip 0 to 0xffff in LCO rather than adding an explicit > indication (i.e. another flag) in SKB that it has a UDP checksum. There isn't any need to add such an indication, nor do we need to start bitflipping the return value from csum_fold in all cases. I think there was just some confusion about UDP checksums vs GRE or TCP checksums. I'd say we are better off keeping this simple. The original patch just needs to drop the check for the resultant checksum being 0 since that is not needed for GRE. - Alex
Alexander Duyck <alexander.duyck@gmail.com> wrote: > There isn't any need to add such an indication, nor do we need to > start bitflipping the return value from csum_fold in all cases. I > think there was just some confusion about UDP checksums vs GRE or TCP > checksums. Yeah. I think I finally got there. The naive software methods will never generate a true 0 unless everything was zero. Real one's complement machines did addition in terms of subtraction so that 1 + -1 would never produce a -0, only a normal 0. Of course a simple adder would produce a -0, making it impossible to get back to a normal 0. > I'd say we are better off keeping this simple. The original patch > just needs to drop the check for the resultant checksum being 0 since > that is not needed for GRE. I'm all in favor of simple. I had just started to worry about a possible change in behavior that might have interoperability problems with some implementations. I wonder if any implementation ever did the addition by subtraction, but also failed to make 0 compare equal to -0? I guess if they knew enough to do the former, they should not have blown the latter. -- Mark Rustad, Networking Division, Intel Corporation
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index 04a48c0..8a589d3 100644 --- a/net/ipv4/ip_gre.c +++ b/net/ipv4/ip_gre.c @@ -440,6 +440,20 @@ drop: return 0; } +static __sum16 gre_checksum(struct sk_buff *skb) +{ + __sum16 csum; + + if (skb->ip_summed == CHECKSUM_PARTIAL) { + csum = csum_fold(lco_csum(skb)); + if (csum == 0) + csum = CSUM_MANGLED_0; + return csum; + } else { + return csum_fold(skb_checksum(skb, 0, skb->len, 0)); + } +} + static void build_header(struct sk_buff *skb, int hdr_len, __be16 flags, __be16 proto, __be32 key, __be32 seq) { @@ -467,8 +481,7 @@ static void build_header(struct sk_buff *skb, int hdr_len, __be16 flags, !(skb_shinfo(skb)->gso_type & (SKB_GSO_GRE | SKB_GSO_GRE_CSUM))) { *ptr = 0; - *(__sum16 *)ptr = csum_fold(skb_checksum(skb, 0, - skb->len, 0)); + *(__sum16 *)ptr = gre_checksum(skb); } } } @@ -493,7 +506,7 @@ static void __gre_xmit(struct sk_buff *skb, struct net_device *dev, static struct sk_buff *gre_handle_offloads(struct sk_buff *skb, bool csum) { - return iptunnel_handle_offloads(skb, csum, + return iptunnel_handle_offloads(skb, false, csum ? SKB_GSO_GRE_CSUM : SKB_GSO_GRE); }
Signed-off-by: Edward Cree <ecree@solarflare.com> --- net/ipv4/ip_gre.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)