[RFC] Move in6_dev_hold under CONFIG_IPV6_PRIVACY

Hello,

I am currently looking at a problem with in6 interface refcnt on a 
really old kernel and I have just noticed something I find suspicious in 
all kernels until now.

The comment at net/ipv6/addrconf.c:396 suggests that the call to 
in6_dev_hold is only for ipv6_regen_rndid.
As a consequence, if CONFIG_IPV6_PRIVACY is not set, then this 
in6_dev_hold will leak a refcnt.

Can someone look at this ?
I did not test this patch, yet it looks reasonable to me.

Thank you.


$ git diff

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 6bc85f7..263fcf3 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c

@@ -393,11 +393,6 @@ static struct inet6_dev *ipv6_add_dev(struct 

net_device *dev)
                 return NULL;
         }

-       /* One reference from device.  We must do this before
-        * we invoke __ipv6_regen_rndid().
-        */
-       in6_dev_hold(ndev);
-
         if (dev->flags & (IFF_NOARP | IFF_LOOPBACK))
                 ndev->cnf.accept_dad = -1;

@@ -410,6 +405,12 @@ static struct inet6_dev *ipv6_add_dev(struct 

net_device *dev)

  #ifdef CONFIG_IPV6_PRIVACY
         INIT_LIST_HEAD(&ndev->tempaddr_list);
+
+       /* One reference from device.  We must do this before
+        * we invoke ipv6_regen_rndid().
+        */
+       in6_dev_hold(ndev);
+
         setup_timer(&ndev->regen_timer, ipv6_regen_rndid, (unsigned 
long)ndev);
         if ((dev->flags&IFF_LOOPBACK) ||
             dev->type == ARPHRD_TUNNEL ||



-- 
David Marchand


This message has been scanned for viruses by BlackSpider MailControl. - www.blackspider.com

On Thu, 2012-08-30 at 11:42 +0200, David Marchand wrote:

Hello,

I am currently looking at a problem with in6 interface refcnt on a 
really old kernel and I have just noticed something I find suspicious in 
all kernels until now.

The comment at net/ipv6/addrconf.c:396 suggests that the call to 
in6_dev_hold is only for ipv6_regen_rndid.
As a consequence, if CONFIG_IPV6_PRIVACY is not set, then this 
in6_dev_hold will leak a refcnt.

Can someone look at this ?
I did not test this patch, yet it looks reasonable to me.

Your patch was mangled by your email client, and its always safer to
test a patch...

Dont trust the comment, its a bit misleading.

Comment intent was to say :

/* One reference from device.  We must do this before
 * we eventually invoke __ipv6_regen_rndid().
 */

On 30/08/2012 17:25, Eric Dumazet wrote:

Your patch was mangled by your email client, and its always safer to
test a patch...

Sorry about that, I am not used to sending patch (tried again, I hope it is fine this time ...).

I did try my patch in my setup, and did not notice any problem at the moment.
I sent this patch as a rfc, mainly because I want to know if my analysis is correct (and if not, understand why).

Dont trust the comment, its a bit misleading.

Comment intent was to say :

/* One reference from device.  We must do this before
 * we eventually invoke __ipv6_regen_rndid().
 */

Mmm, ok.
But if CONFIG_IPV6_PRIVACY is not selected, then I can't find where the associated in6_dev_put is done for this in6_dev_hold.
Besides, __ipv6_regen_rndid is a "CONFIG_IPV6_PRIVACY-only" stuff.

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 6bc85f7..263fcf3 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c

@@ -393,11 +393,6 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
 		return NULL;
 	}

-	/* One reference from device.  We must do this before
-	 * we invoke __ipv6_regen_rndid().
-	 */
-	in6_dev_hold(ndev);
-
 	if (dev->flags & (IFF_NOARP | IFF_LOOPBACK))
 		ndev->cnf.accept_dad = -1;

@@ -410,6 +405,12 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)

 #ifdef CONFIG_IPV6_PRIVACY
 	INIT_LIST_HEAD(&ndev->tempaddr_list);
+
+	/* One reference from device.  We must do this before
+	 * we invoke ipv6_regen_rndid().
+	 */
+	in6_dev_hold(ndev);
+
 	setup_timer(&ndev->regen_timer, ipv6_regen_rndid, (unsigned long)ndev);
 	if ((dev->flags&IFF_LOOPBACK) ||
 	    dev->type == ARPHRD_TUNNEL ||

-- 
David Marchand




This message has been scanned for viruses by BlackSpider MailControl. - www.blackspider.com

On Thu, 2012-08-30 at 18:52 +0200, David Marchand wrote:

Mmm, ok.
But if CONFIG_IPV6_PRIVACY is not selected, then I can't find where the associated in6_dev_put is done for this in6_dev_hold.
Besides, __ipv6_regen_rndid is a "CONFIG_IPV6_PRIVACY-only" stuff.

Okay.

Look at addrconf_ifdown(), line 2850

in6_dev_put(idev);

Will finally remove the last reference

With your patch, I guess there will be a reference problem, and possibly
call in6_dev_finish_destroy() too soon.

You are mistaken, because the reference taken in PRIVACY is at line 421,
right before calling ipv6_regen_rndid()

Look at following commit to have an idea of the underlying problem

http://git.kernel.org/?p=linux/kernel/git/davem/net.git;a=commit;h=144d56e91044181ec0ef67aeca91e9a8b5718348