num_possible_cpus() usage in iptables

num_possible_cpus() usage in iptables

[David S. Miller]

Guys, you can't use this interface like that.

It's a _COUNT_ of the number of possible processors, not the number of
the higest PHYSICAL cpu index.

So you can have cpu's numbered "0" and "2", but num_possible_cpus()
will return 2.  This is not just theoretical, Ultra60 sparc64 systems
have this exact physical cpu numbering.   And as a result, when the
changes went into iptables to use num_possible_cpus() instead of NR_CPUS
it broke iptables.  It OOPS's when you load the module, in fact.

So, on such a system, this loop:

	/* And one copy for every other CPU */
	for (i = 1; i < num_possible_cpus(); i++) {
		memcpy(newinfo->entries + SMP_ALIGN(newinfo->size)*i,
		       newinfo->entries,
		       SMP_ALIGN(newinfo->size));
	}

would initialize the wrong entries.

We need to fix this.

Re: num_possible_cpus() usage in iptables

[Eric Dumazet]

David S. Miller a écrit :
> Guys, you can't use this interface like that.
> 
> It's a _COUNT_ of the number of possible processors, not the number of
> the higest PHYSICAL cpu index.
> 
> So you can have cpu's numbered "0" and "2", but num_possible_cpus()
> will return 2.  This is not just theoretical, Ultra60 sparc64 systems
> have this exact physical cpu numbering.   And as a result, when the
> changes went into iptables to use num_possible_cpus() instead of NR_CPUS
> it broke iptables.  It OOPS's when you load the module, in fact.
> 
> So, on such a system, this loop:
> 
> 	/* And one copy for every other CPU */
> 	for (i = 1; i < num_possible_cpus(); i++) {
> 		memcpy(newinfo->entries + SMP_ALIGN(newinfo->size)*i,
> 		       newinfo->entries,
> 		       SMP_ALIGN(newinfo->size));
> 	}
> 
> would initialize the wrong entries.
> 
> We need to fix this.
> 
> 
Hi David

My previous patch ([PATCH 3/3] netfilter : 3 patches to boost ip_tables 
performance) addressed this problem.


http://marc.theaimsgroup.com/?l=linux-netdev&m=112733887410796&w=2

Relevant parts :

-	for (i = 1; i < num_possible_cpus(); i++) {
-		memcpy(newinfo->entries + SMP_ALIGN(newinfo->size)*i,
-		       newinfo->entries,
-		       SMP_ALIGN(newinfo->size));
+	for_each_cpu(i) {
+		if (newinfo->entries[i] && newinfo->entries[i] != entry0)
+			memcpy(newinfo->entries[i], entry0, newinfo->size);


-		for (i = 0; i < num_possible_cpus(); i++) {
-			table_base =
-				(void *)newinfo->entries
-				+ TABLE_OFFSET(newinfo, i);
-
-			table_base->comefrom = 0xdead57ac;
+		for_each_cpu(cpu) {
+			struct ipt_entry *table_base = newinfo->entries[cpu];
+			if (table_base)
+				table_base->comefrom = 0xdead57ac;

But the whole thing was suspended because nobody wants to put a vmalloc_node() 
inside kernel.

Eric

Re: num_possible_cpus() usage in iptables

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 854 bytes --]

On Sun, Oct 09, 2005 at 08:47:34PM -0700, David S. Miller wrote:
> 
> Guys, you can't use this interface like that.
> 
> It's a _COUNT_ of the number of possible processors, not the number of
> the higest PHYSICAL cpu index.

thanks, Dave.  It's very surprising that nobody apparently hit this
before.  

I'll cook up a patch later today.

> will return 2.  This is not just theoretical, Ultra60 sparc64 systems
> have this exact physical cpu numbering.   

I see :(
-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: num_possible_cpus() usage in iptables

[seb]

On Mon, Oct 10, 2005 at 10:19:57AM +0200, Harald Welte wrote:
> On Sun, Oct 09, 2005 at 08:47:34PM -0700, David S. Miller wrote:
> > 
> > Guys, you can't use this interface like that.
> > 
> > It's a _COUNT_ of the number of possible processors, not the number of
> > the higest PHYSICAL cpu index.
> 
> thanks, Dave.  It's very surprising that nobody apparently hit this
> before.  
> 

Well, I did and it's not so surprising.
2 way Sparc used as firewall are quite uncommon.
Moreover, the bug will bite only on special models that have this numbering.
There shouldn't be too much people in this case.
Please consider me as a worst case scenario :).
(SMP + sparc64 + firewall + cpu 0 and 2).

> I'll cook up a patch later today.
> 
> > will return 2.  This is not just theoretical, Ultra60 sparc64 systems
> > have this exact physical cpu numbering.   
> 
> I see :(

Re: num_possible_cpus() usage in iptables

[David S. Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Mon, 10 Oct 2005 08:23:27 +0200

> My previous patch ([PATCH 3/3] netfilter : 3 patches to boost ip_tables 
> performance) addressed this problem.
> 
> http://marc.theaimsgroup.com/?l=linux-netdev&m=112733887410796&w=2

Long term this is the kind of thing we should be doing.

But for 2.6.14, I would like to suggest that we revert back
to using NR_CPUS as that is the simplest fix for this bug.

I even spent part of this afternoon investigating other kinds of
fixes, and all of them have possible non-trivial regressions.

Yes, using NR_CPUS wastes a lot of ram, but it fixes this
bug in a straightforward and easy to verify fashion.

Comments?

Re: num_possible_cpus() usage in iptables

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 1795 bytes --]

On Mon, Oct 10, 2005 at 05:27:28PM -0700, David S. Miller wrote:
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Mon, 10 Oct 2005 08:23:27 +0200
> 
> > My previous patch ([PATCH 3/3] netfilter : 3 patches to boost ip_tables 
> > performance) addressed this problem.
> > 
> > http://marc.theaimsgroup.com/?l=linux-netdev&m=112733887410796&w=2
> 
> Long term this is the kind of thing we should be doing.
> 
> But for 2.6.14, I would like to suggest that we revert back
> to using NR_CPUS as that is the simplest fix for this bug.
> 
> I even spent part of this afternoon investigating other kinds of
> fixes, and all of them have possible non-trivial regressions.
> 
> Yes, using NR_CPUS wastes a lot of ram, but it fixes this
> bug in a straightforward and easy to verify fashion.

Mh.  The problem is that I know a number of people who were running out
of RAM with moderate ruleset sizes (in the tens of thousands rules), and
the sole reason for switching from 2.4.x to 2.6.x  was the NR_CPU to cpu
number change.

However, given that 2.6.14 seems close (and I cannot do testing on SMP
while on the road), it might be an interim solution.

OTOTH, how many users of systmes with discontiguous processor id space
are there vs. users with large rulesets on 2cpu machines that now have a
factor of 16 increased rule size (which can easily be a couple of 100mb
non-swappable kernel memory).

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: num_possible_cpus() usage in iptables

[David S. Miller]

From: Harald Welte <laforge@netfilter.org>
Date: Tue, 11 Oct 2005 16:35:54 +0200

> Mh.  The problem is that I know a number of people who were running out
> of RAM with moderate ruleset sizes (in the tens of thousands rules), and
> the sole reason for switching from 2.4.x to 2.6.x  was the NR_CPU to cpu
> number change.

I totally agree, using NR_CPUS has a lot of very bad side effects.
Although the simplest, it is an undesirable version of the fix due
to the bloat.

I'm going to put some brain power into this today since it's the
only big issue on my radar wrt. networking in 2.6.14