[patch net-next v3] ipv6: log autoconfiguration failures

[patch net-next v3] ipv6: log autoconfiguration failures

[Denys Vlasenko]

If ipv6 auto-configuration does not work, currently it's hard
to track what's going on. This change adds log messages
(at debug level) on every code path where ipv6 autoconf fails.

v3: changed pr_debug's to pr_warn's.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
---
 net/ipv6/addrconf.c | 45 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 3c3425e..0b354f0 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1694,8 +1694,11 @@ static void addrconf_leave_anycast(struct inet6_ifaddr *ifp)
 
 static int addrconf_ifid_eui48(u8 *eui, struct net_device *dev)
 {
-	if (dev->addr_len != ETH_ALEN)
+	if (dev->addr_len != ETH_ALEN) {
+		pr_warn("IPv6 addrconf: %s: address length %d != %s\n",
+			dev->name, dev->addr_len, "ETH_ALEN");
 		return -1;
+	}
 	memcpy(eui, dev->dev_addr, 3);
 	memcpy(eui + 5, dev->dev_addr + 3, 3);
 
@@ -1725,8 +1728,11 @@ static int addrconf_ifid_eui48(u8 *eui, struct net_device *dev)
 
 static int addrconf_ifid_eui64(u8 *eui, struct net_device *dev)
 {
-	if (dev->addr_len != IEEE802154_ADDR_LEN)
+	if (dev->addr_len != IEEE802154_ADDR_LEN) {
+		pr_warn("IPv6 addrconf: %s: address length %d != %s\n",
+			dev->name, dev->addr_len, "IEEE802154_ADDR_LEN");
 		return -1;
+	}
 	memcpy(eui, dev->dev_addr, 8);
 	eui[0] ^= 2;
 	return 0;
@@ -1736,8 +1742,11 @@ static int addrconf_ifid_ieee1394(u8 *eui, struct net_device *dev)
 {
 	union fwnet_hwaddr *ha;
 
-	if (dev->addr_len != FWNET_ALEN)
+	if (dev->addr_len != FWNET_ALEN) {
+		pr_warn("IPv6 addrconf: %s: address length %d != %s\n",
+			dev->name, dev->addr_len, "FWNET_ALEN");
 		return -1;
+	}
 
 	ha = (union fwnet_hwaddr *)dev->dev_addr;
 
@@ -1749,8 +1758,11 @@ static int addrconf_ifid_ieee1394(u8 *eui, struct net_device *dev)
 static int addrconf_ifid_arcnet(u8 *eui, struct net_device *dev)
 {
 	/* XXX: inherit EUI-64 from other interface -- yoshfuji */
-	if (dev->addr_len != ARCNET_ALEN)
+	if (dev->addr_len != ARCNET_ALEN) {
+		pr_warn("IPv6 addrconf: %s: address length %d != %s\n",
+			dev->name, dev->addr_len, "ARCNET_ALEN");
 		return -1;
+	}
 	memset(eui, 0, 7);
 	eui[7] = *(u8 *)dev->dev_addr;
 	return 0;
@@ -1758,17 +1770,25 @@ static int addrconf_ifid_arcnet(u8 *eui, struct net_device *dev)
 
 static int addrconf_ifid_infiniband(u8 *eui, struct net_device *dev)
 {
-	if (dev->addr_len != INFINIBAND_ALEN)
+	if (dev->addr_len != INFINIBAND_ALEN) {
+		pr_warn("IPv6 addrconf: %s: address length %d != %s\n",
+			dev->name, dev->addr_len, "INFINIBAND_ALEN");
 		return -1;
+	}
 	memcpy(eui, dev->dev_addr + 12, 8);
 	eui[0] |= 2;
 	return 0;
 }
 
-static int __ipv6_isatap_ifid(u8 *eui, __be32 addr)
+static int __ipv6_isatap_ifid(u8 *eui, struct net_device *dev)
 {
-	if (addr == 0)
+	__be32 addr = *(__be32 *)dev->dev_addr;
+
+	if (addr == 0) {
+		pr_warn("IPv6 addrconf: %s: bad dev_addr %pM\n",
+			dev->name, dev->dev_addr);
 		return -1;
+	}
 	eui[0] = (ipv4_is_zeronet(addr) || ipv4_is_private_10(addr) ||
 		  ipv4_is_loopback(addr) || ipv4_is_linklocal_169(addr) ||
 		  ipv4_is_private_172(addr) || ipv4_is_test_192(addr) ||
@@ -1785,13 +1805,14 @@ static int __ipv6_isatap_ifid(u8 *eui, __be32 addr)
 static int addrconf_ifid_sit(u8 *eui, struct net_device *dev)
 {
 	if (dev->priv_flags & IFF_ISATAP)
-		return __ipv6_isatap_ifid(eui, *(__be32 *)dev->dev_addr);
+		return __ipv6_isatap_ifid(eui, dev);
+	pr_warn("IPv6 addrconf: %s: IFF_ISATAP is unset\n", dev->name);
 	return -1;
 }
 
 static int addrconf_ifid_gre(u8 *eui, struct net_device *dev)
 {
-	return __ipv6_isatap_ifid(eui, *(__be32 *)dev->dev_addr);
+	return __ipv6_isatap_ifid(eui, dev);
 }
 
 static int addrconf_ifid_ip6tnl(u8 *eui, struct net_device *dev)
@@ -1825,6 +1846,8 @@ static int ipv6_generate_eui64(u8 *eui, struct net_device *dev)
 	case ARPHRD_TUNNEL6:
 		return addrconf_ifid_ip6tnl(eui, dev);
 	}
+	pr_warn("IPv6 addrconf: %s: dev->type %d is not supported\n",
+		dev->name, dev->type);
 	return -1;
 }
 
@@ -1842,6 +1865,10 @@ static int ipv6_inherit_eui64(u8 *eui, struct inet6_dev *idev)
 		}
 	}
 	read_unlock_bh(&idev->lock);
+	if (err)
+		pr_warn("IPv6 addrconf: "
+			"%s: no link-local address to inherit\n",
+			idev->dev->name);
 	return err;
 }
 
-- 
1.8.1.4

Re: [patch net-next v3] ipv6: log autoconfiguration failures

[Vlad Yasevich]

On 12/13/2013 10:45 AM, Denys Vlasenko wrote:
> If ipv6 auto-configuration does not work, currently it's hard
> to track what's going on. This change adds log messages
> (at debug level) on every code path where ipv6 autoconf fails.
> 
> v3: changed pr_debug's to pr_warn's.
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> ---
>  net/ipv6/addrconf.c | 45 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 3c3425e..0b354f0 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -1694,8 +1694,11 @@ static void addrconf_leave_anycast(struct inet6_ifaddr *ifp)
>  
>  static int addrconf_ifid_eui48(u8 *eui, struct net_device *dev)
>  {
> -	if (dev->addr_len != ETH_ALEN)
> +	if (dev->addr_len != ETH_ALEN) {
> +		pr_warn("IPv6 addrconf: %s: address length %d != %s\n",
> +			dev->name, dev->addr_len, "ETH_ALEN");
>  		return -1;
> +	}
>  	memcpy(eui, dev->dev_addr, 3);
>  	memcpy(eui + 5, dev->dev_addr + 3, 3);
>  
> @@ -1725,8 +1728,11 @@ static int addrconf_ifid_eui48(u8 *eui, struct net_device *dev)
>  
>  static int addrconf_ifid_eui64(u8 *eui, struct net_device *dev)
>  {
> -	if (dev->addr_len != IEEE802154_ADDR_LEN)
> +	if (dev->addr_len != IEEE802154_ADDR_LEN) {
> +		pr_warn("IPv6 addrconf: %s: address length %d != %s\n",
> +			dev->name, dev->addr_len, "IEEE802154_ADDR_LEN");
>  		return -1;
> +	}
>  	memcpy(eui, dev->dev_addr, 8);
>  	eui[0] ^= 2;
>  	return 0;
> @@ -1736,8 +1742,11 @@ static int addrconf_ifid_ieee1394(u8 *eui, struct net_device *dev)
>  {
>  	union fwnet_hwaddr *ha;
>  
> -	if (dev->addr_len != FWNET_ALEN)
> +	if (dev->addr_len != FWNET_ALEN) {
> +		pr_warn("IPv6 addrconf: %s: address length %d != %s\n",
> +			dev->name, dev->addr_len, "FWNET_ALEN");
>  		return -1;
> +	}
>  
>  	ha = (union fwnet_hwaddr *)dev->dev_addr;
>  
> @@ -1749,8 +1758,11 @@ static int addrconf_ifid_ieee1394(u8 *eui, struct net_device *dev)
>  static int addrconf_ifid_arcnet(u8 *eui, struct net_device *dev)
>  {
>  	/* XXX: inherit EUI-64 from other interface -- yoshfuji */
> -	if (dev->addr_len != ARCNET_ALEN)
> +	if (dev->addr_len != ARCNET_ALEN) {
> +		pr_warn("IPv6 addrconf: %s: address length %d != %s\n",
> +			dev->name, dev->addr_len, "ARCNET_ALEN");
>  		return -1;
> +	}
>  	memset(eui, 0, 7);
>  	eui[7] = *(u8 *)dev->dev_addr;
>  	return 0;
> @@ -1758,17 +1770,25 @@ static int addrconf_ifid_arcnet(u8 *eui, struct net_device *dev)
>  
>  static int addrconf_ifid_infiniband(u8 *eui, struct net_device *dev)
>  {
> -	if (dev->addr_len != INFINIBAND_ALEN)
> +	if (dev->addr_len != INFINIBAND_ALEN) {
> +		pr_warn("IPv6 addrconf: %s: address length %d != %s\n",
> +			dev->name, dev->addr_len, "INFINIBAND_ALEN");
>  		return -1;
> +	}
>  	memcpy(eui, dev->dev_addr + 12, 8);
>  	eui[0] |= 2;
>  	return 0;
>  }
>  
> -static int __ipv6_isatap_ifid(u8 *eui, __be32 addr)
> +static int __ipv6_isatap_ifid(u8 *eui, struct net_device *dev)
>  {
> -	if (addr == 0)
> +	__be32 addr = *(__be32 *)dev->dev_addr;
> +
> +	if (addr == 0) {
> +		pr_warn("IPv6 addrconf: %s: bad dev_addr %pM\n",
> +			dev->name, dev->dev_addr);
>  		return -1;
> +	}
>  	eui[0] = (ipv4_is_zeronet(addr) || ipv4_is_private_10(addr) ||
>  		  ipv4_is_loopback(addr) || ipv4_is_linklocal_169(addr) ||
>  		  ipv4_is_private_172(addr) || ipv4_is_test_192(addr) ||
> @@ -1785,13 +1805,14 @@ static int __ipv6_isatap_ifid(u8 *eui, __be32 addr)
>  static int addrconf_ifid_sit(u8 *eui, struct net_device *dev)
>  {
>  	if (dev->priv_flags & IFF_ISATAP)
> -		return __ipv6_isatap_ifid(eui, *(__be32 *)dev->dev_addr);
> +		return __ipv6_isatap_ifid(eui, dev);
> +	pr_warn("IPv6 addrconf: %s: IFF_ISATAP is unset\n", dev->name);
>  	return -1;
>  }
>  
>  static int addrconf_ifid_gre(u8 *eui, struct net_device *dev)
>  {
> -	return __ipv6_isatap_ifid(eui, *(__be32 *)dev->dev_addr);
> +	return __ipv6_isatap_ifid(eui, dev);
>  }
>  
>  static int addrconf_ifid_ip6tnl(u8 *eui, struct net_device *dev)
> @@ -1825,6 +1846,8 @@ static int ipv6_generate_eui64(u8 *eui, struct net_device *dev)
>  	case ARPHRD_TUNNEL6:
>  		return addrconf_ifid_ip6tnl(eui, dev);
>  	}
> +	pr_warn("IPv6 addrconf: %s: dev->type %d is not supported\n",
> +		dev->name, dev->type);
>  	return -1;
>  }

This one should probably be a pr_debug or counter.  This is not a
critical issue and it makes no sense to spam the log if IPv6 is not
supported on a particular interface.

>  
> @@ -1842,6 +1865,10 @@ static int ipv6_inherit_eui64(u8 *eui, struct inet6_dev *idev)
>  		}
>  	}
>  	read_unlock_bh(&idev->lock);
> +	if (err)
> +		pr_warn("IPv6 addrconf: "
> +			"%s: no link-local address to inherit\n",
> +			idev->dev->name);
>  	return err;
>  }

This shouldn't be a warning either.  This is called if
ipv6_generate_eui64() fails and we'd know if it failed for a good
reason.  If that failed, we very likely do not have any other link-local
addresses and would know of any error conditions.

If by some chance, there are unsolicited RAs on the link, you'd end up
spamming the log.

-vlad

Re: [patch net-next v3] ipv6: log autoconfiguration failures

[David Miller]

From: Denys Vlasenko <dvlasenk@redhat.com>
Date: Fri, 13 Dec 2013 16:45:47 +0100

> If ipv6 auto-configuration does not work, currently it's hard
> to track what's going on. This change adds log messages
> (at debug level) on every code path where ipv6 autoconf fails.
> 
> v3: changed pr_debug's to pr_warn's.
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>

I already made it clear I want to see statistics added instead of log
messages for this.

I'm not applying this patch, sorry.

Re: [patch net-next v3] ipv6: log autoconfiguration failures

[Hannes Frederic Sowa]

On Fri, Dec 13, 2013 at 05:57:09PM -0500, David Miller wrote:
> From: Denys Vlasenko <dvlasenk@redhat.com>
> Date: Fri, 13 Dec 2013 16:45:47 +0100
> 
> > If ipv6 auto-configuration does not work, currently it's hard
> > to track what's going on. This change adds log messages
> > (at debug level) on every code path where ipv6 autoconf fails.
> > 
> > v3: changed pr_debug's to pr_warn's.
> > 
> > Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> 
> I already made it clear I want to see statistics added instead of log
> messages for this.

Hmm... I gave this a bit more thought.

I would go with counters, too. What if we add something like per interface
softerror states. So that we e.g. can see that one protocol had an error on an
interface and it needs attention of an administrator?

E.g.

2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP,SOFT_FAIL>

Would catch the eyes more quickly. If counters are implemented we could
do that completly in user space and add that flag as soon as one of the
observed counters are above 0 (and of course, iproute needs colors).

What do you think?