[IPV6] addrconf: Fix IPv6 on tuntap tunnels

[IPV6] addrconf: Fix IPv6 on tuntap tunnels

[Herbert Xu]

Hi Dave:

[IPV6] addrconf: Fix IPv6 on tuntap tunnels

The recent patch that added ipv6_hwtype is broken on tuntap tunnels.
Indeed, it's broken on any device that does not pass the ipv6_hwtype
test.

The reason is that the original test only applies to autoconfiguration,
not IPv6 support.  IPv6 support is allowed on any device.  In fact,
even with the ipv6_hwtype patch applied you can still add IPv6 addresses
to any interface that doesn't pass thw ipv6_hwtype test provided that
they have a sufficiently large MTU.  This is a serious problem because
come deregistration time these devices won't be cleaned up properly.

I've gone back and looked at the rationale for the patch.  It appears
that the real problem is that we were creating IPv6 devices even if the
MTU was too small.  So here's a patch which fixes that and reverts the
ipv6_hwtype stuff.

Thanks to Kanru Chen for reporting this issue.

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

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
--
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 5a5f8bd..f96ed76 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2154,6 +2154,15 @@ static void addrconf_dev_config(struct net_device *dev)
 
 	ASSERT_RTNL();
 
+	if ((dev->type != ARPHRD_ETHER) &&
+	    (dev->type != ARPHRD_FDDI) &&
+	    (dev->type != ARPHRD_IEEE802_TR) &&
+	    (dev->type != ARPHRD_ARCNET) &&
+	    (dev->type != ARPHRD_INFINIBAND)) {
+		/* Alas, we support only Ethernet autoconfiguration. */
+		return;
+	}
+
 	idev = addrconf_add_dev(dev);
 	if (idev == NULL)
 		return;
@@ -2241,36 +2250,16 @@ static void addrconf_ip6_tnl_config(struct net_device *dev)
 	ip6_tnl_add_linklocal(idev);
 }
 
-static int ipv6_hwtype(struct net_device *dev)
-{
-	if ((dev->type == ARPHRD_ETHER) ||
-	    (dev->type == ARPHRD_LOOPBACK) ||
-	    (dev->type == ARPHRD_SIT) ||
-	    (dev->type == ARPHRD_TUNNEL6) ||
-	    (dev->type == ARPHRD_FDDI) ||
-	    (dev->type == ARPHRD_IEEE802_TR) ||
-	    (dev->type == ARPHRD_ARCNET) ||
-	    (dev->type == ARPHRD_INFINIBAND))
-		return 1;
-
-	return 0;
-}
-
 static int addrconf_notify(struct notifier_block *this, unsigned long event,
 			   void * data)
 {
 	struct net_device *dev = (struct net_device *) data;
-	struct inet6_dev *idev;
+	struct inet6_dev *idev = __in6_dev_get(dev);
 	int run_pending = 0;
 
-	if (!ipv6_hwtype(dev))
-		return NOTIFY_OK;
-
-	idev = __in6_dev_get(dev);
-
 	switch(event) {
 	case NETDEV_REGISTER:
-		if (!idev) {
+		if (!idev && dev->mtu >= IPV6_MIN_MTU) {
 			idev = ipv6_add_dev(dev);
 			if (!idev)
 				printk(KERN_WARNING "IPv6: add_dev failed for %s\n",

Re: [IPV6] addrconf: Fix IPv6 on tuntap tunnels

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 14 Jun 2007 18:16:07 +1000

> [IPV6] addrconf: Fix IPv6 on tuntap tunnels
> 
> The recent patch that added ipv6_hwtype is broken on tuntap tunnels.
> Indeed, it's broken on any device that does not pass the ipv6_hwtype
> test.
> 
> The reason is that the original test only applies to autoconfiguration,
> not IPv6 support.  IPv6 support is allowed on any device.  In fact,
> even with the ipv6_hwtype patch applied you can still add IPv6 addresses
> to any interface that doesn't pass thw ipv6_hwtype test provided that
> they have a sufficiently large MTU.  This is a serious problem because
> come deregistration time these devices won't be cleaned up properly.
> 
> I've gone back and looked at the rationale for the patch.  It appears
> that the real problem is that we were creating IPv6 devices even if the
> MTU was too small.  So here's a patch which fixes that and reverts the
> ipv6_hwtype stuff.
> 
> Thanks to Kanru Chen for reporting this issue.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks for fixing this up Herbert.

Patch applied.

Re: [IPV6] addrconf: Fix IPv6 on tuntap tunnels

[Oliver Hartkopp]

Hallo David, hello Herbert,

indeed i have some concerns about reverting the patch as i do not see
that the MTU is the right thing to distinguish whether a netdevice is
capable to have IPv4/IPv6. E.g. is decnet able to run IPv6?

IMHO the autoconf (in any case) should only handle netdevices that are
capable to be auto configured (e.g. with IPv6).

So the question looks like:
"Is this device capable to be auto configured with IPv6?"
and not
"Is the devices MTU >= IPV6_MIN_MTU ?"

My original patch showed the way to ask the (IMO) correct question. And
maybe this has to be improved somehow (like Florian Zumbiel suggested).
And also maybe the  Autoconfiguration has to be a part of code that is
independent from net/ipv6. But to ask about the MTU size does not look
the right way to me.

Best regards,
Oliver

David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Thu, 14 Jun 2007 18:16:07 +1000
>
>   
>> [IPV6] addrconf: Fix IPv6 on tuntap tunnels
>>
>> The recent patch that added ipv6_hwtype is broken on tuntap tunnels.
>> Indeed, it's broken on any device that does not pass the ipv6_hwtype
>> test.
>>
>> The reason is that the original test only applies to autoconfiguration,
>> not IPv6 support.  IPv6 support is allowed on any device.  In fact,
>> even with the ipv6_hwtype patch applied you can still add IPv6 addresses
>> to any interface that doesn't pass thw ipv6_hwtype test provided that
>> they have a sufficiently large MTU.  This is a serious problem because
>> come deregistration time these devices won't be cleaned up properly.
>>
>> I've gone back and looked at the rationale for the patch.  It appears
>> that the real problem is that we were creating IPv6 devices even if the
>> MTU was too small.  So here's a patch which fixes that and reverts the
>> ipv6_hwtype stuff.
>>
>> Thanks to Kanru Chen for reporting this issue.
>>
>> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>>     
>
> Thanks for fixing this up Herbert.
>
> Patch applied.
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>   

Re: [IPV6] addrconf: Fix IPv6 on tuntap tunnels

[Vlad Yasevich]

Oliver Hartkopp wrote:
> Hallo David, hello Herbert,
> 
> indeed i have some concerns about reverting the patch as i do not see
> that the MTU is the right thing to distinguish whether a netdevice is
> capable to have IPv4/IPv6. E.g. is decnet able to run IPv6?
> 
> IMHO the autoconf (in any case) should only handle netdevices that are
> capable to be auto configured (e.g. with IPv6).
> 
> So the question looks like:
> "Is this device capable to be auto configured with IPv6?"
> and not
> "Is the devices MTU >= IPV6_MIN_MTU ?"

No, the questions should really be:

1.  Is IPV6 supported over this media type.
	yes: got to 2
	no:  stop

2.  Is the device MTU >= IPV6_MIN_MTU
	yes: continue
	no:  stop

Autoconfiguration is a layer on top of IPv6.  Whether it's enabled
or not should not dictate whether IPv6 addressed may be configured or not.

-vlad

Re: [IPV6] addrconf: Fix IPv6 on tuntap tunnels

[David Miller]

From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Fri, 15 Jun 2007 16:42:22 -0400

> No, the questions should really be:
> 
> 1.  Is IPV6 supported over this media type.
> 	yes: got to 2
> 	no:  stop
> 
> 2.  Is the device MTU >= IPV6_MIN_MTU
> 	yes: continue
> 	no:  stop
> 
> Autoconfiguration is a layer on top of IPv6.  Whether it's enabled
> or not should not dictate whether IPv6 addressed may be configured or not.

Sounds good to me, patches? :-)

Re: [IPV6] addrconf: Fix IPv6 on tuntap tunnels

[Herbert Xu]

On Fri, Jun 15, 2007 at 03:14:57PM -0700, David Miller wrote:
> 
> > No, the questions should really be:
> > 
> > 1.  Is IPV6 supported over this media type.
> > 	yes: got to 2
> > 	no:  stop
> > 
> > 2.  Is the device MTU >= IPV6_MIN_MTU
> > 	yes: continue
> > 	no:  stop
> > 
> > Autoconfiguration is a layer on top of IPv6.  Whether it's enabled
> > or not should not dictate whether IPv6 addressed may be configured or not.
> 
> Sounds good to me, patches? :-)

I don't think we need any more patches since right now MTU >= IPV6_MIN_MTU
is the only condition we require before we allow IPv6 addresses to be added
to an interface.

The original patch simply confused this basic IPv6 address support with
IPv6 autoconfiguration.

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: [IPV6] addrconf: Fix IPv6 on tuntap tunnels

[Vlad Yasevich]

Herbert Xu wrote:
> On Fri, Jun 15, 2007 at 03:14:57PM -0700, David Miller wrote:
>>> No, the questions should really be:
>>>
>>> 1.  Is IPV6 supported over this media type.
>>> 	yes: got to 2
>>> 	no:  stop
>>>
>>> 2.  Is the device MTU >= IPV6_MIN_MTU
>>> 	yes: continue
>>> 	no:  stop
>>>
>>> Autoconfiguration is a layer on top of IPv6.  Whether it's enabled
>>> or not should not dictate whether IPv6 addressed may be configured or not.
>> Sounds good to me, patches? :-)
> 
> I don't think we need any more patches since right now MTU >= IPV6_MIN_MTU
> is the only condition we require before we allow IPv6 addresses to be added
> to an interface.
> 
> The original patch simply confused this basic IPv6 address support with
> IPv6 autoconfiguration.

Looking over the history, Herbert has mostly right.

The only concern I have is that it's currently permitted to configure
IPv6 addresses on interface that would not have link-local addressing
(ex: IEEE 1394 link).

Now, is some circumstances that's ok (tuntap is a perfect example).
In others, particularly where there is an "IPv6 over foo" spec, not so much.

-vlad

Re: [IPV6] addrconf: Fix IPv6 on tuntap tunnels

[Oliver Hartkopp]

David Miller wrote:
> From: Vlad Yasevich <vladislav.yasevich@hp.com>
> Date: Fri, 15 Jun 2007 16:42:22 -0400
>
>   
>> No, the questions should really be:
>>
>> 1.  Is IPV6 supported over this media type.
>> 	yes: got to 2
>> 	no:  stop
>>
>> 2.  Is the device MTU >= IPV6_MIN_MTU
>> 	yes: continue
>> 	no:  stop
>>
>> Autoconfiguration is a layer on top of IPv6.  Whether it's enabled
>> or not should not dictate whether IPv6 addressed may be configured or not.
>>     
>
> Sounds good to me, patches? :-)
>   

Yes, to me it sounds also very good.
I'm not that deep in IPv6 and i do not know about all the media types
that support IPv6, e.g. if a whitelist (as i suggested) is the better
way - or a blacklist.
So i would just support the request from David for patches :-)

Best regards,
Oliver