[PATCH nf-next 1/2] netfilter: conntrack: protect early_drop by rcu read lock

[PATCH nf-next 1/2] netfilter: conntrack: protect early_drop by rcu read lock

[Liping Zhang]

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

[PATCH nf-next 2/2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht

[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

Re: [PATCH nf-next 1/2] netfilter: conntrack: protect early_drop by rcu read lock

[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.

Re: [PATCH nf-next 2/2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht

[Florian Westphal]

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.

Re: [PATCH nf-next 1/2] netfilter: conntrack: protect early_drop by rcu read lock

[Pablo Neira Ayuso]

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.

Re: [PATCH nf-next 2/2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht

[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.