[PATCH Precise SRU] UBUNTU: SAUCE: net/ipv6: don't take interface down when enabling/disabling use_tempaddr
Andy Whitcroft
apw at canonical.com
Mon Jun 30 12:59:01 UTC 2014
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 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.
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
More information about the kernel-team
mailing list