[PATCH net-next] ipv6: addrconf: fix mcast route for GRE devices

[PATCH net-next] ipv6: addrconf: fix mcast route for GRE devices

[David Lamparter]

GRE devices, for some reason, were coming up with an autoconfigured
address, but no ff00::/8 route in the local table.  This breaks any kind
of multicast, in particular OSPFv3, mDNS, - and ND.  In fact, IPv6 only
works at all because there is little need for ND on PtP devices.

Adding any other IPv6 address on the device would rectify this issue
through inet6_addr_add()/addrconf_add_dev() - and would leave the route
around even if the address was later removed.  (This is probably why
this issue was not discovered earlier.  AFAICS it has been there from
the beginning, e.g. aee80b5 "generate link local address for GRE
tunnel")

(Note: multicast is supported on GRE devices of all kinds, including PtP
GRE, P-t-Mcast GRE and NBMA-GRE.)

Fixes: aee80b54b235 (ipv6: generate link local address for GRE tunnel)
Signed-off-by: David Lamparter <equinox@diac24.net>
Cc: Stephen Hemminger <stephen@networkplumber.org>
---
 net/ipv6/addrconf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 0b239fc..655a0df 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2824,6 +2824,7 @@ static void addrconf_gre_config(struct net_device *dev)
 	}
 
 	addrconf_addr_gen(idev, true);
+	addrconf_add_mroute(dev);
 }
 #endif
 
-- 
1.8.5.5

Re: [PATCH net-next] ipv6: addrconf: fix mcast route for GRE devices

[Hannes Frederic Sowa]

Hi,

On Mi, 2014-07-30 at 02:55 +0200, David Lamparter wrote:
> GRE devices, for some reason, were coming up with an autoconfigured
> address, but no ff00::/8 route in the local table.  This breaks any kind
> of multicast, in particular OSPFv3, mDNS, - and ND.  In fact, IPv6 only
> works at all because there is little need for ND on PtP devices.
> 
> Adding any other IPv6 address on the device would rectify this issue
> through inet6_addr_add()/addrconf_add_dev() - and would leave the route
> around even if the address was later removed.  (This is probably why
> this issue was not discovered earlier.  AFAICS it has been there from
> the beginning, e.g. aee80b5 "generate link local address for GRE
> tunnel")

Yep, this is poor, but changing this will break user space...

> (Note: multicast is supported on GRE devices of all kinds, including PtP
> GRE, P-t-Mcast GRE and NBMA-GRE.)
> 
> Fixes: aee80b54b235 (ipv6: generate link local address for GRE tunnel)
> Signed-off-by: David Lamparter <equinox@diac24.net>
> Cc: Stephen Hemminger <stephen@networkplumber.org>

We should install routes before joining LL, I would go with

        idev = addrconf_add_dev(dev);
        if (IS_ERR(idev))
                return;

in addrconf_gre_config etc.

Looks like we don't correctly configure ARPHRD_IP6GRE gre tunnels...

> ---
>  net/ipv6/addrconf.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 0b239fc..655a0df 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -2824,6 +2824,7 @@ static void addrconf_gre_config(struct net_device *dev)
>  	}
>  
>  	addrconf_addr_gen(idev, true);
> +	addrconf_add_mroute(dev);
>  }
>  #endif
>  

Thanks,
Hannes

Re: [PATCH net-next] ipv6: addrconf: fix mcast route for GRE devices

[David Lamparter]

(New patch following in separate mail - I haven't touched
addrconf_sit_config since I have no clue about ISATAP & the likes.)

On Wed, Jul 30, 2014 at 05:14:42PM +0200, Hannes Frederic Sowa wrote:
> On Mi, 2014-07-30 at 02:55 +0200, David Lamparter wrote:
> > GRE devices, for some reason, were coming up with an autoconfigured
> > address, but no ff00::/8 route in the local table.  This breaks any kind
> > of multicast, in particular OSPFv3, mDNS, - and ND.  In fact, IPv6 only
> > works at all because there is little need for ND on PtP devices.
> > 
> > Adding any other IPv6 address on the device would rectify this issue
> > through inet6_addr_add()/addrconf_add_dev() - and would leave the route
> > around even if the address was later removed.  (This is probably why
> > this issue was not discovered earlier.  AFAICS it has been there from
> > the beginning, e.g. aee80b5 "generate link local address for GRE
> > tunnel")
> 
> Yep, this is poor, but changing this will break user space...

How exactly will this break user space?

> > (Note: multicast is supported on GRE devices of all kinds, including PtP
> > GRE, P-t-Mcast GRE and NBMA-GRE.)
> > 
> > Fixes: aee80b54b235 (ipv6: generate link local address for GRE tunnel)
> > Signed-off-by: David Lamparter <equinox@diac24.net>
> > Cc: Stephen Hemminger <stephen@networkplumber.org>
> 
> We should install routes before joining LL, I would go with
> 
>         idev = addrconf_add_dev(dev);
>         if (IS_ERR(idev))
>                 return;
> 
> in addrconf_gre_config etc.

Well, yeah, to be honest I have no clue why these functions are
separated as they are.  Once fixed up, the only difference between
addrconf_gre_config and addrconf_dev_config is the true/false value on
prefix_route for addrconf_addr_gen...  and what the rationale for that
is, I can only consult an oracle for.

How about pushing the switch(dev->type) down from addrconf_notify into
addrconf_dev_config()?

> Looks like we don't correctly configure ARPHRD_IP6GRE gre tunnels...

IP6GRE doesn't currently auto-generate a link-local address at all, is
that what you meant?  (Yeah we could/should add that too...)


-David

[PATCH v2] ipv6: addrconf: fix mcast route for GRE devices

[David Lamparter]

GRE devices, for some reason, were coming up with an autoconfigured
address, but no ff00::/8 route in the local table.  This breaks any kind
of multicast, in particular OSPFv3, mDNS, - and ND.  In fact, IPv6 only
works at all because there is little need for ND on PtP devices.

Adding any other IPv6 address on the device from userspace would rectify
this issue through inet6_addr_add()/addrconf_add_dev() - and would leave
the route around even if the address was later removed.  (This is
probably why this issue was not discovered earlier.  AFAICS it has been
there from the beginning, e.g. aee80b5 "generate link local address for
GRE tunnel")

(Note: multicast is supported on GRE devices of all kinds, including PtP
GRE, P-t-Mcast GRE and NBMA-GRE.)

Fixes: aee80b54b235 (ipv6: generate link local address for GRE tunnel)
Signed-off-by: David Lamparter <equinox@diac24.net>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Cc: Stephen Hemminger <stephen@networkplumber.org>
---
 net/ipv6/addrconf.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 0b239fc..7540a25 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2818,10 +2818,9 @@ static void addrconf_gre_config(struct net_device *dev)
 
 	ASSERT_RTNL();
 
-	if ((idev = ipv6_find_idev(dev)) == NULL) {
-		pr_debug("%s: add_dev failed\n", __func__);
+	idev = addrconf_add_dev(dev);
+	if (IS_ERR(idev))
 		return;
-	}
 
 	addrconf_addr_gen(idev, true);
 }
-- 
1.8.5.5

Re: [PATCH v2] ipv6: addrconf: fix mcast route for GRE devices

[David Miller]

From: David Lamparter <equinox@diac24.net>
Date: Wed, 30 Jul 2014 17:39:57 +0200

> GRE devices, for some reason, were coming up with an autoconfigured
> address, but no ff00::/8 route in the local table.  This breaks any kind
> of multicast, in particular OSPFv3, mDNS, - and ND.  In fact, IPv6 only
> works at all because there is little need for ND on PtP devices.
> 
> Adding any other IPv6 address on the device from userspace would rectify
> this issue through inet6_addr_add()/addrconf_add_dev() - and would leave
> the route around even if the address was later removed.  (This is
> probably why this issue was not discovered earlier.  AFAICS it has been
> there from the beginning, e.g. aee80b5 "generate link local address for
> GRE tunnel")
> 
> (Note: multicast is supported on GRE devices of all kinds, including PtP
> GRE, P-t-Mcast GRE and NBMA-GRE.)
> 
> Fixes: aee80b54b235 (ipv6: generate link local address for GRE tunnel)
> Signed-off-by: David Lamparter <equinox@diac24.net>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Cc: Stephen Hemminger <stephen@networkplumber.org>

Hannes, are you OK with this version?  Please ACK if so.

Thanks.

Re: [PATCH v2] ipv6: addrconf: fix mcast route for GRE devices

[Hannes Frederic Sowa]

On Do, 2014-07-31 at 12:06 -0700, David Miller wrote:
> From: David Lamparter <equinox@diac24.net>
> Date: Wed, 30 Jul 2014 17:39:57 +0200
> 
> > GRE devices, for some reason, were coming up with an autoconfigured
> > address, but no ff00::/8 route in the local table.  This breaks any kind
> > of multicast, in particular OSPFv3, mDNS, - and ND.  In fact, IPv6 only
> > works at all because there is little need for ND on PtP devices.
> > 
> > Adding any other IPv6 address on the device from userspace would rectify
> > this issue through inet6_addr_add()/addrconf_add_dev() - and would leave
> > the route around even if the address was later removed.  (This is
> > probably why this issue was not discovered earlier.  AFAICS it has been
> > there from the beginning, e.g. aee80b5 "generate link local address for
> > GRE tunnel")
> > 
> > (Note: multicast is supported on GRE devices of all kinds, including PtP
> > GRE, P-t-Mcast GRE and NBMA-GRE.)
> > 
> > Fixes: aee80b54b235 (ipv6: generate link local address for GRE tunnel)
> > Signed-off-by: David Lamparter <equinox@diac24.net>
> > Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > Cc: Stephen Hemminger <stephen@networkplumber.org>
> 
> Hannes, are you OK with this version?  Please ACK if so.

The alternate version does provide the same semantic change and makes
the code more readable. Both changes are technically correct, I acked
the alternate version already.

Thank you,
Hannes

Re: [PATCH v2] ipv6: addrconf: fix mcast route for GRE devices

[David Lamparter]

On Thu, Jul 31, 2014 at 09:37:34PM +0200, Hannes Frederic Sowa wrote:
> On Do, 2014-07-31 at 12:06 -0700, David Miller wrote:
> > From: David Lamparter <equinox@diac24.net>
> > Date: Wed, 30 Jul 2014 17:39:57 +0200
> > 
> > > GRE devices, for some reason, were coming up with an autoconfigured
> > > address, but no ff00::/8 route in the local table.  This breaks any kind
> > > of multicast, in particular OSPFv3, mDNS, - and ND.  In fact, IPv6 only
> > > works at all because there is little need for ND on PtP devices.
> > > 
> > > Adding any other IPv6 address on the device from userspace would rectify
> > > this issue through inet6_addr_add()/addrconf_add_dev() - and would leave
> > > the route around even if the address was later removed.  (This is
> > > probably why this issue was not discovered earlier.  AFAICS it has been
> > > there from the beginning, e.g. aee80b5 "generate link local address for
> > > GRE tunnel")
> > > 
> > > (Note: multicast is supported on GRE devices of all kinds, including PtP
> > > GRE, P-t-Mcast GRE and NBMA-GRE.)
> > > 
> > > Fixes: aee80b54b235 (ipv6: generate link local address for GRE tunnel)
> > > Signed-off-by: David Lamparter <equinox@diac24.net>
> > > Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > 
> > Hannes, are you OK with this version?  Please ACK if so.
> 
> The alternate version does provide the same semantic change and makes
> the code more readable. Both changes are technically correct, I acked
> the alternate version already.

Give me a few min, I'll go base the other one on this one, that way
there's one 2-line patch changing semantics (this one) and another
50-line patch just mechanically moving stuff around.

-David

[PATCH v3] ipv6: addrconf: fix mcast route for GRE devices

[David Lamparter]

This series replaces the previous patches in this thread, reposting them as
- patch 1/2: "ipv6: addrconf: fix mcast route for GRE devices"
which contains the semantic changes for the ff00::/8 route on GRE, and
- patch 2/2:  ipv6: addrconf: clean up device type handling"
which just moves things to make them prettier, and documents the "speshul"
fe80::/64 behaviour (which Nicolas Dichtel added in 08b4465 to make a specific
user case work.)

Tested by poking various devices with a bunch of link ups and downs.

Hannes, can you Ack?

-David

[PATCH 1/2] ipv6: addrconf: fix mcast route for GRE devices

[David Lamparter]

GRE devices, for some reason, were coming up with an autoconfigured
address, but no ff00::/8 route in the local table.  This breaks any kind
of multicast, in particular OSPFv3, mDNS, - and ND.  In fact, IPv6 only
works at all because there is little need for ND on PtP devices.

Adding any other IPv6 address on the device from userspace would rectify
this issue through inet6_addr_add()/addrconf_add_dev() - and would leave
the route around even if the address was later removed.  (This is
probably why this issue was not discovered earlier.  AFAICS it has been
there from the beginning, e.g. aee80b5 "generate link local address for
GRE tunnel")

(Note: multicast is supported on GRE devices of all kinds, including PtP
GRE, P-t-Mcast GRE and NBMA-GRE.)

Fixes: aee80b54b235 (ipv6: generate link local address for GRE tunnel)
Signed-off-by: David Lamparter <equinox@diac24.net>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/ipv6/addrconf.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 0b239fc..7540a25 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2818,10 +2818,9 @@ static void addrconf_gre_config(struct net_device *dev)
 
 	ASSERT_RTNL();
 
-	if ((idev = ipv6_find_idev(dev)) == NULL) {
-		pr_debug("%s: add_dev failed\n", __func__);
+	idev = addrconf_add_dev(dev);
+	if (IS_ERR(idev))
 		return;
-	}
 
 	addrconf_addr_gen(idev, true);
 }
-- 
1.8.5.5

Re: [PATCH 1/2] ipv6: addrconf: fix mcast route for GRE devices

[Hannes Frederic Sowa]

Hi David,

Next time please also specify which tree you want this patch applied to.
We use [PATCH net] for patches which should go into the rc2+ and [PATCH
net-next] for rc1. Thanks!

On Do, 2014-07-31 at 22:53 +0200, David Lamparter wrote:
> GRE devices, for some reason, were coming up with an autoconfigured
> address, but no ff00::/8 route in the local table.  This breaks any kind
> of multicast, in particular OSPFv3, mDNS, - and ND.  In fact, IPv6 only
> works at all because there is little need for ND on PtP devices.
> 
> Adding any other IPv6 address on the device from userspace would rectify
> this issue through inet6_addr_add()/addrconf_add_dev() - and would leave
> the route around even if the address was later removed.  (This is
> probably why this issue was not discovered earlier.  AFAICS it has been
> there from the beginning, e.g. aee80b5 "generate link local address for
> GRE tunnel")
> 
> (Note: multicast is supported on GRE devices of all kinds, including PtP
> GRE, P-t-Mcast GRE and NBMA-GRE.)
> 
> Fixes: aee80b54b235 (ipv6: generate link local address for GRE tunnel)
> Signed-off-by: David Lamparter <equinox@diac24.net>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Dave, it would make sense to queue this up for net and stable (or only
net-next if there is no merge before the release).

Thanks,
Hannes

[PATCH 2/2] ipv6: addrconf: clean up device type handling

[David Lamparter]

This realigns addrconf support for the various lower-layer device types,
and removes a little bit of duplicate code.  Behaviour should be exactly
identical before and after the patch.

(Also adds a comment to explain the weird fe80::/64 on GRE.)

Signed-off-by: David Lamparter <equinox@diac24.net>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/ipv6/addrconf.c | 87 ++++++++++++++++++++++++-----------------------------
 1 file changed, 39 insertions(+), 48 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 7540a25..aa17ee6 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2756,31 +2756,6 @@ static void addrconf_addr_gen(struct inet6_dev *idev, bool prefix_route)
 	}
 }
 
-static void addrconf_dev_config(struct net_device *dev)
-{
-	struct inet6_dev *idev;
-
-	ASSERT_RTNL();
-
-	if ((dev->type != ARPHRD_ETHER) &&
-	    (dev->type != ARPHRD_FDDI) &&
-	    (dev->type != ARPHRD_ARCNET) &&
-	    (dev->type != ARPHRD_INFINIBAND) &&
-	    (dev->type != ARPHRD_IEEE802154) &&
-	    (dev->type != ARPHRD_IEEE1394) &&
-	    (dev->type != ARPHRD_TUNNEL6) &&
-	    (dev->type != ARPHRD_6LOWPAN)) {
-		/* Alas, we support only Ethernet autoconfiguration. */
-		return;
-	}
-
-	idev = addrconf_add_dev(dev);
-	if (IS_ERR(idev))
-		return;
-
-	addrconf_addr_gen(idev, false);
-}
-
 #if IS_ENABLED(CONFIG_IPV6_SIT)
 static void addrconf_sit_config(struct net_device *dev)
 {
@@ -2811,20 +2786,54 @@ static void addrconf_sit_config(struct net_device *dev)
 }
 #endif
 
-#if IS_ENABLED(CONFIG_NET_IPGRE)
-static void addrconf_gre_config(struct net_device *dev)
+static void addrconf_dev_config(struct net_device *dev)
 {
 	struct inet6_dev *idev;
+	bool prefix_route;
 
 	ASSERT_RTNL();
 
+	switch (dev->type) {
+	case ARPHRD_LOOPBACK:
+		init_loopback(dev);
+		return;
+
+	case ARPHRD_ETHER:
+	case ARPHRD_FDDI:
+	case ARPHRD_ARCNET:
+	case ARPHRD_INFINIBAND:
+	case ARPHRD_IEEE802154:
+	case ARPHRD_IEEE1394:
+	case ARPHRD_TUNNEL6:
+	case ARPHRD_6LOWPAN:
+		prefix_route = false;
+		break;
+
+#if IS_ENABLED(CONFIG_NET_IPGRE)
+	case ARPHRD_IPGRE:
+		/* XXX: This special-case behaviour is there to have a
+		 * fe80::/64 route around even if no link-local address gets
+		 * generated, which happens when gre's local == 0.0.0.0.
+		 * See commit 08b4465 for more information. */
+		prefix_route = true;
+		break;
+#endif
+#if IS_ENABLED(CONFIG_IPV6_SIT)
+	case ARPHRD_SIT:
+		addrconf_sit_config(dev);
+		return;
+#endif
+	default:
+		/* No support autoconfiguration on this type */
+		return;
+	}
+
 	idev = addrconf_add_dev(dev);
 	if (IS_ERR(idev))
 		return;
 
-	addrconf_addr_gen(idev, true);
+	addrconf_addr_gen(idev, prefix_route);
 }
-#endif
 
 static int addrconf_notify(struct notifier_block *this, unsigned long event,
 			   void *ptr)
@@ -2882,25 +2891,7 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 			run_pending = 1;
 		}
 
-		switch (dev->type) {
-#if IS_ENABLED(CONFIG_IPV6_SIT)
-		case ARPHRD_SIT:
-			addrconf_sit_config(dev);
-			break;
-#endif
-#if IS_ENABLED(CONFIG_NET_IPGRE)
-		case ARPHRD_IPGRE:
-			addrconf_gre_config(dev);
-			break;
-#endif
-		case ARPHRD_LOOPBACK:
-			init_loopback(dev);
-			break;
-
-		default:
-			addrconf_dev_config(dev);
-			break;
-		}
+		addrconf_dev_config(dev);
 
 		if (!IS_ERR_OR_NULL(idev)) {
 			if (run_pending)
-- 
1.8.5.5

Re: [PATCH 2/2] ipv6: addrconf: clean up device type handling

[Hannes Frederic Sowa]

On Do, 2014-07-31 at 22:53 +0200, David Lamparter wrote:
> This realigns addrconf support for the various lower-layer device types,
> and removes a little bit of duplicate code.  Behaviour should be exactly
> identical before and after the patch.
> 
> (Also adds a comment to explain the weird fe80::/64 on GRE.)
> 
> Signed-off-by: David Lamparter <equinox@diac24.net>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  net/ipv6/addrconf.c | 87 ++++++++++++++++++++++++-----------------------------
>  1 file changed, 39 insertions(+), 48 deletions(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 7540a25..aa17ee6 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -2756,31 +2756,6 @@ static void addrconf_addr_gen(struct inet6_dev *idev, bool prefix_route)
>  	}
>  }
>  
> -static void addrconf_dev_config(struct net_device *dev)
> -{
> -	struct inet6_dev *idev;
> -
> -	ASSERT_RTNL();
> -
> -	if ((dev->type != ARPHRD_ETHER) &&
> -	    (dev->type != ARPHRD_FDDI) &&
> -	    (dev->type != ARPHRD_ARCNET) &&
> -	    (dev->type != ARPHRD_INFINIBAND) &&
> -	    (dev->type != ARPHRD_IEEE802154) &&
> -	    (dev->type != ARPHRD_IEEE1394) &&
> -	    (dev->type != ARPHRD_TUNNEL6) &&
> -	    (dev->type != ARPHRD_6LOWPAN)) {
> -		/* Alas, we support only Ethernet autoconfiguration. */
> -		return;
> -	}
> -
> -	idev = addrconf_add_dev(dev);
> -	if (IS_ERR(idev))
> -		return;
> -
> -	addrconf_addr_gen(idev, false);
> -}
> -
>  #if IS_ENABLED(CONFIG_IPV6_SIT)
>  static void addrconf_sit_config(struct net_device *dev)
>  {
> @@ -2811,20 +2786,54 @@ static void addrconf_sit_config(struct net_device *dev)
>  }
>  #endif
>  
> -#if IS_ENABLED(CONFIG_NET_IPGRE)
> -static void addrconf_gre_config(struct net_device *dev)
> +static void addrconf_dev_config(struct net_device *dev)
>  {
>  	struct inet6_dev *idev;
> +	bool prefix_route;
>  
>  	ASSERT_RTNL();
>  
> +	switch (dev->type) {
> +	case ARPHRD_LOOPBACK:
> +		init_loopback(dev);
> +		return;
> +
> +	case ARPHRD_ETHER:
> +	case ARPHRD_FDDI:
> +	case ARPHRD_ARCNET:
> +	case ARPHRD_INFINIBAND:
> +	case ARPHRD_IEEE802154:
> +	case ARPHRD_IEEE1394:
> +	case ARPHRD_TUNNEL6:
> +	case ARPHRD_6LOWPAN:
> +		prefix_route = false;
> +		break;
> +
> +#if IS_ENABLED(CONFIG_NET_IPGRE)
> +	case ARPHRD_IPGRE:
> +		/* XXX: This special-case behaviour is there to have a
> +		 * fe80::/64 route around even if no link-local address gets
> +		 * generated, which happens when gre's local == 0.0.0.0.
> +		 * See commit 08b4465 for more information. */
> +		prefix_route = true;
> +		break;
> +#endif

I don't see a need for the macro protection any more. We can drop this
now.

David, can you respin this series without the macros and maybe add the

"case ARPHRD_IP6GRE:" stuff

as a separate patch (which should also go into net/stable). If you don't
want to do this, I also can take care of this, no problem.

You can then repost this one separate and state in the comment section
that this cleanup patch depends on the other two, so Dave can merge it
accordingly to net and net-next?

Otherwise this patch is fine, thanks!

Re: [PATCH 2/2] ipv6: addrconf: clean up device type handling

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Fri, 01 Aug 2014 00:13:35 +0200

> David, can you respin this series without the macros and maybe add the
> 
> "case ARPHRD_IP6GRE:" stuff
> 
> as a separate patch (which should also go into net/stable). If you don't
> want to do this, I also can take care of this, no problem.
> 
> You can then repost this one separate and state in the comment section
> that this cleanup patch depends on the other two, so Dave can merge it
> accordingly to net and net-next?

Indeed, please respin this series making the target tree(s) more
explicit, thanks.

[RFC alternate] ipv6: addrconf: clean up device type handling

[David Lamparter]

This realigns addrconf support for the various lower-layer device types,
and removes a little bit of duplicate code.

For GRE devices, this includes a semantic change in that there is now a
ff00::/8 route installed on address autogeneration.  This was previously
missing and broke any kind of IPv6 multicast - unless another address
was configured from userspace (which then added the missing ff00::/8).

Fixes: aee80b54b235 (ipv6: generate link local address for GRE tunnel)
Signed-off-by: David Lamparter <equinox@diac24.net>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Jiri Pirko <jiri@resnulli.us>
---

This is an alternate version, yanking the switch() down and removing
dev_config/gre_config duplication.  I have no idea what rationale is behind
prefix_route - the result is a fe80::/64 route, but no address, which is not a
functioning configuration.  Jiri, you touched this just a few weeks ago, can
you comment?  (The "XXX: why is GRE special?")


 net/ipv6/addrconf.c | 87 +++++++++++++++++++++++------------------------------
 1 file changed, 37 insertions(+), 50 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 0b239fc..45746f7 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2756,31 +2756,6 @@ static void addrconf_addr_gen(struct inet6_dev *idev, bool prefix_route)
 	}
 }
 
-static void addrconf_dev_config(struct net_device *dev)
-{
-	struct inet6_dev *idev;
-
-	ASSERT_RTNL();
-
-	if ((dev->type != ARPHRD_ETHER) &&
-	    (dev->type != ARPHRD_FDDI) &&
-	    (dev->type != ARPHRD_ARCNET) &&
-	    (dev->type != ARPHRD_INFINIBAND) &&
-	    (dev->type != ARPHRD_IEEE802154) &&
-	    (dev->type != ARPHRD_IEEE1394) &&
-	    (dev->type != ARPHRD_TUNNEL6) &&
-	    (dev->type != ARPHRD_6LOWPAN)) {
-		/* Alas, we support only Ethernet autoconfiguration. */
-		return;
-	}
-
-	idev = addrconf_add_dev(dev);
-	if (IS_ERR(idev))
-		return;
-
-	addrconf_addr_gen(idev, false);
-}
-
 #if IS_ENABLED(CONFIG_IPV6_SIT)
 static void addrconf_sit_config(struct net_device *dev)
 {
@@ -2811,21 +2786,51 @@ static void addrconf_sit_config(struct net_device *dev)
 }
 #endif
 
-#if IS_ENABLED(CONFIG_NET_IPGRE)
-static void addrconf_gre_config(struct net_device *dev)
+static void addrconf_dev_config(struct net_device *dev)
 {
 	struct inet6_dev *idev;
+	bool prefix_route;
 
 	ASSERT_RTNL();
 
-	if ((idev = ipv6_find_idev(dev)) == NULL) {
-		pr_debug("%s: add_dev failed\n", __func__);
+	switch (dev->type) {
+	case ARPHRD_LOOPBACK:
+		init_loopback(dev);
+		return;
+
+	case ARPHRD_ETHER:
+	case ARPHRD_FDDI:
+	case ARPHRD_ARCNET:
+	case ARPHRD_INFINIBAND:
+	case ARPHRD_IEEE802154:
+	case ARPHRD_IEEE1394:
+	case ARPHRD_TUNNEL6:
+	case ARPHRD_6LOWPAN:
+		prefix_route = false;
+		break;
+
+#if IS_ENABLED(CONFIG_NET_IPGRE)
+	case ARPHRD_IPGRE:
+		/* XXX: why is GRE special? */
+		prefix_route = true;
+		break;
+#endif
+#if IS_ENABLED(CONFIG_IPV6_SIT)
+	case ARPHRD_SIT:
+		addrconf_sit_config(dev);
+		return;
+#endif
+	default:
+		/* No support autoconfiguration on this type */
 		return;
 	}
 
-	addrconf_addr_gen(idev, true);
+	idev = addrconf_add_dev(dev);
+	if (IS_ERR(idev))
+		return;
+
+	addrconf_addr_gen(idev, prefix_route);
 }
-#endif
 
 static int addrconf_notify(struct notifier_block *this, unsigned long event,
 			   void *ptr)
@@ -2883,25 +2888,7 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 			run_pending = 1;
 		}
 
-		switch (dev->type) {
-#if IS_ENABLED(CONFIG_IPV6_SIT)
-		case ARPHRD_SIT:
-			addrconf_sit_config(dev);
-			break;
-#endif
-#if IS_ENABLED(CONFIG_NET_IPGRE)
-		case ARPHRD_IPGRE:
-			addrconf_gre_config(dev);
-			break;
-#endif
-		case ARPHRD_LOOPBACK:
-			init_loopback(dev);
-			break;
-
-		default:
-			addrconf_dev_config(dev);
-			break;
-		}
+		addrconf_dev_config(dev);
 
 		if (!IS_ERR_OR_NULL(idev)) {
 			if (run_pending)
-- 
1.8.5.5

Re: [RFC alternate] ipv6: addrconf: clean up device type handling

[Hannes Frederic Sowa]

On Mi, 2014-07-30 at 17:58 +0200, David Lamparter wrote:
> This realigns addrconf support for the various lower-layer device types,
> and removes a little bit of duplicate code.
> 
> For GRE devices, this includes a semantic change in that there is now a
> ff00::/8 route installed on address autogeneration.  This was previously
> missing and broke any kind of IPv6 multicast - unless another address
> was configured from userspace (which then added the missing ff00::/8).
> 
> Fixes: aee80b54b235 (ipv6: generate link local address for GRE tunnel)
> Signed-off-by: David Lamparter <equinox@diac24.net>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Jiri Pirko <jiri@resnulli.us>
> ---
> 
> This is an alternate version, yanking the switch() down and removing
> dev_config/gre_config duplication.  I have no idea what rationale is behind
> prefix_route - the result is a fe80::/64 route, but no address, which is not a
> functioning configuration.  Jiri, you touched this just a few weeks ago, can
> you comment?  (The "XXX: why is GRE special?")

Sure, it is valid. You can still use global addresses to talk to link
local addresses on the same link, even from another interface.

I prefer this patch.

Thanks,
Hannes

Re: [RFC alternate] ipv6: addrconf: clean up device type handling

[David Lamparter]

On Wed, Jul 30, 2014 at 06:12:35PM +0200, Hannes Frederic Sowa wrote:
> On Mi, 2014-07-30 at 17:58 +0200, David Lamparter wrote:
[cut]
> > This is an alternate version, yanking the switch() down and removing
> > dev_config/gre_config duplication.  I have no idea what rationale is behind
> > prefix_route - the result is a fe80::/64 route, but no address, which is not a
> > functioning configuration.  Jiri, you touched this just a few weeks ago, can
> > you comment?  (The "XXX: why is GRE special?")
> 
> Sure, it is valid. You can still use global addresses to talk to link
> local addresses on the same link, even from another interface.

Okay, well, that may give some purpose to it, but doesn't really explain
why GRE is special in this regard...

(And it's a violation of RFC4291 section 2.1 - "All interfaces are
required to have at least one Link-Local unicast address" - and I'd bet
on ndisc doing weird things in a setup like that.)


-David

Re: [RFC alternate] ipv6: addrconf: clean up device type handling

[Hannes Frederic Sowa]

On Mi, 2014-07-30 at 18:23 +0200, David Lamparter wrote:
> On Wed, Jul 30, 2014 at 06:12:35PM +0200, Hannes Frederic Sowa wrote:
> > On Mi, 2014-07-30 at 17:58 +0200, David Lamparter wrote:
> [cut]
> > > This is an alternate version, yanking the switch() down and removing
> > > dev_config/gre_config duplication.  I have no idea what rationale is behind
> > > prefix_route - the result is a fe80::/64 route, but no address, which is not a
> > > functioning configuration.  Jiri, you touched this just a few weeks ago, can
> > > you comment?  (The "XXX: why is GRE special?")
> > 
> > Sure, it is valid. You can still use global addresses to talk to link
> > local addresses on the same link, even from another interface.
> 
> Okay, well, that may give some purpose to it, but doesn't really explain
> why GRE is special in this regard...
> 
> (And it's a violation of RFC4291 section 2.1 - "All interfaces are
> required to have at least one Link-Local unicast address" - and I'd bet
> on ndisc doing weird things in a setup like that.)

Yep - sure. But we also allow someone to remove the ll address manually.
And people did do that to transition the interface into an
disable_ipv6=1 mode, which had its own problems.

We don't care if the user does rfc compliant configurations. :)
The settings in the kernel should just be reasonable.

Re: [RFC alternate] ipv6: addrconf: clean up device type handling

[Hannes Frederic Sowa]

On Mi, 2014-07-30 at 17:58 +0200, David Lamparter wrote:
> This realigns addrconf support for the various lower-layer device types,
> and removes a little bit of duplicate code.
> 
> For GRE devices, this includes a semantic change in that there is now a
> ff00::/8 route installed on address autogeneration.  This was previously
> missing and broke any kind of IPv6 multicast - unless another address
> was configured from userspace (which then added the missing ff00::/8).
> 
> Fixes: aee80b54b235 (ipv6: generate link local address for GRE tunnel)
> Signed-off-by: David Lamparter <equinox@diac24.net>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Jiri Pirko <jiri@resnulli.us>
> ---
> 
> This is an alternate version, yanking the switch() down and removing
> dev_config/gre_config duplication.  I have no idea what rationale is behind
> prefix_route - the result is a fe80::/64 route, but no address, which is not a
> functioning configuration.  Jiri, you touched this just a few weeks ago, can
> you comment?  (The "XXX: why is GRE special?")

IIRC some time ago it was decided that randomizing the perm_addr and
generate LL addresses based on that is not the way to go and fragile. We
have this behaviour for ipv6 tunnels, but (IIRC) should not be expanded
and leave user space with this burden.

> -#if IS_ENABLED(CONFIG_NET_IPGRE)
> -static void addrconf_gre_config(struct net_device *dev)
> +static void addrconf_dev_config(struct net_device *dev)
>  {
>  	struct inet6_dev *idev;
> +	bool prefix_route;
>  
>  	ASSERT_RTNL();
>  
> -	if ((idev = ipv6_find_idev(dev)) == NULL) {
> -		pr_debug("%s: add_dev failed\n", __func__);
> +	switch (dev->type) {
> +	case ARPHRD_LOOPBACK:
> +		init_loopback(dev);
> +		return;
> +
> +	case ARPHRD_ETHER:
> +	case ARPHRD_FDDI:
> +	case ARPHRD_ARCNET:
> +	case ARPHRD_INFINIBAND:
> +	case ARPHRD_IEEE802154:
> +	case ARPHRD_IEEE1394:
> +	case ARPHRD_TUNNEL6:
> +	case ARPHRD_6LOWPAN:
> +		prefix_route = false;
> +		break;
> +
> +#if IS_ENABLED(CONFIG_NET_IPGRE)
> +	case ARPHRD_IPGRE:

We can now also add ARPHRD_IP6GRE, maybe in a separate patch.

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Thanks,
Hannes

Re: [PATCH net-next] ipv6: addrconf: fix mcast route for GRE devices

[Hannes Frederic Sowa]

Hi,

On Mi, 2014-07-30 at 17:35 +0200, David Lamparter wrote:
> (New patch following in separate mail - I haven't touched
> addrconf_sit_config since I have no clue about ISATAP & the likes.)

isatap shouldn't use multicast and afaik output path does block
multicast tx. sit in non-ptp mainly depends on 2002:: or 6rd prefix and
drops others packets. sit-ptp can use multicast.

> 
> On Wed, Jul 30, 2014 at 05:14:42PM +0200, Hannes Frederic Sowa wrote:
> > On Mi, 2014-07-30 at 02:55 +0200, David Lamparter wrote:
> > > GRE devices, for some reason, were coming up with an autoconfigured
> > > address, but no ff00::/8 route in the local table.  This breaks any kind
> > > of multicast, in particular OSPFv3, mDNS, - and ND.  In fact, IPv6 only
> > > works at all because there is little need for ND on PtP devices.
> > > 
> > > Adding any other IPv6 address on the device would rectify this issue
> > > through inet6_addr_add()/addrconf_add_dev() - and would leave the route
> > > around even if the address was later removed.  (This is probably why
> > > this issue was not discovered earlier.  AFAICS it has been there from
> > > the beginning, e.g. aee80b5 "generate link local address for GRE
> > > tunnel")
> > 
> > Yep, this is poor, but changing this will break user space...
> 
> How exactly will this break user space?

Because the multicast routes will always be restored after e.g. a route
flush or manual route deletion. Scripts might depend on this.

> > > (Note: multicast is supported on GRE devices of all kinds, including PtP
> > > GRE, P-t-Mcast GRE and NBMA-GRE.)
> > > 
> > > Fixes: aee80b54b235 (ipv6: generate link local address for GRE tunnel)
> > > Signed-off-by: David Lamparter <equinox@diac24.net>
> > > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > 
> > We should install routes before joining LL, I would go with
> > 
> >         idev = addrconf_add_dev(dev);
> >         if (IS_ERR(idev))
> >                 return;
> > 
> > in addrconf_gre_config etc.
> 
> Well, yeah, to be honest I have no clue why these functions are
> separated as they are.  Once fixed up, the only difference between
> addrconf_gre_config and addrconf_dev_config is the true/false value on
> prefix_route for addrconf_addr_gen...  and what the rationale for that
> is, I can only consult an oracle for.

Basically this is a slalom run to keep exisisting behaviour and clean up
the code a little bit.

> How about pushing the switch(dev->type) down from addrconf_notify into
> addrconf_dev_config()?
> 
> > Looks like we don't correctly configure ARPHRD_IP6GRE gre tunnels...
> 
> IP6GRE doesn't currently auto-generate a link-local address at all, is
> that what you meant?  (Yeah we could/should add that too...)

Exactly. 

Thanks,
Hannes

Re: [PATCH net-next] ipv6: addrconf: fix mcast route for GRE devices

[David Lamparter]

On Wed, Jul 30, 2014 at 06:09:27PM +0200, Hannes Frederic Sowa wrote:
[cut]
> > On Wed, Jul 30, 2014 at 05:14:42PM +0200, Hannes Frederic Sowa wrote:
> > > On Mi, 2014-07-30 at 02:55 +0200, David Lamparter wrote:
> > > > GRE devices, for some reason, were coming up with an autoconfigured
> > > > address, but no ff00::/8 route in the local table.  This breaks any kind
> > > > of multicast, in particular OSPFv3, mDNS, - and ND.  In fact, IPv6 only
> > > > works at all because there is little need for ND on PtP devices.
> > > > 
> > > > Adding any other IPv6 address on the device would rectify this issue
> > > > through inet6_addr_add()/addrconf_add_dev() - and would leave the route
> > > > around even if the address was later removed.  (This is probably why
> > > > this issue was not discovered earlier.  AFAICS it has been there from
> > > > the beginning, e.g. aee80b5 "generate link local address for GRE
> > > > tunnel")
> > > 
> > > Yep, this is poor, but changing this will break user space...
> > 
> > How exactly will this break user space?
> 
> Because the multicast routes will always be restored after e.g. a route
> flush or manual route deletion. Scripts might depend on this.

Sorry, I still don't get it.  Without this patch you end up in an
inconsistent state, where a LL addr exists, but multicast doesn't work
(since ff00::/8 is missing from RT6_TABLE_LOCAL).

Userspace is not supposed to touch RT6_TABLE_LOCAL in general, and, the
kernel will actually refuse installing the ff00::/8 route into the local
table from userspace (because there will be other ff00::/8 routes from
other interfaces, so you get "File exists").  You can delete the route
(and thus break mcast), but not add it.  The only way to add it is to
add an address.

Not changing this behaviour keeps breaking userspace;  ospf6d among
other things assumes an interface has working IPv6 when a link-local
address is present.


-David

Re: [PATCH net-next] ipv6: addrconf: fix mcast route for GRE devices

[Hannes Frederic Sowa]

On Mi, 2014-07-30 at 18:31 +0200, David Lamparter wrote:
> On Wed, Jul 30, 2014 at 06:09:27PM +0200, Hannes Frederic Sowa wrote:
> [cut]
> > > On Wed, Jul 30, 2014 at 05:14:42PM +0200, Hannes Frederic Sowa wrote:
> > > > On Mi, 2014-07-30 at 02:55 +0200, David Lamparter wrote:
> > > > > GRE devices, for some reason, were coming up with an autoconfigured
> > > > > address, but no ff00::/8 route in the local table.  This breaks any kind
> > > > > of multicast, in particular OSPFv3, mDNS, - and ND.  In fact, IPv6 only
> > > > > works at all because there is little need for ND on PtP devices.
> > > > > 
> > > > > Adding any other IPv6 address on the device would rectify this issue
> > > > > through inet6_addr_add()/addrconf_add_dev() - and would leave the route
> > > > > around even if the address was later removed.  (This is probably why
> > > > > this issue was not discovered earlier.  AFAICS it has been there from
> > > > > the beginning, e.g. aee80b5 "generate link local address for GRE
> > > > > tunnel")
> > > > 
> > > > Yep, this is poor, but changing this will break user space...
> > > 
> > > How exactly will this break user space?
> > 
> > Because the multicast routes will always be restored after e.g. a route
> > flush or manual route deletion. Scripts might depend on this.
> 
> Sorry, I still don't get it.  Without this patch you end up in an
> inconsistent state, where a LL addr exists, but multicast doesn't work
> (since ff00::/8 is missing from RT6_TABLE_LOCAL).

Sure, people can remove addresses and routes at will.

> Userspace is not supposed to touch RT6_TABLE_LOCAL in general, and, the
> kernel will actually refuse installing the ff00::/8 route into the local
> table from userspace (because there will be other ff00::/8 routes from
> other interfaces, so you get "File exists").  You can delete the route
> (and thus break mcast), but not add it.  The only way to add it is to
> add an address.

People really do flush the routing table.

I'll have a look why the addition of the multicast route throws an
error.

> Not changing this behaviour keeps breaking userspace;  ospf6d among
> other things assumes an interface has working IPv6 when a link-local
> address is present.

Yeah, but in the end, people also can drop specific packets and we
cannot do anything.

Bye,
Hannes

Re: [PATCH net-next] ipv6: addrconf: fix mcast route for GRE devices

[David Lamparter]

On Wed, Jul 30, 2014 at 06:52:21PM +0200, Hannes Frederic Sowa wrote:
> On Mi, 2014-07-30 at 18:31 +0200, David Lamparter wrote:
> > On Wed, Jul 30, 2014 at 06:09:27PM +0200, Hannes Frederic Sowa wrote:
> > [cut]
> > > > On Wed, Jul 30, 2014 at 05:14:42PM +0200, Hannes Frederic Sowa wrote:
> > > > > On Mi, 2014-07-30 at 02:55 +0200, David Lamparter wrote:
> > > > > > GRE devices, for some reason, were coming up with an autoconfigured
> > > > > > address, but no ff00::/8 route in the local table.  This breaks any kind
> > > > > > of multicast, in particular OSPFv3, mDNS, - and ND.  In fact, IPv6 only
> > > > > > works at all because there is little need for ND on PtP devices.
> > > > > > 
> > > > > > Adding any other IPv6 address on the device would rectify this issue
> > > > > > through inet6_addr_add()/addrconf_add_dev() - and would leave the route
> > > > > > around even if the address was later removed.  (This is probably why
> > > > > > this issue was not discovered earlier.  AFAICS it has been there from
> > > > > > the beginning, e.g. aee80b5 "generate link local address for GRE
> > > > > > tunnel")
> > > > > 
> > > > > Yep, this is poor, but changing this will break user space...
> > > > 
> > > > How exactly will this break user space?
> > > 
> > > Because the multicast routes will always be restored after e.g. a route
> > > flush or manual route deletion. Scripts might depend on this.
> > 
> > Sorry, I still don't get it.  Without this patch you end up in an
> > inconsistent state, where a LL addr exists, but multicast doesn't work
> > (since ff00::/8 is missing from RT6_TABLE_LOCAL).
> 
> Sure, people can remove addresses and routes at will.

That's not the point.

user manually adds address on any if: ff00::/8 added.
kernel link-up autoconf addr on !gre: ff00::/8 added.
kernel link-up autoconf addr on gre:  no ff00::/8.

This is about automatic kernel behaviour in device up/change
notifications.  The user can always shoot himself in the foot.  This is
the kernel shooting the user in the foot in one particular and quite
specific instance and without a request to do so.

> > Userspace is not supposed to touch RT6_TABLE_LOCAL in general, and, the
> > kernel will actually refuse installing the ff00::/8 route into the local
> > table from userspace (because there will be other ff00::/8 routes from
> > other interfaces, so you get "File exists").  You can delete the route
> > (and thus break mcast), but not add it.  The only way to add it is to
> > add an address.
> 
> People really do flush the routing table.

This is RT6_TABLE_LOCAL.  Most people aren't even aware it exists.  And
even though I can't find a reference for it, my memory tells me that
"table local" is supposed to be under the kernel's authority.

> I'll have a look why the addition of the multicast route throws an
> error.

While that may be interesting, it won't fix the issue at hand.

(It's probably because the output device is not part of uniqueness
considerations for ff00::/8.  Might need to be treated as "scope link".)

> > Not changing this behaviour keeps breaking userspace;  ospf6d among
> > other things assumes an interface has working IPv6 when a link-local
> > address is present.
>
> Yeah, but in the end, people also can drop specific packets and we
> cannot do anything.

See above, this isn't the user screwing himself, this is the kernel
doing the wrong thing in one specific corner case.  (Which shouldn't
even be a corner case.)


-David

Re: [PATCH net-next] ipv6: addrconf: fix mcast route for GRE devices

[Hannes Frederic Sowa]

On Wed, Jul 30, 2014, at 19:35, David Lamparter wrote:
> On Wed, Jul 30, 2014 at 06:52:21PM +0200, Hannes Frederic Sowa wrote:
> > On Mi, 2014-07-30 at 18:31 +0200, David Lamparter wrote:
> > > On Wed, Jul 30, 2014 at 06:09:27PM +0200, Hannes Frederic Sowa wrote:
> > > [cut]
> > > > > On Wed, Jul 30, 2014 at 05:14:42PM +0200, Hannes Frederic Sowa wrote:
> > > > > > On Mi, 2014-07-30 at 02:55 +0200, David Lamparter wrote:
> > > > > > > GRE devices, for some reason, were coming up with an autoconfigured
> > > > > > > address, but no ff00::/8 route in the local table.  This breaks any kind
> > > > > > > of multicast, in particular OSPFv3, mDNS, - and ND.  In fact, IPv6 only
> > > > > > > works at all because there is little need for ND on PtP devices.
> > > > > > > 
> > > > > > > Adding any other IPv6 address on the device would rectify this issue
> > > > > > > through inet6_addr_add()/addrconf_add_dev() - and would leave the route
> > > > > > > around even if the address was later removed.  (This is probably why
> > > > > > > this issue was not discovered earlier.  AFAICS it has been there from
> > > > > > > the beginning, e.g. aee80b5 "generate link local address for GRE
> > > > > > > tunnel")
> > > > > > 
> > > > > > Yep, this is poor, but changing this will break user space...
> > > > > 
> > > > > How exactly will this break user space?
> > > > 
> > > > Because the multicast routes will always be restored after e.g. a route
> > > > flush or manual route deletion. Scripts might depend on this.
> > > 
> > > Sorry, I still don't get it.  Without this patch you end up in an
> > > inconsistent state, where a LL addr exists, but multicast doesn't work
> > > (since ff00::/8 is missing from RT6_TABLE_LOCAL).
> > 
> > Sure, people can remove addresses and routes at will.
> 
> That's not the point.
> 
> user manually adds address on any if: ff00::/8 added.
> kernel link-up autoconf addr on !gre: ff00::/8 added.
> kernel link-up autoconf addr on gre:  no ff00::/8.

Ah sorry, I am ok with your patch that we generate a ff00::/8 multicast
route if we set a gre interface up. Getting rid of the logic, that we
try to add the ff00::/8 on every addition of an address to an interface
would be a change which concerns because of backward compatibility
reasons.

Especially this also creates multicast routes on sit-non-ptp/isatap
routes, which actually don't need one.

> This is about automatic kernel behaviour in device up/change
> notifications.  The user can always shoot himself in the foot.  This is
> the kernel shooting the user in the foot in one particular and quite
> specific instance and without a request to do so.

Agreed.

> > > Userspace is not supposed to touch RT6_TABLE_LOCAL in general, and, the
> > > kernel will actually refuse installing the ff00::/8 route into the local
> > > table from userspace (because there will be other ff00::/8 routes from
> > > other interfaces, so you get "File exists").  You can delete the route
> > > (and thus break mcast), but not add it.  The only way to add it is to
> > > add an address.
> > 
> > People really do flush the routing table.
> 
> This is RT6_TABLE_LOCAL.  Most people aren't even aware it exists.  And
> even though I can't find a reference for it, my memory tells me that
> "table local" is supposed to be under the kernel's authority.

Yep, but still provide ip -6 route flush table all, which also clears
the local table. And I guess people depend on ff00:: multicast routes
being inserted as soon as they add addresses.

Bye,
Hannes

Re: [PATCH net-next] ipv6: addrconf: fix mcast route for GRE devices

[Dan Williams]

On Wed, 2014-07-30 at 20:03 +0200, Hannes Frederic Sowa wrote:
> On Wed, Jul 30, 2014, at 19:35, David Lamparter wrote:
> > On Wed, Jul 30, 2014 at 06:52:21PM +0200, Hannes Frederic Sowa wrote:
> > > On Mi, 2014-07-30 at 18:31 +0200, David Lamparter wrote:
> > > > On Wed, Jul 30, 2014 at 06:09:27PM +0200, Hannes Frederic Sowa wrote:
> > > > [cut]
> > > > > > On Wed, Jul 30, 2014 at 05:14:42PM +0200, Hannes Frederic Sowa wrote:
> > > > > > > On Mi, 2014-07-30 at 02:55 +0200, David Lamparter wrote:
> > > > > > > > GRE devices, for some reason, were coming up with an autoconfigured
> > > > > > > > address, but no ff00::/8 route in the local table.  This breaks any kind
> > > > > > > > of multicast, in particular OSPFv3, mDNS, - and ND.  In fact, IPv6 only
> > > > > > > > works at all because there is little need for ND on PtP devices.
> > > > > > > > 
> > > > > > > > Adding any other IPv6 address on the device would rectify this issue
> > > > > > > > through inet6_addr_add()/addrconf_add_dev() - and would leave the route
> > > > > > > > around even if the address was later removed.  (This is probably why
> > > > > > > > this issue was not discovered earlier.  AFAICS it has been there from
> > > > > > > > the beginning, e.g. aee80b5 "generate link local address for GRE
> > > > > > > > tunnel")
> > > > > > > 
> > > > > > > Yep, this is poor, but changing this will break user space...
> > > > > > 
> > > > > > How exactly will this break user space?
> > > > > 
> > > > > Because the multicast routes will always be restored after e.g. a route
> > > > > flush or manual route deletion. Scripts might depend on this.
> > > > 
> > > > Sorry, I still don't get it.  Without this patch you end up in an
> > > > inconsistent state, where a LL addr exists, but multicast doesn't work
> > > > (since ff00::/8 is missing from RT6_TABLE_LOCAL).
> > > 
> > > Sure, people can remove addresses and routes at will.
> > 
> > That's not the point.
> > 
> > user manually adds address on any if: ff00::/8 added.
> > kernel link-up autoconf addr on !gre: ff00::/8 added.
> > kernel link-up autoconf addr on gre:  no ff00::/8.
> 
> Ah sorry, I am ok with your patch that we generate a ff00::/8 multicast
> route if we set a gre interface up. Getting rid of the logic, that we
> try to add the ff00::/8 on every addition of an address to an interface
> would be a change which concerns because of backward compatibility
> reasons.

With IPv6LL address generation modes (IFLA_INET6_ADDR_GEN_MODE) the
kernel might not be handling the LL address, so I'd like to keep
addrconf_add_dev() in the inet6_addr_add() codepath.  Having the
multicast route is useful/correct no matter who adds the LL address to
the interface.

I think you alluded to this earlier, but I just wanted to bring it up.

Dan

> Especially this also creates multicast routes on sit-non-ptp/isatap
> routes, which actually don't need one.
> 
> > This is about automatic kernel behaviour in device up/change
> > notifications.  The user can always shoot himself in the foot.  This is
> > the kernel shooting the user in the foot in one particular and quite
> > specific instance and without a request to do so.
> 
> Agreed.
> 
> > > > Userspace is not supposed to touch RT6_TABLE_LOCAL in general, and, the
> > > > kernel will actually refuse installing the ff00::/8 route into the local
> > > > table from userspace (because there will be other ff00::/8 routes from
> > > > other interfaces, so you get "File exists").  You can delete the route
> > > > (and thus break mcast), but not add it.  The only way to add it is to
> > > > add an address.
> > > 
> > > People really do flush the routing table.
> > 
> > This is RT6_TABLE_LOCAL.  Most people aren't even aware it exists.  And
> > even though I can't find a reference for it, my memory tells me that
> > "table local" is supposed to be under the kernel's authority.
> 
> Yep, but still provide ip -6 route flush table all, which also clears
> the local table. And I guess people depend on ff00:: multicast routes
> being inserted as soon as they add addresses.
> 
> Bye,
> Hannes
> --
> 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: [PATCH net-next] ipv6: addrconf: fix mcast route for GRE devices

[David Miller]

From: David Lamparter <equinox@diac24.net>
Date: Wed, 30 Jul 2014 19:35:44 +0200

> This is RT6_TABLE_LOCAL.  Most people aren't even aware it exists.  And
> even though I can't find a reference for it, my memory tells me that
> "table local" is supposed to be under the kernel's authority.

The origin of local routing tables (both in ipv4 and ipv6) has more to
do with internal implementation issues rather than issues pertaining
to how the user should or should not manipulate routes.

It's basically an optimization for local packet delivery, since the
local routing tables get consulted first.  Secondly, separation of
local and non-local routes into different tables theoretically makes
those tables less dense and thus resulting in faster lookups.