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

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [High] The fix is incomplete.
- [Low] The commit message describes the trigger more narrowly than the
  code shows.
- [Low] The new comment in __udp_manip_pkt() presents the offload-target
  test as a way to tell apart only the two states that udp_set_csum()…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the IPVS UDP
  NAT handlers have the same bug.

--

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

This patch changes __udp_manip_pkt() to detect the local checksum offload
(LCO) state. In that state the outer UDP header holds a complete checksum
while csum_start points at the inner transport header. When it sees this
state, it sets ip_summed to CHECKSUM_NONE for the duration of the update,
so the address and port deltas are applied through csum_replace*().

> 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.

[Severity: Low]
Is the trigger described here narrower than the real one?

Any SNAT, DNAT or MASQUERADE that changes the address of an LCO-state
outer UDP header hits the wrong-sign delta in
nf_csum_update()->inet_proto_csum_replace4/16(pseudohdr=true), even if no
port changes. Any port rewrite hits the no-op inet_proto_csum_replace2().

The LCO branch lives in udp_set_csum(). Its callers are
udp_tunnel_xmit_skb() (VXLAN, GENEVE, bareudp and others),
l2tp_xmit_core(), fou_build_udp() and ip_vs_tunnel_xmit().
udp6_set_csum() has the same branch for IPv6, and the patch also changes
IPv6 behaviour through nf_nat_ipv6_csum_update().

The reproducer may have needed --random-fully only because MASQUERADE in a
single netns keeps the same source address. That leaves the port as the
only field that changes.

Could the changelog state the general trigger, so stable triage can judge
the impact correctly?

> diff --git a/net/netfilter/nf_nat_proto.c b/net/netfilter/nf_nat_proto.c
> index 64b9bac228eae..6779e9f2812d9 100644
> --- a/net/netfilter/nf_nat_proto.c
> +++ b/net/netfilter/nf_nat_proto.c
> @@ -54,9 +54,39 @@ __udp_manip_pkt(struct sk_buff *skb,
>  		portptr = &hdr->dest;
>  	}
>  	if (do_csum) {
> +		/* When udp_set_csum() takes the LCO branch (encapsulated

[ ... ]

> +		 * 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.
> +		 */

[Severity: Low]
This isn't a bug, but is this comment complete? It describes the test as
telling apart only the two states that udp_set_csum() creates. The
predicate below is true for any CHECKSUM_PARTIAL non-GSO skb whose
csum_start + csum_offset is not &hdr->check.

One such case is the UDP header embedded in an ICMP or ICMPv6 error. In
nf_nat_icmp_reply_translation() and nf_nat_icmpv6_reply_translation(), it
is rewritten through:

  nf_nat_ipv{4,6}_manip_pkt()
    udp_manip_pkt()
      __udp_manip_pkt()

On that path the offload target is the outer ICMP checksum.

The embedded UDP checksum there is a complete checksum, so switching to
csum_replace*() gives the right result. The only gap is that neither the
comment nor the lco name covers this case.

The comment also names only the IPv4 helpers. This code covers
udp6_set_csum() and inet_proto_csum_replace16() too.

> +		bool lco = skb->ip_summed == CHECKSUM_PARTIAL &&
> +			   !skb_is_gso(skb) &&
> +			   (skb->head + skb->csum_start + skb->csum_offset) !=
> +			   (unsigned char *)&hdr->check;

[Severity: High]
Is this fix complete for flows that are offloaded to an nft flowtable?

The flowtable fast path applies the same conntrack NAT with the old
helpers:

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);
    ...
}

net/netfilter/nf_flow_table_core.c:nf_flow_nat_port_udp() {
    ...
	if (udph->check || skb->ip_summed == CHECKSUM_PARTIAL) {
		inet_proto_csum_replace2(&udph->check, skb, port,
					 new_port, false);
    ...
}

nf_flow_nat_ipv6_udp() follows the same pattern with
inet_proto_csum_replace16().

For an LCO-state outer UDP header, these functions hit the same two
failures this patch fixes in __udp_manip_pkt(): the address delta gets the
wrong sign, and the port update does nothing. Nothing in the flowtable
path filters out CHECKSUM_PARTIAL or encapsulated skbs.

Take a MASQUERADE or SNAT rule plus a flowtable. The first packets of a
UDP tunnel flow get correct checksums through nf_nat. Once the conntrack
is offloaded, the rest of the flow goes through:

  nf_flow_offload_ip_hook()
    nf_flow_offload_forward()
      nf_flow_snat_ip() / nf_flow_dnat_ip()
        nf_flow_nat_ip_l4proto()
          nf_flow_nat_ip_udp()
      nf_flow_snat_port() / nf_flow_dnat_port()
        nf_flow_nat_port_udp()

Would the receiver then drop those packets with UdpInCsumErrors?

tcf_nat_act() in net/sched/act_nat.c uses the same pattern on outer UDP
headers.

All of these callers rely on one assumption in inet_proto_csum_replace*():
under CHECKSUM_PARTIAL, the field being patched is taken to be the
offload target.

net/core/utils.c:inet_proto_csum_replace4() {
    ...
	} else if (pseudohdr)
		*sum = ~csum_fold(csum_add(csum_sub(csum_unfold(*sum),
						    (__force __wsum)from),
					   (__force __wsum)to));
}

Would it work better to compare sum against skb->head + csum_start +
csum_offset inside those shared helpers? That would cover nf_nat, the
flowtable and act_nat in one place, instead of a local workaround in
__udp_manip_pkt().

> +
> +		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;
> +

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but the
IPVS UDP NAT handlers look like they have the same problem.

udp_snat_handler() and udp_dnat_handler() in
net/netfilter/ipvs/ip_vs_proto_udp.c check only ip_summed:

	if (skb->ip_summed == CHECKSUM_PARTIAL) {
		udp_partial_csum_update(cp->af, udph, &cp->daddr, &cp->vaddr,
					htons(oldlen),
					htons(skb->len - udphoff));

udp_partial_csum_update() always treats the field as a seed:

	uhdr->check =
		~csum_fold(ip_vs_check_diff4(oldip->ip, newip->ip,
				ip_vs_check_diff2(oldlen, newlen,
						csum_unfold(uhdr->check))));

It never applies the port change.

For an LCO outer UDP header built by udp_set_csum() or udp6_set_csum(),
won't IPVS also apply the address delta with the wrong sign and skip the
port delta?

After this patch, nf_nat and IPVS would also handle the same skb state
differently.

>  		if (!hdr->check)
>  			hdr->check = CSUM_MANGLED_0;
>  	}

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