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

The precondition for this race is br_flood() cloning skbs that share
one still-unconfirmed ct via skb_clone()'s __nf_copy() (which just
bumps the refcount and copies the same _nfct pointer). By itself,
that's not enough to race: br_flood() loops over its clones
sequentially, on the same CPU, so plain bridge flooding alone never
puts two clones of the same unconfirmed ct on different CPUs at the
same time.

What turns this into a cross-CPU race is our custom queueing hook's
per-packet decision. For a given set of cloned skbs sharing one ct,
our hook decides to send skb1 to NFQUEUE, while skb2 is not queued and
instead continues down the plain, synchronous forward path — both skbs
still pointing at the same unconfirmed ct. skb1 eventually comes back
into the kernel via nf_reinject(), driven by the userspace verdict,
and that reinject can run on a different CPU than the one that
originally queued it. Meanwhile skb2's synchronous path has been
running independently, on its own CPU, the whole time. So by the time
skb1 is reinjected, its SNAT processing in nf_nat_setup_info() can be
running concurrently with skb2's own SNAT processing, on two different
CPUs, both still holding the same unconfirmed ct — which is exactly
the scenario that corrupts nat_bysource.


On Thu, Aug 20, 2026 at 10:24 PM Florian Westphal <[email protected]> wrote:
>
> Vimal Agrawal <[email protected]> wrote:
> > 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.
>
> Yes, but why is that not enough?
>
> > 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.
>
> Yes, but that is serialized, no?
>
> If checks in nfnetlink_queue are supposed to make sure that we can't
> have concurrent execution for unconfirmed conntracks, something that
> plain bridge isn't supposed to cause (its a for-each loop, so first
> skb->nfct gets confirmed, and rest is safe).
>
> If even one skb gets processed by nfqueue, then the existing checks
> are supposed to drop the packet on either queue or reinject.
>
> See nf_ct_drop_unconfirmed() and entry->nf_ct_is_unconfirmed check
> in nfqnl_reinject().
>
> > +++ 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);
>
> Looks fine.  But I'd like to understand why its necessary resp.
> what path is triggering the cloned-and-parallel case in the first place.
>
> > 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?
>
> Yes, its fine, but I'd like to understand WHY this is happening first.
> If we really have legitimate parallel cases that we can't handle, we
> will eventually need a major redesign of conntrack engine.