Re: Regression caused by commit "netfilter: iptables: lock free counters"

Re: Regression caused by commit "netfilter: iptables: lock free counters"

[David Miller]

From: Graham Murray <graham@gmurray.org.uk>
Date: Sun, 05 Apr 2009 08:05:17 +0100

Please CC the appropriate mailing lists (as I have now) when reporting
this incredibly useful information.  The networking and netfilter
developers largely do not read linux-kernel.

> Roman Mindalev <r000n@r000n.net> writes:
> 
> > Result of the bisection:
> >
> > 784544739a25c30637397ace5489eeb6e15d7d49 is first bad commit
> > commit 784544739a25c30637397ace5489eeb6e15d7d49             
> 
> I am seeing a different problem which also bisects to this commit. There are
> no kernel messages but ip6tables fails to run.
> 
> newton ~ # ip6tables -L -v
> FATAL: Module ip6_tables not found.
> ip6tables v1.4.3.1: can't initialize ip6tables table `filter': Memory allocation problem
> Perhaps ip6tables or your kernel needs to be upgraded.
> 
> I get this error no matter which ip6tables sub-command I run. Ip6tables
> is built into the kernel, not as modules.
> 
> An strace shows the failure to be 
> socket(PF_INET6, SOCK_RAW, IPPROTO_RAW) = 3
> getsockopt(3, SOL_IPV6, 0x40 /* IPV6_??? */, "filter\0\305\0w~\300\0wb\305P\24\312\t\0009b\305\216\23\0\0\310\341/g\16"..., [84]) = 0
> brk(0)                                  = 0x8273000
> brk(0x8294000)                          = 0x8294000
> getsockopt(3, SOL_IPV6, 0x41 /* IPV6_??? */, 0x8273090, 0xbfd23628) = -1 ENOMEM (Cannot allocate memory)
> close(3)                                = 0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: Regression caused by commit "netfilter: iptables: lock free counters"

[Eric Dumazet]

David Miller a écrit :
> From: Graham Murray <graham@gmurray.org.uk>
> Date: Sun, 05 Apr 2009 08:05:17 +0100
> 
> Please CC the appropriate mailing lists (as I have now) when reporting
> this incredibly useful information.  The networking and netfilter
> developers largely do not read linux-kernel.
> 
>> Roman Mindalev <r000n@r000n.net> writes:
>>
>>> Result of the bisection:
>>>
>>> 784544739a25c30637397ace5489eeb6e15d7d49 is first bad commit
>>> commit 784544739a25c30637397ace5489eeb6e15d7d49             
>> I am seeing a different problem which also bisects to this commit. There are
>> no kernel messages but ip6tables fails to run.
>>
>> newton ~ # ip6tables -L -v
>> FATAL: Module ip6_tables not found.
>> ip6tables v1.4.3.1: can't initialize ip6tables table `filter': Memory allocation problem
>> Perhaps ip6tables or your kernel needs to be upgraded.
>>
>> I get this error no matter which ip6tables sub-command I run. Ip6tables
>> is built into the kernel, not as modules.
>>
>> An strace shows the failure to be 
>> socket(PF_INET6, SOCK_RAW, IPPROTO_RAW) = 3
>> getsockopt(3, SOL_IPV6, 0x40 /* IPV6_??? */, "filter\0\305\0w~\300\0wb\305P\24\312\t\0009b\305\216\23\0\0\310\341/g\16"..., [84]) = 0
>> brk(0)                                  = 0x8273000
>> brk(0x8294000)                          = 0x8294000

so ip6tables allocates about 128 Kbytes of ram in order to get rules from kernel.

>> getsockopt(3, SOL_IPV6, 0x41 /* IPV6_??? */, 0x8273090, 0xbfd23628) = -1 ENOMEM (Cannot allocate memory)
>> close(3)                                = 0
>>


This is a big problem yes, since "iptables|ip6tables" -L needs to allocate kernel memory
to perform the momentary swap.

On x86, this is potentially a problem if vmalloc space is exhausted or fragmented,
(or lowmem exhausted) and/or many cpus are online/possible.

Graham, could you please give us :

# cat /proc/vmallocinfo
# cat /proc/meminfo

I wonder if your machine is in a state where even an "ip6tables -A ..." would fail anyway
since it should allocate same amount of memory than "ip6tables -L "


This could probably be solved using a single "table" containing rules only, that could
be shared for every cpus. Only counters should be percpu. This should save a lot of ram,
over previous situation (2.6.29 or current one)

(current scheme is to allocate a copy of all rules logic *and* counters per cpu)

Then if we want to be sure "iptables -L" cannot fail, we should reserve this extra space
at load time (iptables -{A|I}", instead.

Other possibility is to use a percpu seqlock as Stephen did in one of his patch, and not swap tables
when doing "iptables -L".
This would slowdown fast path a litle bit (one spinlock/spinunlock) per ipt_do_table() call. 


--
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: Regression caused by commit "netfilter: iptables: lock free counters"

[Eric Dumazet]

Graham Murray a écrit :
> Eric Dumazet <dada1@cosmosbay.com> writes:
> 
>>>>> 784544739a25c30637397ace5489eeb6e15d7d49 is first bad commit
>>>>> commit 784544739a25c30637397ace5489eeb6e15d7d49             
>>>> I am seeing a different problem which also bisects to this commit. There are
>>>> no kernel messages but ip6tables fails to run.
>>>>
>>>> newton ~ # ip6tables -L -v
>>>> FATAL: Module ip6_tables not found.
>>>> ip6tables v1.4.3.1: can't initialize ip6tables table `filter': Memory allocation problem
>>>> Perhaps ip6tables or your kernel needs to be upgraded.
>>>>
>>>> I get this error no matter which ip6tables sub-command I run. Ip6tables
>>>> is built into the kernel, not as modules.
>>>>
>>>> An strace shows the failure to be 
>>>> socket(PF_INET6, SOCK_RAW, IPPROTO_RAW) = 3
>>>> getsockopt(3, SOL_IPV6, 0x40 /* IPV6_??? */, "filter\0\305\0w~\300\0wb\305P\24\312\t\0009b\305\216\23\0\0\310\341/g\16"..., [84]) = 0
>>>> brk(0)                                  = 0x8273000
>>>> brk(0x8294000)                          = 0x8294000
>> so ip6tables allocates about 128 Kbytes of ram in order to get rules from kernel.
>>
>>>> getsockopt(3, SOL_IPV6, 0x41 /* IPV6_??? */, 0x8273090, 0xbfd23628) = -1 ENOMEM (Cannot allocate memory)
>>>> close(3)                                = 0
>>>>
>>
>> This is a big problem yes, since "iptables|ip6tables" -L needs to allocate kernel memory
>> to perform the momentary swap.
>>
>> On x86, this is potentially a problem if vmalloc space is exhausted or fragmented,
>> (or lowmem exhausted) and/or many cpus are online/possible.
> 
> iptables gives me no problems at all, it is just ip6tables that
> fails. The first indication of this is during the init scripts when
> ip6tables-restore fails.

I see, its a plain bug in net/ipv6/netfilter/ip6_tables.c
function alloc_counters() always returns -ENOMEM

Unfortunatly , its Sunday here and I have to run for lunch time with family :)

If nobody beats me, I will do the fix in a couple of hours...

Thank you

Re: Regression caused by commit "netfilter: iptables: lock free counters"

[Jan Engelhardt]

On Sunday 2009-04-05 12:01, Eric Dumazet wrote:
>
>This could probably be solved using a single "table" containing
>rules only, that could be shared for every cpus. Only counters
>should be percpu. This should save a lot of ram, over previous
>situation (2.6.29 or current one)

Why would counters stay separate?

I recognize all of this table copying is related to do NUMA
optimizations, and I think I heard cache bouncing too somewhere else.

[ http://marc.info/?l=netfilter-devel&m=119903624211253&w=2 ]

[PATCH] netfilter: ip6tables fix

[Eric Dumazet]

Eric Dumazet a écrit :
> Graham Murray a écrit :
>> iptables gives me no problems at all, it is just ip6tables that
>> fails. The first indication of this is during the init scripts when
>> ip6tables-restore fails.
> 
> I see, its a plain bug in net/ipv6/netfilter/ip6_tables.c
> function alloc_counters() always returns -ENOMEM
> 
> Unfortunatly , its Sunday here and I have to run for lunch time with family :)
> 
> If nobody beats me, I will do the fix in a couple of hours...

Here is the fix, thanks Graham for the report !

[PATCH] netfilter: ip6tables fix

ip6_tables.c	 alloc_counters() misses a return statement, making
ip6tables -N always failing and leaking memory.

Reported-by: Graham Murray <graham@gmurray.org.uk>
Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
---

diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index dfed176..800ae85 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -1033,6 +1033,8 @@ static struct xt_counters *alloc_counters(struct xt_table *table)
 
 	xt_free_table_info(info);
 
+	return counters;
+
  free_counters:
 	vfree(counters);
  nomem:

Re: Regression caused by commit "netfilter: iptables: lock free counters"

[Eric Dumazet]

Jan Engelhardt a écrit :
> On Sunday 2009-04-05 12:01, Eric Dumazet wrote:
>> This could probably be solved using a single "table" containing
>> rules only, that could be shared for every cpus. Only counters
>> should be percpu. This should save a lot of ram, over previous
>> situation (2.6.29 or current one)
> 
> Why would counters stay separate?
> 
> I recognize all of this table copying is related to do NUMA
> optimizations, and I think I heard cache bouncing too somewhere else.
> 
> [ http://marc.info/?l=netfilter-devel&m=119903624211253&w=2 ]
> 
> 

Not only NUMA, but SMP too. counters are integrated in rules themselves.
So in order to avoid ping pongs between cpus, we choose to allocate
one copy of rules/counters per cpu.

But with some changes, we could let the rules read-only and shared by
all cpus, and shadow counters only on percpu variables, thus reducing
memory costs.


--
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: ip6tables fix

[Eric Dumazet]

Eric Dumazet a écrit :
> Eric Dumazet a écrit :
>> Graham Murray a écrit :
>>> iptables gives me no problems at all, it is just ip6tables that
>>> fails. The first indication of this is during the init scripts when
>>> ip6tables-restore fails.
>> I see, its a plain bug in net/ipv6/netfilter/ip6_tables.c
>> function alloc_counters() always returns -ENOMEM
>>
>> Unfortunatly , its Sunday here and I have to run for lunch time with family :)
>>
>> If nobody beats me, I will do the fix in a couple of hours...
> 
> Here is the fix, thanks Graham for the report !
> 
> [PATCH] netfilter: ip6tables fix
> 
> ip6_tables.c	 alloc_counters() misses a return statement, making
> ip6tables -N always failing and leaking memory.

Oh well, this ChangeLog is not correct :(

Here is an updated patch with correct ChangeLog, sorry David.
(-L instead of -N, and there was no memory leak involved)

[PATCH] netfilter: ip6tables fix

ip6_tables.c alloc_counters() misses a return statement, making
ip6tables -L always failing.

Reported-by: Graham Murray <graham@gmurray.org.uk>
Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
---

diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index dfed176..800ae85 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -1033,6 +1033,8 @@ static struct xt_counters *alloc_counters(struct xt_table *table)
 
 	xt_free_table_info(info);
 
+	return counters;
+
  free_counters:
 	vfree(counters);
  nomem:

Re: [PATCH] netfilter: ip6tables fix

[Patrick McHardy]

Eric Dumazet wrote:
> [PATCH] netfilter: ip6tables fix
> 
> ip6_tables.c alloc_counters() misses a return statement, making
> ip6tables -L always failing.
> 
> Reported-by: Graham Murray <graham@gmurray.org.uk>
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

Applied, thanks Eric.