bpf: Refine inner packet L4 checksum detection

yushoyamaguchi · pchaigno · commit 1a018d56d623 · 2025-11-17T05:12:13.000Z
The previous commit added handling for errors that occurred when NAT was applied to the inner header of an ICMP error packet too short to include the inner TCP L4 checksum. That implementation decided whether the inner packet had an L4 checksum by using the length attribute defined in RFC 4884: https://datatracker.ietf.org/doc/html/rfc4884. However, the length attribute is optional. For example, even in Linux kernel 6.17, ICMP error messages are sent without it: https://github.com/torvalds/linux/blob/v6.17/net/ipv4/icmp.c#L734-L739. Assuming that the attribute is always present is causing incorrect behavior. When the attribute exists, it is placed in the reserved bits. When it does not, the bits are zero-filled, and the logic wrongly assumes that the inner TCP checksum is missing, in all cases. This commit changes the logic to use the packet length stored in `ctx` instead of relying on the length attribute. Fix: 3c3f80bd48bc ("bpf: nat: Fix short packet MTU path discovery") Signed-off-by: YushoYamaguchi <ysh.824@outlook.jp>
diff --git a/bpf/lib/nat.h b/bpf/lib/nat.h
@@ -781,9 +781,8 @@ snat_v4_nat_handle_icmp_error(struct __ctx_buff *ctx, __u64 off,
 	__u32 icmpoff;
 	__u8 type;
 	int ret;
-	__u32 icmp_xlen_off = (__u32)off + offsetof(struct icmphdr, un.frag.__unused) + 1;
-	__u8 icmp_xlen;
-	bool icmp_has_full_l4_header;
+	bool icmp_has_inner_l4_csum = true;
+	__u32 total_inner_len = (__u32)ctx_full_len(ctx) - inner_l3_off;
 
 	/* According to the RFC 5508, any networking equipment that is
 	 * responding with an ICMP Error packet should embed the original
@@ -841,15 +840,10 @@ snat_v4_nat_handle_icmp_error(struct __ctx_buff *ctx, __u64 off,
 	if (!*state)
 		return NAT_PUNT_TO_STACK;
 
-	/*
-	 * The snat_v4_rewrite_headers() call only rewrites the checksum for
-	 * TCP and UDP.  UDP's checksum is covered by the RFC 792 inclusion
-	 * of the 1st 64 bits of the datagram following the IP header, but
-	 * TCP needs an additional 3 32-bit words to include the checksum.
-	 */
-	if (ctx_load_bytes(ctx, icmp_xlen_off, &icmp_xlen, sizeof(__u8)) < 0)
-		return DROP_INVALID;
-	icmp_has_full_l4_header = icmp_xlen >= ((tuple.nexthdr == IPPROTO_TCP) ? 3 : 0);
+	/* Check if the inner L4 header has checksum */
+	if (tuple.nexthdr == IPPROTO_TCP &&
+	    total_inner_len < iphdr.ihl + TCP_CSUM_OFF + sizeof(__u16))
+		icmp_has_inner_l4_csum = false;
 
 	/* We found SNAT entry to NAT embedded packet. The destination addr
 	 * should be NATed according to the entry.
@@ -860,7 +854,7 @@ snat_v4_nat_handle_icmp_error(struct __ctx_buff *ctx, __u64 off,
 	/* Failing to update the inner L4 checksum is not fatal if the header
 	 * is incomplete.
 	 */
-	if (!icmp_has_full_l4_header && ret == DROP_CSUM_L4)
+	if (!icmp_has_inner_l4_csum && ret == DROP_CSUM_L4)
 		ret = 0;
 
 	return ret;
@@ -1006,11 +1000,10 @@ snat_v4_rev_nat_handle_icmp_error(struct __ctx_buff *ctx,
 	__u16 port_off;
 	__u32 icmpoff;
 	__u8 type;
-	__u8 icmp_xlen;
-	bool icmp_has_full_l4_header;
+	bool icmp_has_inner_l4_csum = true;
 	int ret;
-	__u32 icmp_xlen_off = (__u32) inner_l3_off - sizeof(struct icmphdr) +
-	  offsetof(struct icmphdr, un.frag.__unused) + 1;
+	__u32 total_inner_len = (__u32)(ctx_full_len(ctx) - inner_l3_off);
+
 
 	/* According to the RFC 5508, any networking equipment that is
 	 * responding with an ICMP Error packet should embed the original
@@ -1070,15 +1063,10 @@ snat_v4_rev_nat_handle_icmp_error(struct __ctx_buff *ctx,
 	if (!*state)
 		return NAT_PUNT_TO_STACK;
 
-	/*
-	 * The snat_v4_rewrite_headers() call only rewrites the checksum for
-	 * TCP and UDP.  UDP's checksum is covered by the RFC 792 inclusion
-	 * of the 1st 64 bits of the datagram following the IP header, but
-	 * TCP needs an additional 3 32-bit words to include the checksum.
-	 */
-	if (ctx_load_bytes(ctx, icmp_xlen_off, &icmp_xlen, sizeof(__u8)) < 0)
-		return DROP_INVALID;
-	icmp_has_full_l4_header = icmp_xlen >= ((tuple.nexthdr == IPPROTO_TCP) ? 3 : 0);
+	/* Check if the inner L4 header has checksum */
+	if (tuple.nexthdr == IPPROTO_TCP &&
+	    total_inner_len < iphdr.ihl + TCP_CSUM_OFF + sizeof(__u16))
+		icmp_has_inner_l4_csum = false;
 
 	/* The embedded packet was SNATed on egress. Reverse it again: */
 	ret = snat_v4_rewrite_headers(ctx, tuple.nexthdr, (int)inner_l3_off, true, icmpoff,
@@ -1087,7 +1075,7 @@ snat_v4_rev_nat_handle_icmp_error(struct __ctx_buff *ctx,
 	/* Failing to update the inner L4 checksum is not fatal if the header
 	 * is incomplete.
 	 */
-	if (!icmp_has_full_l4_header && ret == DROP_CSUM_L4)
+	if (!icmp_has_inner_l4_csum && ret == DROP_CSUM_L4)
 		ret = 0;
 	return ret;
 }
