diff mbox

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

Message ID 1403271147-3275-1-git-send-email-tim.gardner@canonical.com
State New
Headers show

Commit Message

Tim Gardner June 20, 2014, 1:32 p.m. UTC 
From: Malcolm Scott <debianpkg@malc.org.uk>

BugLink: http://bugs.launchpad.net/bugs/994931

Altering use_tempaddr drops all IPv6 addresses.

Signed-off-by: Malcolm Scott <debianpkg@malc.org.uk>
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 net/ipv6/addrconf.c | 57 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 14 deletions(-)

Comments

Tim Gardner June 20, 2014, 1:36 p.m. UTC | #1 
On 06/20/2014 07:32 AM, Tim Gardner wrote:
> From: Malcolm Scott <debianpkg@malc.org.uk>
>
> BugLink: http://bugs.launchpad.net/bugs/994931
>
> Altering use_tempaddr drops all IPv6 addresses.
>
> Signed-off-by: Malcolm Scott <debianpkg@malc.org.uk>
> Signed-off-by: Tim Gardner <tim.gardner@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();
>   	}
>   #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);
>   }
>

Malcolm - what is the benefit of ipv6_regen_rndid_tick() ? AFAICT it is 
merely reorganizing the locking, but there is no functional difference.

rtg
Andy Whitcroft June 20, 2014, 2:45 p.m. UTC | #2 
On Fri, Jun 20, 2014 at 07:32:27AM -0600, Tim Gardner wrote:
> From: Malcolm Scott <debianpkg@malc.org.uk>
> 
> BugLink: http://bugs.launchpad.net/bugs/994931
> 
> Altering use_tempaddr drops all IPv6 addresses.
> 
> Signed-off-by: Malcolm Scott <debianpkg@malc.org.uk>
> Signed-off-by: Tim Gardner <tim.gardner@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();
>  	}
>  #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);
> +			}
> +		}
>  	}
>  }

That does appear to do what is claimed.  The spagetti seems rather
different in 3.16 so I cannot trivally tell if the same issue appears
there?  I think they are just left to expire and are not recreated
rather than activly managed, but it would be good to have confirmation.

This ought to be easily testable, with such testing.

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw
Stefan Bader June 30, 2014, 8:28 a.m. UTC | #3 
On 20.06.2014 15:32, Tim Gardner wrote:
> From: Malcolm Scott <debianpkg@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@malc.org.uk>
> Signed-off-by: Tim Gardner <tim.gardner@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
>  
>
Andy Whitcroft June 30, 2014, 12:21 p.m. UTC | #4 
On Fri, Jun 20, 2014 at 07:36:54AM -0600, Tim Gardner wrote:
> On 06/20/2014 07:32 AM, Tim Gardner wrote:
> >From: Malcolm Scott <debianpkg@malc.org.uk>
> >
> >BugLink: http://bugs.launchpad.net/bugs/994931
> >
> >Altering use_tempaddr drops all IPv6 addresses.
> >
> >Signed-off-by: Malcolm Scott <debianpkg@malc.org.uk>
> >Signed-off-by: Tim Gardner <tim.gardner@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();
> >  	}
> >  #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);
> >  }
> >
> 
> Malcolm - what is the benefit of ipv6_regen_rndid_tick() ? AFAICT it is
> merely reorganizing the locking, but there is no functional difference.

I believe this has been rearranged, to expose the unlocked form now
confusingly called ipv6_regen_rndid() to be called directly.

-apw
Andy Whitcroft June 30, 2014, 12:59 p.m. UTC | #5 
On Mon, Jun 30, 2014 at 10:28:31AM +0200, Stefan Bader wrote:
> On 20.06.2014 15:32, Tim Gardner wrote:
> > From: Malcolm Scott <debianpkg@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@malc.org.uk>
> > Signed-off-by: Tim Gardner <tim.gardner@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.

I think this is still right.  Before the fix we did this:

ipv6_add_dev()
	in6_dev_hold()	-> 1
	in6_dev_hold()  -> 2
	ipv6_regen_rndid()
		if (timer_now_running)
			in6_dev_hold() -> 3
		in6_dev_put() -> 2

tick fires
	ipv6_regen_rndid()
		if (timer_now_running)
			in6_dev_hold() -> 3
		in6_dev_put() -> 2

This keeps the reference count at 2/3 at all times ie greater than 0.

If we now look at the new code this is as below:
ipv6_add_dev()
	in6_dev_hold()	-> 1
	ipv6_regen_rndid()
		if (timer_now_running)
			in6_dev_hold() -> 2

tick fires
	ipv6_regen_rndid_tick()
		ipv6_regen_rndid()
			if (timer_now_running)
				in6_dev_hold() -> 3
		in6_dev_put() -> 2

This keeps the reference count at 2/3 at all times ie greater than 0.

I belive this makes them equivalent, modulo the handling of RCU being
changed.

> 
> >  	}
> >  #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...

I think there is a question mark here on the addrconf_use_tempaddr()
caller which probabally does need some rcu_locking.   I think I have
covered off the second half of this above.

Malcom?  Could you comment on the rcu locking here.

-apw
diff mbox

Patch

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();
 	}
 #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);
+			}
+		}
 	}
 }