Re: Frequent Oops on Shutdown 2.6.10

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

[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  

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

[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

[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

[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

[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

[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

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

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