Bug in ipv6_ifa_notify?

Bug in ipv6_ifa_notify?

[Herbert Xu]

Hi:

I'm reviewing the changes between 2.6.8.1 and 2.6.9.  The following
change caught my eye:

# ChangeSet
#   2004/08/17 11:25:16+09:00 yoshfuji@linux-ipv6.org
#   [IPV6] refer inet6 device via corresponding local route from address structure.

In particular, it changed the handling of RTM_NEWADDR in ipv6_ifa_notify.
Previously if you received duplicate RTM_NEWADDR notifications
ip6_rt_addr_add would allocate a new rt and then free it since
ip6_ins_rt would fail.

With the new code, it will call ip6_ins_rt on the *same* rt
again which will cause it to be dst_free'd.  I don't see any
way for this to lead to dst underflows yet, but it'll certainly
corrupt the routing table since dst_free modifies rt->u.next.

Now the question is is it possible to get dupliate RTM_NEWADDR
notifications?

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: Bug in ipv6_ifa_notify?

[Herbert Xu]

On Mon, Nov 08, 2004 at 05:15:29PM +1100, herbert wrote:
> 
> Now the question is is it possible to get dupliate RTM_NEWADDR
> notifications?

Actually, it's easier than that.

RTM_DELADDR will call ip6_del_rt which will free the rt using
dst_free.  Since it is still referenced it gets put on the gc
list.  The next RTM_NEWADDR will put it back in the table.
>From this point onwards anything that gets linked after it will
be on the gc list!

Jeff/Arnaldo, could you please try shutting down an interface with
IPv6 addresses and bringing it back up a few times? This might be
the trigger we are looking for.

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: Bug in ipv6_ifa_notify?

[Herbert Xu]

On Mon, Nov 08, 2004 at 06:34:41PM +1100, herbert wrote:
> 
> RTM_DELADDR will call ip6_del_rt which will free the rt using
> dst_free.  Since it is still referenced it gets put on the gc
> list.  The next RTM_NEWADDR will put it back in the table.
> From this point onwards anything that gets linked after it will
> be on the gc list!

Actually, address objects dont't seem to be kept across interface
down/up operations, so this probably can't happen...
-- 
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: Bug in ipv6_ifa_notify?

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20041108061529.GA1774@gondor.apana.org.au> (at Mon, 8 Nov 2004 17:15:29 +1100), Herbert Xu <herbert@gondor.apana.org.au> says:

> Hi:
> 
> I'm reviewing the changes between 2.6.8.1 and 2.6.9.  The following
> change caught my eye:
> 
> # ChangeSet
> #   2004/08/17 11:25:16+09:00 yoshfuji@linux-ipv6.org
> #   [IPV6] refer inet6 device via corresponding local route from address structure.
> 
> In particular, it changed the handling of RTM_NEWADDR in ipv6_ifa_notify.
> Previously if you received duplicate RTM_NEWADDR notifications
> ip6_rt_addr_add would allocate a new rt and then free it since
> ip6_ins_rt would fail.
> 
> With the new code, it will call ip6_ins_rt on the *same* rt
> again which will cause it to be dst_free'd.  I don't see any
> way for this to lead to dst underflows yet, but it'll certainly
> corrupt the routing table since dst_free modifies rt->u.next.
> 
> Now the question is is it possible to get dupliate RTM_NEWADDR
> notifications?

It used to (before http://linux.bkbits.net:8080/linux-2.5/cset@41216bc8qWlBbT2LRIeGJegBQQxczg).
But, it does not happen now; RTM_NEWADDR notification is called 
 1. when we has successfully finished DAD
or
 2. if the interface does not need DAD

We've tested simple (but including multiple multiple) up/down case.

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

Re: Bug in ipv6_ifa_notify?

[Herbert Xu]

On Mon, Nov 08, 2004 at 11:10:40PM +1100, herbert wrote:
> On Mon, Nov 08, 2004 at 06:34:41PM +1100, herbert wrote:
> > 
> > RTM_DELADDR will call ip6_del_rt which will free the rt using
> > dst_free.  Since it is still referenced it gets put on the gc
> > list.  The next RTM_NEWADDR will put it back in the table.
> > From this point onwards anything that gets linked after it will
> > be on the gc list!
> 
> Actually, address objects dont't seem to be kept across interface
> down/up operations, so this probably can't happen...

Well I've found a way for this to occur, but I must say that it is
fairly unlikely.  When you bring the interface down, addrconf_ifdown
will try to to delete the addrconf timer and then notify DELADDR.
Since it doesn't wait for the timer to complete, it might still be
executing.  Therefore it is possible to have a NEWADDR after a DELADDR
event.
-- 
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: Bug in ipv6_ifa_notify?

[Herbert Xu]

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

On Tue, Nov 09, 2004 at 07:37:41AM +1100, herbert wrote:
> 
> Well I've found a way for this to occur, but I must say that it is
> fairly unlikely.  When you bring the interface down, addrconf_ifdown
> will try to to delete the addrconf timer and then notify DELADDR.
> Since it doesn't wait for the timer to complete, it might still be
> executing.  Therefore it is possible to have a NEWADDR after a DELADDR
> event.

The attractive thing about this theory is that it is an SMP-only
race.  This could explain why only Jeff/Lennert are seeing it.

Jeff/Lennert, could you please go back to 2.6.9, and then apply
this patch? If this warning triggers, then that would confirm
this theory.

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

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 388 bytes --]

===== net/ipv6/addrconf.c 1.115 vs edited =====
--- 1.115/net/ipv6/addrconf.c	2004-10-26 14:11:35 +10:00
+++ edited/net/ipv6/addrconf.c	2004-11-09 22:44:35 +11:00
@@ -2989,6 +2989,7 @@
 		dst_hold(&ifp->rt->u.dst);
 		if (ip6_ins_rt(ifp->rt, NULL, NULL))
 			dst_release(&ifp->rt->u.dst);
+		WARN_ON(ifp->dead);
 		if (ifp->idev->cnf.forwarding)
 			addrconf_join_anycast(ifp);
 		break;

Re: Bug in ipv6_ifa_notify?

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20041109114839.GA1942@gondor.apana.org.au> (at Tue, 9 Nov 2004 22:48:39 +1100), Herbert Xu <herbert@gondor.apana.org.au> says:

> On Tue, Nov 09, 2004 at 07:37:41AM +1100, herbert wrote:
> > 
> > Well I've found a way for this to occur, but I must say that it is
> > fairly unlikely.  When you bring the interface down, addrconf_ifdown
> > will try to to delete the addrconf timer and then notify DELADDR.
> > Since it doesn't wait for the timer to complete, it might still be
> > executing.  Therefore it is possible to have a NEWADDR after a DELADDR
> > event.
> 
> The attractive thing about this theory is that it is an SMP-only
> race.  This could explain why only Jeff/Lennert are seeing it.
> 
> Jeff/Lennert, could you please go back to 2.6.9, and then apply
> this patch? If this warning triggers, then that would confirm
> this theory.

I think this only happens when you shut down interface while DAD;
for example, "ifup eth0 &; ifdown eth0" or something like that.

--yoshfuji

Re: Bug in ipv6_ifa_notify?

[Herbert Xu]

On Tue, Nov 09, 2004 at 09:57:59AM -0500, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> 
> I think this only happens when you shut down interface while DAD;
> for example, "ifup eth0 &; ifdown eth0" or something like that.

Exactly.  However, since ifup doesn't wait for DAD to complete,
you need something like 'ifup eth0; sleep 3; ifdown eth0' to
trigger it.

BTW, couldn't we move stuff like DAD to user-space?

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: Bug in ipv6_ifa_notify?

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20041109203509.GA5109@gondor.apana.org.au> (at Wed, 10 Nov 2004 07:35:09 +1100), Herbert Xu <herbert@gondor.apana.org.au> says:

> BTW, couldn't we move stuff like DAD to user-space?

I don't think it is the right way.

--yoshfuji