Re: netfilter: nf_nat: race in nf_nat_setup_info() corrupts nat_bysource list (GPF / soft lockup)

Hi Florian,

The specific precondition — br_flood() cloning skbs that share one
unconfirmed ct — is stock br_netfilter behavior, not anything in our
custom module; nf_ct_drop_unconfirmed()'s own comment already names
this exact case (br_netfilter/multicast routing). The only thing our
module does is decide, per-packet, whether to route a given skb
through NFQUEUE — which is exactly what the NFQUEUE API is for, not an
unusual or abusive use of it. What's missing isn't anything in our
module; it's that nf_ct_drop_unconfirmed()'s "am I the exclusive owner
of this unconfirmed ct" check only runs at NFQUEUE-enqueue time. A
sibling clone that never goes through NFQUEUE — e.g. one that takes
the plain synchronous forward path while another clone of the same ct
is off in NFQUEUE + reinject — never hits that check at all, and stock
kernel code doesn't check it anywhere else on that path either.

Agreed, and we're not disputing that this is not a complete fix. But
auditing and converting every extension-add call site and every
non-atomic ct->status write across nf_conntrack_core.c, every ALG
helper (ftp/sip/pptp/h323/irc), and ctnetlink is a much larger effort
than we can responsibly take on as part of resolving two specific
production crashes. We'd like to keep the scope of this fix limited to
nf_nat_setup_info() — the exact function both crashes faulted in, with
live disassembly confirming the fault site in both cases — rather than
trying to fix the whole class in one patch. A broader fix (e.g. a
common checkpoint that prevents the sharing from ever reaching any of
these unguarded sites, generalizing what nf_ct_drop_unconfirmed()
already does at NFQUEUE-enqueue time) seems like the right direction
longer-term, but feels like it deserves its own design discussion
rather than being folded into a fix for this specific bug report.

With that scope, here's what we're proposing (corrected from your
sketch: test_and_set_bit() takes the bit number first, address second
— yours had them swapped — and this wires up the actual drop on the
losing path, which your version fell through past):

+++ b/net/netfilter/nf_nat_core.c
@@ -839,17 +839,25 @@ nf_nat_setup_info(struct nf_conn *ct,
                                      &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple);
                lock = &nf_nat_locks[srchash % CONNTRACK_LOCKS];
                spin_lock_bh(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). test_and_set_bit()
+                * makes the check-and-mark atomic under this same lock, so
+                * only the first caller links the node; a losing caller
+                * drops instead of double-linking and corrupting the list.
+                */
+               if (test_and_set_bit(IPS_SRC_NAT_DONE_BIT, &ct->status)) {
+                       spin_unlock_bh(lock);
+                       return NF_DROP;
+               }
                hlist_add_head_rcu(&ct->nat_bysource,
                                   &nf_nat_bysource[srchash]);
                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);

This makes the check-and-mark atomic under the same lock that protects
the list insert, so a losing concurrent caller drops instead of
double-linking nat_bysource. Does keeping this fix scoped to
nf_nat_setup_info() seem reasonable to you, with the broader
extension/status-write question left open for separate discussion?

I can send the formal patch thereafter.

Vimal

On Mon, Aug 17, 2026 at 8:43 PM Florian Westphal <[email protected]> wrote:
>
> Vimal Agrawal <[email protected]> wrote:
> > I do see some conntrack updates passed nf_nat_initialized check in
> > nf_nat_setup_info so ideally it should not do anything for the second
> > or other run for the same conntrack.
> > How about locking out the whole of nf_nat_setup_info on ct->lock. i..e
> > spin_lock_bh(&ct->lock) at the start of the function and unlock before
> > return.
>
> It would be good to understand that this isn't a 3rd party module bug
> first.
>
> As I said, conntrack doesn't expect unconfirmed conntracks racing
> on multiple CPUs.
>
> Minimal hack (not even compile tested):
> diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> --- a/net/netfilter/nf_nat_core.c
> +++ b/net/netfilter/nf_nat_core.c
> @@ -815,16 +815,12 @@ nf_nat_setup_info(struct nf_conn *ct,
>                                       &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple);
>                 lock = &nf_nat_locks[srchash % CONNTRACK_LOCKS];
>                 spin_lock_bh(lock);
> -               hlist_add_head_rcu(&ct->nat_bysource,
> -                                  &nf_nat_bysource[srchash]);
> +               if (!test_and_set_bit(&ct->status, IPS_SRC_NAT_DONE_BIT))
> +                       hlist_add_head_rcu(&ct->nat_bysource,
> +                                          &nf_nat_bysource[srchash]);
>                 spin_unlock_bh(lock);
> -       }
> -
> -       /* It's done. */
> -       if (maniptype == NF_NAT_MANIP_DST)
> -               ct->status |= IPS_DST_NAT_DONE;
> -       else
> -               ct->status |= IPS_SRC_NAT_DONE;
> +       } else
> +               set_bit(&ct->status, IPS_DST_NAT_DONE_BIT);
>
>         return NF_ACCEPT;
>  }
>
> But its not enough, if we assume racing unconfirmed conntracks are now allowed, then
> all places adding conntrack extensions are buggy, and all direct ct->status changes
> (not using set_bit APIs) are buggy too.