Skip to content

Commit 6043b79

Browse files
pchaignokuba-moo
authored andcommitted
net: Fix checksum update for ILA adj-transport
During ILA address translations, the L4 checksums can be handled in different ways. One of them, adj-transport, consist in parsing the transport layer and updating any found checksum. This logic relies on inet_proto_csum_replace_by_diff and produces an incorrect skb->csum when in state CHECKSUM_COMPLETE. This bug can be reproduced with a simple ILA to SIR mapping, assuming packets are received with CHECKSUM_COMPLETE: $ ip a show dev eth0 14: eth0@if15: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000 link/ether 62:ae:35:9e:0f:8d brd ff:ff:ff:ff:ff:ff link-netnsid 0 inet6 3333:0:0:1::c078/64 scope global valid_lft forever preferred_lft forever inet6 fd00:10:244:1::c078/128 scope global nodad valid_lft forever preferred_lft forever inet6 fe80::60ae:35ff:fe9e:f8d/64 scope link proto kernel_ll valid_lft forever preferred_lft forever $ ip ila add loc_match fd00:10:244:1 loc 3333:0:0:1 \ csum-mode adj-transport ident-type luid dev eth0 Then I hit [fd00:10:244:1::c078]:8000 with a server listening only on [3333:0:0:1::c078]:8000. With the bug, the SYN packet is dropped with SKB_DROP_REASON_TCP_CSUM after inet_proto_csum_replace_by_diff changed skb->csum. The translation and drop are visible on pwru [1] traces: IFACE TUPLE FUNC eth0:9 [fd00:10:244:3::3d8]:51420->[fd00:10:244:1::c078]:8000(tcp) ipv6_rcv eth0:9 [fd00:10:244:3::3d8]:51420->[fd00:10:244:1::c078]:8000(tcp) ip6_rcv_core eth0:9 [fd00:10:244:3::3d8]:51420->[fd00:10:244:1::c078]:8000(tcp) nf_hook_slow eth0:9 [fd00:10:244:3::3d8]:51420->[fd00:10:244:1::c078]:8000(tcp) inet_proto_csum_replace_by_diff eth0:9 [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp) tcp_v6_early_demux eth0:9 [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp) ip6_route_input eth0:9 [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp) ip6_input eth0:9 [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp) ip6_input_finish eth0:9 [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp) ip6_protocol_deliver_rcu eth0:9 [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp) raw6_local_deliver eth0:9 [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp) ipv6_raw_deliver eth0:9 [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp) tcp_v6_rcv eth0:9 [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp) __skb_checksum_complete eth0:9 [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp) kfree_skb_reason(SKB_DROP_REASON_TCP_CSUM) eth0:9 [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp) skb_release_head_state eth0:9 [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp) skb_release_data eth0:9 [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp) skb_free_head eth0:9 [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp) kfree_skbmem This is happening because inet_proto_csum_replace_by_diff is updating skb->csum when it shouldn't. The L4 checksum is updated such that it "cancels" the IPv6 address change in terms of checksum computation, so the impact on skb->csum is null. Note this would be different for an IPv4 packet since three fields would be updated: the IPv4 address, the IP checksum, and the L4 checksum. Two would cancel each other and skb->csum would still need to be updated to take the L4 checksum change into account. This patch fixes it by passing an ipv6 flag to inet_proto_csum_replace_by_diff, to skip the skb->csum update if we're in the IPv6 case. Note the behavior of the only other user of inet_proto_csum_replace_by_diff, the BPF subsystem, is left as is in this patch and fixed in the subsequent patch. With the fix, using the reproduction from above, I can confirm skb->csum is not touched by inet_proto_csum_replace_by_diff and the TCP SYN proceeds to the application after the ILA translation. Link: https://github.com/cilium/pwru [1] Fixes: 65d7ab8 ("net: Identifier Locator Addressing module") Signed-off-by: Paul Chaignon <[email protected]> Acked-by: Daniel Borkmann <[email protected]> Link: https://patch.msgid.link/b5539869e3550d46068504feb02d37653d939c0b.1748509484.git.paul.chaignon@gmail.com Signed-off-by: Jakub Kicinski <[email protected]>
1 parent 44abca1 commit 6043b79

File tree

4 files changed

+7
-7
lines changed

4 files changed

+7
-7
lines changed

include/net/checksum.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ void inet_proto_csum_replace16(__sum16 *sum, struct sk_buff *skb,
152152
const __be32 *from, const __be32 *to,
153153
bool pseudohdr);
154154
void inet_proto_csum_replace_by_diff(__sum16 *sum, struct sk_buff *skb,
155-
__wsum diff, bool pseudohdr);
155+
__wsum diff, bool pseudohdr, bool ipv6);
156156

157157
static __always_inline
158158
void inet_proto_csum_replace2(__sum16 *sum, struct sk_buff *skb,

net/core/filter.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1987,7 +1987,7 @@ BPF_CALL_5(bpf_l4_csum_replace, struct sk_buff *, skb, u32, offset,
19871987
if (unlikely(from != 0))
19881988
return -EINVAL;
19891989

1990-
inet_proto_csum_replace_by_diff(ptr, skb, to, is_pseudo);
1990+
inet_proto_csum_replace_by_diff(ptr, skb, to, is_pseudo, false);
19911991
break;
19921992
case 2:
19931993
inet_proto_csum_replace2(ptr, skb, from, to, is_pseudo);

net/core/utils.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -473,11 +473,11 @@ void inet_proto_csum_replace16(__sum16 *sum, struct sk_buff *skb,
473473
EXPORT_SYMBOL(inet_proto_csum_replace16);
474474

475475
void inet_proto_csum_replace_by_diff(__sum16 *sum, struct sk_buff *skb,
476-
__wsum diff, bool pseudohdr)
476+
__wsum diff, bool pseudohdr, bool ipv6)
477477
{
478478
if (skb->ip_summed != CHECKSUM_PARTIAL) {
479479
csum_replace_by_diff(sum, diff);
480-
if (skb->ip_summed == CHECKSUM_COMPLETE && pseudohdr)
480+
if (skb->ip_summed == CHECKSUM_COMPLETE && pseudohdr && !ipv6)
481481
skb->csum = ~csum_sub(diff, skb->csum);
482482
} else if (pseudohdr) {
483483
*sum = ~csum_fold(csum_add(diff, csum_unfold(*sum)));

net/ipv6/ila/ila_common.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ static void ila_csum_adjust_transport(struct sk_buff *skb,
8686

8787
diff = get_csum_diff(ip6h, p);
8888
inet_proto_csum_replace_by_diff(&th->check, skb,
89-
diff, true);
89+
diff, true, true);
9090
}
9191
break;
9292
case NEXTHDR_UDP:
@@ -97,7 +97,7 @@ static void ila_csum_adjust_transport(struct sk_buff *skb,
9797
if (uh->check || skb->ip_summed == CHECKSUM_PARTIAL) {
9898
diff = get_csum_diff(ip6h, p);
9999
inet_proto_csum_replace_by_diff(&uh->check, skb,
100-
diff, true);
100+
diff, true, true);
101101
if (!uh->check)
102102
uh->check = CSUM_MANGLED_0;
103103
}
@@ -111,7 +111,7 @@ static void ila_csum_adjust_transport(struct sk_buff *skb,
111111

112112
diff = get_csum_diff(ip6h, p);
113113
inet_proto_csum_replace_by_diff(&ih->icmp6_cksum, skb,
114-
diff, true);
114+
diff, true, true);
115115
}
116116
break;
117117
}

0 commit comments

Comments
 (0)