* [PATCH nf-next 1/2] netfilter: conntrack: protect early_drop by rcu read lock
@ 2016-07-12 11:45 Liping Zhang
2016-07-12 11:45 ` [PATCH nf-next 2/2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht Liping Zhang
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Liping Zhang @ 2016-07-12 11:45 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, Liping Zhang, Florian Westphal
From: Liping Zhang <liping.zhang@spreadtrum.com>
User can add ct entry via nfnetlink(IPCTNL_MSG_CT_NEW), and if the total
number reach the nf_conntrack_max, we will try to drop some ct entries.
But in this case(the main function call path is ctnetlink_create_conntrack
-> nf_conntrack_alloc -> early_drop), rcu_read_lock is not held, so race
with hash resize will happen.
Fixes: 242922a02717 ("netfilter: conntrack: simplify early_drop")
Cc: Florian Westphal <fw@strlen.de>
Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
net/netfilter/nf_conntrack_core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index e0e9c9a..2d46225 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -880,6 +880,7 @@ static noinline int early_drop(struct net *net, unsigned int _hash)
struct hlist_nulls_head *ct_hash;
unsigned hash, sequence, drops;
+ rcu_read_lock();
do {
sequence = read_seqcount_begin(&nf_conntrack_generation);
hash = scale_hash(_hash++);
@@ -887,6 +888,8 @@ static noinline int early_drop(struct net *net, unsigned int _hash)
} while (read_seqcount_retry(&nf_conntrack_generation, sequence));
drops = early_drop_list(net, &ct_hash[hash]);
+ rcu_read_unlock();
+
if (drops) {
NF_CT_STAT_ADD_ATOMIC(net, early_drop, drops);
return true;
--
2.5.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH nf-next 2/2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht 2016-07-12 11:45 [PATCH nf-next 1/2] netfilter: conntrack: protect early_drop by rcu read lock Liping Zhang @ 2016-07-12 11:45 ` Liping Zhang 2016-07-12 13:03 ` Florian Westphal 2016-07-12 13:01 ` [PATCH nf-next 1/2] netfilter: conntrack: protect early_drop by rcu read lock Florian Westphal 2016-07-12 14:34 ` Pablo Neira Ayuso 2 siblings, 1 reply; 6+ messages in thread From: Liping Zhang @ 2016-07-12 11:45 UTC (permalink / raw) To: pablo; +Cc: netfilter-devel, Liping Zhang From: Liping Zhang <liping.zhang@spreadtrum.com> Since Commit 64b87639c9cb ("netfilter: conntrack: fix race between nf_conntrack proc read and hash resize") introdue the nf_conntrack_get_ht, so there's no need to check nf_conntrack_generation again and again to get the hash table and hash size. Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com> --- net/netfilter/nf_conntrack_core.c | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c index 2d46225..bafebf7 100644 --- a/net/netfilter/nf_conntrack_core.c +++ b/net/netfilter/nf_conntrack_core.c @@ -461,7 +461,8 @@ nf_ct_key_equal(struct nf_conntrack_tuple_hash *h, } /* must be called with rcu read lock held */ -void nf_conntrack_get_ht(struct hlist_nulls_head **hash, unsigned int *hsize) +inline void +nf_conntrack_get_ht(struct hlist_nulls_head **hash, unsigned int *hsize) { struct hlist_nulls_head *hptr; unsigned int sequence, hsz; @@ -489,14 +490,11 @@ ____nf_conntrack_find(struct net *net, const struct nf_conntrack_zone *zone, struct nf_conntrack_tuple_hash *h; struct hlist_nulls_head *ct_hash; struct hlist_nulls_node *n; - unsigned int bucket, sequence; + unsigned int bucket, hsize; begin: - do { - sequence = read_seqcount_begin(&nf_conntrack_generation); - bucket = scale_hash(hash); - ct_hash = nf_conntrack_hash; - } while (read_seqcount_retry(&nf_conntrack_generation, sequence)); + nf_conntrack_get_ht(&ct_hash, &hsize); + bucket = reciprocal_scale(hash, hsize); hlist_nulls_for_each_entry_rcu(h, n, &ct_hash[bucket], hnnode) { if (nf_ct_key_equal(h, tuple, zone, net)) { @@ -801,18 +799,15 @@ nf_conntrack_tuple_taken(const struct nf_conntrack_tuple *tuple, const struct nf_conntrack_zone *zone; struct nf_conntrack_tuple_hash *h; struct hlist_nulls_head *ct_hash; - unsigned int hash, sequence; + unsigned int hash, hsize; struct hlist_nulls_node *n; struct nf_conn *ct; zone = nf_ct_zone(ignored_conntrack); rcu_read_lock(); - do { - sequence = read_seqcount_begin(&nf_conntrack_generation); - hash = hash_conntrack(net, tuple); - ct_hash = nf_conntrack_hash; - } while (read_seqcount_retry(&nf_conntrack_generation, sequence)); + nf_conntrack_get_ht(&ct_hash, &hsize); + hash = __hash_conntrack(net, tuple, hsize); hlist_nulls_for_each_entry_rcu(h, n, &ct_hash[hash], hnnode) { ct = nf_ct_tuplehash_to_ctrack(h); @@ -878,14 +873,11 @@ static noinline int early_drop(struct net *net, unsigned int _hash) for (i = 0; i < NF_CT_EVICTION_RANGE; i++) { struct hlist_nulls_head *ct_hash; - unsigned hash, sequence, drops; + unsigned int hash, hsize, drops; rcu_read_lock(); - do { - sequence = read_seqcount_begin(&nf_conntrack_generation); - hash = scale_hash(_hash++); - ct_hash = nf_conntrack_hash; - } while (read_seqcount_retry(&nf_conntrack_generation, sequence)); + nf_conntrack_get_ht(&ct_hash, &hsize); + hash = reciprocal_scale(_hash++, hsize); drops = early_drop_list(net, &ct_hash[hash]); rcu_read_unlock(); -- 2.5.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH nf-next 2/2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht 2016-07-12 11:45 ` [PATCH nf-next 2/2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht Liping Zhang @ 2016-07-12 13:03 ` Florian Westphal 2016-07-13 8:13 ` Liping Zhang 0 siblings, 1 reply; 6+ messages in thread From: Florian Westphal @ 2016-07-12 13:03 UTC (permalink / raw) To: Liping Zhang; +Cc: pablo, netfilter-devel, Liping Zhang Liping Zhang <zlpnobody@163.com> wrote: > From: Liping Zhang <liping.zhang@spreadtrum.com> > > Since Commit 64b87639c9cb ("netfilter: conntrack: fix race between > nf_conntrack proc read and hash resize") introdue the > nf_conntrack_get_ht, so there's no need to check nf_conntrack_generation > again and again to get the hash table and hash size. > > Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com> > --- > net/netfilter/nf_conntrack_core.c | 30 +++++++++++------------------- > 1 file changed, 11 insertions(+), 19 deletions(-) > > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c > index 2d46225..bafebf7 100644 > --- a/net/netfilter/nf_conntrack_core.c > +++ b/net/netfilter/nf_conntrack_core.c > @@ -461,7 +461,8 @@ nf_ct_key_equal(struct nf_conntrack_tuple_hash *h, > } > > /* must be called with rcu read lock held */ > -void nf_conntrack_get_ht(struct hlist_nulls_head **hash, unsigned int *hsize) > +inline void > +nf_conntrack_get_ht(struct hlist_nulls_head **hash, unsigned int *hsize) Which "inline void"? This is very unusual. I would suggest to not add it, and ... > { > struct hlist_nulls_head *hptr; > unsigned int sequence, hsz; > @@ -489,14 +490,11 @@ ____nf_conntrack_find(struct net *net, const struct nf_conntrack_zone *zone, > struct nf_conntrack_tuple_hash *h; > struct hlist_nulls_head *ct_hash; > struct hlist_nulls_node *n; > - unsigned int bucket, sequence; > + unsigned int bucket, hsize; > > begin: > - do { > - sequence = read_seqcount_begin(&nf_conntrack_generation); > - bucket = scale_hash(hash); > - ct_hash = nf_conntrack_hash; > - } while (read_seqcount_retry(&nf_conntrack_generation, sequence)); > + nf_conntrack_get_ht(&ct_hash, &hsize); > + bucket = reciprocal_scale(hash, hsize); leave ____nf_conntrack_find alone, but convert > @@ -801,18 +799,15 @@ nf_conntrack_tuple_taken(const struct nf_conntrack_tuple *tuple, This one .. > @@ -878,14 +873,11 @@ static noinline int early_drop(struct net *net, unsigned int _hash) ... and this one too. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH nf-next 2/2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht 2016-07-12 13:03 ` Florian Westphal @ 2016-07-13 8:13 ` Liping Zhang 0 siblings, 0 replies; 6+ messages in thread From: Liping Zhang @ 2016-07-13 8:13 UTC (permalink / raw) To: Florian Westphal; +Cc: pablo, netfilter-devel, Liping Zhang Hi Florian, At 2016-07-12 21:03:03, "Florian Westphal" <fw@strlen.de> wrote: >Liping Zhang <zlpnobody@163.com> wrote: >> +inline void >> +nf_conntrack_get_ht(struct hlist_nulls_head **hash, unsigned int *hsize) > >Which "inline void"? This is very unusual. > >I would suggest to not add it, and ... Yes, but we can still find a very few sample codes using inline without static in current kernel source tree. For example: inline void raise_softirq_irqoff(unsigned int nr) { ... } inline const struct nf_nat_l3proto * __nf_nat_l3proto_find(u8 family) { ... } And from the description of the gcc's document(https://gcc.gnu.org/onlinedocs/gcc/Inline.html): "When an inline function is not static, then the compiler must assume that there may be calls from other source files; since a global symbol can be defined only once in any program, the function must not be defined in the other source files, so the calls therein cannot be integrated". We can find that "inline void nf_conntrack_get_ht() { ... }" here will be integrated in nf_conntrack_core.c and in other places will be a function call. And I test this on my x86 mechain, it works as expected. >leave ____nf_conntrack_find alone, but convert > >> @@ -801,18 +799,15 @@ nf_conntrack_tuple_taken(const struct nf_conntrack_tuple *tuple, > >This one .. > >> @@ -878,14 +873,11 @@ static noinline int early_drop(struct net *net, unsigned int _hash) > >... and this one too. > IMO, leave ____nf_conntrack_find alone seems not very good. If you strongly dislike "inline void", I can keep nf_conntrack_get_ht unchanged. But I will convert all these three to use nf_conntrack_get_ht. I think even in ____nf_conntrack_find, inline nf_conntrack_get_ht will earn very very little performace here, almost none. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH nf-next 1/2] netfilter: conntrack: protect early_drop by rcu read lock 2016-07-12 11:45 [PATCH nf-next 1/2] netfilter: conntrack: protect early_drop by rcu read lock Liping Zhang 2016-07-12 11:45 ` [PATCH nf-next 2/2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht Liping Zhang @ 2016-07-12 13:01 ` Florian Westphal 2016-07-12 14:34 ` Pablo Neira Ayuso 2 siblings, 0 replies; 6+ messages in thread From: Florian Westphal @ 2016-07-12 13:01 UTC (permalink / raw) To: Liping Zhang; +Cc: pablo, netfilter-devel, Liping Zhang, Florian Westphal Liping Zhang <zlpnobody@163.com> wrote: > From: Liping Zhang <liping.zhang@spreadtrum.com> > > User can add ct entry via nfnetlink(IPCTNL_MSG_CT_NEW), and if the total > number reach the nf_conntrack_max, we will try to drop some ct entries. > > But in this case(the main function call path is ctnetlink_create_conntrack > -> nf_conntrack_alloc -> early_drop), rcu_read_lock is not held, so race > with hash resize will happen. Right, this was fine before because we held one of the spinlocks which blocks vs. resize. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH nf-next 1/2] netfilter: conntrack: protect early_drop by rcu read lock 2016-07-12 11:45 [PATCH nf-next 1/2] netfilter: conntrack: protect early_drop by rcu read lock Liping Zhang 2016-07-12 11:45 ` [PATCH nf-next 2/2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht Liping Zhang 2016-07-12 13:01 ` [PATCH nf-next 1/2] netfilter: conntrack: protect early_drop by rcu read lock Florian Westphal @ 2016-07-12 14:34 ` Pablo Neira Ayuso 2 siblings, 0 replies; 6+ messages in thread From: Pablo Neira Ayuso @ 2016-07-12 14:34 UTC (permalink / raw) To: Liping Zhang; +Cc: netfilter-devel, Liping Zhang, Florian Westphal On Tue, Jul 12, 2016 at 07:45:00PM +0800, Liping Zhang wrote: > From: Liping Zhang <liping.zhang@spreadtrum.com> > > User can add ct entry via nfnetlink(IPCTNL_MSG_CT_NEW), and if the total > number reach the nf_conntrack_max, we will try to drop some ct entries. > > But in this case(the main function call path is ctnetlink_create_conntrack > -> nf_conntrack_alloc -> early_drop), rcu_read_lock is not held, so race > with hash resize will happen. Applied, thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-07-13 8:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-07-12 11:45 [PATCH nf-next 1/2] netfilter: conntrack: protect early_drop by rcu read lock Liping Zhang 2016-07-12 11:45 ` [PATCH nf-next 2/2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht Liping Zhang 2016-07-12 13:03 ` Florian Westphal 2016-07-13 8:13 ` Liping Zhang 2016-07-12 13:01 ` [PATCH nf-next 1/2] netfilter: conntrack: protect early_drop by rcu read lock Florian Westphal 2016-07-12 14:34 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).