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

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.