* Re: [PATCH v2] netfilter: nf_nat: Fix stale outer UDP checksum on VXLAN encapsulated packets
2026-09-21 13:03 [PATCH v2] netfilter: nf_nat: Fix stale outer UDP checksum on VXLAN encapsulated packets lvjunyu
@ 2026-09-24 7:04 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-09-24 7:04 UTC (permalink / raw)
To: lvjunyu
Cc: pablo, fw, phil, netfilter-devel, netdev, linux-kernel, stable, kuba
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
^ permalink raw reply [flat|nested] 2+ messages in thread