[PATCH Precise SRU] UBUNTU: SAUCE: net/ipv6: don't take interface down when enabling/disabling use_tempaddr

[Stefan Bader]

On 20.06.2014 15:32, Tim Gardner wrote:
> From: Malcolm Scott <debianpkg at malc.org.uk>
> 
> BugLink: http://bugs.launchpad.net/bugs/994931

This did not seem to have made progress, so I decided to look at the patch...

> 
> Altering use_tempaddr drops all IPv6 addresses.
> 
> Signed-off-by: Malcolm Scott <debianpkg at malc.org.uk>
> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
> ---
>  net/ipv6/addrconf.c | 57 ++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 43 insertions(+), 14 deletions(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 246a170..6e576c6 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -125,7 +125,8 @@ static inline void addrconf_sysctl_unregister(struct inet6_dev *idev)
>  #ifdef CONFIG_IPV6_PRIVACY
>  static int __ipv6_regen_rndid(struct inet6_dev *idev);
>  static int __ipv6_try_regen_rndid(struct inet6_dev *idev, struct in6_addr *tmpaddr);
> -static void ipv6_regen_rndid(unsigned long data);
> +static void ipv6_regen_rndid(struct inet6_dev *idev);
> +static void ipv6_regen_rndid_tick(unsigned long data);
>  #endif
>  
>  static int ipv6_generate_eui64(u8 *eui, struct net_device *dev);
> @@ -409,7 +410,7 @@ static struct inet6_dev * ipv6_add_dev(struct net_device *dev)
>  
>  #ifdef CONFIG_IPV6_PRIVACY
>  	INIT_LIST_HEAD(&ndev->tempaddr_list);
> -	setup_timer(&ndev->regen_timer, ipv6_regen_rndid, (unsigned long)ndev);
> +	setup_timer(&ndev->regen_timer, ipv6_regen_rndid_tick, (unsigned long)ndev);
>  	if ((dev->flags&IFF_LOOPBACK) ||
>  	    dev->type == ARPHRD_TUNNEL ||
>  	    dev->type == ARPHRD_TUNNEL6 ||
> @@ -417,8 +418,9 @@ static struct inet6_dev * ipv6_add_dev(struct net_device *dev)
>  	    dev->type == ARPHRD_NONE) {
>  		ndev->cnf.use_tempaddr = -1;
>  	} else {
> -		in6_dev_hold(ndev);
> -		ipv6_regen_rndid((unsigned long) ndev);
> +		rcu_read_lock_bh();
> +		ipv6_regen_rndid(ndev);
> +		rcu_read_unlock_bh();

It could be ok to drop the in6_dev_hold() call here since it looks like this is
done another time here (in ipv6_add_dev()) before. Or should the other call be
dropped (since it refers to calling to __ipv6_regen_rndid which I cannot see
called directly. In that case it probably still needs to be done here.

>  	}
>  #endif
>  
> @@ -1649,12 +1651,21 @@ regen:
>  	return 0;
>  }
>  
> -static void ipv6_regen_rndid(unsigned long data)
> +static void ipv6_regen_rndid_tick(unsigned long data)
>  {
>  	struct inet6_dev *idev = (struct inet6_dev *) data;
> -	unsigned long expires;
>  
>  	rcu_read_lock_bh();
> +	ipv6_regen_rndid(idev);
> +	rcu_read_unlock_bh();
> +	in6_dev_put(idev);
> +}
> +
> +/* called with rcu_read_lock_bh() */
> +static void ipv6_regen_rndid(struct inet6_dev *idev)
> +{
> +	unsigned long expires;
> +
>  	write_lock_bh(&idev->lock);
>  
>  	if (idev->dead)
> @@ -1679,8 +1690,6 @@ static void ipv6_regen_rndid(unsigned long data)
>  
>  out:
>  	write_unlock_bh(&idev->lock);
> -	rcu_read_unlock_bh();
> -	in6_dev_put(idev);
>  }
>  
>  static int __ipv6_try_regen_rndid(struct inet6_dev *idev, struct in6_addr *tmpaddr) {
> @@ -4390,12 +4399,32 @@ static void dev_tempaddr_change(struct inet6_dev *idev)
>  	if (!idev || !idev->dev)
>  		return;
>  
> -	if (!idev->cnf.disable_ipv6) {
> -		/* If ipv6 is enabled, try to bring down and back up the
> -		 * interface to get new temporary addresses created
> -		 */
> -		addrconf_notify(NULL, NETDEV_DOWN, idev->dev);
> -		addrconf_notify(NULL, NETDEV_UP, idev->dev);
> +	/* Add/remove temporary addresses if necessary */
> +	if (!idev->cnf.disable_ipv6 && idev->cnf.autoconf) {
> +		if (idev->cnf.use_tempaddr > 0) {
> +			struct inet6_ifaddr *ifp, *ifn;
> +
> +			/*
> +			 * Create a temporary address for every non-temporary,
> +			 * non-permanent (i.e. autoconfigured) address
> +			 */
> +
> +			ipv6_regen_rndid(idev);
> +
> +			list_for_each_entry_safe(ifp, ifn, &idev->addr_list, if_list) {
> +				if (!(ifp->flags & (IFA_F_TEMPORARY | IFA_F_PERMANENT))) {
> +					ipv6_create_tempaddr(ifp, NULL);
> +				}
> +			}
> +		} else {
> +			struct inet6_ifaddr *ifa;
> +
> +			while (!list_empty(&idev->tempaddr_list)) {
> +				ifa = list_first_entry(&idev->tempaddr_list,
> +						       struct inet6_ifaddr, tmp_list);
> +				ipv6_del_addr(ifa);
> +			}
> +		}
>  	}
>  }

There seem to be to callers of dev_tempaddr_change(). One
(addrconf_tempaddr_change()) calls rcu_read_lock (not _bh) before which is
probably ok. But the other (addrconf_use_tempaddr()) does not seem to do any
locking. Also there is the question of the reference. The notify calls before
did not need to care. But I think the idea would be to call ipv6_regen_rndid()
with an additional reference taken to avoid freeing it. The function can take
another reference when the timer was successfully modified, but otherwise drops
the passed in reference on exit. This is changed by the patch...

-Stefan
>  
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20140630/44016ca6/attachment.sig>