[PATCH v2] netfilter: nf_nat: Fix stale outer UDP checksum on VXLAN encapsulated packets

[PATCH v2] netfilter: nf_nat: Fix stale outer UDP checksum on VXLAN encapsulated packets

[lvjunyu]

When MASQUERADE --random-fully rewrites the outer UDP source port and
address of a VXLAN-encapsulated packet whose inner header has
CHECKSUM_PARTIAL, the outer UDP checksum is not updated correctly.

udp_set_csum() takes the LCO (Local Checksum Offload) branch for
encapsulated CHECKSUM_PARTIAL non-GSO packets, writing a complete
(folded and complemented) checksum into uh->check while leaving
csum_start pointing at the inner transport header.  In this state:

  - inet_proto_csum_replace2() is a no-op (pseudohdr=false,
    CHECKSUM_PARTIAL)
  - nf_csum_update() -> inet_proto_csum_replace4(pseudohdr=true)
    takes the pseudo-header branch which treats uh->check as an
    uncomplemented seed, applying the address delta with the wrong
    sign

Fix this by using the offload target (csum_start + csum_offset) to
distinguish the LCO state from the seed/offload state, and temporarily
flipping ip_summed to CHECKSUM_NONE so both the address and port updates
use csum_replace*() (the complete-checksum path).

Test results (10 connections, single netns vxlan + veth environment):
  Before fix: ~1080ms avg first-connection latency, 10 SYN retransmits,
              UdpInCsumErrors incremented per connection
  After fix:  ~29ms avg first-connection latency, 0 SYN retransmits,
              UdpInCsumErrors unchanged

Fixes: faec18dbb0405 ("netfilter: nat: remove l4proto->manip_pkt")
Cc: stable@vger.kernel.org
Signed-off-by: lvjunyu <lvjunyu@cmss.chinamobile.com>

---

Changes in v2:
- Fix both address and port delta (v1 only fixed port delta)
- Use offload-target test instead of skb->encapsulation as the LCO
  discriminator (addresses Sashiko AI review feedback)
- Update comment to describe both CHECKSUM_PARTIAL states

v1: https://lore.kernel.org/netdev/20260920074543.525572-1-lvjunyu@cmss.chinamobile.com/
---
 net/netfilter/nf_nat_proto.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/net/netfilter/nf_nat_proto.c b/net/netfilter/nf_nat_proto.c
index 64b9bac228e..cb70d85716b 100644
--- a/net/netfilter/nf_nat_proto.c
+++ b/net/netfilter/nf_nat_proto.c
@@ -54,9 +54,35 @@ __udp_manip_pkt(struct sk_buff *skb,
 		portptr = &hdr->dest;
 	}
 	if (do_csum) {
+		/* When udp_set_csum() takes the LCO branch (encapsulated
+		 * CHECKSUM_PARTIAL, non-GSO), uh->check holds a complete
+		 * (folded and complemented) checksum while csum_start
+		 * points at the inner transport header, not at this outer
+		 * udphdr.  In that state, inet_proto_csum_replace*() and
+		 * nf_csum_update() take the pseudo-header branch which
+		 * operates on an uncomplemented seed, applying the address
+		 * and port deltas with the wrong sign.  Temporarily flip
+		 * ip_summed so they use csum_replace*() instead.
+		 *
+		 * The seed/offload branch of udp_set_csum() sets csum_start
+		 * to the outer UDP header, so the offload-target test
+		 * below distinguishes the two states.
+		 */
+		bool lco = skb->ip_summed == CHECKSUM_PARTIAL &&
+			   !skb_is_gso(skb) &&
+			   (skb->head + skb->csum_start + skb->csum_offset) !=
+			   (unsigned char *)&hdr->check;
+
+		if (lco)
+			skb->ip_summed = CHECKSUM_NONE;
+
 		nf_csum_update(skb, iphdroff, &hdr->check, tuple, maniptype);
 		inet_proto_csum_replace2(&hdr->check, skb, *portptr, newport,
 					 false);
+
+		if (lco)
+			skb->ip_summed = CHECKSUM_PARTIAL;
+
 		if (!hdr->check)
 			hdr->check = CSUM_MANGLED_0;
 	}
--
2.43.0

Re: [PATCH v2] netfilter: nf_nat: Fix stale outer UDP checksum on VXLAN encapsulated packets

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Peer call-site divergence: after this patch nf_nat's
  __udp_manip_pkt() treats a CHECKSUM_PARTIAL skb whose offload target…
- [Low] The new comment in __udp_manip_pkt() states that both the address
  and the port delta are "applying the address and port deltas with the…

Pre-existing issues:
- [Medium] Incomplete/asymmetric fix inside net/netfilter/nf_nat_proto.c:
  the new "offload target is not this header" discriminator was added…

--

netfilter: nf_nat: Fix stale outer UDP checksum on VXLAN encapsulated packets

This patch makes __udp_manip_pkt() detect the state left behind by the LCO
branch of udp_set_csum(), where uh->check holds a complete checksum but
csum_start still points at the inner transport header.  It does so by
comparing the skb's csum offload target against the udphdr being mangled,
and temporarily setting ip_summed to CHECKSUM_NONE across the address and
port fixups so the complete-checksum helpers are used.

A few questions below.

> diff --git a/net/netfilter/nf_nat_proto.c b/net/netfilter/nf_nat_proto.c
> index 64b9bac228eae..cb70d85716bd3 100644
> --- a/net/netfilter/nf_nat_proto.c
> +++ b/net/netfilter/nf_nat_proto.c
> @@ -54,9 +54,35 @@ __udp_manip_pkt(struct sk_buff *skb,
>  		portptr = &hdr->dest;
>  	}
>  	if (do_csum) {
> +		/* When udp_set_csum() takes the LCO branch (encapsulated
> +		 * CHECKSUM_PARTIAL, non-GSO), uh->check holds a complete
> +		 * (folded and complemented) checksum while csum_start
> +		 * points at the inner transport header, not at this outer
> +		 * udphdr.  In that state, inet_proto_csum_replace*() and
> +		 * nf_csum_update() take the pseudo-header branch which
> +		 * operates on an uncomplemented seed, applying the address
> +		 * and port deltas with the wrong sign.  Temporarily flip

[Severity: Low]
This isn't a bug in the code change itself, but does the comment describe
the port delta correctly?  The port fixup just below passes pseudohdr =
false:

	inet_proto_csum_replace2(&hdr->check, skb, *portptr, newport,
				 false);

and inet_proto_csum_replace4() in net/core/utils.c only has a
CHECKSUM_PARTIAL path guarded by pseudohdr:

	if (skb->ip_summed != CHECKSUM_PARTIAL) {
		csum_replace4(sum, from, to);
		...
	} else if (pseudohdr)
		*sum = ~csum_fold(csum_add(csum_sub(csum_unfold(*sum),
						    (__force __wsum)from),
					   (__force __wsum)to));

So with pseudohdr = false and CHECKSUM_PARTIAL the port delta is not
applied at all, rather than applied with the wrong sign.  The changelog
already says this ("inet_proto_csum_replace2() is a no-op (pseudohdr=false,
CHECKSUM_PARTIAL)"), so could the comment be reworded to keep the two cases
distinct?

> +		 * ip_summed so they use csum_replace*() instead.
> +		 *
> +		 * The seed/offload branch of udp_set_csum() sets csum_start
> +		 * to the outer UDP header, so the offload-target test
> +		 * below distinguishes the two states.
> +		 */
> +		bool lco = skb->ip_summed == CHECKSUM_PARTIAL &&
> +			   !skb_is_gso(skb) &&
> +			   (skb->head + skb->csum_start + skb->csum_offset) !=
> +			   (unsigned char *)&hdr->check;
> +
> +		if (lco)
> +			skb->ip_summed = CHECKSUM_NONE;
> +

[Severity: Medium]
Does the same fixup need to happen in the flowtable software fast path?
nf_flow_nat_ip_udp() in net/netfilter/nf_flow_table_ip.c rewrites the same
outer UDP header for a NATed, offloaded conntrack entry and keeps the
pseudo-header form:

net/netfilter/nf_flow_table_ip.c:nf_flow_nat_ip_udp() {
	if (udph->check || skb->ip_summed == CHECKSUM_PARTIAL) {
		inet_proto_csum_replace4(&udph->check, skb, addr,
					 new_addr, true);
		if (!udph->check)
			udph->check = CSUM_MANGLED_0;
	}
}

nf_flow_nat_ipv6_udp() does the same with inet_proto_csum_replace16(...,
true).  For an LCO-state skb (ip_summed == CHECKSUM_PARTIAL, csum_start at
the inner transport header) that reaches nf_flow_offload_ip_hook() ->
nf_flow_snat_ip() -> nf_flow_nat_ip_l4proto() -> nf_flow_nat_ip_udp(),
isn't uh->check still treated as an uncomplemented seed there, leaving the
outer UDP checksum wrong in exactly the way the changelog describes?

After this patch the two in-tree implementations disagree about the meaning
of uh->check for an identical packet state.

There is also a smaller difference in the zero-checksum gate: udp_manip_pkt()
passes do_csum = !!hdr->check, so an outer UDP header that deliberately
carries no checksum is left alone, while the flowtable condition above also
fires when udph->check == 0 as long as ip_summed == CHECKSUM_PARTIAL, and
then writes a checksum or CSUM_MANGLED_0 into a header that had none.  Is
that difference intentional?

>  		nf_csum_update(skb, iphdroff, &hdr->check, tuple, maniptype);
>  		inet_proto_csum_replace2(&hdr->check, skb, *portptr, newport,
>  					 false);

[Severity: Medium]
This isn't a bug introduced by this patch, but the same unguarded
nf_csum_update() pattern exists at three sibling sites in this file, and
they are left untouched.  Were they considered?

tcp_manip_pkt() ends with:

	nf_csum_update(skb, iphdroff, &hdr->check, tuple, maniptype);
	inet_proto_csum_replace2(&hdr->check, skb, oldport, newport, false);

icmpv6_manip_pkt() does:

	nf_csum_update(skb, iphdroff, &hdr->icmp6_cksum, tuple, maniptype);

and the CHECKSUM_PARTIAL branches of nf_nat_ipv4_csum_recalc() and
nf_nat_ipv6_csum_recalc() do:

	} else {
		inet_proto_csum_replace2(check, skb,
					 htons(oldlen), htons(datalen), true);
	}

For tcp_manip_pkt() and icmpv6_manip_pkt() the header being mangled can be
the one embedded in an ICMP error, reached via nf_nat_ipv4_fn() ->
nf_nat_icmp_reply_translation() -> nf_nat_ipv4_manip_pkt(), and that caller
already has an explicit "if (skb->ip_summed != CHECKSUM_PARTIAL)" path for
the outer ICMP checksum, so CHECKSUM_PARTIAL skbs are an expected input
there.  In that case the offload target is the outer ICMP checksum rather
than the embedded TCP/ICMPv6 checksum, which looks like the same
precondition this patch guards against for UDP.

Since all three manip_pkt callers funnel through nf_csum_update() and always
pass pseudohdr = true, would putting the offload-target test in
nf_csum_update() (or a small shared helper) cover every caller instead of
only the UDP one?

> +
> +		if (lco)
> +			skb->ip_summed = CHECKSUM_PARTIAL;
> +
>  		if (!hdr->check)
>  			hdr->check = CSUM_MANGLED_0;
>  	}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921130312.686493-1-lvjunyu%40cmss.chinamobile.com