[PATCH IPv6] Fix race condition in ipv6_add_addr

[PATCH IPv6] Fix race condition in ipv6_add_addr

[Olaf Kirch]

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

Here's a patch originally from Keir Fraser, which we included in SLES10,
but which we forgot to submit upstream so far.

During stress testing, machines were frequently crashing in
__ipv6_ifa_notify on dst_hold(&ifp->rt.u_dst), with ifp->rt being a
NULL pointer.

The attached patch fixes the problem.

Thanks,
Olaf
-- 
Olaf Kirch   |  --- o --- Nous sommes du soleil we love when we play
okir@suse.de |    / | \   sol.dhoop.naytheet.ah kin.ir.samse.qurax

[-- Attachment #2: ipv6-add-addr-install-dstentry --]
[-- Type: text/plain, Size: 1007 bytes --]

From: Keir Fraser <keir.fraser@cl.cam.ac.uk>
Subject: ipv6_add_addr should install dstentry earlier

ipv6_add_addr allocates a struct inet6_ifaddr and a dstentry, but it
doesn't install the dstentry in ifa->rt until after it releases the
addrconf_hash_lock. This means other CPUs will be able to see the new
address while it hasn't been initialized completely yet.

One possible fix would be to grab the ifp->lock spinlock when
creating the address struct; a simpler fix is to just move the
assignment.

Acked-by: jbeulich@novell.com
Acked-by: okir@suse.de

--- linux-2.6.16.13-old/net/ipv6/addrconf.c	2006-05-02 22:38:44.000000000 +0100
+++ linux-2.6.16.13-new/net/ipv6/addrconf.c	2006-06-18 10:16:50.000000000 +0100
@@ -549,6 +549,8 @@
 	ifa->flags = flags | IFA_F_TENTATIVE;
 	ifa->cstamp = ifa->tstamp = jiffies;
 
+	ifa->rt = rt;
+
 	ifa->idev = idev;
 	in6_dev_hold(idev);
 	/* For caller */
@@ -575,8 +577,6 @@
 	}
 #endif
 
-	ifa->rt = rt;
-
 	in6_ifa_hold(ifa);
 	write_unlock(&idev->lock);
 out2:

Re: [PATCH IPv6] Fix race condition in ipv6_add_addr

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20060829083536.GA10403@suse.de> (at Tue, 29 Aug 2006 10:35:36 +0200), Olaf Kirch <okir@suse.de> says:

> During stress testing, machines were frequently crashing in
> __ipv6_ifa_notify on dst_hold(&ifp->rt.u_dst), with ifp->rt being a
> NULL pointer.
> 
> The attached patch fixes the problem.

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

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

Re: [PATCH IPv6] Fix race condition in ipv6_add_addr

[David Miller]

From: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Date: Tue, 29 Aug 2006 18:21:28 +0900 (JST)

> In article <20060829083536.GA10403@suse.de> (at Tue, 29 Aug 2006 10:35:36 +0200), Olaf Kirch <okir@suse.de> says:
> 
> > During stress testing, machines were frequently crashing in
> > __ipv6_ifa_notify on dst_hold(&ifp->rt.u_dst), with ifp->rt being a
> > NULL pointer.
> > 
> > The attached patch fixes the problem.
> 
> Acked-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>

Applied, thanks everyone.

Although the only reason this is a problem is due to the fact that we
use rwlock's here.  If addrconf_lock were a normal spinlock, or taken
as a writer in this code path, the problematic cases would not be
possible.

I guess this is a reminder that I need to revisit my patches which
move all IPV6 address and inet6_dev changes out of software interrupt
context, plus add use of RCU. :-)