[PATCH] net: ipv6: Remove addresses for failures with strict DAD

[PATCH] net: ipv6: Remove addresses for failures with strict DAD

[Mike Manning]

If DAD fails with accept_dad set to 2, global addresses and host routes
are incorrectly left in place. Even though disable_ipv6 is set,
contrary to documentation, the addresses are not dynamically deleted
from the interface. It is only on a subsequent link down/up that these
are removed. The fix is not only to set the disable_ipv6 flag, but to
actually disable IPv6 when DAD fails in this case. This results in the
addresses and routes being immediately deleted. The DAD failure
for the LL addr is determined as before via netlink, or by the absence
of the LL addr (which also previously would have had to be checked for
in case of an intervening link down/up).

Previous behavior:

root@vm1:/# sysctl net.ipv6.conf.eth3.accept_dad=2
net.ipv6.conf.eth3.accept_dad = 2
root@vm1:/# ip -6 addr add 2000::10/64 dev eth3
root@vm1:/# ip link set up eth3
root@vm1:/# ip -6 addr show dev eth3
5: eth3: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qlen 1000
    inet6 2000::10/64 scope global
       valid_lft forever preferred_lft forever
    inet6 fe80::5054:ff:fe43:dd5a/64 scope link tentative dadfailed
       valid_lft forever preferred_lft forever
root@vm1:/# ip -6 route show dev eth3
2000::/64  proto kernel  metric 256
fe80::/64  proto kernel  metric 256
root@vm1:/# ip link set down eth3
root@vm1:/# ip link set up eth3
root@vm1:/# ip -6 addr show dev eth3
root@vm1:/# ip -6 route show dev eth3
root@vm1:/#

New behavior:

root@vm1:/# sysctl net.ipv6.conf.eth3.accept_dad=2
net.ipv6.conf.eth3.accept_dad = 2
root@vm1:/# ip -6 addr add 2000::10/64 dev eth3
root@vm1:/# ip link set up eth3
root@vm1:/# ip -6 addr show dev eth3
root@vm1:/# ip -6 route show dev eth3
root@vm1:/#

Signed-off-by: Mike Manning <mmanning@brocade.com>
---
 net/ipv6/addrconf.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index df8425f..6be5a95 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -135,6 +135,7 @@ static int ipv6_count_addresses(struct inet6_dev *idev);
 static int ipv6_generate_stable_address(struct in6_addr *addr,
 					u8 dad_count,
 					const struct inet6_dev *idev);
+static void dev_disable_change(struct inet6_dev *idev);
 
 /*
  *	Configured unicast address hash table
@@ -1945,6 +1946,12 @@ lock_errdad:
 
 			pr_info("%s: IPv6 being disabled!\n",
 				ifp->idev->dev->name);
+			spin_unlock_bh(&ifp->lock);
+			addrconf_dad_stop(ifp, 1);
+			rtnl_lock();
+			dev_disable_change(idev);
+			rtnl_unlock();
+			return;
 		}
 	}
 
-- 
1.7.10.4

Re: [PATCH] net: ipv6: Remove addresses for failures with strict DAD

[kbuild test robot]

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

Hi Mike,

[auto build test WARNING on net/master]
[also build test WARNING on v4.8-rc2 next-20160817]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Mike-Manning/net-ipv6-Remove-addresses-for-failures-with-strict-DAD/20160817-183122
config: x86_64-randconfig-s4-08172039 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

>> net/ipv6/addrconf.c:138:13: warning: 'dev_disable_change' used but never defined
    static void dev_disable_change(struct inet6_dev *idev);
                ^~~~~~~~~~~~~~~~~~

vim +/dev_disable_change +138 net/ipv6/addrconf.c

   122	}
   123	
   124	static inline void addrconf_sysctl_unregister(struct inet6_dev *idev)
   125	{
   126	}
   127	#endif
   128	
   129	static void __ipv6_regen_rndid(struct inet6_dev *idev);
   130	static void __ipv6_try_regen_rndid(struct inet6_dev *idev, struct in6_addr *tmpaddr);
   131	static void ipv6_regen_rndid(unsigned long data);
   132	
   133	static int ipv6_generate_eui64(u8 *eui, struct net_device *dev);
   134	static int ipv6_count_addresses(struct inet6_dev *idev);
   135	static int ipv6_generate_stable_address(struct in6_addr *addr,
   136						u8 dad_count,
   137						const struct inet6_dev *idev);
 > 138	static void dev_disable_change(struct inet6_dev *idev);
   139	
   140	/*
   141	 *	Configured unicast address hash table
   142	 */
   143	static struct hlist_head inet6_addr_lst[IN6_ADDR_HSIZE];
   144	static DEFINE_SPINLOCK(addrconf_hash_lock);
   145	
   146	static void addrconf_verify(void);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 26846 bytes --]

Re: [PATCH] net: ipv6: Remove addresses for failures with strict DAD

[kbuild test robot]

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

Hi Mike,

[auto build test ERROR on net/master]
[also build test ERROR on v4.8-rc2 next-20160817]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Mike-Manning/net-ipv6-Remove-addresses-for-failures-with-strict-DAD/20160817-183122
config: x86_64-randconfig-s4-08172039 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   net/built-in.o: In function `addrconf_dad_failure':
>> (.text+0x1241dc): undefined reference to `dev_disable_change'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 26846 bytes --]

Re: [PATCH] net: ipv6: Remove addresses for failures with strict DAD

[Hannes Frederic Sowa]

On 17.08.2016 12:28, Mike Manning wrote:
> +static void dev_disable_change(struct inet6_dev *idev);
>  
>  /*
>   *	Configured unicast address hash table
> @@ -1945,6 +1946,12 @@ lock_errdad:
>  
>  			pr_info("%s: IPv6 being disabled!\n",
>  				ifp->idev->dev->name);
> +			spin_unlock_bh(&ifp->lock);
> +			addrconf_dad_stop(ifp, 1);
> +			rtnl_lock();
> +			dev_disable_change(idev);
> +			rtnl_unlock();
> +			return;
>  		}
>  	}

You can't take rtnl_lock at that point but must postpone the actions and
do that in addrconf_dad_work.

Probably the whole ... else if (idev->cnf.accept_dad > 1 && ...) needs
to move there.

Bye,
Hannes

Re: [PATCH] net: ipv6: Remove addresses for failures with strict DAD

[Mike Manning]

On 08/17/2016 04:40 PM, Hannes Frederic Sowa wrote:
> On 17.08.2016 12:28, Mike Manning wrote:
>> +static void dev_disable_change(struct inet6_dev *idev);
>>  
>>  /*
>>   *	Configured unicast address hash table
>> @@ -1945,6 +1946,12 @@ lock_errdad:
>>  
>>  			pr_info("%s: IPv6 being disabled!\n",
>>  				ifp->idev->dev->name);
>> +			spin_unlock_bh(&ifp->lock);
>> +			addrconf_dad_stop(ifp, 1);
>> +			rtnl_lock();
>> +			dev_disable_change(idev);
>> +			rtnl_unlock();
>> +			return;
>>  		}
>>  	}
> 
> You can't take rtnl_lock at that point but must postpone the actions and
> do that in addrconf_dad_work.
> 
> Probably the whole ... else if (idev->cnf.accept_dad > 1 && ...) needs
> to move there.
> 
> Bye,
> Hannes
> 
> 

Thanks for the prompt review, I will look into making these changes.

Also these changes caused a build error due to conditional compilation without CONFIG_SYSCTL, which is resolved by replacing the call to dev_disable_change(idev) by directly calling addrconf_ifdown(idev->dev, 0) instead.

I would appreciate any further comments if the suggested change in behavior is not acceptable.

Thanks
Mike

Re: [PATCH] net: ipv6: Remove addresses for failures with strict DAD

[Hannes Frederic Sowa]

On Wed, Aug 17, 2016, at 18:08, Mike Manning wrote:
> On 08/17/2016 04:40 PM, Hannes Frederic Sowa wrote:
> > On 17.08.2016 12:28, Mike Manning wrote:
> >> +static void dev_disable_change(struct inet6_dev *idev);
> >>  
> >>  /*
> >>   *	Configured unicast address hash table
> >> @@ -1945,6 +1946,12 @@ lock_errdad:
> >>  
> >>  			pr_info("%s: IPv6 being disabled!\n",
> >>  				ifp->idev->dev->name);
> >> +			spin_unlock_bh(&ifp->lock);
> >> +			addrconf_dad_stop(ifp, 1);
> >> +			rtnl_lock();
> >> +			dev_disable_change(idev);
> >> +			rtnl_unlock();
> >> +			return;
> >>  		}
> >>  	}
> > 
> > You can't take rtnl_lock at that point but must postpone the actions and
> > do that in addrconf_dad_work.
> > 
> > Probably the whole ... else if (idev->cnf.accept_dad > 1 && ...) needs
> > to move there.
> > 
> > Bye,
> > Hannes
> > 
> > 
> 
> Thanks for the prompt review, I will look into making these changes.
> 
> Also these changes caused a build error due to conditional compilation
> without CONFIG_SYSCTL, which is resolved by replacing the call to
> dev_disable_change(idev) by directly calling addrconf_ifdown(idev->dev,
> 0) instead.
> 
> I would appreciate any further comments if the suggested change in
> behavior is not acceptable.

What you describe in the changelog what is happening right now looks
like a bug to me thus your patch made sense to me.

Bye,
Hannes