Re: [PATCH net-next 6/8] net: gre: Implement LCO for GRE over IPv4

On Wed, Jan 20, 2016 at 11:58 AM, Tom Herbert [off-list ref] wrote:

On Wed, Jan 20, 2016 at 11:35 AM, Alexander Duyck
[off-list ref] wrote:

quoted

On Wed, Jan 20, 2016 at 11:11 AM, Rustad, Mark D
[off-list ref] wrote:

quoted

Alexander Duyck [off-list ref] wrote:

quoted

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