Improve zero checksum handling #46
Conversation
As per the suggestion by @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 ?
|
@ayourtch Please review the change. Thanks. |
| if ((udp->check == 0) && !zero_csum_pass) { | ||
| /* | ||
| * Checksum will still be broken if we don't have full payload | ||
| * since we can't do the full calculation in this case. |
There was a problem hiding this comment.
This new conditional seems to be a no-op ? Why adding it ?
There was a problem hiding this comment.
Put UDP reassembly aside. So UDP fragments associated with zero checksum, the corner case, will not be well handled even if we do recalculate checksum before adjusting via the ip6_update_csum method. I therefore drop a comment in the ip6_update_csum to mark such condition. Both csum_partial and csum_ipv6_magic methods are still serviced for an UDP fragment with zero checksum. But the result is incorrect.
| @@ -1007,6 +1007,7 @@ int xlate_payload6_to4(nat46_instance_t *nat46, void *pv6, void *ptrans_hdr, int | |||
| /* zero checksum and the config to pass it is set - do nothing with it */ | |||
| break; | |||
| } | |||
| /* pretend checksum is correct not null */ | |||
There was a problem hiding this comment.
Do we need to add a logic to recalculate the checksum here if it is zero ? (Reviewing on the phone, so might be wrong).
There was a problem hiding this comment.
I don't think recalculation over here is required. In the xlate_payload6_to4 method, we care about translation of a payload inside an ICMPv6. Although this payload is structured by an inner packet which has its own IPv6 header + L4 header + payload. But it is usually truncated. So checksum recalculation happens to the inner L4 header is also usually not possible.
|
@ayourtch Good to see you fix the change in no matter the wording/comment or any others. |
|
Closing as the change is still buggy. |
As per the suggestion by @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 ?