Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> This patch introduces nf_ct_resolve_clash() to resolve race condition on
> conntrack insertions.
>
> This is particularly a problem for connection-less protocols such as
> UDP, with no initial handshake. Two or more packets may race to insert
> the entry resulting in packet drops.
>
> Another problematic scenario are packets enqueued to userspace via
> NFQUEUE after the raw table, that make it easier to trigger this
> race.
>
> To resolve this, the idea is to reset the conntrack entry to the one
> that won race. Packet and bytes counters are also merged.
>
> The 'insert_failed' stats still accounts for this situation, after
> this patch, the drop counter is bumped whenever we drop packets, so we
> can watch for unresolved clashes.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> ---
> v2: drop refcount of the old conntrack entry, otherwise we leak this.
> Call nf_ct_add_to_dying_list() before clash resolution.
> +/* Resolve race on insertion if this protocol allows this. */
> +static int nf_ct_resolve_clash(struct net *net, struct sk_buff *skb,
> + struct nf_conn *old_ct,
> + enum ip_conntrack_info ctinfo,
> + struct nf_conntrack_tuple_hash *h)
> +{
> + struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h);
> + struct nf_conntrack_l4proto *l4proto;
> +
> + l4proto = __nf_ct_l4proto_find(nf_ct_l3num(ct), nf_ct_protonum(ct));
> + if (l4proto->allow_clash &&
> + !nf_ct_is_dying(ct) &&
> + atomic_inc_not_zero(&ct->ct_general.use)) {
I found this confusing, perhaps add small one-liner comment that
*ct is in fact ct already in the table, not the one that was attached
to skb->nfct (perhaps I just need more coffee, sorry).
> + /* Don't modify skb->nfctinfo, we're at POSTROUTING so this
> + * packet is already leaving our framework, it is too late.
> + */
Note that this might be loopback in which case this skb will
reappear on PREROUTING.
> + skb->nfct = &ct->ct_general;
> + nf_ct_acct_merge(ct, ctinfo, old_ct);
> + nf_ct_put(old_ct);
Perhaps it would be better to not have old_ct and instead
nf_conntrack_put(skb->nfct);
skb->nfct = &ct->ct_general;
?
> + int ret;
>
> ct = nf_ct_get(skb, &ctinfo);
> net = nf_ct_net(ct);
> @@ -727,10 +770,11 @@ __nf_conntrack_confirm(struct sk_buff *skb)
>
> out:
> nf_ct_add_to_dying_list(ct);
> + ret = nf_ct_resolve_clash(net, skb, ct, ctinfo, h);
Is this safe?
Seems we jump to out label in other cases as well, not
just for clashes.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
