Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve zero checksum handling #46

Closed
wants to merge 1 commit into from
Closed

Improve zero checksum handling #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions nat46/modules/nat46-core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add a logic to recalculate the checksum here if it is zero ? (Reviewing on the phone, so might be wrong).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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) {
Expand Down Expand Up @@ -1671,9 +1672,12 @@ int nat46_ipv6_input(struct sk_buff *old_skb) {
case NEXTHDR_UDP: {
struct udphdr *udp = add_offset(ip6h, v6packet_l3size);
u16 sum1, sum2;
if ((udp->check == 0) && zero_csum_pass) {
/* zero checksum and the config to pass it is set - do nothing with it */
break;
if (udp->check == 0) {
if (zero_csum_pass) {
/* zero checksum and the config to pass it is set - do nothing with it */
break;
}
ip6_update_csum(old_skb, ip6h, ip6h->nexthdr == NEXTHDR_FRAGMENT); /* recalculate checksum before adjusting */
}
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);
Expand Down Expand Up @@ -1767,8 +1771,17 @@ void ip6_update_csum(struct sk_buff * skb, struct ipv6hdr * ip6hdr, int do_atomi
unsigned udplen = ntohs(ip6hdr->payload_len) - (do_atomic_frag?8:0); /* UDP hdr + payload */

if ((udp->check == 0) && zero_csum_pass) {
/* zero checksum and the config to pass it is set - do nothing with it */
break;
/* zero checksum and the config to pass it is set - do nothing with it */
break;
}

if (do_atomic_frag) {
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.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new conditional seems to be a no-op ? Why adding it ?

Copy link
Contributor Author

@angus19 angus19 Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
}
}

oldsum = udp->check;
Expand Down
2 changes: 2 additions & 0 deletions nat46/modules/nat46-core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,6 @@ nat46_instance_t *get_nat46_instance(struct sk_buff *sk);
nat46_instance_t *alloc_nat46_instance(int npairs, nat46_instance_t *old, int from_ipair, int to_ipair, int remove_ipair);
void release_nat46_instance(nat46_instance_t *nat46);

void ip6_update_csum(struct sk_buff * skb, struct ipv6hdr * ip6hdr, int do_atomic_frag);

#endif