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

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.