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

index 6bc85f7..263fcf3 100644
@@ -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

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(). */

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).

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.

index 6bc85f7..263fcf3 100644
@@ -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

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