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

[Florian Westphal <fw@strlen.de>]

Vimal Agrawal <avimalin@gmail.com> wrote:
> Our custom module can decide to send the first packet of a conntrack
> to userspace via NFQUEUE while the rest of the packets for that
> conntrack follow the standard kernel forward path.
> In the case of a bridge, br_flood() clones skbs, and those clones
> share one unconfirmed conntrack for the initial packet(s) (via
> skb_clone()'s __nf_copy(), which just bumps the refcount and copies
> the same ct pointer).
> 
> br_flood() clones the skb; Skb1 and Skb2 share one unconfirmed ct:
>   Skb1 (cpu1): -> nfqueue -> [userspace verdict] -> reinject -> SNAT
> -> confirm (cpu2)
>   Skb2 (cpu1): -> SNAT -> confirm (cpu1)
> Skb1's reinject-triggered SNAT (cpu2) races Skb2's synchronous SNAT
> (cpu1) on the same, still-unconfirmed ct.
> 
> Because of this, SNAT for the two skbs sharing the unconfirmed
> conntrack races across two different CPUs. nf_nat_setup_info() sees on
> both CPUs that the conntrack isn't yet SNAT-initialized, and both try
> to initialize it — pushing the same conntrack node onto the
> nat_bysource hash chain twice, corrupting the list.

Yes, conntrack assumes exclusive access, this is why nfqueue has all
these hacks to drop cloned skbs where we don't own the unconfirmed skb,
e.g. in nfqnl_enqueue_packet().

We could add more hacks but "fixing" nf_nat_setup_info() doesn't really
help, its only one of many possible races.

If you want to add more hacks, you could move:

        /* It's done. */
        if (maniptype == NF_NAT_MANIP_DST)
                ct->status |= IPS_DST_NAT_DONE;
        else
                ct->status |= IPS_SRC_NAT_DONE;

in nf_nat_setup_info to test_and_Set_bit() done while holding the
lock to add to unconfirmed list, failure causes drop.
But it won't solve the underlying design defects.