[PATCH] netfilter: don't xt_jumpstack_alloc twice in xt_register_table

[PATCH] netfilter: don't xt_jumpstack_alloc twice in xt_register_table

[Xiaotian Feng]

In xt_register_table, xt_jumpstack_alloc is called first, later
xt_replace_table is used. But in xt_replace_table, xt_jumpstack_alloc
will be used again. Then the memory allocated by previous xt_jumpstack_alloc
will be leaked. We can simply remove the previous xt_jumpstack_alloc because
there aren't any users of newinfo between xt_jumpstack_alloc and
xt_replace_table.

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
Cc: Patrick McHardy <kaber@trash.net>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jan Engelhardt <jengelh@medozas.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
---
 net/netfilter/x_tables.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 445de70..47b1e79 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -844,10 +844,6 @@ struct xt_table *xt_register_table(struct net *net,
 	struct xt_table_info *private;
 	struct xt_table *t, *table;
 
-	ret = xt_jumpstack_alloc(newinfo);
-	if (ret < 0)
-		return ERR_PTR(ret);
-
 	/* Don't add one object to multiple lists. */
 	table = kmemdup(input_table, sizeof(struct xt_table), GFP_KERNEL);
 	if (!table) {
-- 
1.7.0.1

Re: [PATCH] netfilter: don't xt_jumpstack_alloc twice in xt_register_table

[Jan Engelhardt]

On Monday 2010-05-31 13:06, Xiaotian Feng wrote:

>In xt_register_table, xt_jumpstack_alloc is called first, later
>xt_replace_table is used. But in xt_replace_table, xt_jumpstack_alloc
>will be used again. Then the memory allocated by previous xt_jumpstack_alloc
>will be leaked. We can simply remove the previous xt_jumpstack_alloc because
>there aren't any users of newinfo between xt_jumpstack_alloc and
>xt_replace_table.

Indeed that seems to be so.

>diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
>index 445de70..47b1e79 100644
>--- a/net/netfilter/x_tables.c
>+++ b/net/netfilter/x_tables.c
>@@ -844,10 +844,6 @@ struct xt_table *xt_register_table(struct net *net,
> 	struct xt_table_info *private;
> 	struct xt_table *t, *table;
> 
>-	ret = xt_jumpstack_alloc(newinfo);
>-	if (ret < 0)
>-		return ERR_PTR(ret);
>-
> 	/* Don't add one object to multiple lists. */
> 	table = kmemdup(input_table, sizeof(struct xt_table), GFP_KERNEL);
> 	if (!table) {

[PATCH] netfilter: xtables: stackptr should be percpu

[Eric Dumazet]

Le lundi 31 mai 2010 à 13:51 +0200, Jan Engelhardt a écrit :
> On Monday 2010-05-31 13:06, Xiaotian Feng wrote:
> 
> >In xt_register_table, xt_jumpstack_alloc is called first, later
> >xt_replace_table is used. But in xt_replace_table, xt_jumpstack_alloc
> >will be used again. Then the memory allocated by previous xt_jumpstack_alloc
> >will be leaked. We can simply remove the previous xt_jumpstack_alloc because
> >there aren't any users of newinfo between xt_jumpstack_alloc and
> >xt_replace_table.
> 
> Indeed that seems to be so.

An official "Acked-by: ..." would be fine Jan :)

BTW I noticed a _big_ slowdown of iptables lately, and located the
reason.

All cpus share a single cache line for their 'stackptr' storage,
introduced in commit f3c5c1bfd4

This is a stable candidate (2.6.34)

Note : We also should use alloc_percpu() for jumpstack but this is not a
critical thing and can be a net-next patch.


[PATCH] netfilter: xtables: stackptr should be percpu

commit f3c5c1bfd4 (netfilter: xtables: make ip_tables reentrant)
introduced a performance regression, because stackptr array is shared by
all cpus, adding cache line ping pongs. (16 cpus share a 64 bytes cache
line)

Fix this using alloc_percpu()

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 include/linux/netfilter/x_tables.h |    2 +-
 net/ipv4/netfilter/ip_tables.c     |    2 +-
 net/ipv6/netfilter/ip6_tables.c    |    2 +-
 net/netfilter/x_tables.c           |   13 +++----------
 4 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
index c00cc0c..24e5d01 100644
--- a/include/linux/netfilter/x_tables.h
+++ b/include/linux/netfilter/x_tables.h
@@ -397,7 +397,7 @@ struct xt_table_info {
 	 * @stacksize jumps (number of user chains) can possibly be made.
 	 */
 	unsigned int stacksize;
-	unsigned int *stackptr;
+	unsigned int __percpu *stackptr;
 	void ***jumpstack;
 	/* ipt_entry tables: one per CPU */
 	/* Note : this field MUST be the last one, see XT_TABLE_INFO_SZ */
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 63958f3..4b6c5ca 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -336,7 +336,7 @@ ipt_do_table(struct sk_buff *skb,
 	cpu        = smp_processor_id();
 	table_base = private->entries[cpu];
 	jumpstack  = (struct ipt_entry **)private->jumpstack[cpu];
-	stackptr   = &private->stackptr[cpu];
+	stackptr   = per_cpu_ptr(private->stackptr, cpu);
 	origptr    = *stackptr;
 
 	e = get_entry(table_base, private->hook_entry[hook]);
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 6f517bd..9d2d68f 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -363,7 +363,7 @@ ip6t_do_table(struct sk_buff *skb,
 	cpu        = smp_processor_id();
 	table_base = private->entries[cpu];
 	jumpstack  = (struct ip6t_entry **)private->jumpstack[cpu];
-	stackptr   = &private->stackptr[cpu];
+	stackptr   = per_cpu_ptr(private->stackptr, cpu);
 	origptr    = *stackptr;
 
 	e = get_entry(table_base, private->hook_entry[hook]);
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 445de70..7e8a93d 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -699,10 +699,8 @@ void xt_free_table_info(struct xt_table_info *info)
 		vfree(info->jumpstack);
 	else
 		kfree(info->jumpstack);
-	if (sizeof(unsigned int) * nr_cpu_ids > PAGE_SIZE)
-		vfree(info->stackptr);
-	else
-		kfree(info->stackptr);
+
+	free_percpu(info->stackptr);
 
 	kfree(info);
 }
@@ -753,14 +751,9 @@ static int xt_jumpstack_alloc(struct xt_table_info *i)
 	unsigned int size;
 	int cpu;
 
-	size = sizeof(unsigned int) * nr_cpu_ids;
-	if (size > PAGE_SIZE)
-		i->stackptr = vmalloc(size);
-	else
-		i->stackptr = kmalloc(size, GFP_KERNEL);
+	i->stackptr = alloc_percpu(unsigned int);
 	if (i->stackptr == NULL)
 		return -ENOMEM;
-	memset(i->stackptr, 0, size);
 
 	size = sizeof(void **) * nr_cpu_ids;
 	if (size > PAGE_SIZE)

Re: [PATCH] netfilter: xtables: stackptr should be percpu

[Jan Engelhardt]

On Monday 2010-05-31 15:13, Eric Dumazet wrote:
>
>All cpus share a single cache line for their 'stackptr' storage,
>introduced in commit f3c5c1bfd4
>
>This is a stable candidate (2.6.34)

Stackptr was first introduced for 2.6.35-rcX.

>+	i->stackptr = alloc_percpu(unsigned int);
> 	if (i->stackptr == NULL)
> 		return -ENOMEM;
>-	memset(i->stackptr, 0, size);
> 
> 	size = sizeof(void **) * nr_cpu_ids;
> 	if (size > PAGE_SIZE)

Are alloc_percpu areas cleared?

Acked-By: Jan Engelhardt <jengelh@medozas.de>

Re: [PATCH] netfilter: xtables: stackptr should be percpu

[Eric Dumazet]

Le lundi 31 mai 2010 à 15:22 +0200, Jan Engelhardt a écrit :
> On Monday 2010-05-31 15:13, Eric Dumazet wrote:
> >
> >All cpus share a single cache line for their 'stackptr' storage,
> >introduced in commit f3c5c1bfd4
> >
> >This is a stable candidate (2.6.34)
> 
> Stackptr was first introduced for 2.6.35-rcX.
> 

Indeed, I was fooled by 'git describe'

> >+	i->stackptr = alloc_percpu(unsigned int);
> > 	if (i->stackptr == NULL)
> > 		return -ENOMEM;
> >-	memset(i->stackptr, 0, size);
> > 
> > 	size = sizeof(void **) * nr_cpu_ids;
> > 	if (size > PAGE_SIZE)
> 
> Are alloc_percpu areas cleared?
> 

Yes, allocated chunks are cleared.


--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] netfilter: xtables: stackptr should be percpu

[Jan Engelhardt]

On Monday 2010-05-31 15:44, Eric Dumazet wrote:
>Le lundi 31 mai 2010 à 15:22 +0200, Jan Engelhardt a écrit :
>> On Monday 2010-05-31 15:13, Eric Dumazet wrote:
>> >
>> >All cpus share a single cache line for their 'stackptr' storage,
>> >introduced in commit f3c5c1bfd4
>> >
>> >This is a stable candidate (2.6.34)
>> 
>> Stackptr was first introduced for 2.6.35-rcX.
>
>Indeed, I was fooled by 'git describe'

Keep your friends close, and your enemies closer ;-)

	git describe --contains f3c5c1bfd
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] netfilter: xtables: stackptr should be percpu

[Eric Dumazet]

Le lundi 31 mai 2010 à 16:09 +0200, Jan Engelhardt a écrit :

> Keep your friends close, and your enemies closer ;-)
> 
> 	git describe --contains f3c5c1bfd


Yes, --contains should be the default, and --predates the option :)

This is a bit OT anyway :)


--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] netfilter: xtables: stackptr should be percpu

[Patrick McHardy]

Jan Engelhardt wrote:
> On Monday 2010-05-31 15:13, Eric Dumazet wrote:
>> All cpus share a single cache line for their 'stackptr' storage,
>> introduced in commit f3c5c1bfd4
>>
>> This is a stable candidate (2.6.34)
> 
> Stackptr was first introduced for 2.6.35-rcX.
> 
>> +	i->stackptr = alloc_percpu(unsigned int);
>> 	if (i->stackptr == NULL)
>> 		return -ENOMEM;
>> -	memset(i->stackptr, 0, size);
>>
>> 	size = sizeof(void **) * nr_cpu_ids;
>> 	if (size > PAGE_SIZE)
> 
> Are alloc_percpu areas cleared?
> 
> Acked-By: Jan Engelhardt <jengelh@medozas.de>

Applied, thanks.

Re: [PATCH] netfilter: don't xt_jumpstack_alloc twice in xt_register_table

[Jan Engelhardt]

On Monday 2010-05-31 13:51, Jan Engelhardt wrote:
>On Monday 2010-05-31 13:06, Xiaotian Feng wrote:
>
>>In xt_register_table, xt_jumpstack_alloc is called first, later
>>xt_replace_table is used. But in xt_replace_table, xt_jumpstack_alloc
>>will be used again. Then the memory allocated by previous xt_jumpstack_alloc
>>will be leaked. We can simply remove the previous xt_jumpstack_alloc because
>>there aren't any users of newinfo between xt_jumpstack_alloc and
>>xt_replace_table.
>
>Indeed that seems to be so.

Acked-By: Jan Engelhardt <jengelh@medozas.de>

>
>>diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
>>index 445de70..47b1e79 100644
>>--- a/net/netfilter/x_tables.c
>>+++ b/net/netfilter/x_tables.c
>>@@ -844,10 +844,6 @@ struct xt_table *xt_register_table(struct net *net,
>> 	struct xt_table_info *private;
>> 	struct xt_table *t, *table;
>> 
>>-	ret = xt_jumpstack_alloc(newinfo);
>>-	if (ret < 0)
>>-		return ERR_PTR(ret);
>>-
>> 	/* Don't add one object to multiple lists. */
>> 	table = kmemdup(input_table, sizeof(struct xt_table), GFP_KERNEL);
>> 	if (!table) {
>--
>To unsubscribe from this list: send the line "unsubscribe netfilter" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] netfilter: don't xt_jumpstack_alloc twice in xt_register_table

[Patrick McHardy]

Jan Engelhardt wrote:
> On Monday 2010-05-31 13:51, Jan Engelhardt wrote:
>> On Monday 2010-05-31 13:06, Xiaotian Feng wrote:
>>
>>> In xt_register_table, xt_jumpstack_alloc is called first, later
>>> xt_replace_table is used. But in xt_replace_table, xt_jumpstack_alloc
>>> will be used again. Then the memory allocated by previous xt_jumpstack_alloc
>>> will be leaked. We can simply remove the previous xt_jumpstack_alloc because
>>> there aren't any users of newinfo between xt_jumpstack_alloc and
>>> xt_replace_table.
>> Indeed that seems to be so.
> 
> Acked-By: Jan Engelhardt <jengelh@medozas.de>

Applied, thanks everyone.

Re: [PATCH] netfilter: don't xt_jumpstack_alloc twice in xt_register_table

[Patrick McHardy]

Jan Engelhardt wrote:
> On Monday 2010-05-31 13:51, Jan Engelhardt wrote:
>> On Monday 2010-05-31 13:06, Xiaotian Feng wrote:
>>
>>> In xt_register_table, xt_jumpstack_alloc is called first, later
>>> xt_replace_table is used. But in xt_replace_table, xt_jumpstack_alloc
>>> will be used again. Then the memory allocated by previous xt_jumpstack_alloc
>>> will be leaked. We can simply remove the previous xt_jumpstack_alloc because
>>> there aren't any users of newinfo between xt_jumpstack_alloc and
>>> xt_replace_table.
>> Indeed that seems to be so.
> 
> Acked-By: Jan Engelhardt <jengelh@medozas.de>

Applied, thanks.