[NET] IPv6 oops bisected

The commit

commit 5f5dace1ce001b24fb8286e09ffd3c4d2b547e09
Author: Denis V. Lunev [off-list ref]
Date:   Thu Sep 27 12:41:26 2007 -0700

     [NET]: Various dst_ifdown routines to catch refcounting bugs

causes a 100% reproducible oops in net-2.6.24.git, in ip6_route_add().

Platform: x86-64 Fedora 7

Config and lspci attached.

Jeff Garzik wrote:

The commit

commit 5f5dace1ce001b24fb8286e09ffd3c4d2b547e09
Author: Denis V. Lunev [off-list ref]
Date:   Thu Sep 27 12:41:26 2007 -0700

    [NET]: Various dst_ifdown routines to catch refcounting bugs

causes a 100% reproducible oops in net-2.6.24.git, in ip6_route_add().

Platform: x86-64 Fedora 7

Config and lspci attached.

And indeed, reverting this commit with 'patch -R' makes things work again.

	Jeff

Jeff Garzik wrote:

Jeff Garzik wrote:

quoted

The commit

commit 5f5dace1ce001b24fb8286e09ffd3c4d2b547e09
Author: Denis V. Lunev [off-list ref]
Date:   Thu Sep 27 12:41:26 2007 -0700

    [NET]: Various dst_ifdown routines to catch refcounting bugs

causes a 100% reproducible oops in net-2.6.24.git, in ip6_route_add().

Platform: x86-64 Fedora 7

Config and lspci attached.

And indeed, reverting this commit with 'patch -R' makes things work again.

    Jeff

Can you me command to execute to reproduce the problem? and oops if
possible.

Regards,
	Den

Denis V. Lunev wrote:

Jeff Garzik wrote:

quoted

Jeff Garzik wrote:

quoted

The commit

commit 5f5dace1ce001b24fb8286e09ffd3c4d2b547e09
Author: Denis V. Lunev [off-list ref]
Date:   Thu Sep 27 12:41:26 2007 -0700

    [NET]: Various dst_ifdown routines to catch refcounting bugs

causes a 100% reproducible oops in net-2.6.24.git, in ip6_route_add().

Platform: x86-64 Fedora 7

Config and lspci attached.

And indeed, reverting this commit with 'patch -R' makes things work again.

    Jeff

Can you me command to execute to reproduce the problem? and oops if
possible.

The command is Fedora 7 initscripts...

(typing in by hand)

/etc/sysconfig/network-scripts/network-functions-ipv6: line 246: 1760 Killed
	LC_ALL=C /sbin/ip $options


NULL pointer dereference at 0x000003f8

backtrace:
:ipv6:ip6_route_add+0x1b1/0x543
rtnetlink_rcv_msg
inet6_rtm_newroute
netlink_run_queue
rtnetlink_rcv
netlink_data_ready
netlink_sendskb
netlink_sendmsg
sock_send_msg
mntput_no_expire
autoremove_wake_function
find_lock_page
zone_statistics
get_page_from_freelist
__alloc_pages
move_addr_to_kernel
verify_iovec
sys_sendmsg

From: Jeff Garzik <redacted>
Date: Sun, 07 Oct 2007 10:56:07 -0400

/etc/sysconfig/network-scripts/network-functions-ipv6: line 246: 1760 Killed
	LC_ALL=C /sbin/ip $options


NULL pointer dereference at 0x000003f8

backtrace:
:ipv6:ip6_route_add+0x1b1/0x543

'dev' can be NULL in that code branch of ip6_route_add(),
yet we're deferencing it to get dev->nd_net.

 	if ((cfg->fc_flags & RTF_REJECT) ||
 	    (dev && (dev->flags&IFF_LOOPBACK) && !(addr_type&IPV6_ADDR_LOOPBACK))) {
 		/* hold loopback dev/idev if we haven't done so. */
-		if (dev != init_net.loopback_dev) {
+		if (dev != dev->nd_net->loopback_dev) {

I'll add the appropriate check for NULL as follows:

commit b3c1427c21f9bac4ceaa02e875f3b2c9a5592132
Author: David S. Miller [off-list ref]
Date:   Sun Oct 7 23:15:56 2007 -0700

    [IPV6]: Fix OOPS introduced by 5f5dace1ce001b24fb8286e09ffd3c4d2b547e09.
    
    In ip6_add_route(), 'dev' can be NULL, so check that before
    we try to deref dev->nd_net.
    
    Based upon a crash report by Jeff Garzik.
    
    Signed-off-by: David S. Miller [off-list ref]

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index a7db84c..7109ad6 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c

@@ -1188,7 +1188,7 @@ int ip6_route_add(struct fib6_config *cfg)
 	if ((cfg->fc_flags & RTF_REJECT) ||
 	    (dev && (dev->flags&IFF_LOOPBACK) && !(addr_type&IPV6_ADDR_LOOPBACK))) {
 		/* hold loopback dev/idev if we haven't done so. */
-		if (dev != dev->nd_net->loopback_dev) {
+		if (!dev || (dev != dev->nd_net->loopback_dev)) {
 			if (dev) {
 				dev_put(dev);
 				in6_dev_put(idev);

On Sun, Oct 07, 2007 at 11:16:08PM -0700, David Miller wrote:

Unfortunately this'll still oops a few lines down when it tries
to assign dev->nd_net->loopabck_dev to dev.  The issue here is
which namespace are we in if dev is NULL.

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

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 8 Oct 2007 14:19:42 +0800

On Sun, Oct 07, 2007 at 11:16:08PM -0700, David Miller wrote:

quoted

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index a7db84c..7109ad6 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c

@@ -1188,7 +1188,7 @@ int ip6_route_add(struct fib6_config *cfg)
 	if ((cfg->fc_flags & RTF_REJECT) ||
 	    (dev && (dev->flags&IFF_LOOPBACK) && !(addr_type&IPV6_ADDR_LOOPBACK))) {
 		/* hold loopback dev/idev if we haven't done so. */
-		if (dev != dev->nd_net->loopback_dev) {
+		if (!dev || (dev != dev->nd_net->loopback_dev)) {
 			if (dev) {
 				dev_put(dev);
 				in6_dev_put(idev);

Unfortunately this'll still oops a few lines down when it tries
to assign dev->nd_net->loopabck_dev to dev.  The issue here is
which namespace are we in if dev is NULL.

Good catch.

I'm just going to revert my bogus fix and the original change
for now.  Denis can resubmit the original patch once this
is resolved.

David Miller wrote:

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 8 Oct 2007 14:19:42 +0800

quoted

On Sun, Oct 07, 2007 at 11:16:08PM -0700, David Miller wrote:

quoted

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index a7db84c..7109ad6 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c

@@ -1188,7 +1188,7 @@ int ip6_route_add(struct fib6_config *cfg)
 	if ((cfg->fc_flags & RTF_REJECT) ||
 	    (dev && (dev->flags&IFF_LOOPBACK) && !(addr_type&IPV6_ADDR_LOOPBACK))) {
 		/* hold loopback dev/idev if we haven't done so. */
-		if (dev != dev->nd_net->loopback_dev) {
+		if (!dev || (dev != dev->nd_net->loopback_dev)) {
 			if (dev) {
 				dev_put(dev);
 				in6_dev_put(idev);

Unfortunately this'll still oops a few lines down when it tries
to assign dev->nd_net->loopabck_dev to dev.  The issue here is
which namespace are we in if dev is NULL.

Good catch.

I'm just going to revert my bogus fix and the original change
for now.  Denis can resubmit the original patch once this
is resolved.

OK. I am installing Fedora 7 right now...

From: "Denis V. Lunev" <redacted>
Date: Mon, 08 Oct 2007 10:34:23 +0400

OK. I am installing Fedora 7 right now...

You don't need to install Fedora, just read the code! :-)

The bug is obvious and it's been explained thoroughly in this
thread.

When 'dev' is NULL in ip6_route_add() we need to figure out what
namespace and/or loopback device you want to use.

There is no reason to do an entire dist install to work on fixing this
bug, yikes!

David Miller wrote:

From: "Denis V. Lunev" <redacted>
Date: Mon, 08 Oct 2007 10:34:23 +0400

quoted

OK. I am installing Fedora 7 right now...

You don't need to install Fedora, just read the code! :-)

The bug is obvious and it's been explained thoroughly in this
thread.

When 'dev' is NULL in ip6_route_add() we need to figure out what
namespace and/or loopback device you want to use.

There is no reason to do an entire dist install to work on fixing this
bug, yikes!

I do understand the conditions when the bug happens. Its completely
clear :) Though I do not understand how to trigger it from command line
to test that the problem is resolved. Jeff was not kind enough to give
exact command line :(

The unfortunate thing with this place is that original Eric's code is
also broken here. I was too optimistic working on the original patchset.
In other way, but broken :(

So, I must stop and think...