Re: Frequent Oops on Shutdown 2.6.10

Re: Frequent Oops on Shutdown 2.6.10

[AndyLiebman]

In a message dated 2/23/2005 11:42:42 A.M. Eastern Standard Time,  
yoshfuji@linux-ipv6.org writes:
In article  <b9.522b6602.2f4df280@aol.com> (at Wed, 23 Feb 2005 09:51:44 
EST),  AndyLiebman@aol.com says:

> Should I bother to apply this patch, or  should I wait for you to make this 
 
> last change? What did you  think about my comment that the Oops only 
occurred  
> when the  Ethernet cable had been unplugged during operation? 

Well, not sure, but  I think it is worth trying.

Thanks.

--yoshfuji
Hi Yoshi,  

I just thought I would let you know that I applied the patch to the  2.6.10 
kernel and recompiled. It seems to have made the Oops go away. At least,  I 
can't make the Oops happen any more by unplugging the Ethernet cables during  
operation and then shutting down. 

I might add that when I tried to apply  the patch with:

patch --dry-run -p1 -d dir < patchfile

I got all kinds of errors about this line or that line. I can't remember  
what they were. In the end, I VERY CAREFULLY cut and pasted your patch, and  
removed the lines the patch was supposed to remove. 

Did you attempt to  apply the patch? Anyway, looks good. 
 
Regards, 
Andy Liebman
 

Re: Frequent Oops on Shutdown 2.6.10

[AndyLiebman]

Hello, Yoshi

Should I bother to apply  this patch, or should I wait for you to make this 
last change? What did you  think about my comment that the Oops only occurred 
when the Ethernet cable had  been unplugged during operation? 

Regards, 
Andy Liebman


In  a message dated 2/23/2005 4:52:59 A.M. Eastern Standard Time,  
herbert@gondor.apana.org.au writes:
On Wed, Feb 23, 2005 at 06:35:55PM +0900,  YOSHIFUJI Hideaki / ?$B5HF#1QL@ 
wrote:
> 
> What do you think of  this?

Thanks, this looks great.  There is just one technical  detail
to patch up.

> -int snmp6_unregister_dev(struct inet6_dev  *idev)
> +int snmp6_free_dev(struct inet6_dev *idev)
>   {
>      snmp6_mib_free((void  **)idev->stats.icmpv6);
>      return  0;
>  }

You need to check whether icmpv6[0] is NULL either  here or in
snmp6_mib_free.  Otherwise when snmp6_alloc_dev fails  we'll
wind up here and then call free_percpu on a pair of NULL  pointers.

Cheers,
-- 
Visit Openswan at  http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~}  <herbert@gondor.apana.org.au>
Home Page:  http://gondor.apana.org.au/~herbert/
PGP Key:  http://gondor.apana.org.au/~herbert/pubkey.txt  

Re: Frequent Oops on Shutdown 2.6.10

[AndyLiebman]

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

 
Hello, Yoshi
 
Should I bother to apply this patch, or should I wait for you to make this  
last change? What did you think about my comment that the Oops only occurred  
when the Ethernet cable had been unplugged during operation? 
 
Regards, 
Andy Liebman

On Wed,  Feb 23, 2005 at 06:35:55PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@  
wrote:
> 
> What do you think of this?

Thanks, this looks  great.  There is just one technical detail
to patch up.

>  -int snmp6_unregister_dev(struct inet6_dev *idev)
> +int  snmp6_free_dev(struct inet6_dev *idev)
>  {
>     snmp6_mib_free((void **)idev->stats.icmpv6);
>     return 0;
>  }

You need to check whether icmpv6[0] is  NULL either here or in
snmp6_mib_free.  Otherwise when snmp6_alloc_dev  fails we'll
wind up here and then call free_percpu on a pair of NULL  pointers.

Cheers,
-- 
Visit Openswan at  http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~}  <herbert@gondor.apana.org.au>
Home Page:  http://gondor.apana.org.au/~herbert/
PGP Key:  http://gondor.apana.org.au/~herbert/pubkey.txt





[-- Attachment #2: Type: text/html, Size: 1989 bytes --]

Re: Frequent Oops on Shutdown 2.6.10

[YOSHIFUJI Hideaki / 吉藤英明]

In article <b9.522b6602.2f4df280@aol.com> (at Wed, 23 Feb 2005 09:51:44 EST), AndyLiebman@aol.com says:

> Should I bother to apply this patch, or should I wait for you to make this  
> last change? What did you think about my comment that the Oops only occurred  
> when the Ethernet cable had been unplugged during operation? 

Well, not sure, but I think it is worth trying.

Thanks.

--yoshfuji

Re: Frequent Oops on Shutdown 2.6.10

[AndyLiebman]

I tested my server today several  times. I booted up and shutdown the server 
eight times while running the 2.6.10  kernel. Six times the sever shut down 
fine. Two times -- only when I had  unplugged the Ethernet connection during my 
session -- I got the Oops when I  shut down Ini both cases, it was about 2 
minutes after unplugging the Ethernet  cable.  

So, do you know what this means? Sure, the easy solution  would be "don't 
unplug the Ethernet cable while you're running." I can follow  that rule, but out 
in the field where the servers go, there will be accidents.  

Should I still try that patch? 

Regards, 
Andy Liebman  

Re: Frequent Oops on Shutdown 2.6.10

[AndyLiebman]

For what it's worth, I believe I only get this  Oops if I have unplugged an 
Ethernet cable while running the server. 

I  have 4 Ethernet ports on the server -- and in fact I am testing and 
configuring  many servers  at the same time. All servers are set up with the exact 
same  image, and the same set of IP addresses. Sometimes for convenience, I 
unplug an  ethernet cable from one server and plug it into another server -- 
while they're  running -- so that I can operate a machine remotely (I never 
connect more than  one server to my network at a time, to avoid IP address 
conflicts). Unplugging  and plugging Ethernet cables while running ALWAYS leads to nmbd 
errors on  shutdown -- guaranteed -- but with the 2.6.6 kernel never an Oops. 
I only get an  Oops with the 2.6.10 kernel. 

I'm going to do a more rigorous test today  to see if the Oops behavior 
really is 100 percent correlated with unplugging and  plugging the Ethernet cable. 

So, should I test the patch? 

Andy  Liebman

-------------------------------------------------------OLD  MESSAGES BELOW 
--------
In a message dated 2/22/2005 5:17:19 A.M. Eastern  Standard Time, 
herbert@gondor.apana.org.au writes:
On Tue, Feb 22, 2005 at  08:57:19PM +1100, Herbert Xu wrote:
> YOSHIFUJI Hideaki / ????  <yoshfuji@linux-ipv6.org> wrote:
> > In article  <20050221.162241.24618885.yoshfuji@linux-ipv6.org> (at Mon, 
21 Feb 2005  16:22:41 +0900 (JST)), YOSHIFUJI Hideaki / ???? 
<yoshfuji@linux-ipv6.org>  says:
> > 
> >> [IPV6] Don't remove dev_snmp6 procfs entry  until all users gone.
> 
> Sorry, but I don't see how this patch  explains the oops the
> people saw.

OK, I think I see what you were  trying to fix now.  Unfortunately
I think this patch doesn't quite cure  the problem.

First of all you can't sleep in snmp6_unregister_dev so  semaphores
are out.  More importantly, the race is still on.

Here  is what  happens:

CPU0                     CPU1
ifdown eth0
...
ifup  eth0
snmp6_register_dev
adds proc entry
in6_dev_finish_destroy
snmp6_unregister_dev
deletes new  proc entry

The next ifdown may fail because snmp6_unregister_dev will  retrieve the
name from a proc entry that's already been deleted.

I see  two solutions:

1) Unregister the proc entry earlier.  In other  words, do it in
addrconf_ifdown.  Since this is highly serialised it  means that
we can't add the new proc entry before the old proc entry  has
been deleted.

2) Fix procfs so that we delete by pointer instead  of name.  This
makes sense from a semantic pointer of view.   However, for this
particular instance it means that we may have two "eth0"  entries
for as long as the old idev entry sticks around.

Cheers,
--  
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu  ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page:  http://gondor.apana.org.au/~herbert/
PGP Key:  http://gondor.apana.org.au/~herbert/pubkey.txt  

Fw: Frequent Oops on Shutdown 2.6.10

[Andrew Morton]

Begin forwarded message:

Date: Sun, 20 Feb 2005 09:56:04 EST
From: AndyLiebman@aol.com
To: linux-kernel@vger.kernel.org
Subject: Frequent Oops on Shutdown 2.6.10


Hi, 
I compiled the 2.6.10 kernel with HyperThreading optimization (I'm  running a 
3.06 Ghz single Xeon processor with HT enabled). More or less, I'm  running 
Mandrake 10 Official, but with my own kernel. Can anybody help explain  why I'm 
getting this Oops on shutdown? It doesn't happen all the time -- about  50 
percent of the time. Never happens with the 2.6.6 kernel. I configured the  
2.6.10 kernel with mostly the same settings -- saying "no" to everything new,  
except optimizing for P4/Xeon processor, enabling HT optimization, and NOT  
enabling lots of "ham radio" and "ISDN" stuff that seemed to be enabled in the  
Mandrake kernel. 
 
Some relevant things about the machine: 
Single Xeon 3.06 processor
2 GB ECC RAM
2x 3ware 9500S-8 SATA RAID Cards
16 SATA drives
2 built-in GigE ports on motherboard
2 Intel 1000 MT Server Adapters on each of the two 133 Mhz  PCI-X slots
 
Here's the output from the Oops


*pde = 00000000
Oops:  0000 [#1]
SMP
Modules linked in: raid) appletalk xfs sd_mod sg sr_mod  3w_9xxx scsi_mod 
nfsd exportfs ipv6 af_packet raw ide_floppy ide_tape ide_cd  cdrom e1000 uhci_hcd 
usbcore rtc ext3 jbd
CPU: 1
EIP:  0060:[<c018b600>] Not tainted VLI
EFLAGS: 00010246  (2.6.10es-feb06)
EIP is at remove_proc_entry+0x2a/0x166
eax: 00000000 ebx:  f66a4e00 ecx: ffffffff edx: f6da1300
esi: f7cfb000 edi: 00000005 ebp:  c2183eb4 esp: c2183e94
ds: 007b es: 007b ss: 0068
Process swapper (pid: 0,  threadinfo=c2182000 task=c214e520)
Stack: c043402c c043402c 00000000 c2024980  00000005 f66a4e00 f7cfb000 
00000000 c2183ec8 f8c4f051 00000005 f6da1300 f66a4e00  c2183ee8 f8c2cc7b f66a4e00 
c2024980 00000002 00000000 f6da7e80 f6da6080 c2183f04  c0289967 f66a4e00
Call Trace: 
[<c0103352>]  show_stack+0xaf/0xb7
[<c01034d9>]  show_registers+0x15f/0x1d2
[<c01036dd>]  die+0xfa/0x180
[<c011365e>]  do_page_fault+0x464/0x646
[<c0102fbf>]  error_code+0x2b/0x30
[<f8c4f051>] snmp6_unregister_dev+0x41/0x57  [ipv6]
[<f8c2cc7b>] in6_dev_finish_destroy+0x35/0xb6  [ipv6]
[<c0289967>] dst_destroy+0xa2/0xcd
[<c028969a>]  dst_run_gc+0x72/0xfb
[<c0123584>]  run_timer_softirq+0xc4/0x185
[<c011f631>]  __do_softirq+0x65/0xd3
[<c011f6d0>]  do_softirq+0x31/0x33
[<c0102f18>]  apic_timer_interrupt+0x1c/0x24
[<c0100747>]  cpu_idle+0x31/0x3f
[<00000000>] 0x0
[<c2183fbc>]  0xc2183fbc
Code: e2 55 89 ef 83 ec 20 8b 55 0c 8b 4d 08 89 5d f4 85 d2 89 75  f8 89 7d 
fc 89 4d f0 0f 84 b0 00 00 00 8b 7d f0 31 c0 b9 ff ff ff ff <f2>  ae f7 d1 49 
8b 42 34 8d 5a 34 85 c0 89 ce 0f 84 84 00 00 00 
<0>Kernel  panic ___ not syncing: Fatal exception in interrupt
 

Re: Frequent Oops on Shutdown 2.6.10

[YOSHIFUJI Hideaki / 吉藤英明]

Hello.

In article <20050220220304.1f2bd42f.akpm@osdl.org> (at Sun, 20 Feb 2005 22:03:04 -0800), Andrew Morton <akpm@osdl.org> says:

> EIP is at remove_proc_entry+0x2a/0x166
:
> [<f8c4f051>] snmp6_unregister_dev+0x41/0x57  [ipv6]
> [<f8c2cc7b>] in6_dev_finish_destroy+0x35/0xb6  [ipv6]
> [<c0289967>] dst_destroy+0xa2/0xcd

Would you test this patch, please?
Thanks.

------
[IPV6] Don't remove dev_snmp6 procfs entry until all users gone.

Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>

===== net/ipv6/proc.c 1.25 vs edited =====
--- 1.25/net/ipv6/proc.c	2004-07-08 07:17:29 +09:00
+++ edited/net/ipv6/proc.c	2005-02-21 16:05:49 +09:00
@@ -31,7 +31,9 @@
 #include <net/ipv6.h>
 
 #ifdef CONFIG_PROC_FS
-static struct proc_dir_entry *proc_net_devsnmp6;
+struct proc_dir_entry *proc_net_devsnmp6;
+static DECLARE_MUTEX(proc_net_devsnmp6_sem);
+static unsigned int proc_net_devsnmp6_users;
 
 static int fold_prot_inuse(struct proto *proto)
 {
@@ -211,13 +213,26 @@
 			   __alignof__(struct icmpv6_mib)) < 0)
 		goto err_icmp;
 
-	if (!proc_net_devsnmp6) {
-		err = -ENOENT;
-		goto err_proc;
+	down(&proc_net_devsnmp6_sem);
+	if (!proc_net_devsnmp6_users) {
+		proc_net_devsnmp6 = proc_mkdir("dev_snmp6", proc_net);
+		if (!proc_net_devsnmp6) {
+			err = -ENOMEM;
+			printk(KERN_ERR "%s(): failed to create dev_snmp6.\n",
+			       __FUNCTION__);
+			goto err_proc;
+		}
 	}
 	p = create_proc_entry(idev->dev->name, S_IRUGO, proc_net_devsnmp6);
-	if (!p)
+	if (!p) {
+		if (!proc_net_devsnmp6_users)
+			proc_net_remove("dev_snmp6");
 		goto err_proc;
+	}
+
+	proc_net_devsnmp6_users++;
+	up(&proc_net_devsnmp6_sem);
+
 	p->data = idev;
 	p->proc_fops = &snmp6_seq_fops;
 
@@ -225,6 +240,7 @@
 	return 0;
 
 err_proc:
+	up(&proc_net_devsnmp6_sem);
 	snmp6_mib_free((void **)idev->stats.icmpv6);
 err_icmp:
 	return err;
@@ -232,12 +248,23 @@
 
 int snmp6_unregister_dev(struct inet6_dev *idev)
 {
-	if (!proc_net_devsnmp6)
+	down(&proc_net_devsnmp6_sem);
+	if (!proc_net_devsnmp6) {
+		up(&proc_net_devsnmp6_sem);
 		return -ENOENT;
-	if (!idev || !idev->stats.proc_dir_entry)
+	}
+	if (!idev || !idev->stats.proc_dir_entry) {
+		up(&proc_net_devsnmp6_sem);
 		return -EINVAL;
+	}
 	remove_proc_entry(idev->stats.proc_dir_entry->name,
 			  proc_net_devsnmp6);
+	if (!--proc_net_devsnmp6_users) {
+		proc_net_remove("dev_snmp6");
+		proc_net_devsnmp6 = NULL;
+	}
+	up(&proc_net_devsnmp6_sem);
+
 	snmp6_mib_free((void **)idev->stats.icmpv6);
 
 	return 0;
@@ -250,18 +277,12 @@
 	if (!proc_net_fops_create("snmp6", S_IRUGO, &snmp6_seq_fops))
 		goto proc_snmp6_fail;
 
-	proc_net_devsnmp6 = proc_mkdir("dev_snmp6", proc_net);
-	if (!proc_net_devsnmp6)
-		goto proc_dev_snmp6_fail;
-
 	if (!proc_net_fops_create("sockstat6", S_IRUGO, &sockstat6_seq_fops))
 		goto proc_sockstat6_fail;
 out:
 	return rc;
 
 proc_sockstat6_fail:
-	proc_net_remove("dev_snmp6");
-proc_dev_snmp6_fail:
 	proc_net_remove("snmp6");
 proc_snmp6_fail:
 	rc = -ENOMEM;
@@ -271,7 +292,6 @@
 void ipv6_misc_proc_exit(void)
 {
 	proc_net_remove("sockstat6");
-	proc_net_remove("dev_snmp6");
 	proc_net_remove("snmp6");
 }
 

-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: Frequent Oops on Shutdown 2.6.10

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20050221.162241.24618885.yoshfuji@linux-ipv6.org> (at Mon, 21 Feb 2005 16:22:41 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org> says:

> [IPV6] Don't remove dev_snmp6 procfs entry until all users gone.
> 
> Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
> 
> ===== net/ipv6/proc.c 1.25 vs edited =====
> --- 1.25/net/ipv6/proc.c	2004-07-08 07:17:29 +09:00
> +++ edited/net/ipv6/proc.c	2005-02-21 16:05:49 +09:00
:
> -	if (!p)
> +	if (!p) {
> +		if (!proc_net_devsnmp6_users)
> +			proc_net_remove("dev_snmp6");
>  		goto err_proc;
> +	}

Oops, sorry, I made a mistake here.
Please try this instead.

Thanks.

------
[IPV6] Don't remove dev_snmp6 procfs entry until all users gone.

Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>

===== net/ipv6/proc.c 1.25 vs edited =====
--- 1.25/net/ipv6/proc.c	2004-07-08 07:17:29 +09:00
+++ edited/net/ipv6/proc.c	2005-02-21 16:23:57 +09:00
@@ -31,7 +31,9 @@
 #include <net/ipv6.h>
 
 #ifdef CONFIG_PROC_FS
-static struct proc_dir_entry *proc_net_devsnmp6;
+struct proc_dir_entry *proc_net_devsnmp6;
+static DECLARE_MUTEX(proc_net_devsnmp6_sem);
+static unsigned int proc_net_devsnmp6_users;
 
 static int fold_prot_inuse(struct proto *proto)
 {
@@ -211,13 +213,28 @@
 			   __alignof__(struct icmpv6_mib)) < 0)
 		goto err_icmp;
 
-	if (!proc_net_devsnmp6) {
-		err = -ENOENT;
-		goto err_proc;
+	down(&proc_net_devsnmp6_sem);
+	if (!proc_net_devsnmp6_users) {
+		proc_net_devsnmp6 = proc_mkdir("dev_snmp6", proc_net);
+		if (!proc_net_devsnmp6) {
+			err = -ENOMEM;
+			printk(KERN_ERR "%s(): failed to create dev_snmp6.\n",
+			       __FUNCTION__);
+			goto err_proc;
+		}
 	}
 	p = create_proc_entry(idev->dev->name, S_IRUGO, proc_net_devsnmp6);
-	if (!p)
+	if (!p) {
+		if (!proc_net_devsnmp6_users) {
+			proc_net_remove("dev_snmp6");
+			proc_net_devsnmp6 = NULL;
+		}
 		goto err_proc;
+	}
+
+	proc_net_devsnmp6_users++;
+	up(&proc_net_devsnmp6_sem);
+
 	p->data = idev;
 	p->proc_fops = &snmp6_seq_fops;
 
@@ -225,6 +242,7 @@
 	return 0;
 
 err_proc:
+	up(&proc_net_devsnmp6_sem);
 	snmp6_mib_free((void **)idev->stats.icmpv6);
 err_icmp:
 	return err;
@@ -232,12 +250,23 @@
 
 int snmp6_unregister_dev(struct inet6_dev *idev)
 {
-	if (!proc_net_devsnmp6)
+	down(&proc_net_devsnmp6_sem);
+	if (!proc_net_devsnmp6) {
+		up(&proc_net_devsnmp6_sem);
 		return -ENOENT;
-	if (!idev || !idev->stats.proc_dir_entry)
+	}
+	if (!idev || !idev->stats.proc_dir_entry) {
+		up(&proc_net_devsnmp6_sem);
 		return -EINVAL;
+	}
 	remove_proc_entry(idev->stats.proc_dir_entry->name,
 			  proc_net_devsnmp6);
+	if (!--proc_net_devsnmp6_users) {
+		proc_net_remove("dev_snmp6");
+		proc_net_devsnmp6 = NULL;
+	}
+	up(&proc_net_devsnmp6_sem);
+
 	snmp6_mib_free((void **)idev->stats.icmpv6);
 
 	return 0;
@@ -250,18 +279,12 @@
 	if (!proc_net_fops_create("snmp6", S_IRUGO, &snmp6_seq_fops))
 		goto proc_snmp6_fail;
 
-	proc_net_devsnmp6 = proc_mkdir("dev_snmp6", proc_net);
-	if (!proc_net_devsnmp6)
-		goto proc_dev_snmp6_fail;
-
 	if (!proc_net_fops_create("sockstat6", S_IRUGO, &sockstat6_seq_fops))
 		goto proc_sockstat6_fail;
 out:
 	return rc;
 
 proc_sockstat6_fail:
-	proc_net_remove("dev_snmp6");
-proc_dev_snmp6_fail:
 	proc_net_remove("snmp6");
 proc_snmp6_fail:
 	rc = -ENOMEM;
@@ -271,7 +294,6 @@
 void ipv6_misc_proc_exit(void)
 {
 	proc_net_remove("sockstat6");
-	proc_net_remove("dev_snmp6");
 	proc_net_remove("snmp6");
 }
 

-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: Frequent Oops on Shutdown 2.6.10

[Herbert Xu]

YOSHIFUJI Hideaki / ???? <yoshfuji@linux-ipv6.org> wrote:
> In article <20050221.162241.24618885.yoshfuji@linux-ipv6.org> (at Mon, 21 Feb 2005 16:22:41 +0900 (JST)), YOSHIFUJI Hideaki / ???? <yoshfuji@linux-ipv6.org> says:
> 
>> [IPV6] Don't remove dev_snmp6 procfs entry until all users gone.

Sorry, but I don't see how this patch explains the oops the
people saw.

I googled for this oops and found three distinct occurences.  They
share the property that idev->stats.proc_dir_entry->name contains
a bogus value.  In two cases it had 0x5 and 0xa respectively, in
the other it had 0x61696265.

The last one could be an important clue.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Frequent Oops on Shutdown 2.6.10

[Herbert Xu]

On Tue, Feb 22, 2005 at 08:57:19PM +1100, Herbert Xu wrote:
> YOSHIFUJI Hideaki / ???? <yoshfuji@linux-ipv6.org> wrote:
> > In article <20050221.162241.24618885.yoshfuji@linux-ipv6.org> (at Mon, 21 Feb 2005 16:22:41 +0900 (JST)), YOSHIFUJI Hideaki / ???? <yoshfuji@linux-ipv6.org> says:
> > 
> >> [IPV6] Don't remove dev_snmp6 procfs entry until all users gone.
> 
> Sorry, but I don't see how this patch explains the oops the
> people saw.

OK, I think I see what you were trying to fix now.  Unfortunately
I think this patch doesn't quite cure the problem.

First of all you can't sleep in snmp6_unregister_dev so semaphores
are out.  More importantly, the race is still on.

Here is what happens:

CPU0					CPU1
ifdown eth0
	...
					ifup eth0
						snmp6_register_dev
							adds proc entry
in6_dev_finish_destroy
	snmp6_unregister_dev
		deletes new proc entry

The next ifdown may fail because snmp6_unregister_dev will retrieve the
name from a proc entry that's already been deleted.

I see two solutions:

1) Unregister the proc entry earlier.  In other words, do it in
addrconf_ifdown.  Since this is highly serialised it means that
we can't add the new proc entry before the old proc entry has
been deleted.

2) Fix procfs so that we delete by pointer instead of name.  This
makes sense from a semantic pointer of view.  However, for this
particular instance it means that we may have two "eth0" entries
for as long as the old idev entry sticks around.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Frequent Oops on Shutdown 2.6.10

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20050222101526.GA5814@gondor.apana.org.au> (at Tue, 22 Feb 2005 21:15:26 +1100), Herbert Xu <herbert@gondor.apana.org.au> says:

> On Tue, Feb 22, 2005 at 08:57:19PM +1100, Herbert Xu wrote:
> > YOSHIFUJI Hideaki / ???? <yoshfuji@linux-ipv6.org> wrote:
> > > In article <20050221.162241.24618885.yoshfuji@linux-ipv6.org> (at Mon, 21 Feb 2005 16:22:41 +0900 (JST)), YOSHIFUJI Hideaki / ???? <yoshfuji@linux-ipv6.org> says:
> > > 
> > >> [IPV6] Don't remove dev_snmp6 procfs entry until all users gone.
> > 
> > Sorry, but I don't see how this patch explains the oops the
> > people saw.
:
> I see two solutions:
> 
> 1) Unregister the proc entry earlier.  In other words, do it in
> addrconf_ifdown.  Since this is highly serialised it means that
> we can't add the new proc entry before the old proc entry has
> been deleted.

Okay, it sounds reasonable.

What do you think of this?

Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>

===== include/net/ipv6.h 1.42 vs edited =====
--- 1.42/include/net/ipv6.h	2005-01-15 06:30:07 +09:00
+++ edited/include/net/ipv6.h	2005-02-23 17:10:38 +09:00
@@ -149,6 +149,8 @@
 
 int snmp6_register_dev(struct inet6_dev *idev);
 int snmp6_unregister_dev(struct inet6_dev *idev);
+int snmp6_alloc_dev(struct inet6_dev *idev);
+int snmp6_free_dev(struct inet6_dev *idev);
 int snmp6_mib_init(void *ptr[2], size_t mibsize, size_t mibalign);
 void snmp6_mib_free(void *ptr[2]);
 
===== net/ipv6/addrconf.c 1.129 vs edited =====
--- 1.129/net/ipv6/addrconf.c	2005-01-18 06:13:31 +09:00
+++ edited/net/ipv6/addrconf.c	2005-02-23 17:59:56 +09:00
@@ -308,7 +308,7 @@
 		printk("Freeing alive inet6 device %p\n", idev);
 		return;
 	}
-	snmp6_unregister_dev(idev);
+	snmp6_free_dev(idev);
 	kfree(idev);
 }
 
@@ -339,6 +339,16 @@
 		/* We refer to the device */
 		dev_hold(dev);
 
+		if (snmp6_alloc_dev(ndev) < 0) {
+			ADBG((KERN_WARNING
+				"%s(): cannot allocate memory for statistics; dev=%s.\n",
+				__FUNCTION__, dev->name));
+			neigh_parms_release(&nd_tbl, ndev->nd_parms);
+			ndev->dead = 1;
+			in6_dev_finish_destroy(ndev);
+			return NULL;
+		}
+
 		if (snmp6_register_dev(ndev) < 0) {
 			ADBG((KERN_WARNING
 				"%s(): cannot create /proc/net/dev_snmp6/%s\n",
@@ -2013,6 +2023,10 @@
 		dev->ip6_ptr = NULL;
 		idev->dead = 1;
 		write_unlock_bh(&addrconf_lock);
+
+		/* Step 1.5: remove snmp6 entry */
+		snmp6_unregister_dev(idev);
+
 	}
 
 	/* Step 2: clear hash table */
===== net/ipv6/proc.c 1.25 vs edited =====
--- 1.25/net/ipv6/proc.c	2004-07-08 07:17:29 +09:00
+++ edited/net/ipv6/proc.c	2005-02-23 18:05:27 +09:00
@@ -201,33 +201,23 @@
 
 int snmp6_register_dev(struct inet6_dev *idev)
 {
-	int err = -ENOMEM;
 	struct proc_dir_entry *p;
 
 	if (!idev || !idev->dev)
 		return -EINVAL;
 
-	if (snmp6_mib_init((void **)idev->stats.icmpv6, sizeof(struct icmpv6_mib),
-			   __alignof__(struct icmpv6_mib)) < 0)
-		goto err_icmp;
+	if (!proc_net_devsnmp6)
+		return -ENOENT;
 
-	if (!proc_net_devsnmp6) {
-		err = -ENOENT;
-		goto err_proc;
-	}
 	p = create_proc_entry(idev->dev->name, S_IRUGO, proc_net_devsnmp6);
 	if (!p)
-		goto err_proc;
+		return -ENOMEM;
+
 	p->data = idev;
 	p->proc_fops = &snmp6_seq_fops;
 
 	idev->stats.proc_dir_entry = p;
 	return 0;
-
-err_proc:
-	snmp6_mib_free((void **)idev->stats.icmpv6);
-err_icmp:
-	return err;
 }
 
 int snmp6_unregister_dev(struct inet6_dev *idev)
@@ -238,8 +228,6 @@
 		return -EINVAL;
 	remove_proc_entry(idev->stats.proc_dir_entry->name,
 			  proc_net_devsnmp6);
-	snmp6_mib_free((void **)idev->stats.icmpv6);
-
 	return 0;
 }
 
@@ -274,12 +262,21 @@
 	proc_net_remove("dev_snmp6");
 	proc_net_remove("snmp6");
 }
-
 #else	/* CONFIG_PROC_FS */
 
-
 int snmp6_register_dev(struct inet6_dev *idev)
 {
+	return 0;
+}
+
+int snmp6_unregister_dev(struct inet6_dev *idev)
+{
+	return 0;
+}
+#endif	/* CONFIG_PROC_FS */
+
+int snmp6_alloc_dev(struct inet6_dev *idev)
+{
 	int err = -ENOMEM;
 
 	if (!idev || !idev->dev)
@@ -295,11 +292,10 @@
 	return err;
 }
 
-int snmp6_unregister_dev(struct inet6_dev *idev)
+int snmp6_free_dev(struct inet6_dev *idev)
 {
 	snmp6_mib_free((void **)idev->stats.icmpv6);
 	return 0;
 }
 
-#endif
 

-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: Frequent Oops on Shutdown 2.6.10

[Herbert Xu]

On Wed, Feb 23, 2005 at 06:35:55PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> 
> What do you think of this?

Thanks, this looks great.  There is just one technical detail
to patch up.
 
> -int snmp6_unregister_dev(struct inet6_dev *idev)
> +int snmp6_free_dev(struct inet6_dev *idev)
>  {
>  	snmp6_mib_free((void **)idev->stats.icmpv6);
>  	return 0;
>  }

You need to check whether icmpv6[0] is NULL either here or in
snmp6_mib_free.  Otherwise when snmp6_alloc_dev fails we'll
wind up here and then call free_percpu on a pair of NULL pointers.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Frequent Oops on Shutdown 2.6.10

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20050223095144.GB16820@gondor.apana.org.au> (at Wed, 23 Feb 2005 20:51:44 +1100), Herbert Xu <herbert@gondor.apana.org.au> says:

> You need to check whether icmpv6[0] is NULL either here or in
> snmp6_mib_free.  Otherwise when snmp6_alloc_dev fails we'll
> wind up here and then call free_percpu on a pair of NULL pointers.

Ok, right...

Here's the (hopefuly) final one.


Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>

===== include/net/ipv6.h 1.42 vs edited =====
--- 1.42/include/net/ipv6.h	2005-01-15 06:30:07 +09:00
+++ edited/include/net/ipv6.h	2005-02-23 17:10:38 +09:00
@@ -149,6 +149,8 @@
 
 int snmp6_register_dev(struct inet6_dev *idev);
 int snmp6_unregister_dev(struct inet6_dev *idev);
+int snmp6_alloc_dev(struct inet6_dev *idev);
+int snmp6_free_dev(struct inet6_dev *idev);
 int snmp6_mib_init(void *ptr[2], size_t mibsize, size_t mibalign);
 void snmp6_mib_free(void *ptr[2]);
 
===== net/ipv6/addrconf.c 1.129 vs edited =====
--- 1.129/net/ipv6/addrconf.c	2005-01-18 06:13:31 +09:00
+++ edited/net/ipv6/addrconf.c	2005-02-23 17:59:56 +09:00
@@ -308,7 +308,7 @@
 		printk("Freeing alive inet6 device %p\n", idev);
 		return;
 	}
-	snmp6_unregister_dev(idev);
+	snmp6_free_dev(idev);
 	kfree(idev);
 }
 
@@ -339,6 +339,16 @@
 		/* We refer to the device */
 		dev_hold(dev);
 
+		if (snmp6_alloc_dev(ndev) < 0) {
+			ADBG((KERN_WARNING
+				"%s(): cannot allocate memory for statistics; dev=%s.\n",
+				__FUNCTION__, dev->name));
+			neigh_parms_release(&nd_tbl, ndev->nd_parms);
+			ndev->dead = 1;
+			in6_dev_finish_destroy(ndev);
+			return NULL;
+		}
+
 		if (snmp6_register_dev(ndev) < 0) {
 			ADBG((KERN_WARNING
 				"%s(): cannot create /proc/net/dev_snmp6/%s\n",
@@ -2013,6 +2023,10 @@
 		dev->ip6_ptr = NULL;
 		idev->dead = 1;
 		write_unlock_bh(&addrconf_lock);
+
+		/* Step 1.5: remove snmp6 entry */
+		snmp6_unregister_dev(idev);
+
 	}
 
 	/* Step 2: clear hash table */
===== net/ipv6/af_inet6.c 1.88 vs edited =====
--- 1.88/net/ipv6/af_inet6.c	2005-01-14 13:41:05 +09:00
+++ edited/net/ipv6/af_inet6.c	2005-02-24 00:58:11 +09:00
@@ -652,8 +652,10 @@
 {
 	if (ptr == NULL)
 		return;
-	free_percpu(ptr[0]);
-	free_percpu(ptr[1]);
+	if (ptr[0])
+		free_percpu(ptr[0]);
+	if (ptr[1])
+		free_percpu(ptr[1]);
 	ptr[0] = ptr[1] = NULL;
 }
 
===== net/ipv6/proc.c 1.25 vs edited =====
--- 1.25/net/ipv6/proc.c	2004-07-08 07:17:29 +09:00
+++ edited/net/ipv6/proc.c	2005-02-24 01:00:44 +09:00
@@ -201,33 +201,23 @@
 
 int snmp6_register_dev(struct inet6_dev *idev)
 {
-	int err = -ENOMEM;
 	struct proc_dir_entry *p;
 
 	if (!idev || !idev->dev)
 		return -EINVAL;
 
-	if (snmp6_mib_init((void **)idev->stats.icmpv6, sizeof(struct icmpv6_mib),
-			   __alignof__(struct icmpv6_mib)) < 0)
-		goto err_icmp;
+	if (!proc_net_devsnmp6)
+		return -ENOENT;
 
-	if (!proc_net_devsnmp6) {
-		err = -ENOENT;
-		goto err_proc;
-	}
 	p = create_proc_entry(idev->dev->name, S_IRUGO, proc_net_devsnmp6);
 	if (!p)
-		goto err_proc;
+		return -ENOMEM;
+
 	p->data = idev;
 	p->proc_fops = &snmp6_seq_fops;
 
 	idev->stats.proc_dir_entry = p;
 	return 0;
-
-err_proc:
-	snmp6_mib_free((void **)idev->stats.icmpv6);
-err_icmp:
-	return err;
 }
 
 int snmp6_unregister_dev(struct inet6_dev *idev)
@@ -238,8 +228,6 @@
 		return -EINVAL;
 	remove_proc_entry(idev->stats.proc_dir_entry->name,
 			  proc_net_devsnmp6);
-	snmp6_mib_free((void **)idev->stats.icmpv6);
-
 	return 0;
 }
 
@@ -280,6 +268,17 @@
 
 int snmp6_register_dev(struct inet6_dev *idev)
 {
+	return 0;
+}
+
+int snmp6_unregister_dev(struct inet6_dev *idev)
+{
+	return 0;
+}
+#endif	/* CONFIG_PROC_FS */
+
+int snmp6_alloc_dev(struct inet6_dev *idev)
+{
 	int err = -ENOMEM;
 
 	if (!idev || !idev->dev)
@@ -295,11 +294,10 @@
 	return err;
 }
 
-int snmp6_unregister_dev(struct inet6_dev *idev)
+int snmp6_free_dev(struct inet6_dev *idev)
 {
 	snmp6_mib_free((void **)idev->stats.icmpv6);
 	return 0;
 }
 
-#endif
 


-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: Frequent Oops on Shutdown 2.6.10

[Herbert Xu]

On Thu, Feb 24, 2005 at 01:41:48AM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> 
> Here's the (hopefuly) final one.

Looks good to me.

> Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Frequent Oops on Shutdown 2.6.10

[David S. Miller]

On Thu, 24 Feb 2005 09:48:27 +1100
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Thu, Feb 24, 2005 at 01:41:48AM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> > 
> > Here's the (hopefuly) final one.
> 
> Looks good to me.
> 
> > Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied to 2.6.11, thanks everyone.