| Message ID | 20200203163707.27254-1-fw@strlen.de |
|---|---|
| Headers | show |
| Series | netfilter: conntrack: allow insertion of clashing entries | expand |
On Mon, Feb 03, 2020 at 05:37:03PM +0100, Florian Westphal wrote: > This series allows conntrack to insert a duplicate conntrack entry > if the reply direction doesn't result in a clash with a different > original connection. Applied, thanks for your patience. I introduced the late clash resolution approach to deal with nfqueue, now this is extended to cover more cases, let's give it a try. >Alternatives considered were: >1. Confirm ct entries at allocation time, not in postrouting. > a. will cause uneccesarry work when the skb that creates the > conntrack is dropped by ruleset. > b. in case nat is applied, ct entry would need to be moved in > the table, which requires another spinlock pair to be taken. > c. breaks the 'unconfirmed entry is private to cpu' assumption: > we would need to guard all nfct->ext allocation requests with > ct->lock spinlock. > >2. Make the unconfirmed list a hash table instead of a pcpu list. > Shares drawback c) of the first alternative. The spinlock would need to be grabbed rarely, right? My mean, most extension allocations happen before insertion to the unconfirmed list. Only _ext_add() invocations coming after init_conntrack() might require this. Thanks.
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Mon, Feb 03, 2020 at 05:37:03PM +0100, Florian Westphal wrote: > > This series allows conntrack to insert a duplicate conntrack entry > > if the reply direction doesn't result in a clash with a different > > original connection. > > Applied, thanks for your patience. > > I introduced the late clash resolution approach to deal with nfqueue, > now this is extended to cover more cases, let's give it a try. Yes, nfqueue is one way this can happen, changes to resolver libraries to issue parallel requests have exposed this race for non-nfqueue case too. > >Alternatives considered were: > >1. Confirm ct entries at allocation time, not in postrouting. > > a. will cause uneccesarry work when the skb that creates the > > conntrack is dropped by ruleset. > > b. in case nat is applied, ct entry would need to be moved in > > the table, which requires another spinlock pair to be taken. > > c. breaks the 'unconfirmed entry is private to cpu' assumption: > > we would need to guard all nfct->ext allocation requests with > > ct->lock spinlock. > > > >2. Make the unconfirmed list a hash table instead of a pcpu list. > > Shares drawback c) of the first alternative. > > The spinlock would need to be grabbed rarely, right? My mean, most > extension allocations happen before insertion to the unconfirmed list. > Only _ext_add() invocations coming after init_conntrack() might > require this. Right, we could add __nf_ct_ext_add() which is unlocked and convert the additions happening before unconfirmed list insertion there. But there are additional problems that I forgot: a) need for one additional lookup after negative result from main table (this time in unconfirmed list). b) Need to asynchronously re-insert the skb at a later time, once the racing entry is confirmed. We can't use the unconfirmed ct as-is, because it may be incomplete. For instance, the racing skb might not yet have hit the nat table, so the ct contains wrong NAT info. I think b) is a non-starter for all of the alternatives, unfortunately.
This series allows conntrack to insert a duplicate conntrack entry if the reply direction doesn't result in a clash with a different original connection. Background: kubernetes creates load-balancing rules for DNS using -m statistics, e.g.: -p udp --dport 53 -m statistics --mode random ... -j DNAT --to-destination x -p udp --dport 53 -m statistics --mode random ... -j DNAT --to-destination y When the resolver sends an A and AAAA request back-to-back from different threads on the same socket, this has a high chance of a connection tracking clash at insertion time. This in turn results in a drop of the clashing udp packet which then results in a 5 second DNS timeout. The clash cannot be resolved with the current logic because the two conntracks entries have different NAT transformations, the first one from s:highport to x.53, the second from s:highport to y.53. One solution is to change rules to use a consistent mapping, e.g. using -m cluster or nftables 'jhash' expression. This would cause the A and AAAA requests coming from same socket to match the same rule and thus share the same NAT information. However, I do not believe this is a realistic course of action. This change adds a second clash resolution/drop avoidance step: A clashing entry will be added anyway provided the reply direction is unique. Because this results in duplicate conntrack entries for the original direction, this comes with strings attached: 1. The clashed entry will only be around for 1 second 2. The clashed entry can only be found in reply direction (not inserted for ORIGINAL) 3. The clashed entry is auto-removed once first reply comes in 4 The clashed entry is never assured and can thus be evicted if conntrack table becomes full. Major change since RFC: 1. Do not insert the duplicate/clash in original dir. 2. This implicitly hides the entry from "conntrack -L". 3. use an internal status bit to auto-remove the conntrack when first reply comes in. 4. Extend the commit message of last patch to include a summary of alternate proposals (and why they did not work out). I'm sending this for nf rather than nf-next because I consider this a bug fix, but I am fine if this is deferred for nf-next instead. Florian Westphal (4): netfilter: conntrack: remove two args from resolve_clash netfilter: conntrack: place confirm-bit setting in a helper netfilter: conntrack: split resolve_clash function netfilter: conntrack: allow insertion of clashing entries include/linux/rculist_nulls.h | 7 +++++ include/uapi/linux/netfilter/nf_conntrack_common.h | 12 ++++++++- net/netfilter/nf_conntrack_core.c | 192 ++++++++++++++++------ net/netfilter/nf_conntrack_proto_udp.c | 20 ++++++++++-- 4 files changed, 198 insertions(+), 33 deletions(-)