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(>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(>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

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(>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

[kbuild test robot]

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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

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

[kbuild test robot]

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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

[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:  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 
---
 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(>lock);
+   addrconf_dad_stop(ifp, 1);
+   rtnl_lock();
+   dev_disable_change(idev);
+   rtnl_unlock();
+   return;
}
}
 
-- 
1.7.10.4