[PATCH nf] netfilter: nf_nat: do not link a conntrack twice into nat_bysource
DORMANTno replies
REVIEWED: 1 (0M)
From: Vimal Agrawal <hidden>
Date: 2026-10-22 11:02:09
Also in:
netfilter-devel
Subsystem:
netfilter, networking [general], the rest · Maintainers:
Pablo Neira Ayuso, Florian Westphal, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
1 review trailer.
From: Vimal Agrawal <redacted> nf_nat_setup_info() assumes it owns an unconfirmed conntrack, but the clones made by br_flood() share one (skb_clone() only takes a reference). That is safe while they are handled one after another on the same CPU. If one clone is sent to NFQUEUE and another is not, the queued one is reinjected later from process context by nf_reinject() (BH enabled, any CPU) and can run NAT concurrently with the other. Both callers can then reach hlist_add_head_rcu(&ct->nat_bysource, ...). Linking a node twice makes it point to itself, so a walk of that bucket never ends. Also, "ct->status |= IPS_SRC_NAT" is a plain read-modify-write outside nf_nat_locks[]; it can erase a concurrently set IPS_SRC_NAT_DONE, and nf_nat_cleanup_conntrack() then leaves the node of a freed conntrack in the bucket. We hit this on 6.6.49 (tainted, out-of-tree modules) as a general protection fault in the bucket walk, from a packet reinjected from NFQUEUE: general protection fault, probably for non-canonical address 0xabeb549edb10a8d4: 0000 [#1] SMP NOPTI RIP: 0010:nf_nat_setup_info+0x8b6/0xba0 [nf_nat] nf_nat_masquerade_ipv4+0x11d/0x200 [nf_nat] ipt_do_table+0x2b9/0x410 [ip_tables] nf_nat_inet_fn+0x14f/0x260 [nf_nat] nf_nat_ipv4_out+0x4d/0xe0 [nf_nat] nf_reinject+0x177/0x1d0 ... Use set_bit() for IPS_SRC_NAT/IPS_DST_NAT. Test IPS_SRC_NAT_DONE under nf_nat_locks[] before linking and set it only after the node is linked. A losing caller returns NF_ACCEPT, as before, without linking again. The early nf_nat_initialized() check is unchanged, and two callers that both create a real mapping are not covered. Link: https://lore.kernel.org/netfilter-devel/CALkUMdQ_6aU8Sy8vTORReQ2=vr4TskbkhZQt3WRazwUMTNEOtg@mail.gmail.com/ (local) Signed-off-by: Vimal Agrawal <redacted> Reviewed-by: Anirudh Gupta <redacted> --- net/netfilter/nf_nat_core.c | 41 +++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 9 deletions(-)
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index cc8e1e81006d..3a89c894a520 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c@@ -791,11 +791,14 @@ nf_nat_setup_info(struct nf_conn *ct, nf_ct_invert_tuple(&reply, &new_tuple); nf_conntrack_alter_reply(ct, &reply); - /* Non-atomic: we own this at the moment. */ + /* Atomic: a concurrent caller on the same unconfirmed ct may + * set IPS_SRC_NAT_DONE (set_bit() below) at the same + * time, and a plain read-modify-write here could erase it. + */ if (maniptype == NF_NAT_MANIP_SRC) - ct->status |= IPS_SRC_NAT; + set_bit(IPS_SRC_NAT_BIT, &ct->status); else - ct->status |= IPS_DST_NAT; + set_bit(IPS_DST_NAT_BIT, &ct->status); if (nfct_help(ct) && !nfct_seqadj(ct)) if (!nfct_seqadj_ext_add(ct))
@@ -806,21 +809,41 @@ nf_nat_setup_info(struct nf_conn *ct, unsigned int srchash; spinlock_t *lock; + /* Concurrent callers can reach here for the same, + * still-unconfirmed ct (skb_clone()/GSO segmentation/ + * nf_ct_attach() can share one ct across skbs processed on + * different CPUs before it is confirmed). The DONE bit is + * tested and set under this lock, and only after the node is + * linked, so a set bit always means the node is in the hash + * (nf_nat_cleanup_conntrack() relies on that). Only the first + * caller links the node; a losing caller does not link it + * again, which would corrupt the list. + * + * The winner sets the mapping (ct->status/reply tuple) before + * taking this lock, so it is final once the DONE bit is seen. + * The loser's packet is therefore accepted: nf_nat_packet() + * applies the winner's mapping to it. This is the common case + * for bridge-flooded broadcasts (e.g. DHCP) racing with a + * packet reinjected from NFQUEUE (nf_reinject() runs the + * remaining hooks with BH enabled), where the mapping is an + * identity null binding on both sides. + */ srchash = hash_by_src(net, nf_ct_zone(ct), &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple); lock = &nf_nat_locks[srchash % CONNTRACK_LOCKS]; spin_lock_bh(lock); + if (test_bit(IPS_SRC_NAT_DONE_BIT, &ct->status)) { + spin_unlock_bh(lock); + return NF_ACCEPT; + } hlist_add_head_rcu(&ct->nat_bysource, &nf_nat_bysource[srchash]); + set_bit(IPS_SRC_NAT_DONE_BIT, &ct->status); spin_unlock_bh(lock); + } else { + set_bit(IPS_DST_NAT_DONE_BIT, &ct->status); } - /* It's done. */ - if (maniptype == NF_NAT_MANIP_DST) - ct->status |= IPS_DST_NAT_DONE; - else - ct->status |= IPS_SRC_NAT_DONE; - return NF_ACCEPT; } EXPORT_SYMBOL(nf_nat_setup_info);
--
2.43.0