Zero checksum handling for UDP4->UDP6

[angus19]

MAP should be transparent to the UDP csum zero in both directions.

[ayourtch]

Was a zero UDP checksum standardized in IPv6 for all applications ? If yes - would you happen to have some references?

The closest I can find it https://www.rfc-editor.org/rfc/rfc6936, and it’s merely a discussion;
And I think even standandardization was mainly in the context of tunneled packets - so the zero checksum in IPv4 and IPv6 are not equivalent, I think.

That was my motivation in not handling it with equivalence - but if you could confirm that in practice that was a wrong idea, I am happy to merge this change.

[angus19]

My change comes out because of referring to the CERNET MAP implementation. And in it UDP csum zero is retained. Thanks.

[ayourtch]

Ok, fair enough, thanks for the info. I guess should be no harm merging - if someone complains we can revisit this later.

[ayourtch]

Since this is what CERNET implementation did, let’s merge this.

[r-pats]

I'm all for this as we're seeing real world user impact without it, and we have it in the UDP6->UDP4 direction already (with the config flag set).

RFC 8200 does codify an exception for UDP-based tunnelling protocols. Maybe if we squint enough we can pretend it's applicable here?

As an exception to the default behavior, protocols that use UDP
as a tunnel encapsulation may enable zero-checksum mode for a
specific port (or set of ports) for sending and/or receiving.
Any node implementing zero-checksum mode must follow the
requirements specified in "Applicability Statement for the Use
of IPv6 UDP Datagrams with Zero Checksums" [RFC6936].

Thanks @angus19

[ayourtch]

Cool, if we see a real world impact without the change, makes sense to have merged it. Thanks a lot @angus19 !

[angus19]

@ayourtch @r-pats A defect is observed when loading the module with zero_csum_pass=0.
Let's check nat46-core.c starting from L1674, which is shown below.
if ((udp->check == 0) && zero_csum_pass) {
/* zero checksum and the config to pass it is set - do nothing with it */
break;
}
sum1 = csum_ipv6_unmagic(nat46, &ip6h->saddr, &ip6h->daddr, l3_infrag_payload_len, NEXTHDR_UDP, udp->check);
sum2 = csum_tcpudp_remagic(v4saddr, v4daddr, l3_infrag_payload_len, NEXTHDR_UDP, sum1);
udp->check = sum2;
The logic at L1678-L1680 is quite good to fit either fragmented or non-fragmented UDP6 packet if its L4 header is visible.
When doing UDP6->UDP4 based on zero checksum and zero_csum_pass is 0, we won't get correct UDP4 checksum in the end.
The received UDP6 checksum is null (0) so counting on csum_ipv6_unmagic and csum_tcpudp_remagic to generate a new UDP4 checksum is not possible.
There is a simple solution that zero_csum_pass has to be set to 1 not 0 by default.
Any other fixes at L1678-L1680 but allow zero_csum_pass to be 0?
I haven't found one yet.

[ayourtch]

Well setting zero_csum_pass to 1 is not much of a fix, is it - it rather masks the problem in the code. Also it is a change of defaults, which i would be a bit hesitant to do…

Here’s an idea, what if we rewrite that bit as follows:

if (!udp->check) {
if (zero_csum_pass) {
// do nothing
break;
}
// recalculate the IPv6 checksum before adjusting
ip6_update_csum(…)
}
// here we will have nonzero checksum, unmagic + remagic

so, if it’s UDP and the checksum is zero and we don’t pass the zero checksum, recalculate it ?

It will still be broken if we don’t have full payload since we can’t do the full calculation in this case, but should take care of other cases ?

what do you think ?

[angus19]

LGTM.

It will still be broken if we don’t have full payload since we can’t do the full calculation in this case, but should take care of other cases ?

We may reassemble fragmented UDPs to have a full payload first before recalculating the checksum.
This should start with the UDP fragment and its L4 checksum is 0, when zero_csum_pass=0 is configured.

[ayourtch]

Cool. Let’s do step by step. Reassembly is a much larger thing and a much smaller hopefully corner case, i am not fully sure on the benefit vs complexity and fragility it brings.

I am away from computer now, only with phone - will either push the patch with this change in a few days, or you can push it and I merge it.

[angus19]

Reassembly is a much larger thing and a much smaller hopefully corner case, i am not fully sure on the benefit vs complexity and fragility it brings.

I do not expect every incoming UDP fragment goes into reassembling process under zero_csum_pass=0.
What we care about are those fragments belonging to the checksum zero but, this is not easy to handle even if we already have a try_reassembly method.
Perhaps should put the reassembly aside... ?
P.S. According to my understanding of the CERNET implementation, UDP zero checksum is retained anyway in either direction.

[angus19]

Hmm, we seem to have the same issue at L1006-L1015 (nat46-core.c).
if ((udp->check == 0) && zero_csum_pass) {
/* zero checksum and the config to pass it is set - do nothing with it */
break;
}
sum1 = csum_ipv6_unmagic(nat46, &ip6h->saddr, &ip6h->daddr, infrag_payload_len, NEXTHDR_UDP, udp->check);
sum2 = csum_tcpudp_remagic(v4saddr, v4daddr, infrag_payload_len, NEXTHDR_UDP, sum1); /* add pseudoheader */
if(ul_sum) {
*ul_sum = csum16_upd(*ul_sum, udp->check, sum2);
}
udp->check = sum2;