On Tue, May 03, 2016 at 02:14:26PM +0200, Florian Westphal wrote:
> 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).
OK, will add this.
> > + /* 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.
The comment intention is that we probably already applied a filtering
decision, so changing the ctinfo here seems awkward to me.
In NFQUEUE, this packet may have spent quite a bit of time so it may
even get a different ctinfo if we reevaluate, but as I said, having
packets changing its original ctinfo is...
> > + 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;
>
> ?
I can use this if you find it more readable, no problem.
> > + 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.
We're jumping out for dying conntracks too, and clash is handling this
already so I considered not adding more code. I could just run
nf_ct_resolve_clash() iff !nf_ct_dying() but I don't see much of a
benefit on this.
--
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
