(repost) netfilter: conntrack: optimization, remove central spinlock
Subsystems (3)
cover (repost) netfilter: conntrack: optimization, remove central spinlock
(Repost to netfilter-devel list)
This patchset change the conntrack locking and provides a huge performance improvements.
This patchset is based upon Eric Dumazet's proposed patch:
http://thread.gmane.org/gmane.linux.network/268758/focus=47306
I have in agreement with Eric Dumazet, taken over this patch (and turned it into a entire patchset).
Primary focus is to remove the central spinlock nf_conntrack_lock. This requires several steps to be acheived.
Patch01: Trivial cleanups
Patch02: Moves the "special" dying/unconfirmed/template lists to use a
per cpu spinlock.
Patch03: Is preparing for patch04, as it address a race
condition. Doing this a seperate patch for reviewers sake.
Patch04: Seperates expect locking from nf_conntrack_lock. The expect
list is small (default max 256), this it just get a single lock.
Patch05: Finally can remove nf_conntrack_lock, and instead uses an
array of hashed spinlocks to protect insertions/deletions of conntracks into the hash table. While still allowing dynamic resizing of the hash table.
Testing
-------
For expectations I've mostly tested the FTP nf_conntrack_ftp helper module, by commands:
for x in seq 1 300; do \
echo $x; \
echo -e "USER anonymous\nPASS nothing\nPASV" | nc 192.168.42.129 21; \
done
wget ftp://192.168.42.129/pub/delete.me.4k -O /dev/null
For overload/DoS testing, I've primarily done, SYN-flood attack testing. Results on a 24-core E5-2695v2(ES) with 10Gbit/s ixgbe (with tool trafgen)
Base kernel : New 810.405 conntrack/sec
Fixed kernel: New 2.233.876 conntrack/sec
Notice other floods attack (SYN+ACK or ACK) can easily be deflected using:
# iptables -A INPUT -m state --state INVALID -j DROP # sysctl -w net/netfilter/nf_conntrack_tcp_loose=0
E.g. this machine can reflect 6.481.463 "invalid" conntrack/sec (from an ACK-flood).
Perf data:
----------
The nf_conntrack_lock is suffers from huge contention on current generation servers (8 or more core/threads). Data from under SYN-flooding (without a listen socket)
Perf locking congestion is very "visible" on a base kernel:
- 72.56% ksoftirqd/6 [kernel.kallsyms] [k] _raw_spin_lock_bh
- _raw_spin_lock_bh
+ 25.33% init_conntrack
+ 24.86% nf_ct_delete_from_lists
+ 24.62% __nf_conntrack_confirm
+ 24.38% destroy_conntrack
+ 0.70% tcp_packet
+ 2.21% ksoftirqd/6 [kernel.kallsyms] [k] fib_table_lookup + 1.15% ksoftirqd/6 [kernel.kallsyms] [k] __slab_free + 0.77% ksoftirqd/6 [kernel.kallsyms] [k] inet_getpeer + 0.70% ksoftirqd/6 [nf_conntrack] [k] nf_ct_delete + 0.55% ksoftirqd/6 [ip_tables] [k] ipt_do_table
Perf after the patchset (SYN-flood attack):
+ 9.62% ksoftirqd/6 [kernel.kallsyms] [k] fib_table_lookup
+ 3.78% ksoftirqd/6 [kernel.kallsyms] [k] __slab_free
+ 2.71% ksoftirqd/6 [kernel.kallsyms] [k] inet_getpeer
+ 2.55% ksoftirqd/6 [kernel.kallsyms] [k] check_leaf
+ 2.38% ksoftirqd/6 [ip_tables] [k] ipt_do_table
+ 2.06% ksoftirqd/6 [kernel.kallsyms] [k] __slab_alloc
+ 1.94% ksoftirqd/6 [nf_conntrack] [k] __nf_conntrack_alloc
- 1.94% ksoftirqd/6 [kernel.kallsyms] [k] _raw_spin_lock
- _raw_spin_lock
+ 90.32% nf_conntrack_double_lock
+ 3.61% get_partial_node
+ 1.81% nf_ct_delete_from_lists
+ 1.68% __nf_conntrack_confirm
+ 1.03% sch_direct_xmit
+ 0.52% scheduler_tick
+ 1.86% ksoftirqd/6 [kernel.kallsyms] [k] nf_iterate
+ 1.80% ksoftirqd/6 [nf_conntrack] [k] init_conntrack
+ 1.77% ksoftirqd/6 [kernel.kallsyms] [k] __neigh_event_send
- 1.70% ksoftirqd/6 [kernel.kallsyms] [k] _raw_spin_lock_bh
- _raw_spin_lock_bh
+ 32.55% nf_ct_del_from_dying_or_unconfirmed_list
+ 25.33% init_conntrack
+ 19.88% tcp_packet
+ 17.97% nf_ct_delete_from_lists
+ 1.62% nf_conntrack_in
+ 1.33% ixgbe_poll
+ 0.74% destroy_conntrack
+ 1.64% ksoftirqd/6 [nf_conntrack] [k] hash_conntrack_raw
+ 1.58% ksoftirqd/6 [kernel.kallsyms] [k] __netif_receive_skb_core
+ 1.51% ksoftirqd/6 [nf_conntrack] [k] __nf_conntrack_find_get
+ 1.48% ksoftirqd/6 [kernel.kallsyms] [k] __cmpxchg_double_slab
+ 1.46% ksoftirqd/6 [nf_conntrack] [k] nf_conntrack_in
+ 1.45% ksoftirqd/6 [kernel.kallsyms] [k] __local_bh_enable_ip---
Jesper Dangaard Brouer (5):
netfilter: conntrack: remove central spinlock nf_conntrack_lock
netfilter: conntrack: seperate expect locking from nf_conntrack_lock
netfilter: avoid race with exp->master ct
netfilter: conntrack: spinlock per cpu to protect special lists.
netfilter: trivial code cleanup and doc changes
include/net/netfilter/nf_conntrack.h | 11 + include/net/netfilter/nf_conntrack_core.h | 9 + include/net/netns/conntrack.h | 13 + net/netfilter/nf_conntrack_core.c | 427 ++++++++++++++++++++--------- net/netfilter/nf_conntrack_expect.c | 36 ++ net/netfilter/nf_conntrack_h323_main.c | 4 net/netfilter/nf_conntrack_helper.c | 37 ++- net/netfilter/nf_conntrack_netlink.c | 128 +++++---- net/netfilter/nf_conntrack_sip.c | 8 - 9 files changed, 456 insertions(+), 217 deletions(-)
PATCH 1/5 netfilter: trivial code cleanup and doc changes
Changes while reading through the netfilter code.
Added hint about how conntrack nf_conn refcnt is accessed. And renamed repl_hash to reply_hash for readability
---
include/net/netfilter/nf_conntrack.h | 8 +++++++- net/netfilter/nf_conntrack_core.c | 20 ++++++++++---------- 2 files changed, 17 insertions(+), 11 deletions(-)
include/net/netfilter/nf_conntrack.h +7 -1
@@ -73,7 +73,13 @@ struct nf_conn_help {struct nf_conn {/* Usage count in here is 1 for hash table/destruct timer, 1 per skb, -plus 1 for any connection(s) we are `master' for */ +* plus 1 for any connection(s) we are `master' for +* +* Hint, SKB address this struct and refcnt via skb->nfct and +* helpers nf_conntrack_get() and nf_conntrack_put(). +* Helper nf_ct_put() equals nf_conntrack_put() by dec refcnt, +* beware nf_ct_get() is different and don't inc refcnt. +*/ struct nf_conntrack ct_general;spinlock_t lock;
net/netfilter/nf_conntrack_core.c +10 -10
@@ -408,21 +408,21 @@ EXPORT_SYMBOL_GPL(nf_conntrack_find_get);static void __nf_conntrack_hash_insert (struct nf_conn *ct,unsigned int hash, -unsigned int repl_hash) +unsigned int reply_hash) {struct net *net =nf_ct_net (ct);hlist_nulls_add_head_rcu (&ct->tuplehash[IP_CT_DIR_ORIGINAL ].hnnode, &net->ct.hash[hash]);hlist_nulls_add_head_rcu (&ct->tuplehash[IP_CT_DIR_REPLY ].hnnode, - &net->ct.hash[repl_hash]); + &net->ct.hash[reply_hash]); }int nf_conntrack_hash_check_insert (struct nf_conn *ct) {struct net *net =nf_ct_net (ct); -unsigned int hash, repl_hash; +unsigned int hash, reply_hash;struct nf_conntrack_tuple_hash *h;struct hlist_nulls_node *n;u16 zone;
@@ -430,7 +430,7 @@ nf_conntrack_hash_check_insert(struct nf_conn *ct) zone =nf_ct_zone (ct); hash =hash_conntrack (net, zone, &ct->tuplehash[IP_CT_DIR_ORIGINAL ].tuple); - repl_hash =hash_conntrack (net, zone, + reply_hash =hash_conntrack (net, zone, &ct->tuplehash[IP_CT_DIR_REPLY ].tuple);spin_lock_bh (&nf_conntrack_lock);
@@ -441,7 +441,7 @@ nf_conntrack_hash_check_insert(struct nf_conn *ct) &h->tuple) && zone == nf_ct_zone(nf_ct_tuplehash_to_ctrack(h))) goto out; -hlist_nulls_for_each_entry (h, n, &net->ct.hash[repl_hash], hnnode) +hlist_nulls_for_each_entry (h, n, &net->ct.hash[reply_hash], hnnode)if (nf_ct_tuple_equal (&ct->tuplehash[IP_CT_DIR_REPLY ].tuple, &h->tuple) && zone ==nf_ct_zone (nf_ct_tuplehash_to_ctrack (h)))
@@ -451,7 +451,7 @@ nf_conntrack_hash_check_insert(struct nf_conn *ct)smp_wmb ();/* The caller holds a reference to this object */ atomic_set (&ct->ct_general.use,2 ); -__nf_conntrack_hash_insert (ct, hash, repl_hash); +__nf_conntrack_hash_insert (ct, hash, reply_hash);NF_CT_STAT_INC (net, insert);spin_unlock_bh (&nf_conntrack_lock);
@@ -483,7 +483,7 @@ EXPORT_SYMBOL_GPL(nf_conntrack_tmpl_insert);int __nf_conntrack_confirm (struct sk_buff *skb) { -unsigned int hash, repl_hash; +unsigned int hash, reply_hash;struct nf_conntrack_tuple_hash *h;struct nf_conn *ct;struct nf_conn_help *help;
@@ -507,7 +507,7 @@ __nf_conntrack_confirm(struct sk_buff *skb)/* reuse the hash saved before */ hash = *(unsigned long *)&ct->tuplehash[IP_CT_DIR_REPLY ].hnnode.pprev; hash =hash_bucket (hash, net); - repl_hash =hash_conntrack (net, zone, + reply_hash =hash_conntrack (net, zone, &ct->tuplehash[IP_CT_DIR_REPLY ].tuple);/* We're not in hash table, and we refuse to set up related
@@ -540,7 +540,7 @@ __nf_conntrack_confirm(struct sk_buff *skb) &h->tuple) && zone == nf_ct_zone(nf_ct_tuplehash_to_ctrack(h))) goto out; -hlist_nulls_for_each_entry (h, n, &net->ct.hash[repl_hash], hnnode) +hlist_nulls_for_each_entry (h, n, &net->ct.hash[reply_hash], hnnode)if (nf_ct_tuple_equal (&ct->tuplehash[IP_CT_DIR_REPLY ].tuple, &h->tuple) && zone ==nf_ct_zone (nf_ct_tuplehash_to_ctrack (h)))
@@ -570,7 +570,7 @@ __nf_conntrack_confirm(struct sk_buff *skb)* guarantee that no other CPU can find the conntrack before the above * stores are visible. */ -__nf_conntrack_hash_insert (ct, hash, repl_hash); +__nf_conntrack_hash_insert (ct, hash, reply_hash);NF_CT_STAT_INC (net, insert);spin_unlock_bh (&nf_conntrack_lock);
PATCH 2/5 netfilter: conntrack: spinlock per cpu to protect special lists.
In reply to Jesper Dangaard BrouerOne spinlock per cpu to protect dying/unconfirmed/template special lists. (These lists are now per cpu, a bit like the untracked ct) Add a @cpu field to nf_conn, to make sure we hold the appropriate spinlock at removal time.
---
include/net/netfilter/nf_conntrack.h | 3 - include/net/netns/conntrack.h | 11 ++- net/netfilter/nf_conntrack_core.c | 139 +++++++++++++++++++++++++--------- net/netfilter/nf_conntrack_helper.c | 11 ++- net/netfilter/nf_conntrack_netlink.c | 81 +++++++++++--------- 5 files changed, 166 insertions(+), 79 deletions(-)
include/net/netfilter/nf_conntrack.h +2 -1
@@ -82,7 +82,8 @@ struct nf_conn {*/ struct nf_conntrack ct_general; -spinlock_t lock; +spinlock_t lock; +u16 cpu;/* XXX should I move this to the tail ? - Y.K */ /* These are my tuples; original and reply */
include/net/netns/conntrack.h +8 -3
@@ -62,6 +62,13 @@ struct nf_ip_net {#endif }; +struct ct_pcpu { +spinlock_t lock; +struct hlist_nulls_head unconfirmed; +struct hlist_nulls_head dying; +struct hlist_nulls_head tmpl; +}; +struct netns_ct {atomic_t count;unsigned int expect_count;
@@ -86,9 +93,7 @@ struct netns_ct {struct kmem_cache *nf_conntrack_cachep;struct hlist_nulls_head *hash;struct hlist_head *expect_hash; -struct hlist_nulls_head unconfirmed; -struct hlist_nulls_head dying; -struct hlist_nulls_head tmpl; +struct ct_pcpu __percpu *pcpu_lists;struct ip_conntrack_stat __percpu *stat;struct nf_ct_event_notifier __rcu *nf_conntrack_event_cb;struct nf_exp_event_notifier __rcu *nf_expect_event_cb;
net/netfilter/nf_conntrack_core.c +101 -38
241 lines not on this page · raw message
net/netfilter/nf_conntrack_helper.c +9 -2
@@ -396,6 +396,7 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me,const struct hlist_node *next;const struct hlist_nulls_node *nn;unsigned int i; +int cpu;/* Get rid of expectations */ for (i =0 ; i < nf_ct_expect_hsize; i++) {
@@ -414,8 +415,14 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me, }/* Get rid of expecteds, set helpers to NULL. */ -hlist_nulls_for_each_entry (h, nn, &net->ct.unconfirmed, hnnode) -unhelp (h, me); +for_each_possible_cpu (cpu) { +struct ct_pcpu *pcpu =per_cpu_ptr (net->ct.pcpu_lists, cpu); + +spin_lock_bh (&pcpu->lock); +hlist_nulls_for_each_entry (h, nn, &pcpu->unconfirmed, hnnode) +unhelp (h, me); +spin_unlock_bh (&pcpu->lock); + }for (i =0 ; i < net->ct.htable_size; i++) {hlist_nulls_for_each_entry (h, nn, &net->ct.hash[i], hnnode)unhelp (h, me);
net/netfilter/nf_conntrack_netlink.c +46 -35
114 lines not on this page · raw message
PATCH 3/5 netfilter: avoid race with exp->master ct
In reply to Jesper Dangaard BrouerPreparation for disconnecting the nf_conntrack_lock from the expectations code. Once the nf_conntrack_lock is lifted, a race condition is exposed.
The expectations master conntrack exp->master, can race with delete operations, as the refcnt increment happens too late in init_conntrack(). Race is against other CPUs invoking ->destroy() (destroy_conntrack()), or nf_ct_delete() (via timeout or early_drop()).
Avoid this race in nf_ct_find_expectation() by using atomic_inc_not_zero(), and checking if nf_ct_is_dying() (path via nf_ct_delete()).
---
net/netfilter/nf_conntrack_core.c | 2 +- net/netfilter/nf_conntrack_expect.c | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-)
net/netfilter/nf_conntrack_core.c +1 -1
@@ -898,6 +898,7 @@ init_conntrack(struct net *net, struct nf_conn *tmpl, ct, exp);/* Welcome, Mr. Bond. We've been expecting you... */ __set_bit (IPS_EXPECTED_BIT , &ct->status); +/* exp->master safe, refcnt bumped in nf_ct_find_expectation */ ct->master = exp->master;if (exp->helper) { help = nf_ct_helper_ext_add(ct, exp->helper,
@@ -912,7 +913,6 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,#ifdef CONFIG_NF_CONNTRACK_SECMARK ct->secmark = exp->master->secmark;#endif -nf_conntrack_get (&ct->master->ct_general);NF_CT_STAT_INC (net, expect_new); }else {__nf_ct_try_assign_helper (ct, tmpl,GFP_ATOMIC );
net/netfilter/nf_conntrack_expect.c +15 -1
@@ -147,13 +147,27 @@ nf_ct_find_expectation(struct net *net, u16 zone,if (!exp)return NULL ; +/* Avoid race with other CPUs, that for exp->master ct, is +* about to invoke ->destroy(), or nf_ct_delete() via timeout +* or early_drop(). +* +* The atomic_inc_not_zero() check tells: If that fails, we +* know that the ct is being destroyed. If it succeeds, we +* can be sure the ct cannot disappear underneath. +*/ +if (unlikely (nf_ct_is_dying (exp->master) || + !atomic_inc_not_zero (&exp->master->ct_general.use))) +return NULL ; +/* If master is not in hash table yet (ie. packet hasn't left this machine yet), how can other end know about expected? Hence these are not the droids you are looking for (if master ct never got confirmed, we'd hold a reference to it and weird things would happen to future packets). */ -if (!nf_ct_is_confirmed (exp->master)) +if (!nf_ct_is_confirmed (exp->master)) { +atomic_dec (&exp->master->ct_general.use);return NULL ; + }if (exp->flags &NF_CT_EXPECT_PERMANENT ) {atomic_inc (&exp->use);
PATCH 4/5 netfilter: conntrack: seperate expect locking from nf_conntrack_lock
In reply to Jesper Dangaard BrouerNetfilter expectations are protected with the same lock as conntrack entries (nf_conntrack_lock). This patch split out expectations locking to use it's own lock (nf_conntrack_expect_lock).
---
include/net/netfilter/nf_conntrack_core.h | 2 + net/netfilter/nf_conntrack_core.c | 61 ++++++++++++++++------------- net/netfilter/nf_conntrack_expect.c | 20 +++++----- net/netfilter/nf_conntrack_h323_main.c | 4 +- net/netfilter/nf_conntrack_helper.c | 14 ++++--- net/netfilter/nf_conntrack_netlink.c | 32 ++++++++------- net/netfilter/nf_conntrack_sip.c | 8 ++-- 7 files changed, 76 insertions(+), 65 deletions(-)
include/net/netfilter/nf_conntrack_core.h +2 -0
@@ -79,4 +79,6 @@ print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,extern spinlock_t nf_conntrack_lock ; +extern spinlock_t nf_conntrack_expect_lock; +#endif /* _NF_CONNTRACK_CORE_H */
net/netfilter/nf_conntrack_core.c +33 -28
113 lines not on this page · raw message
net/netfilter/nf_conntrack_expect.c +11 -9
@@ -66,9 +66,9 @@ static void nf_ct_expectation_timed_out(unsigned long ul_expect) {struct nf_conntrack_expect *exp = (void *)ul_expect; -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);nf_ct_unlink_expect (exp); -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);nf_ct_expect_put (exp); }
@@ -191,12 +191,14 @@ void nf_ct_remove_expectations(struct nf_conn *ct)if (!help)return ; +spin_lock_bh (&nf_conntrack_expect_lock);hlist_for_each_entry_safe (exp, next, &help->expectations, lnode) {if (del_timer (&exp->timeout)) {nf_ct_unlink_expect (exp);nf_ct_expect_put (exp); } } +spin_unlock_bh (&nf_conntrack_expect_lock); }EXPORT_SYMBOL_GPL (nf_ct_remove_expectations);
@@ -231,12 +233,12 @@ static inline int expect_matches(const struct nf_conntrack_expect *a,/* Generally a bad idea to call this: could have matched already. */ void nf_ct_unexpect_related (struct nf_conntrack_expect *exp) { -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);if (del_timer (&exp->timeout)) {nf_ct_unlink_expect (exp);nf_ct_expect_put (exp); } -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock); }EXPORT_SYMBOL_GPL (nf_ct_unexpect_related);
@@ -349,7 +351,7 @@ static int nf_ct_expect_insert(struct nf_conntrack_expect *exp)setup_timer (&exp->timeout, nf_ct_expectation_timed_out, (unsigned long )exp); helper =rcu_dereference_protected (master_help->helper, -lockdep_is_held (&nf_conntrack_lock)); +lockdep_is_held (&nf_conntrack_expect_lock));if (helper) { exp->timeout.expires = jiffies + helper->expect_policy[exp->class].timeout *HZ ;
@@ -409,7 +411,7 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect) }/* Will be over limit? */ helper =rcu_dereference_protected (master_help->helper, -lockdep_is_held (&nf_conntrack_lock)); +lockdep_is_held (&nf_conntrack_expect_lock));if (helper) { p = &helper->expect_policy[expect->class]; if (p->max_expected &&
@@ -436,7 +438,7 @@ int nf_ct_expect_related_report(struct nf_conntrack_expect *expect, {int ret; -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock); ret =__nf_ct_expect_check (expect);if (ret <=0 ) goto out;
@@ -444,11 +446,11 @@ int nf_ct_expect_related_report(struct nf_conntrack_expect *expect, ret =nf_ct_expect_insert (expect);if (ret <0 ) goto out; -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);nf_ct_expect_event_report (IPEXP_NEW , expect, portid, report);return ret; out: -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);return ret; }EXPORT_SYMBOL_GPL (nf_ct_expect_related_report);
net/netfilter/nf_conntrack_h323_main.c +2 -2
@@ -1476,7 +1476,7 @@ static int process_rcf(struct sk_buff *skb, struct nf_conn *ct,nf_ct_refresh (ct, skb, info->timeout *HZ );/* Set expect timeout */ -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock); exp =find_expect (ct, &ct->tuplehash[dir].tuple.dst.u3, info->sig_port[!dir]);if (exp) {
@@ -1486,7 +1486,7 @@ static int process_rcf(struct sk_buff *skb, struct nf_conn *ct,nf_ct_dump_tuple (&exp->tuple);set_expect_timeout (exp, info->timeout); } -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock); }return 0 ;
net/netfilter/nf_conntrack_helper.c +8 -6
@@ -258,7 +258,7 @@ static inline int unhelp(struct nf_conntrack_tuple_hash *i,if (help &&rcu_dereference_protected ( help->helper, -lockdep_is_held (&nf_conntrack_lock) +lockdep_is_held (&nf_conntrack_expect_lock) ) == me) {nf_conntrack_event (IPCT_HELPER , ct);RCU_INIT_POINTER (help->helper,NULL );
@@ -284,17 +284,17 @@ static LIST_HEAD(nf_ct_helper_expectfn_list);void nf_ct_helper_expectfn_register (struct nf_ct_helper_expectfn *n) { -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);list_add_rcu (&n->head, &nf_ct_helper_expectfn_list); -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock); }EXPORT_SYMBOL_GPL (nf_ct_helper_expectfn_register);void nf_ct_helper_expectfn_unregister (struct nf_ct_helper_expectfn *n) { -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);list_del_rcu (&n->head); -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock); }EXPORT_SYMBOL_GPL (nf_ct_helper_expectfn_unregister);
@@ -399,13 +399,14 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me,int cpu;/* Get rid of expectations */ +spin_lock_bh (&nf_conntrack_expect_lock);for (i =0 ; i < nf_ct_expect_hsize; i++) {hlist_for_each_entry_safe (exp, next, &net->ct.expect_hash[i], hnode) {struct nf_conn_help *help =nfct_help (exp->master);if ((rcu_dereference_protected ( help->helper, -lockdep_is_held (&nf_conntrack_lock) +lockdep_is_held (&nf_conntrack_expect_lock) ) == me || exp->helper == me) &&del_timer (&exp->timeout)) {nf_ct_unlink_expect (exp);
@@ -413,6 +414,7 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me, } } } +spin_unlock_bh (&nf_conntrack_expect_lock);/* Get rid of expecteds, set helpers to NULL. */ for_each_possible_cpu (cpu) {
net/netfilter/nf_conntrack_netlink.c +16 -16
@@ -1377,14 +1377,14 @@ ctnetlink_change_helper(struct nf_conn *ct, const struct nlattr * const cda[]) nf_ct_protonum(ct));if (helper ==NULL ) {#ifdef CONFIG_MODULES -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);if (request_module ("nfct-helper-%s" , helpname) <0 ) { -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);return -EOPNOTSUPP ; } -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock); helper =__nf_conntrack_helper_find (helpname,nf_ct_l3num (ct),nf_ct_protonum (ct));if (helper)
@@ -1822,9 +1822,9 @@ ctnetlink_new_conntrack(struct sock *ctnl, struct sk_buff *skb, err = -EEXIST ; ct =nf_ct_tuplehash_to_ctrack (h);if (!(nlh->nlmsg_flags &NLM_F_EXCL )) { -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock); err =ctnetlink_change_conntrack (ct, cda); -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);if (err ==0 ) { nf_conntrack_eventmask_report((1 << IPCT_REPLY) | (1 << IPCT_ASSURED) |
@@ -2153,9 +2153,9 @@ ctnetlink_nfqueue_parse(const struct nlattr *attr, struct nf_conn *ct)if (ret <0 )return ret; -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock); ret =ctnetlink_nfqueue_parse_ct ((const struct nlattr **)cda, ct); -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);return ret; }
@@ -2710,13 +2710,13 @@ ctnetlink_del_expect(struct sock *ctnl, struct sk_buff *skb, }/* after list removal, usage count == 1 */ -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);if (del_timer (&exp->timeout)) {nf_ct_unlink_expect_report (exp,NETLINK_CB (skb).portid,nlmsg_report (nlh));nf_ct_expect_put (exp); } -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);/* have to put what we 'get' above. * after this line usage count == 0 */ nf_ct_expect_put (exp);
@@ -2725,7 +2725,7 @@ ctnetlink_del_expect(struct sock *ctnl, struct sk_buff *skb,struct nf_conn_help *m_help;/* delete all expectations for this helper */ -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);for (i =0 ; i < nf_ct_expect_hsize; i++) { hlist_for_each_entry_safe(exp, next, &net->ct.expect_hash[i],
@@ -2740,10 +2740,10 @@ ctnetlink_del_expect(struct sock *ctnl, struct sk_buff *skb, } } } -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock); }else {/* This basically means we have to flush everything*/ -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);for (i =0 ; i < nf_ct_expect_hsize; i++) { hlist_for_each_entry_safe(exp, next, &net->ct.expect_hash[i],
@@ -2756,7 +2756,7 @@ ctnetlink_del_expect(struct sock *ctnl, struct sk_buff *skb, } } } -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock); }return 0 ;
@@ -2982,11 +2982,11 @@ ctnetlink_new_expect(struct sock *ctnl, struct sk_buff *skb,if (err <0 )return err; -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock); exp =__nf_ct_expect_find (net, zone, &tuple);if (!exp) { -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock); err = -ENOENT ;if (nlh->nlmsg_flags &NLM_F_CREATE ) { err = ctnetlink_create_expect(net, zone, cda,
@@ -3000,7 +3000,7 @@ ctnetlink_new_expect(struct sock *ctnl, struct sk_buff *skb, err = -EEXIST ;if (!(nlh->nlmsg_flags &NLM_F_EXCL )) err =ctnetlink_change_expect (exp, cda); -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);return err; }
net/netfilter/nf_conntrack_sip.c +4 -4
@@ -800,7 +800,7 @@ static int refresh_signalling_expectation(struct nf_conn *ct,struct hlist_node *next;int found =0 ; -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);hlist_for_each_entry_safe (exp, next, &help->expectations, lnode) { if (exp->class != SIP_EXPECT_SIGNALLING || !nf_inet_addr_cmp(&exp->tuple.dst.u3, addr) ||
@@ -815,7 +815,7 @@ static int refresh_signalling_expectation(struct nf_conn *ct, found =1 ;break ; } -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);return found; }
@@ -825,7 +825,7 @@ static void flush_expectations(struct nf_conn *ct, bool media)struct nf_conntrack_expect *exp;struct hlist_node *next; -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);hlist_for_each_entry_safe (exp, next, &help->expectations, lnode) {if ((exp->class !=SIP_EXPECT_SIGNALLING ) ^ media)continue ;
@@ -836,7 +836,7 @@ static void flush_expectations(struct nf_conn *ct, bool media)if (!media)break ; } -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock); } static int set_expected_rtp_rtcp(struct sk_buff *skb, unsigned int protoff,
PATCH 5/5 netfilter: conntrack: remove central spinlock nf_conntrack_lock
In reply to Jesper Dangaard Brouernf_conntrack_lock is a monolithic lock and suffers from huge contention on current generation servers (8 or more core/threads).
Perf locking congestion is clear on base kernel:
- 72.56% ksoftirqd/6 [kernel.kallsyms] [k] _raw_spin_lock_bh
- _raw_spin_lock_bh
+ 25.33% init_conntrack
+ 24.86% nf_ct_delete_from_lists
+ 24.62% __nf_conntrack_confirm
+ 24.38% destroy_conntrack
+ 0.70% tcp_packet
+ 2.21% ksoftirqd/6 [kernel.kallsyms] [k] fib_table_lookup + 1.15% ksoftirqd/6 [kernel.kallsyms] [k] __slab_free + 0.77% ksoftirqd/6 [kernel.kallsyms] [k] inet_getpeer + 0.70% ksoftirqd/6 [nf_conntrack] [k] nf_ct_delete + 0.55% ksoftirqd/6 [ip_tables] [k] ipt_do_table
This patch change conntrack locking and provides a huge performance improvement. SYN-flood attack tested on a 24-core E5-2695v2(ES) with 10Gbit/s ixgbe (with tool trafgen):
Base kernel: 810.405 new conntrack/sec
After patch: 2.233.876 new conntrack/sec
Notice other floods attack (SYN+ACK or ACK) can easily be deflected using:
# iptables -A INPUT -m state --state INVALID -j DROP # sysctl -w net/netfilter/nf_conntrack_tcp_loose=0
Use an array of hashed spinlocks to protect insertions/deletions of conntracks into the hash table. 1024 spinlocks seem to give good results, at minimal cost (4KB memory). Due to lockdep max depth, 1024 becomes 8 if CONFIG_LOCKDEP=y
The hash resize is a bit tricky, because we need to take all locks in the array. A seqcount_t is used to synchronize the hash table users with the resizing process.
---
include/net/netfilter/nf_conntrack_core.h | 7 + include/net/netns/conntrack.h | 2 net/netfilter/nf_conntrack_core.c | 219 +++++++++++++++++++++-------- net/netfilter/nf_conntrack_helper.c | 12 +- net/netfilter/nf_conntrack_netlink.c | 15 ++ 5 files changed, 188 insertions(+), 67 deletions(-)
include/net/netfilter/nf_conntrack_core.h +6 -1
@@ -77,7 +77,12 @@ print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,const struct nf_conntrack_l3proto *l3proto, const struct nf_conntrack_l4proto *proto); -extern spinlock_t nf_conntrack_lock ; +#ifdef CONFIG_LOCKDEP +# define CONNTRACK_LOCKS 8 +#else +# define CONNTRACK_LOCKS 1024 +#endif +extern spinlock_t nf_conntrack_locks[CONNTRACK_LOCKS ];extern spinlock_t nf_conntrack_expect_lock;
include/net/netns/conntrack.h +2 -0
@@ -5,6 +5,7 @@#include <linux/list_nulls.h> #include <linux/atomic.h> #include <linux/netfilter/nf_conntrack_tcp.h> +#include <linux/seqlock.h> struct ctl_table_header ;struct nf_conntrack_ecache ;
@@ -90,6 +91,7 @@ struct netns_ct {int sysctl_checksum;unsigned int htable_size; +seqcount_t generation;struct kmem_cache *nf_conntrack_cachep;struct hlist_nulls_head *hash;struct hlist_head *expect_hash;
net/netfilter/nf_conntrack_core.c +159 -60
372 lines not on this page · raw message
net/netfilter/nf_conntrack_helper.c +8 -4
@@ -425,10 +425,16 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me,unhelp (h, me);spin_unlock_bh (&pcpu->lock); } +local_bh_disable ();for (i =0 ; i < net->ct.htable_size; i++) { -hlist_nulls_for_each_entry (h, nn, &net->ct.hash[i], hnnode) -unhelp (h, me); +spin_lock (&nf_conntrack_locks[i %CONNTRACK_LOCKS ]); +if (i < net->ct.htable_size) { +hlist_nulls_for_each_entry (h, nn, &net->ct.hash[i], hnnode) +unhelp (h, me); + } +spin_unlock (&nf_conntrack_locks[i %CONNTRACK_LOCKS ]); } +local_bh_enable (); }void nf_conntrack_helper_unregister (struct nf_conntrack_helper *me)
@@ -446,10 +452,8 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)synchronize_rcu ();rtnl_lock (); -spin_lock_bh (&nf_conntrack_lock);for_each_net (net)__nf_conntrack_helper_unregister (me, net); -spin_unlock_bh (&nf_conntrack_lock);rtnl_unlock (); }EXPORT_SYMBOL_GPL (nf_conntrack_helper_unregister);
net/netfilter/nf_conntrack_netlink.c +13 -2
@@ -764,14 +764,23 @@ ctnetlink_dump_table(struct sk_buff *skb, struct netlink_callback *cb)struct nfgenmsg *nfmsg =nlmsg_data (cb->nlh);u_int8_t l3proto = nfmsg->nfgen_family;int res; +spinlock_t *lockp; +#ifdef CONFIG_NF_CONNTRACK_MARK const struct ctnetlink_dump_filter *filter = cb->data;#endif -spin_lock_bh (&nf_conntrack_lock); last = (struct nf_conn *)cb->args[1 ]; + +local_bh_disable ();for (; cb->args[0 ] < net->ct.htable_size; cb->args[0 ]++) { restart: + lockp = &nf_conntrack_locks[cb->args[0 ] %CONNTRACK_LOCKS ]; +spin_lock (lockp); +if (cb->args[0 ] >= net->ct.htable_size) { +spin_unlock (lockp); + goto out; + }hlist_nulls_for_each_entry (h, n, &net->ct.hash[cb->args[0 ]], hnnode) {if (NF_CT_DIRECTION (h) !=IP_CT_DIR_ORIGINAL )
@@ -803,16 +812,18 @@ restart:if (res <0 ) {nf_conntrack_get (&ct->ct_general); cb->args[1 ] = (unsigned long )ct; +spin_unlock (lockp); goto out; } } +spin_unlock (lockp);if (cb->args[1 ]) { cb->args[1 ] =0 ; goto restart; } } out: -spin_unlock_bh (&nf_conntrack_lock); +local_bh_enable ();if (last)nf_ct_put (last);
Florian Westphal
maintainerRe: [nf-next PATCH 3/5] netfilter: avoid race with exp->master ct
In reply to Jesper Dangaard BrouerQuoted 61 lines · Jesper Dangaard Brouer
Preparation for disconnecting the
nf_conntrack_lockfrom the expectations code. Once thenf_conntrack_lockis lifted, a race condition is exposed.The expectations master conntrack
exp->master, can race with delete operations, as the refcnt increment happens too late ininit_conntrack(). Race is against other CPUs invoking ->destroy() (destroy_conntrack()), ornf_ct_delete()(via timeout orearly_drop()).Avoid this race in
nf_ct_find_expectation()by usingatomic_inc_not_zero(), and checking ifnf_ct_is_dying()(path vianf_ct_delete()).Signed-off-by Jesper Dangaard Brouer <[email protected]>Signed-off-by Florian Westphal <[email protected]>---
net/netfilter/nf_conntrack_core.c | 2 +- net/netfilter/nf_conntrack_expect.c | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-)net/netfilter/nf_conntrack_core.c +1
-1@@ -898,6 +898,7 @@ init_conntrack(struct net *net, struct nf_conn *tmpl, ct, exp);/* Welcome, Mr. Bond. We've been expecting you... */ __set_bit (IPS_EXPECTED_BIT , &ct->status); +/* exp->master safe, refcnt bumped in nf_ct_find_expectation */ ct->master = exp->master;if (exp->helper) { help = nf_ct_helper_ext_add(ct, exp->helper,@@ -912,7 +913,6 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,#ifdef CONFIG_NF_CONNTRACK_SECMARK ct->secmark = exp->master->secmark;#endif -nf_conntrack_get (&ct->master->ct_general);NF_CT_STAT_INC (net, expect_new); }else {__nf_ct_try_assign_helper (ct, tmpl,GFP_ATOMIC );net/netfilter/nf_conntrack_expect.c +12
-0@@ -147,13 +147,27 @@ nf_ct_find_expectation(struct net *net, u16 zone,if (!exp)return NULL ; +/* Avoid race with other CPUs, that for exp->master ct, is +* about to invoke ->destroy(), or nf_ct_delete() via timeout +* or early_drop(). +* +* The atomic_inc_not_zero() check tells: If that fails, we +* know that the ct is being destroyed. If it succeeds, we +* can be sure the ct cannot disappear underneath. +*/ +if (unlikely (nf_ct_is_dying (exp->master) || + !atomic_inc_not_zero (&exp->master->ct_general.use))) +return NULL ; +
net/netfilter/nf_conntrack_expect.c nf_ct_find_expectation(struct net *net, u16 zone, · line 170
Quoted 10 lines
net/netfilter/nf_conntrack_expect.c +3
-1/* If master is not in hash table yet (ie. packet hasn't left this machine yet), how can other end know about expected? Hence these are not the droids you are looking for (if master ct never got confirmed, we'd hold a reference to it and weird things would happen to future packets). */ -if (!nf_ct_is_confirmed (exp->master)) +if (!nf_ct_is_confirmed (exp->master)) { +atomic_dec (&exp->master->ct_general.use);return NULL ; + }
Not sure if this is safe.
What about:
CPU0: atomic_inc_not_zero()
CPU1: calls nf_conntrack_put()
CPU0: atomic_dec() -> zero refcnt without invocation of ->destroy
[ Cannot happen now because of nf_conntrack_lock ]
I'd suggest to test nf_ct_is_confirmed() first, it avoids the need to undo the atomic_inc_not_zero.
You also need to deal with the "timer-deletion-fails" a bit later in the same function:
if (exp->flags & NF_CT_EXPECT_PERMANENT) {
atomic_inc(&exp->use);
return exp;
} else if (del_timer(&exp->timeout)) {
nf_ct_unlink_expect(exp);
return exp;
}
// Problem: exp->master ref was bumped
nf_ct_put(exp->master); // missing
return NULL;David Miller
maintainerFrom: Jesper Dangaard Brouer <[email protected]>
Date: Thu, 27 Feb 2014 19:23:24 +0100
Quoted 5 lines
(Repost to netfilter-devel list)
This patchset change the conntrack locking and provides a huge performance improvements.
Quoted 7 lines
This patchset is based upon Eric Dumazet's proposed patch:
http://thread.gmane.org/gmane.linux.network/268758/focus=47306
I have in agreement with Eric Dumazet, taken over this patch (and turned it into a entire patchset).Primary focus is to remove the central spinlock
nf_conntrack_lock. This requires several steps to be acheived.
I only worry about the raw_smp_processor_id()'s.
If preemption will be disabled in these contexts, then it's safe and we can just use plain smp_processor_id().
If preemption is not necessarily disabled in these spots, the use is not correct. We'll need to use get_cpu/put_cpu sequences, or (considering what these patches are doing) something like:
struct ct_pcpu *pcpu; /* add this conntrack to the (per cpu) unconfirmed list */ local_bh_disable(); ct->cpu = smp_processor_id(); pcpu = per_cpu_ptr(nf_ct_net(ct)->ct.pcpu_lists, ct->cpu); spin_lock(&pcpu->lock); hlist_nulls_add_head(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode, &pcpu->unconfirmed); spin_unlock_bh(&pcpu->lock);
Quoted 13 lines · On Thu, 27 Feb 2014 18:34:15 -0500 (EST) David Miller <[email protected]>…
From: Jesper Dangaard Brouer <[email protected]>
Date: Thu, 27 Feb 2014 19:23:24 +0100Quoted 10 lines
(Repost to netfilter-devel list)
This patchset change the conntrack locking and provides a huge performance improvements.
This patchset is based upon Eric Dumazet's proposed patch:
http://thread.gmane.org/gmane.linux.network/268758/focus=47306
I have in agreement with Eric Dumazet, taken over this patch (and turned it into a entire patchset).
Quoted 7 lines
Quoted 2 lines
Primary focus is to remove the central spinlock
nf_conntrack_lock. This requires several steps to be acheived.I only worry about the raw_smp_processor_id()'s.
If preemption will be disabled in these contexts, then it's safe and we can just use plain
smp_processor_id().
Most of the contexts are safe, one were not by mistake, and one is not. I've corrected the code (patch diff below) and tested with lockdep etc.
Quoted 15 lines
If preemption is not necessarily disabled in these spots, the use is not correct. We'll need to use get_cpu/put_cpu sequences, or (considering what these patches are doing) something like:
struct ct_pcpu *pcpu; /* add this conntrack to the (per cpu) unconfirmed list */ local_bh_disable(); ct->cpu = smp_processor_id(); pcpu = per_cpu_ptr(nf_ct_net(ct)->ct.pcpu_lists, ct->cpu); spin_lock(&pcpu->lock); hlist_nulls_add_head(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode, &pcpu->unconfirmed); spin_unlock_bh(&pcpu->lock);
Signature · 5 lines
Best regards, Jesper Dangaard Brouer MSc.CS, Sr. Network Kernel Developer at Red Hat Author of http://www.iptv-analyzer.org LinkedIn: http://www.linkedin.com/in/brouer
net/netfilter/nf_conntrack_core.c +15 -13
@@ -192,34 +192,37 @@ clean_from_lists(struct nf_conn *ct)nf_ct_remove_expectations (ct); } +/* must be called with local_bh_disable */ static void nf_ct_add_to_dying_list (struct nf_conn *ct) {struct ct_pcpu *pcpu;/* add this conntrack to the (per cpu) dying list */ - ct->cpu =raw_smp_processor_id (); + ct->cpu =smp_processor_id (); pcpu =per_cpu_ptr (nf_ct_net (ct)->ct.pcpu_lists, ct->cpu); -spin_lock_bh (&pcpu->lock); +spin_lock (&pcpu->lock);hlist_nulls_add_head (&ct->tuplehash[IP_CT_DIR_ORIGINAL ].hnnode, &pcpu->dying); -spin_unlock_bh (&pcpu->lock); +spin_unlock (&pcpu->lock); } +/* must be called with local_bh_disable */ static void nf_ct_add_to_unconfirmed_list (struct nf_conn *ct) {struct ct_pcpu *pcpu;/* add this conntrack to the (per cpu) unconfirmed list */ - ct->cpu =raw_smp_processor_id (); + ct->cpu =smp_processor_id (); pcpu =per_cpu_ptr (nf_ct_net (ct)->ct.pcpu_lists, ct->cpu); -spin_lock_bh (&pcpu->lock); +spin_lock (&pcpu->lock);hlist_nulls_add_head (&ct->tuplehash[IP_CT_DIR_ORIGINAL ].hnnode, &pcpu->unconfirmed); -spin_unlock_bh (&pcpu->lock); +spin_unlock (&pcpu->lock); } +/* must be called with local_bh_disable */ static void nf_ct_del_from_dying_or_unconfirmed_list (struct nf_conn *ct) {struct ct_pcpu *pcpu;
@@ -227,10 +230,10 @@ static void nf_ct_del_from_dying_or_unconfirmed_list(struct nf_conn *ct)/* We overload first tuple to link into unconfirmed or dying list.*/ pcpu =per_cpu_ptr (nf_ct_net (ct)->ct.pcpu_lists, ct->cpu); -spin_lock_bh (&pcpu->lock); +spin_lock (&pcpu->lock);BUG_ON (hlist_nulls_unhashed (&ct->tuplehash[IP_CT_DIR_ORIGINAL ].hnnode));hlist_nulls_del_rcu (&ct->tuplehash[IP_CT_DIR_ORIGINAL ].hnnode); -spin_unlock_bh (&pcpu->lock); +spin_unlock (&pcpu->lock); } static void
@@ -511,10 +514,11 @@ void nf_conntrack_tmpl_insert(struct net *net, struct nf_conn *tmpl)nf_conntrack_get (&tmpl->ct_general);/* add this conntrack to the (per cpu) tmpl list */ - tmpl->cpu =raw_smp_processor_id (); +local_bh_disable (); + tmpl->cpu =smp_processor_id (); pcpu =per_cpu_ptr (nf_ct_net (tmpl)->ct.pcpu_lists, tmpl->cpu); -spin_lock_bh (&pcpu->lock); +spin_lock (&pcpu->lock);/* Overload tuple linked list to put us in template list. */ hlist_nulls_add_head_rcu (&tmpl->tuplehash[IP_CT_DIR_ORIGINAL ].hnnode, &pcpu->tmpl);
@@ -921,11 +925,9 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,/* Now it is inserted into the unconfirmed list, bump refcount */ nf_conntrack_get (&ct->ct_general); - -spin_unlock_bh (&nf_conntrack_lock); -nf_ct_add_to_unconfirmed_list (ct); +spin_unlock_bh (&nf_conntrack_lock);if (exp) {if (exp->expectfn)
Re: [nf-next PATCH 3/5] netfilter: avoid race with exp->master ct
In reply to Florian WestphalQuoted 75 lines · On Thu, 27 Feb 2014 22:34:52 +0100 Florian Westphal <[email protected]> wrote:
Quoted 61 lines · Jesper Dangaard Brouer
Preparation for disconnecting the
nf_conntrack_lockfrom the expectations code. Once thenf_conntrack_lockis lifted, a race condition is exposed.The expectations master conntrack
exp->master, can race with delete operations, as the refcnt increment happens too late ininit_conntrack(). Race is against other CPUs invoking ->destroy() (destroy_conntrack()), ornf_ct_delete()(via timeout orearly_drop()).Avoid this race in
nf_ct_find_expectation()by usingatomic_inc_not_zero(), and checking ifnf_ct_is_dying()(path vianf_ct_delete()).Signed-off-by Jesper Dangaard Brouer <[email protected]>Signed-off-by Florian Westphal <[email protected]>---
net/netfilter/nf_conntrack_core.c | 2 +- net/netfilter/nf_conntrack_expect.c | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-)net/netfilter/nf_conntrack_core.c +1
-1@@ -898,6 +898,7 @@ init_conntrack(struct net *net, struct nf_conn *tmpl, ct, exp);/* Welcome, Mr. Bond. We've been expecting you... */ __set_bit (IPS_EXPECTED_BIT , &ct->status); +/* exp->master safe, refcnt bumped in nf_ct_find_expectation */ ct->master = exp->master;if (exp->helper) { help = nf_ct_helper_ext_add(ct, exp->helper,@@ -912,7 +913,6 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,#ifdef CONFIG_NF_CONNTRACK_SECMARK ct->secmark = exp->master->secmark;#endif -nf_conntrack_get (&ct->master->ct_general);NF_CT_STAT_INC (net, expect_new); }else {__nf_ct_try_assign_helper (ct, tmpl,GFP_ATOMIC );net/netfilter/nf_conntrack_expect.c +12
-0@@ -147,13 +147,27 @@ nf_ct_find_expectation(struct net *net, u16 zone,if (!exp)return NULL ; +/* Avoid race with other CPUs, that for exp->master ct, is +* about to invoke ->destroy(), or nf_ct_delete() via timeout +* or early_drop(). +* +* The atomic_inc_not_zero() check tells: If that fails, we +* know that the ct is being destroyed. If it succeeds, we +* can be sure the ct cannot disappear underneath. +*/ +if (unlikely (nf_ct_is_dying (exp->master) || + !atomic_inc_not_zero (&exp->master->ct_general.use))) +return NULL ; +net/netfilter/nf_conntrack_expect.c nf_ct_find_expectation(struct net *net, u16 zone, · line 170
Quoted 10 lines
net/netfilter/nf_conntrack_expect.c +3
-1/* If master is not in hash table yet (ie. packet hasn't left this machine yet), how can other end know about expected? Hence these are not the droids you are looking for (if master ct never got confirmed, we'd hold a reference to it and weird things would happen to future packets). */ -if (!nf_ct_is_confirmed (exp->master)) +if (!nf_ct_is_confirmed (exp->master)) { +atomic_dec (&exp->master->ct_general.use);return NULL ; + }Not sure if this is safe.
Quoted 4 lines
What about:
CPU0:atomic_inc_not_zero()
CPU1: callsnf_conntrack_put()
CPU0:atomic_dec()-> zero refcnt without invocation of ->destroy
Okay, so, you are saying CPU0 should use nf_ct_put() or nf_conntrack_put().
Quoted 4 lines
[ Cannot happen now because of
nf_conntrack_lock]I'd suggest to test
nf_ct_is_confirmed()first, it avoids the need to undo theatomic_inc_not_zero.
Okay, guess that should be okay.
Quoted 13 lines
You also need to deal with the "timer-deletion-fails" a bit later in the same function:
if (exp->flags & NF_CT_EXPECT_PERMANENT) { atomic_inc(&exp->use); return exp; } else if (del_timer(&exp->timeout)) { nf_ct_unlink_expect(exp); return exp; } // Problem: exp->master ref was bumped nf_ct_put(exp->master); // missing return NULL;
True, and yes, use of use nf_ct_put() or nf_conntrack_put() would be necessary here instead of manual refcnt dec.
Thanks for your review, I will fix it up...
Signature · 5 lines
Best regards, Jesper Dangaard Brouer MSc.CS, Sr. Network Kernel Developer at Red Hat Author of http://www.iptv-analyzer.org LinkedIn: http://www.linkedin.com/in/brouer
v2 cover netfilter: conntrack: optimization, remove central spinlock
In reply to Jesper Dangaard BrouerThis patchset change the conntrack locking and provides a huge performance improvements.
This patchset is based upon Eric Dumazet's proposed patch:
http://thread.gmane.org/gmane.linux.network/268758/focus=47306
I have in agreement with Eric Dumazet, taken over this patch (and turned it into a entire patchset).
Primary focus is to remove the central spinlock nf_conntrack_lock. This requires several steps to be acheived.
Patch01: Trivial cleanups
Patch02: Moves the "special" dying/unconfirmed/template lists to use a
per cpu spinlock.
Patch03: Is preparing for patch04, as it address a race
condition. Doing this a seperate patch for reviewers sake.
Patch04: Seperates expect locking from nf_conntrack_lock. The expect
list is small (default max 256), this it just get a single lock.
Patch05: Finally can remove nf_conntrack_lock, and instead uses an
array of hashed spinlocks to protect insertions/deletions of conntracks into the hash table. While still allowing dynamic resizing of the hash table.
Testing
-------
For expectations I've mostly tested the FTP nf_conntrack_ftp helper module, by commands:
for x in seq 1 300; do \
echo $x; \
echo -e "USER anonymous\nPASS nothing\nPASV" | nc 192.168.42.129 21; \
done
wget ftp://192.168.42.129/pub/delete.me.4k -O /dev/null
For overload/DoS testing, I've primarily done, SYN-flood attack testing. Results on a 24-core E5-2695v2(ES) with 10Gbit/s ixgbe (with tool trafgen)
Base kernel : New 810.405 conntrack/sec
Fixed kernel: New 2.233.876 conntrack/sec
Notice other floods attack (SYN+ACK or ACK) can easily be deflected using:
# iptables -A INPUT -m state --state INVALID -j DROP # sysctl -w net/netfilter/nf_conntrack_tcp_loose=0
E.g. this machine can reflect 6.481.463 "invalid" conntrack/sec (from an ACK-flood).
Perf data:
----------
The nf_conntrack_lock is suffers from huge contention on current generation servers (8 or more core/threads). Data from under SYN-flooding (without a listen socket)
Perf locking congestion is very "visible" on a base kernel:
- 72.56% ksoftirqd/6 [kernel.kallsyms] [k] _raw_spin_lock_bh
- _raw_spin_lock_bh
+ 25.33% init_conntrack
+ 24.86% nf_ct_delete_from_lists
+ 24.62% __nf_conntrack_confirm
+ 24.38% destroy_conntrack
+ 0.70% tcp_packet
+ 2.21% ksoftirqd/6 [kernel.kallsyms] [k] fib_table_lookup + 1.15% ksoftirqd/6 [kernel.kallsyms] [k] __slab_free + 0.77% ksoftirqd/6 [kernel.kallsyms] [k] inet_getpeer + 0.70% ksoftirqd/6 [nf_conntrack] [k] nf_ct_delete + 0.55% ksoftirqd/6 [ip_tables] [k] ipt_do_table
Perf after the patchset (SYN-flood attack):
+ 9.62% ksoftirqd/6 [kernel.kallsyms] [k] fib_table_lookup
+ 3.78% ksoftirqd/6 [kernel.kallsyms] [k] __slab_free
+ 2.71% ksoftirqd/6 [kernel.kallsyms] [k] inet_getpeer
+ 2.55% ksoftirqd/6 [kernel.kallsyms] [k] check_leaf
+ 2.38% ksoftirqd/6 [ip_tables] [k] ipt_do_table
+ 2.06% ksoftirqd/6 [kernel.kallsyms] [k] __slab_alloc
+ 1.94% ksoftirqd/6 [nf_conntrack] [k] __nf_conntrack_alloc
- 1.94% ksoftirqd/6 [kernel.kallsyms] [k] _raw_spin_lock
- _raw_spin_lock
+ 90.32% nf_conntrack_double_lock
+ 3.61% get_partial_node
+ 1.81% nf_ct_delete_from_lists
+ 1.68% __nf_conntrack_confirm
+ 1.03% sch_direct_xmit
+ 0.52% scheduler_tick
+ 1.86% ksoftirqd/6 [kernel.kallsyms] [k] nf_iterate
+ 1.80% ksoftirqd/6 [nf_conntrack] [k] init_conntrack
+ 1.77% ksoftirqd/6 [kernel.kallsyms] [k] __neigh_event_send
- 1.70% ksoftirqd/6 [kernel.kallsyms] [k] _raw_spin_lock_bh
- _raw_spin_lock_bh
+ 32.55% nf_ct_del_from_dying_or_unconfirmed_list
+ 25.33% init_conntrack
+ 19.88% tcp_packet
+ 17.97% nf_ct_delete_from_lists
+ 1.62% nf_conntrack_in
+ 1.33% ixgbe_poll
+ 0.74% destroy_conntrack
+ 1.64% ksoftirqd/6 [nf_conntrack] [k] hash_conntrack_raw
+ 1.58% ksoftirqd/6 [kernel.kallsyms] [k] __netif_receive_skb_core
+ 1.51% ksoftirqd/6 [nf_conntrack] [k] __nf_conntrack_find_get
+ 1.48% ksoftirqd/6 [kernel.kallsyms] [k] __cmpxchg_double_slab
+ 1.46% ksoftirqd/6 [nf_conntrack] [k] nf_conntrack_in
+ 1.45% ksoftirqd/6 [kernel.kallsyms] [k] __local_bh_enable_ip---
Jesper Dangaard Brouer (5):
netfilter: conntrack: remove central spinlock nf_conntrack_lock
netfilter: conntrack: seperate expect locking from nf_conntrack_lock
netfilter: avoid race with exp->master ct
netfilter: conntrack: spinlock per cpu to protect special lists.
netfilter: trivial code cleanup and doc changes
include/net/netfilter/nf_conntrack.h | 11 + include/net/netfilter/nf_conntrack_core.h | 9 + include/net/netns/conntrack.h | 13 + net/netfilter/nf_conntrack_core.c | 432 ++++++++++++++++++++--------- net/netfilter/nf_conntrack_expect.c | 34 ++ net/netfilter/nf_conntrack_h323_main.c | 4 net/netfilter/nf_conntrack_helper.c | 37 ++ net/netfilter/nf_conntrack_netlink.c | 128 +++++---- net/netfilter/nf_conntrack_sip.c | 8 - 9 files changed, 459 insertions(+), 217 deletions(-)
PATCH v2 1/5 netfilter: trivial code cleanup and doc changes
Changes while reading through the netfilter code.
Added hint about how conntrack nf_conn refcnt is accessed. And renamed repl_hash to reply_hash for readability
---
include/net/netfilter/nf_conntrack.h | 8 +++++++- net/netfilter/nf_conntrack_core.c | 20 ++++++++++---------- 2 files changed, 17 insertions(+), 11 deletions(-)
include/net/netfilter/nf_conntrack.h +7 -1
@@ -73,7 +73,13 @@ struct nf_conn_help {struct nf_conn {/* Usage count in here is 1 for hash table/destruct timer, 1 per skb, -plus 1 for any connection(s) we are `master' for */ +* plus 1 for any connection(s) we are `master' for +* +* Hint, SKB address this struct and refcnt via skb->nfct and +* helpers nf_conntrack_get() and nf_conntrack_put(). +* Helper nf_ct_put() equals nf_conntrack_put() by dec refcnt, +* beware nf_ct_get() is different and don't inc refcnt. +*/ struct nf_conntrack ct_general;spinlock_t lock;
net/netfilter/nf_conntrack_core.c +10 -10
@@ -408,21 +408,21 @@ EXPORT_SYMBOL_GPL(nf_conntrack_find_get);static void __nf_conntrack_hash_insert (struct nf_conn *ct,unsigned int hash, -unsigned int repl_hash) +unsigned int reply_hash) {struct net *net =nf_ct_net (ct);hlist_nulls_add_head_rcu (&ct->tuplehash[IP_CT_DIR_ORIGINAL ].hnnode, &net->ct.hash[hash]);hlist_nulls_add_head_rcu (&ct->tuplehash[IP_CT_DIR_REPLY ].hnnode, - &net->ct.hash[repl_hash]); + &net->ct.hash[reply_hash]); }int nf_conntrack_hash_check_insert (struct nf_conn *ct) {struct net *net =nf_ct_net (ct); -unsigned int hash, repl_hash; +unsigned int hash, reply_hash;struct nf_conntrack_tuple_hash *h;struct hlist_nulls_node *n;u16 zone;
@@ -430,7 +430,7 @@ nf_conntrack_hash_check_insert(struct nf_conn *ct) zone =nf_ct_zone (ct); hash =hash_conntrack (net, zone, &ct->tuplehash[IP_CT_DIR_ORIGINAL ].tuple); - repl_hash =hash_conntrack (net, zone, + reply_hash =hash_conntrack (net, zone, &ct->tuplehash[IP_CT_DIR_REPLY ].tuple);spin_lock_bh (&nf_conntrack_lock);
@@ -441,7 +441,7 @@ nf_conntrack_hash_check_insert(struct nf_conn *ct) &h->tuple) && zone == nf_ct_zone(nf_ct_tuplehash_to_ctrack(h))) goto out; -hlist_nulls_for_each_entry (h, n, &net->ct.hash[repl_hash], hnnode) +hlist_nulls_for_each_entry (h, n, &net->ct.hash[reply_hash], hnnode)if (nf_ct_tuple_equal (&ct->tuplehash[IP_CT_DIR_REPLY ].tuple, &h->tuple) && zone ==nf_ct_zone (nf_ct_tuplehash_to_ctrack (h)))
@@ -451,7 +451,7 @@ nf_conntrack_hash_check_insert(struct nf_conn *ct)smp_wmb ();/* The caller holds a reference to this object */ atomic_set (&ct->ct_general.use,2 ); -__nf_conntrack_hash_insert (ct, hash, repl_hash); +__nf_conntrack_hash_insert (ct, hash, reply_hash);NF_CT_STAT_INC (net, insert);spin_unlock_bh (&nf_conntrack_lock);
@@ -483,7 +483,7 @@ EXPORT_SYMBOL_GPL(nf_conntrack_tmpl_insert);int __nf_conntrack_confirm (struct sk_buff *skb) { -unsigned int hash, repl_hash; +unsigned int hash, reply_hash;struct nf_conntrack_tuple_hash *h;struct nf_conn *ct;struct nf_conn_help *help;
@@ -507,7 +507,7 @@ __nf_conntrack_confirm(struct sk_buff *skb)/* reuse the hash saved before */ hash = *(unsigned long *)&ct->tuplehash[IP_CT_DIR_REPLY ].hnnode.pprev; hash =hash_bucket (hash, net); - repl_hash =hash_conntrack (net, zone, + reply_hash =hash_conntrack (net, zone, &ct->tuplehash[IP_CT_DIR_REPLY ].tuple);/* We're not in hash table, and we refuse to set up related
@@ -540,7 +540,7 @@ __nf_conntrack_confirm(struct sk_buff *skb) &h->tuple) && zone == nf_ct_zone(nf_ct_tuplehash_to_ctrack(h))) goto out; -hlist_nulls_for_each_entry (h, n, &net->ct.hash[repl_hash], hnnode) +hlist_nulls_for_each_entry (h, n, &net->ct.hash[reply_hash], hnnode)if (nf_ct_tuple_equal (&ct->tuplehash[IP_CT_DIR_REPLY ].tuple, &h->tuple) && zone ==nf_ct_zone (nf_ct_tuplehash_to_ctrack (h)))
@@ -570,7 +570,7 @@ __nf_conntrack_confirm(struct sk_buff *skb)* guarantee that no other CPU can find the conntrack before the above * stores are visible. */ -__nf_conntrack_hash_insert (ct, hash, repl_hash); +__nf_conntrack_hash_insert (ct, hash, reply_hash);NF_CT_STAT_INC (net, insert);spin_unlock_bh (&nf_conntrack_lock);
PATCH v2 2/5 netfilter: conntrack: spinlock per cpu to protect special lists.
In reply to Jesper Dangaard BrouerOne spinlock per cpu to protect dying/unconfirmed/template special lists. (These lists are now per cpu, a bit like the untracked ct) Add a @cpu field to nf_conn, to make sure we hold the appropriate spinlock at removal time.
---
V2: Address DaveM's concerns
* Use smp_processor_id() instead of raw_smp_processor_id()
* Make use of local_bh_disable/enable to
(1) make smp_process_id() safe,
(2) and save some spin_lock_bh() calls
include/net/netfilter/nf_conntrack.h | 3 - include/net/netns/conntrack.h | 11 ++- net/netfilter/nf_conntrack_core.c | 141 +++++++++++++++++++++++++--------- net/netfilter/nf_conntrack_helper.c | 11 ++- net/netfilter/nf_conntrack_netlink.c | 81 +++++++++++--------- 5 files changed, 168 insertions(+), 79 deletions(-)
include/net/netfilter/nf_conntrack.h +2 -1
@@ -82,7 +82,8 @@ struct nf_conn {*/ struct nf_conntrack ct_general; -spinlock_t lock; +spinlock_t lock; +u16 cpu;/* XXX should I move this to the tail ? - Y.K */ /* These are my tuples; original and reply */
include/net/netns/conntrack.h +8 -3
@@ -62,6 +62,13 @@ struct nf_ip_net {#endif }; +struct ct_pcpu { +spinlock_t lock; +struct hlist_nulls_head unconfirmed; +struct hlist_nulls_head dying; +struct hlist_nulls_head tmpl; +}; +struct netns_ct {atomic_t count;unsigned int expect_count;
@@ -86,9 +93,7 @@ struct netns_ct {struct kmem_cache *nf_conntrack_cachep;struct hlist_nulls_head *hash;struct hlist_head *expect_hash; -struct hlist_nulls_head unconfirmed; -struct hlist_nulls_head dying; -struct hlist_nulls_head tmpl; +struct ct_pcpu __percpu *pcpu_lists;struct ip_conntrack_stat __percpu *stat;struct nf_ct_event_notifier __rcu *nf_conntrack_event_cb;struct nf_exp_event_notifier __rcu *nf_expect_event_cb;
net/netfilter/nf_conntrack_core.c +103 -38
241 lines not on this page · raw message
net/netfilter/nf_conntrack_helper.c +9 -2
@@ -396,6 +396,7 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me,const struct hlist_node *next;const struct hlist_nulls_node *nn;unsigned int i; +int cpu;/* Get rid of expectations */ for (i =0 ; i < nf_ct_expect_hsize; i++) {
@@ -414,8 +415,14 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me, }/* Get rid of expecteds, set helpers to NULL. */ -hlist_nulls_for_each_entry (h, nn, &net->ct.unconfirmed, hnnode) -unhelp (h, me); +for_each_possible_cpu (cpu) { +struct ct_pcpu *pcpu =per_cpu_ptr (net->ct.pcpu_lists, cpu); + +spin_lock_bh (&pcpu->lock); +hlist_nulls_for_each_entry (h, nn, &pcpu->unconfirmed, hnnode) +unhelp (h, me); +spin_unlock_bh (&pcpu->lock); + }for (i =0 ; i < net->ct.htable_size; i++) {hlist_nulls_for_each_entry (h, nn, &net->ct.hash[i], hnnode)unhelp (h, me);
net/netfilter/nf_conntrack_netlink.c +46 -35
114 lines not on this page · raw message
PATCH v2 3/5 netfilter: avoid race with exp->master ct
In reply to Jesper Dangaard BrouerPreparation for disconnecting the nf_conntrack_lock from the expectations code. Once the nf_conntrack_lock is lifted, a race condition is exposed.
The expectations master conntrack exp->master, can race with delete operations, as the refcnt increment happens too late in init_conntrack(). Race is against other CPUs invoking ->destroy() (destroy_conntrack()), or nf_ct_delete() (via timeout or early_drop()).
Avoid this race in nf_ct_find_expectation() by using atomic_inc_not_zero(), and checking if nf_ct_is_dying() (path via nf_ct_delete()).
---
V2: Address Florian Westphal concerns
* add check after !nf_ct_is_confirmed()
* handle exit case if del_timer(&exp->timeout) fails
net/netfilter/nf_conntrack_core.c | 2 +- net/netfilter/nf_conntrack_expect.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletions(-)
net/netfilter/nf_conntrack_core.c +1 -1
@@ -902,6 +902,7 @@ init_conntrack(struct net *net, struct nf_conn *tmpl, ct, exp);/* Welcome, Mr. Bond. We've been expecting you... */ __set_bit (IPS_EXPECTED_BIT , &ct->status); +/* exp->master safe, refcnt bumped in nf_ct_find_expectation */ ct->master = exp->master;if (exp->helper) { help = nf_ct_helper_ext_add(ct, exp->helper,
@@ -916,7 +917,6 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,#ifdef CONFIG_NF_CONNTRACK_SECMARK ct->secmark = exp->master->secmark;#endif -nf_conntrack_get (&ct->master->ct_general);NF_CT_STAT_INC (net, expect_new); }else {__nf_ct_try_assign_helper (ct, tmpl,GFP_ATOMIC );
net/netfilter/nf_conntrack_expect.c +14 -0
@@ -155,6 +155,18 @@ nf_ct_find_expectation(struct net *net, u16 zone,if (!nf_ct_is_confirmed (exp->master))return NULL ; +/* Avoid race with other CPUs, that for exp->master ct, is +* about to invoke ->destroy(), or nf_ct_delete() via timeout +* or early_drop(). +* +* The atomic_inc_not_zero() check tells: If that fails, we +* know that the ct is being destroyed. If it succeeds, we +* can be sure the ct cannot disappear underneath. +*/ +if (unlikely (nf_ct_is_dying (exp->master) || + !atomic_inc_not_zero (&exp->master->ct_general.use))) +return NULL ; +if (exp->flags &NF_CT_EXPECT_PERMANENT ) {atomic_inc (&exp->use);return exp;
@@ -162,6 +174,8 @@ nf_ct_find_expectation(struct net *net, u16 zone,nf_ct_unlink_expect (exp);return exp; } +/* Undo exp->master refcnt increase, if del_timer() failed */ +nf_ct_put (exp->master);return NULL ; }
PATCH v2 4/5 netfilter: conntrack: seperate expect locking from nf_conntrack_lock
In reply to Jesper Dangaard BrouerNetfilter expectations are protected with the same lock as conntrack entries (nf_conntrack_lock). This patch split out expectations locking to use it's own lock (nf_conntrack_expect_lock).
---
include/net/netfilter/nf_conntrack_core.h | 2 + net/netfilter/nf_conntrack_core.c | 60 ++++++++++++++++------------- net/netfilter/nf_conntrack_expect.c | 20 +++++----- net/netfilter/nf_conntrack_h323_main.c | 4 +- net/netfilter/nf_conntrack_helper.c | 14 ++++--- net/netfilter/nf_conntrack_netlink.c | 32 ++++++++------- net/netfilter/nf_conntrack_sip.c | 8 ++-- 7 files changed, 76 insertions(+), 64 deletions(-)
include/net/netfilter/nf_conntrack_core.h +2 -0
@@ -79,4 +79,6 @@ print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,extern spinlock_t nf_conntrack_lock ; +extern spinlock_t nf_conntrack_expect_lock; +#endif /* _NF_CONNTRACK_CORE_H */
net/netfilter/nf_conntrack_core.c +33 -27
109 lines not on this page · raw message
net/netfilter/nf_conntrack_expect.c +11 -9
@@ -66,9 +66,9 @@ static void nf_ct_expectation_timed_out(unsigned long ul_expect) {struct nf_conntrack_expect *exp = (void *)ul_expect; -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);nf_ct_unlink_expect (exp); -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);nf_ct_expect_put (exp); }
@@ -191,12 +191,14 @@ void nf_ct_remove_expectations(struct nf_conn *ct)if (!help)return ; +spin_lock_bh (&nf_conntrack_expect_lock);hlist_for_each_entry_safe (exp, next, &help->expectations, lnode) {if (del_timer (&exp->timeout)) {nf_ct_unlink_expect (exp);nf_ct_expect_put (exp); } } +spin_unlock_bh (&nf_conntrack_expect_lock); }EXPORT_SYMBOL_GPL (nf_ct_remove_expectations);
@@ -231,12 +233,12 @@ static inline int expect_matches(const struct nf_conntrack_expect *a,/* Generally a bad idea to call this: could have matched already. */ void nf_ct_unexpect_related (struct nf_conntrack_expect *exp) { -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);if (del_timer (&exp->timeout)) {nf_ct_unlink_expect (exp);nf_ct_expect_put (exp); } -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock); }EXPORT_SYMBOL_GPL (nf_ct_unexpect_related);
@@ -349,7 +351,7 @@ static int nf_ct_expect_insert(struct nf_conntrack_expect *exp)setup_timer (&exp->timeout, nf_ct_expectation_timed_out, (unsigned long )exp); helper =rcu_dereference_protected (master_help->helper, -lockdep_is_held (&nf_conntrack_lock)); +lockdep_is_held (&nf_conntrack_expect_lock));if (helper) { exp->timeout.expires = jiffies + helper->expect_policy[exp->class].timeout *HZ ;
@@ -409,7 +411,7 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect) }/* Will be over limit? */ helper =rcu_dereference_protected (master_help->helper, -lockdep_is_held (&nf_conntrack_lock)); +lockdep_is_held (&nf_conntrack_expect_lock));if (helper) { p = &helper->expect_policy[expect->class]; if (p->max_expected &&
@@ -436,7 +438,7 @@ int nf_ct_expect_related_report(struct nf_conntrack_expect *expect, {int ret; -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock); ret =__nf_ct_expect_check (expect);if (ret <=0 ) goto out;
@@ -444,11 +446,11 @@ int nf_ct_expect_related_report(struct nf_conntrack_expect *expect, ret =nf_ct_expect_insert (expect);if (ret <0 ) goto out; -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);nf_ct_expect_event_report (IPEXP_NEW , expect, portid, report);return ret; out: -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);return ret; }EXPORT_SYMBOL_GPL (nf_ct_expect_related_report);
net/netfilter/nf_conntrack_h323_main.c +2 -2
@@ -1476,7 +1476,7 @@ static int process_rcf(struct sk_buff *skb, struct nf_conn *ct,nf_ct_refresh (ct, skb, info->timeout *HZ );/* Set expect timeout */ -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock); exp =find_expect (ct, &ct->tuplehash[dir].tuple.dst.u3, info->sig_port[!dir]);if (exp) {
@@ -1486,7 +1486,7 @@ static int process_rcf(struct sk_buff *skb, struct nf_conn *ct,nf_ct_dump_tuple (&exp->tuple);set_expect_timeout (exp, info->timeout); } -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock); }return 0 ;
net/netfilter/nf_conntrack_helper.c +8 -6
@@ -258,7 +258,7 @@ static inline int unhelp(struct nf_conntrack_tuple_hash *i,if (help &&rcu_dereference_protected ( help->helper, -lockdep_is_held (&nf_conntrack_lock) +lockdep_is_held (&nf_conntrack_expect_lock) ) == me) {nf_conntrack_event (IPCT_HELPER , ct);RCU_INIT_POINTER (help->helper,NULL );
@@ -284,17 +284,17 @@ static LIST_HEAD(nf_ct_helper_expectfn_list);void nf_ct_helper_expectfn_register (struct nf_ct_helper_expectfn *n) { -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);list_add_rcu (&n->head, &nf_ct_helper_expectfn_list); -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock); }EXPORT_SYMBOL_GPL (nf_ct_helper_expectfn_register);void nf_ct_helper_expectfn_unregister (struct nf_ct_helper_expectfn *n) { -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);list_del_rcu (&n->head); -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock); }EXPORT_SYMBOL_GPL (nf_ct_helper_expectfn_unregister);
@@ -399,13 +399,14 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me,int cpu;/* Get rid of expectations */ +spin_lock_bh (&nf_conntrack_expect_lock);for (i =0 ; i < nf_ct_expect_hsize; i++) {hlist_for_each_entry_safe (exp, next, &net->ct.expect_hash[i], hnode) {struct nf_conn_help *help =nfct_help (exp->master);if ((rcu_dereference_protected ( help->helper, -lockdep_is_held (&nf_conntrack_lock) +lockdep_is_held (&nf_conntrack_expect_lock) ) == me || exp->helper == me) &&del_timer (&exp->timeout)) {nf_ct_unlink_expect (exp);
@@ -413,6 +414,7 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me, } } } +spin_unlock_bh (&nf_conntrack_expect_lock);/* Get rid of expecteds, set helpers to NULL. */ for_each_possible_cpu (cpu) {
net/netfilter/nf_conntrack_netlink.c +16 -16
@@ -1377,14 +1377,14 @@ ctnetlink_change_helper(struct nf_conn *ct, const struct nlattr * const cda[]) nf_ct_protonum(ct));if (helper ==NULL ) {#ifdef CONFIG_MODULES -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);if (request_module ("nfct-helper-%s" , helpname) <0 ) { -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);return -EOPNOTSUPP ; } -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock); helper =__nf_conntrack_helper_find (helpname,nf_ct_l3num (ct),nf_ct_protonum (ct));if (helper)
@@ -1822,9 +1822,9 @@ ctnetlink_new_conntrack(struct sock *ctnl, struct sk_buff *skb, err = -EEXIST ; ct =nf_ct_tuplehash_to_ctrack (h);if (!(nlh->nlmsg_flags &NLM_F_EXCL )) { -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock); err =ctnetlink_change_conntrack (ct, cda); -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);if (err ==0 ) { nf_conntrack_eventmask_report((1 << IPCT_REPLY) | (1 << IPCT_ASSURED) |
@@ -2153,9 +2153,9 @@ ctnetlink_nfqueue_parse(const struct nlattr *attr, struct nf_conn *ct)if (ret <0 )return ret; -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock); ret =ctnetlink_nfqueue_parse_ct ((const struct nlattr **)cda, ct); -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);return ret; }
@@ -2710,13 +2710,13 @@ ctnetlink_del_expect(struct sock *ctnl, struct sk_buff *skb, }/* after list removal, usage count == 1 */ -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);if (del_timer (&exp->timeout)) {nf_ct_unlink_expect_report (exp,NETLINK_CB (skb).portid,nlmsg_report (nlh));nf_ct_expect_put (exp); } -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);/* have to put what we 'get' above. * after this line usage count == 0 */ nf_ct_expect_put (exp);
@@ -2725,7 +2725,7 @@ ctnetlink_del_expect(struct sock *ctnl, struct sk_buff *skb,struct nf_conn_help *m_help;/* delete all expectations for this helper */ -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);for (i =0 ; i < nf_ct_expect_hsize; i++) { hlist_for_each_entry_safe(exp, next, &net->ct.expect_hash[i],
@@ -2740,10 +2740,10 @@ ctnetlink_del_expect(struct sock *ctnl, struct sk_buff *skb, } } } -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock); }else {/* This basically means we have to flush everything*/ -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);for (i =0 ; i < nf_ct_expect_hsize; i++) { hlist_for_each_entry_safe(exp, next, &net->ct.expect_hash[i],
@@ -2756,7 +2756,7 @@ ctnetlink_del_expect(struct sock *ctnl, struct sk_buff *skb, } } } -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock); }return 0 ;
@@ -2982,11 +2982,11 @@ ctnetlink_new_expect(struct sock *ctnl, struct sk_buff *skb,if (err <0 )return err; -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock); exp =__nf_ct_expect_find (net, zone, &tuple);if (!exp) { -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock); err = -ENOENT ;if (nlh->nlmsg_flags &NLM_F_CREATE ) { err = ctnetlink_create_expect(net, zone, cda,
@@ -3000,7 +3000,7 @@ ctnetlink_new_expect(struct sock *ctnl, struct sk_buff *skb, err = -EEXIST ;if (!(nlh->nlmsg_flags &NLM_F_EXCL )) err =ctnetlink_change_expect (exp, cda); -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);return err; }
net/netfilter/nf_conntrack_sip.c +4 -4
@@ -800,7 +800,7 @@ static int refresh_signalling_expectation(struct nf_conn *ct,struct hlist_node *next;int found =0 ; -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);hlist_for_each_entry_safe (exp, next, &help->expectations, lnode) { if (exp->class != SIP_EXPECT_SIGNALLING || !nf_inet_addr_cmp(&exp->tuple.dst.u3, addr) ||
@@ -815,7 +815,7 @@ static int refresh_signalling_expectation(struct nf_conn *ct, found =1 ;break ; } -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock);return found; }
@@ -825,7 +825,7 @@ static void flush_expectations(struct nf_conn *ct, bool media)struct nf_conntrack_expect *exp;struct hlist_node *next; -spin_lock_bh (&nf_conntrack_lock); +spin_lock_bh (&nf_conntrack_expect_lock);hlist_for_each_entry_safe (exp, next, &help->expectations, lnode) {if ((exp->class !=SIP_EXPECT_SIGNALLING ) ^ media)continue ;
@@ -836,7 +836,7 @@ static void flush_expectations(struct nf_conn *ct, bool media)if (!media)break ; } -spin_unlock_bh (&nf_conntrack_lock); +spin_unlock_bh (&nf_conntrack_expect_lock); } static int set_expected_rtp_rtcp(struct sk_buff *skb, unsigned int protoff,
PATCH v2 5/5 netfilter: conntrack: remove central spinlock nf_conntrack_lock
In reply to Jesper Dangaard Brouernf_conntrack_lock is a monolithic lock and suffers from huge contention on current generation servers (8 or more core/threads).
Perf locking congestion is clear on base kernel:
- 72.56% ksoftirqd/6 [kernel.kallsyms] [k] _raw_spin_lock_bh
- _raw_spin_lock_bh
+ 25.33% init_conntrack
+ 24.86% nf_ct_delete_from_lists
+ 24.62% __nf_conntrack_confirm
+ 24.38% destroy_conntrack
+ 0.70% tcp_packet
+ 2.21% ksoftirqd/6 [kernel.kallsyms] [k] fib_table_lookup + 1.15% ksoftirqd/6 [kernel.kallsyms] [k] __slab_free + 0.77% ksoftirqd/6 [kernel.kallsyms] [k] inet_getpeer + 0.70% ksoftirqd/6 [nf_conntrack] [k] nf_ct_delete + 0.55% ksoftirqd/6 [ip_tables] [k] ipt_do_table
This patch change conntrack locking and provides a huge performance improvement. SYN-flood attack tested on a 24-core E5-2695v2(ES) with 10Gbit/s ixgbe (with tool trafgen):
Base kernel: 810.405 new conntrack/sec
After patch: 2.233.876 new conntrack/sec
Notice other floods attack (SYN+ACK or ACK) can easily be deflected using:
# iptables -A INPUT -m state --state INVALID -j DROP # sysctl -w net/netfilter/nf_conntrack_tcp_loose=0
Use an array of hashed spinlocks to protect insertions/deletions of conntracks into the hash table. 1024 spinlocks seem to give good results, at minimal cost (4KB memory). Due to lockdep max depth, 1024 becomes 8 if CONFIG_LOCKDEP=y
The hash resize is a bit tricky, because we need to take all locks in the array. A seqcount_t is used to synchronize the hash table users with the resizing process.
---
include/net/netfilter/nf_conntrack_core.h | 7 + include/net/netns/conntrack.h | 2 net/netfilter/nf_conntrack_core.c | 219 +++++++++++++++++++++-------- net/netfilter/nf_conntrack_helper.c | 12 +- net/netfilter/nf_conntrack_netlink.c | 15 ++ 5 files changed, 188 insertions(+), 67 deletions(-)
include/net/netfilter/nf_conntrack_core.h +6 -1
@@ -77,7 +77,12 @@ print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,const struct nf_conntrack_l3proto *l3proto, const struct nf_conntrack_l4proto *proto); -extern spinlock_t nf_conntrack_lock ; +#ifdef CONFIG_LOCKDEP +# define CONNTRACK_LOCKS 8 +#else +# define CONNTRACK_LOCKS 1024 +#endif +extern spinlock_t nf_conntrack_locks[CONNTRACK_LOCKS ];extern spinlock_t nf_conntrack_expect_lock;
include/net/netns/conntrack.h +2 -0
@@ -5,6 +5,7 @@#include <linux/list_nulls.h> #include <linux/atomic.h> #include <linux/netfilter/nf_conntrack_tcp.h> +#include <linux/seqlock.h> struct ctl_table_header ;struct nf_conntrack_ecache ;
@@ -90,6 +91,7 @@ struct netns_ct {int sysctl_checksum;unsigned int htable_size; +seqcount_t generation;struct kmem_cache *nf_conntrack_cachep;struct hlist_nulls_head *hash;struct hlist_head *expect_hash;
net/netfilter/nf_conntrack_core.c +159 -60
372 lines not on this page · raw message
net/netfilter/nf_conntrack_helper.c +8 -4
@@ -425,10 +425,16 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me,unhelp (h, me);spin_unlock_bh (&pcpu->lock); } +local_bh_disable ();for (i =0 ; i < net->ct.htable_size; i++) { -hlist_nulls_for_each_entry (h, nn, &net->ct.hash[i], hnnode) -unhelp (h, me); +spin_lock (&nf_conntrack_locks[i %CONNTRACK_LOCKS ]); +if (i < net->ct.htable_size) { +hlist_nulls_for_each_entry (h, nn, &net->ct.hash[i], hnnode) +unhelp (h, me); + } +spin_unlock (&nf_conntrack_locks[i %CONNTRACK_LOCKS ]); } +local_bh_enable (); }void nf_conntrack_helper_unregister (struct nf_conntrack_helper *me)
@@ -446,10 +452,8 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)synchronize_rcu ();rtnl_lock (); -spin_lock_bh (&nf_conntrack_lock);for_each_net (net)__nf_conntrack_helper_unregister (me, net); -spin_unlock_bh (&nf_conntrack_lock);rtnl_unlock (); }EXPORT_SYMBOL_GPL (nf_conntrack_helper_unregister);
net/netfilter/nf_conntrack_netlink.c +13 -2
@@ -764,14 +764,23 @@ ctnetlink_dump_table(struct sk_buff *skb, struct netlink_callback *cb)struct nfgenmsg *nfmsg =nlmsg_data (cb->nlh);u_int8_t l3proto = nfmsg->nfgen_family;int res; +spinlock_t *lockp; +#ifdef CONFIG_NF_CONNTRACK_MARK const struct ctnetlink_dump_filter *filter = cb->data;#endif -spin_lock_bh (&nf_conntrack_lock); last = (struct nf_conn *)cb->args[1 ]; + +local_bh_disable ();for (; cb->args[0 ] < net->ct.htable_size; cb->args[0 ]++) { restart: + lockp = &nf_conntrack_locks[cb->args[0 ] %CONNTRACK_LOCKS ]; +spin_lock (lockp); +if (cb->args[0 ] >= net->ct.htable_size) { +spin_unlock (lockp); + goto out; + }hlist_nulls_for_each_entry (h, n, &net->ct.hash[cb->args[0 ]], hnnode) {if (NF_CT_DIRECTION (h) !=IP_CT_DIR_ORIGINAL )
@@ -803,16 +812,18 @@ restart:if (res <0 ) {nf_conntrack_get (&ct->ct_general); cb->args[1 ] = (unsigned long )ct; +spin_unlock (lockp); goto out; } } +spin_unlock (lockp);if (cb->args[1 ]) { cb->args[1 ] =0 ; goto restart; } } out: -spin_unlock_bh (&nf_conntrack_lock); +local_bh_enable ();if (last)nf_ct_put (last);
Florian Westphal
maintainerRe: [nf-next PATCH V2 4/5] netfilter: conntrack: seperate expect locking from nf_conntrack_lock
In reply to Jesper Dangaard BrouerQuoted 6 lines · Jesper Dangaard Brouer
Netfilter expectations are protected with the same lock as conntrack entries (
nf_conntrack_lock). This patch split out expectations locking to use it's own lock (nf_conntrack_expect_lock).
net/netfilter/nf_conntrack_helper.c static inline int unhelp(struct nf_conntrack_tuple_hash *i, · line 264
Quoted 8 lines
net/netfilter/nf_conntrack_helper.c +1
-1@@ -258,7 +258,7 @@ static inline int unhelp(struct nf_conntrack_tuple_hash *i,if (help &&rcu_dereference_protected ( help->helper, -lockdep_is_held (&nf_conntrack_lock) +lockdep_is_held (&nf_conntrack_expect_lock) ) == me) {nf_conntrack_event (IPCT_HELPER , ct);RCU_INIT_POINTER (help->helper,NULL );
Not sure if the lockdep_is_held is correct.
Quoted 17 lines
net/netfilter/nf_conntrack_helper.c +2
-1@@ -399,13 +399,14 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me,int cpu;/* Get rid of expectations */ +spin_lock_bh (&nf_conntrack_expect_lock);for (i =0 ; i < nf_ct_expect_hsize; i++) {hlist_for_each_entry_safe (exp, next, &net->ct.expect_hash[i], hnode) {struct nf_conn_help *help =nfct_help (exp->master);if ((rcu_dereference_protected ( help->helper, -lockdep_is_held (&nf_conntrack_lock) +lockdep_is_held (&nf_conntrack_expect_lock) ) == me || exp->helper == me) &&del_timer (&exp->timeout)) {nf_ct_unlink_expect (exp);
net/netfilter/nf_conntrack_helper.c static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me, · line 417
Quoted 4 lines
net/netfilter/nf_conntrack_helper.c +1
-0@@ -413,6 +414,7 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me, } } } +spin_unlock_bh (&nf_conntrack_expect_lock);
expect_lock is released here but
net/netfilter/nf_conntrack_helper.c static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me, · line 420
Quoted 2 lines
net/netfilter/nf_conntrack_helper.c +0
-0/* Get rid of expecteds, set helpers to NULL. */ for_each_possible_cpu (cpu) {
will invoke unhelp()
AFAIU unhelp() is safe in all cases even withoutnf_conntrack_expect_lock being held:
* in first loop we hold nf_conntrack_expect_lock
* in 2nd loop we are holding the list lock, i.e.
if the ct is in the list it cannot disappear underneath
* in last loop you'll hold the hashed lock for the particular hash list, so can't go away either.
So I think the lockdep annotation in uhelp is incorrect and not the patch itself.
David Miller
maintainerRe: [nf-next PATCH V2 0/5] netfilter: conntrack: optimization, remove central spinlock
In reply to Jesper Dangaard BrouerFrom: Jesper Dangaard Brouer <[email protected]>
Date: Fri, 28 Feb 2014 13:16:15 +0100
Quoted 2 lines
This patchset change the conntrack locking and provides a huge performance improvements.
I no longer have any concerns, looks good to me.
Re: [nf-next PATCH V2 4/5] netfilter: conntrack: seperate expect locking from nf_conntrack_lock
In reply to Florian WestphalQuoted 50 lines · On Fri, 28 Feb 2014 16:08:52 +0100 Florian Westphal <[email protected]> wrote:
Quoted 6 lines · Jesper Dangaard Brouer
Netfilter expectations are protected with the same lock as conntrack entries (
nf_conntrack_lock). This patch split out expectations locking to use it's own lock (nf_conntrack_expect_lock).net/netfilter/nf_conntrack_helper.c static inline int unhelp(struct nf_conntrack_tuple_hash *i, · line 264
Quoted 8 lines
net/netfilter/nf_conntrack_helper.c +1
-1@@ -258,7 +258,7 @@ static inline int unhelp(struct nf_conntrack_tuple_hash *i,if (help &&rcu_dereference_protected ( help->helper, -lockdep_is_held (&nf_conntrack_lock) +lockdep_is_held (&nf_conntrack_expect_lock) ) == me) {nf_conntrack_event (IPCT_HELPER , ct);RCU_INIT_POINTER (help->helper,NULL );Not sure if the
lockdep_is_heldis correct.Quoted 17 lines
net/netfilter/nf_conntrack_helper.c +2
-1@@ -399,13 +399,14 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me,int cpu;/* Get rid of expectations */ +spin_lock_bh (&nf_conntrack_expect_lock);for (i =0 ; i < nf_ct_expect_hsize; i++) {hlist_for_each_entry_safe (exp, next, &net->ct.expect_hash[i], hnode) {struct nf_conn_help *help =nfct_help (exp->master);if ((rcu_dereference_protected ( help->helper, -lockdep_is_held (&nf_conntrack_lock) +lockdep_is_held (&nf_conntrack_expect_lock) ) == me || exp->helper == me) &&del_timer (&exp->timeout)) {nf_ct_unlink_expect (exp);net/netfilter/nf_conntrack_helper.c static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me, · line 417
Quoted 4 lines
net/netfilter/nf_conntrack_helper.c +1
-0@@ -413,6 +414,7 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me, } } } +spin_unlock_bh (&nf_conntrack_expect_lock);
expect_lockis released here butnet/netfilter/nf_conntrack_helper.c static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me, · line 420
Quoted 2 lines
net/netfilter/nf_conntrack_helper.c +0
-0/* Get rid of expecteds, set helpers to NULL. */ for_each_possible_cpu (cpu) {will invoke
unhelp()AFAIU
unhelp()is safe in all cases even withoutnf_conntrack_expect_lockbeing held:
Quoted 8 lines
* in first loop we hold
nf_conntrack_expect_lock
* in 2nd loop we are holding the list lock, i.e.
if the ct is in the list it cannot disappear underneath
* in last loop you'll hold the hashed lock for the particular hash list, so can't go away either.So I think the lockdep annotation in uhelp is incorrect and not the patch itself.
Yes, I agree with your analysis. The code is safe, and the lockdep annotation in unhelp is just incorrect.
I think I'm going to use rcu_dereference_raw() in unhelp this case, as I don't think I can use rcu_dereference() there (because we are not in a rcu read section). And I'll add a comment to unhelp, that ct locking is needed. Besides in the call points of unhelp, it is quite visible/clear that we are taking a lock protecting the ct's.
I'll send a V3 of the patchset soon with this update.
Signature · 5 lines
Best regards, Jesper Dangaard Brouer MSc.CS, Sr. Network Kernel Developer at Red Hat Author of http://www.iptv-analyzer.org LinkedIn: http://www.linkedin.com/in/brouer