"new style" netdevice allocation patch for TUN driver (2.4.18 kernel)

"new style" netdevice allocation patch for TUN driver (2.4.18 kernel)

[Jacek Konieczny]

I had a lot of problem with tun devices created with both openvpn and
vtund. When I wanted to shut down my system when the devices were in
use (eg. TCP connection established on tun0 interface), even if the
tunneling daemon was killed, it stopped while trying to deconfigure
network. And "unregister_netdevice: waiting for tun0 to become free"
message was displayed again and again. I tried to resolve this problem
using Google, but I have only found out, that this is behaviour of 2.4
kernels, and that it is proper. After further investigation, in kernel
sources, I found out, that there are "old style" and "new style" network
devices, and that only the "old style" devices have this problem.
I had similar problem with VLAN devices some time ago, so I checked VLAN
driver sources too. As I suspected, it was "new style" device now.
The patch below is my try to make tun device "new style" too. It seems
to work for me, but I am not sure if it is 100% proper. This is patch
against 2.4.18 sources.

Sorry, for spamming all those addresses, but I am not sure which one is
correct. Driver on URL given in MAINTAINERS file seems to be a bit
outdated.

Greets,
	Jacek


--- linux/drivers/net/tun.c.orig	Sun Sep 30 21:26:07 2001
+++ linux/drivers/net/tun.c	Thu Aug  1 14:41:12 2002
@@ -20,6 +20,14 @@
  *    Modifications for 2.3.99-pre5 kernel.
  */
 
+/*
+ *  01.08.2002
+ *  Jacek Konieczny <jajcus@pld.org.pl>
+ *    Modifications for "new style" device allocation
+ *    (fixes "wating for tunX to become free" problem)
+ */
+
+
 #define TUN_VER "1.4"
 
 #include <linux/config.h>
@@ -159,6 +167,17 @@
 	return 0;
 }
 
+void tun_net_destruct(struct net_device *dev)
+{
+	if (dev) {
+		if (dev->priv) {
+			kfree(dev->priv);
+			dev->priv=NULL;
+			MOD_DEC_USE_COUNT;
+		}
+	}
+}
+
 /* Character device part */
 
 /* Poll */
@@ -204,14 +223,14 @@
 	skb_reserve(skb, 2);
 	copy_from_user(skb_put(skb, len), ptr, len); 
 
-	skb->dev = &tun->dev;
+	skb->dev = tun->dev;
 	switch (tun->flags & TUN_TYPE_MASK) {
 	case TUN_TUN_DEV:
 		skb->mac.raw = skb->data;
 		skb->protocol = pi.proto;
 		break;
 	case TUN_TAP_DEV:
-		skb->protocol = eth_type_trans(skb, &tun->dev);
+		skb->protocol = eth_type_trans(skb, tun->dev);
 		break;
 	};
 
@@ -310,7 +329,7 @@
 			schedule();
 			continue;
 		}
-		netif_start_queue(&tun->dev);
+		netif_start_queue(tun->dev);
 
 		if (!verify_area(VERIFY_WRITE, buf, count))
 			ret = tun_put_user(tun, skb, buf, count);
@@ -357,8 +376,6 @@
 		init_waitqueue_head(&tun->read_wait);
 
 		tun->owner = -1;
-		tun->dev.init = tun_net_init;
-		tun->dev.priv = tun;
 
 		err = -EINVAL;
 
@@ -377,17 +394,21 @@
 		if (*ifr->ifr_name)
 			name = ifr->ifr_name;
 
-		if ((err = dev_alloc_name(&tun->dev, name)) < 0)
-			goto failed;
-		if ((err = register_netdevice(&tun->dev)))
-			goto failed;
-	
-		MOD_INC_USE_COUNT;
+		dev = dev_alloc(name, &err);
+		if (!dev) goto failed;
+		
+		tun->dev=dev;
+		dev->init = tun_net_init;
+		dev->priv = tun;
+		dev->destructor = tun_net_destruct;
+		dev->features |= NETIF_F_DYNALLOC;
+		tun->name = dev->name;
 
-		tun->name = tun->dev.name;
-	}
+	        err=register_netdevice(dev);
+		if (err<0) goto failed;
 
-	DBG(KERN_INFO "%s: tun_set_iff\n", tun->name);
+		MOD_INC_USE_COUNT;
+	}
 
 	if (ifr->ifr_flags & IFF_NO_PI)
 		tun->flags |= TUN_NO_PI;
@@ -402,6 +423,7 @@
 	return 0;
 
 failed:
+	kfree(dev);
 	kfree(tun);
 	return err;
 }
@@ -532,10 +554,8 @@
 	skb_queue_purge(&tun->readq);
 
 	if (!(tun->flags & TUN_PERSIST)) {
-		dev_close(&tun->dev);
-		unregister_netdevice(&tun->dev);
-		kfree(tun);
-		MOD_DEC_USE_COUNT;
+		dev_close(tun->dev);
+		unregister_netdevice(tun->dev);
 	}
 
 	rtnl_unlock();
--- linux/include/linux/if_tun.h.orig	Tue Jun 12 04:15:27 2001
+++ linux/include/linux/if_tun.h	Thu Aug  1 14:33:40 2002
@@ -40,7 +40,7 @@
 	wait_queue_head_t	read_wait;
 	struct sk_buff_head	readq;
 
-	struct net_device	dev;
+	struct net_device	*dev;
 	struct net_device_stats	stats;
 
 	struct fasync_struct    *fasync;

Re: "new style" netdevice allocation patch for TUN driver (2.4.18 kernel)

[kuznet]

Hello!

> network. And "unregister_netdevice: waiting for tun0 to become free"

This smells like a leakage. I think this should be investigated.


> devices, and that only the "old style" devices have this problem.
> I had similar problem with VLAN devices some time ago, so I checked VLAN
> driver sources too.

Well, switching to "new style" does not solve the problem. If we have
a leakage, it will continue silently which is even worse.
Probably, you should to define NET_REFCNT_DEBUG in net/core/dev.c
to track what happens with the device.

Alexey

Re: "new style" netdevice allocation patch for TUN driver (2.4.18 kernel)

[Jacek Konieczny]

On Thu, Aug 01, 2002 at 09:43:23PM +0400, kuznet@ms2.inr.ac.ru wrote:
> Hello!
> 
> > network. And "unregister_netdevice: waiting for tun0 to become free"
> 
> This smells like a leakage. I think this should be investigated.
I am not sure. Sometimes it is not easy to free such device.

> > devices, and that only the "old style" devices have this problem.
> > I had similar problem with VLAN devices some time ago, so I checked VLAN
> > driver sources too.
> 
> Well, switching to "new style" does not solve the problem. If we have
> a leakage, it will continue silently which is even worse.
> Probably, you should to define NET_REFCNT_DEBUG in net/core/dev.c
> to track what happens with the device.
After applying my patch, the device unregistration is usually defered,
but finally it is finished. On system shutdown, when all routes are
removed, all interfaces downed and all processes killed the device will
not be used any more and the device is freed. Without the patch my
system doesn't even come to this point, as it stops on the first process
which tries to access any network device (eg. any iproute2 command).
Usually it stops on postfix shutdown. Probably because I use the tun
device for tunneling SMTP connections to my home machine. Probably
shutting down postfix before openvpn would help, but failing to do this
should not hang the system.

And even if there is a bug in tun module it should never prevent system
from clean shutdown. Even if the device will be never freed and module
unloaded. So I thing this patch (or similar) should be applied.

Greets,
        Jacek

Re: "new style" netdevice allocation patch for TUN driver (2.4.18 kernel)

[kuznet]

Hello!

> but finally it is finished. On system shutdown, when all routes are
> removed, all interfaces downed and all processes killed the device will
> not be used any more and the device is freed.

I do not understand this. It is leak and we have to find who continues
to use the device.


> And even if there is a bug

Nope. The bug is the first, the cleanup is the second.

Alexey

two net_device ?

[??]

Hi, there.

I implemented 32 pseudo interface(eth1 ~ eth32) using a module.
When I up the interfaces using "ifconfig eth1 xx.xx.xx.xx" command,
there are two "eth1" interface. Before I up the interfaces, there is a "eth1" interface.
I can't find what thing goes wrong.

For test, I made just 2 pseudo interface using same mechanizm. then
above problem doesn't occur.

any suggestion, thanks..
I use 2.4.17 kernel.

Re: "new style" netdevice allocation patch for TUN driver (2.4.18 kernel)

[Maksim (Max) Krasnyanskiy]

Hi Jacek,

>I had a lot of problem with tun devices created with both openvpn and
>vtund. When I wanted to shut down my system when the devices were in
>use (eg. TCP connection established on tun0 interface), even if the
>tunneling daemon was killed, it stopped while trying to deconfigure
>network. And "unregister_netdevice: waiting for tun0 to become free"
>message was displayed again and again. I tried to resolve this problem
>using Google, but I have only found out, that this is behaviour of 2.4
>kernels, and that it is proper. After further investigation, in kernel
>sources, I found out, that there are "old style" and "new style" network
>devices, and that only the "old style" devices have this problem.
>I had similar problem with VLAN devices some time ago, so I checked VLAN
>driver sources too. As I suspected, it was "new style" device now.
>The patch below is my try to make tun device "new style" too. It seems
>to work for me, but I am not sure if it is 100% proper. This is patch
>against 2.4.18 sources.
You're fixing the wrong problem. It seems that some subsystem is not releasing
tun device during shutdown/deregistration. (See comment in 
net/core/dev.c:unregister_netdev).
You're not gonna see "waiting for" warning anymore if you change to new 
style allocation.
But you're gonna leak tun devices because destructor is not called unless 
refcount is zero.

>Sorry, for spamming all those addresses, but I am not sure which one is
>correct. Driver on URL given in MAINTAINERS file seems to be a bit
>outdated.
URL is ok. Mailing list has to be update though.

Max

Re: "new style" netdevice allocation patch for TUN driver (2.4.18 kernel)

[Maksim (Max) Krasnyanskiy]

>On Fri, Aug 02, 2002 at 04:54:02PM -0700, Maksim (Max) Krasnyanskiy wrote:
> > You're fixing the wrong problem.
>Probably I am. Alexey Kuznietzov told be the same :-)
>
> >It seems that some subsystem is not  releasing
> > tun device during shutdown/deregistration. (See comment in
> > net/core/dev.c:unregister_netdev).
> > You're not gonna see "waiting for" warning anymore if you change to new
> > style allocation.
>I will not see "waiting for" warning, but I will also be able to control
>all other network devices. Without this "fix" I am not able to shutdown
>network at all. Every "ip" command just hangs forever.
Yeah, this should be fixed. unregister_netdevice() sleeps under rtnl_lock().
Which means that any other activity that needs this lock will be blocked.

Dave, how about this

--- net/core/dev.c.orig Mon Aug  5 21:48:54 2002
+++ net/core/dev.c      Mon Aug  5 21:54:01 2002
@@ -2577,6 +2577,11 @@

          */

+       /* We don't have to hold rtnl semaphore while we're waiting for
+          device to become free.
+        */
+       rtnl_unlock();
+
         now = warning_time = jiffies;
         while (atomic_read(&dev->refcnt) != 1) {
                 if ((jiffies - now) > 1*HZ) {
@@ -2593,6 +2598,10 @@
                         warning_time = jiffies;
                 }
         }
+
+       /* Our caller expects it to be locked */
+       rtnl_lock();
+
         dev_put(dev);
         return 0;
  }

----
We don't have to hold rtnl look while sleeping. Device is already unlinked
from the list so nobody can grab and bump refcount.

> > But you're gonna leak tun devices because destructor is not called unless
> > refcount is zero.
>But it seems it is eventually called. The refcount eventually goes to 0
>(1 in factm - selfreference). Without this patch it never went to 0, as
>system shutdown was stopped "waitnig for...".
It'd be nice to trace what part of the kernel is actually holding refcount.

>I don't have enough time and probably knowledge to write a proper fix
>for the problem, however my patch fixes it well enough for me.
>All this "new style" device allocation would be useless if the kernel
>was bug-free. With this patch it is more bug-proof.
:) No, the point of device destructors is not to hide kernel bugs.

>On protuction system it is sometimes even more important, because kernel
>(or any other big piece of software) will never be bug-free.
That why it's important to trace and fix the subsystem that is holding devices
for a long time after deregistration. I may very well be doing it for a 
good reason
but warning is helpful anyway.
And we should fix sleep in unregister_netdevice() (ie patch above).

Max

Re: "new style" netdevice allocation patch for TUN driver (2.4.18 kernel)

[David S. Miller]

   From: "Maksim (Max) Krasnyanskiy" <maxk@qualcomm.com>
   Date: Tue, 06 Aug 2002 10:07:49 -0700
   
   Dave, how about this
   
   --- net/core/dev.c.orig Mon Aug  5 21:48:54 2002
   +++ net/core/dev.c      Mon Aug  5 21:54:01 2002
   @@ -2577,6 +2577,11 @@

First, the call-chain notifiers are probably not safe
to run without rtnl_lock held.

Second, why not just fix the bug instead of applying band-aids
to device unregistry?  I know it's nice in that it allows you
to configure devices some more, but it doesn't make the real
problem go away.

Re: "new style" netdevice allocation patch for TUN driver (2.4.18 kernel)

[Maksim (Max) Krasnyanskiy]

>    From: "Maksim (Max) Krasnyanskiy" <maxk@qualcomm.com>
>    Date: Tue, 06 Aug 2002 10:07:49 -0700
>
>    Dave, how about this
>
>    --- net/core/dev.c.orig Mon Aug  5 21:48:54 2002
>    +++ net/core/dev.c      Mon Aug  5 21:54:01 2002
>    @@ -2577,6 +2577,11 @@
>
>First, the call-chain notifiers are probably not safe
>to run without rtnl_lock held.
Good point.

>Second, why not just fix the bug instead of applying band-aids
>to device unregistry?  I know it's nice in that it allows you
>to configure devices some more, but it doesn't make the real
>problem go away.
I completely agree. However sleeping and holding a lock that you
don't have to hold is a bug on it's own :).
Things like sockets drop the lock before calling schedule() and acquire
it on wakeup. I think we should do the same in unregister_netdevice().

How about this:

--- dev.c.orig  Tue Aug  6 00:58:46 2002
+++ dev.c       Tue Aug  6 01:00:00 2002
@@ -2584,9 +2584,12 @@
                         notifier_call_chain(&netdev_chain, 
NETDEV_UNREGISTER, dev);
                 }

+               rtnl_unlock();
                 current->state = TASK_INTERRUPTIBLE;
                 schedule_timeout(HZ/4);
                 current->state = TASK_RUNNING;
+               rtnl_lock();
+
                 if ((jiffies - warning_time) > 10*HZ) {
                         printk(KERN_EMERG "unregister_netdevice: waiting 
for %s to "
                                         "become free. Usage count = %d\n",
-----

Max

Re: "new style" netdevice allocation patch for TUN driver (2.4.18 kernel)

[Jacek Konieczny]

On Tue, Aug 06, 2002 at 10:07:49AM -0700, Maksim (Max) Krasnyanskiy wrote:
> >I will not see "waiting for" warning, but I will also be able to control
> >all other network devices. Without this "fix" I am not able to shutdown
> >network at all. Every "ip" command just hangs forever.
> Yeah, this should be fixed. unregister_netdevice() sleeps under rtnl_lock().
> Which means that any other activity that needs this lock will be blocked.
I am happy, that someone (smarter than me) seems to be interested in
fixing this bug :-)

> >But it seems it is eventually called. The refcount eventually goes to 0
> >(1 in factm - selfreference). Without this patch it never went to 0, as
> >system shutdown was stopped "waitnig for...".
> It'd be nice to trace what part of the kernel is actually holding refcount.
If I ever find something more precise I'll write.

> >was bug-free. With this patch it is more bug-proof.
> :) No, the point of device destructors is not to hide kernel bugs.
>[...]
> for a long time after deregistration. I may very well be doing it for a 
> good reason but warning is helpful anyway.
I undarstand this. Even with my (now I know - broken) patch kernel warns
that device is in use, and that destruction will be delayed.
But you are right, warning displayed every 3 seconds, when nothing else
works is much easier to notice :-)

> And we should fix sleep in unregister_netdevice() (ie patch above).
I think this would be enough for me. 

Greets,
        Jacek

"new style" netdevice allocation patch for TUN driver (2.4.18 kernel)

[Jacek Konieczny]

I had a lot of problem with tun devices created with both openvpn and
vtund. When I wanted to shut down my system when the devices were in
use (eg. TCP connection established on tun0 interface), even if the
tunneling daemon was killed, it stopped while trying to deconfigure
network. And "unregister_netdevice: waiting for tun0 to become free"
message was displayed again and again. I tried to resolve this problem
using Google, but I have only found out, that this is behaviour of 2.4
kernels, and that it is proper. After further investigation, in kernel
sources, I found out, that there are "old style" and "new style" network
devices, and that only the "old style" devices have this problem.
I had similar problem with VLAN devices some time ago, so I checked VLAN
driver sources too. As I suspected, it was "new style" device now.
The patch below is my try to make tun device "new style" too. It seems
to work for me, but I am not sure if it is 100% proper. This is patch
against 2.4.18 sources.

Sorry, for spamming all those addresses, but I am not sure which one is
correct. Driver on URL given in MAINTAINERS file seems to be a bit
outdated.

Greets,
	Jacek


--- linux/drivers/net/tun.c.orig	Sun Sep 30 21:26:07 2001
+++ linux/drivers/net/tun.c	Thu Aug  1 14:41:12 2002
@@ -20,6 +20,14 @@
  *    Modifications for 2.3.99-pre5 kernel.
  */
 
+/*
+ *  01.08.2002
+ *  Jacek Konieczny <jajcus@pld.org.pl>
+ *    Modifications for "new style" device allocation
+ *    (fixes "wating for tunX to become free" problem)
+ */
+
+
 #define TUN_VER "1.4"
 
 #include <linux/config.h>
@@ -159,6 +167,17 @@
 	return 0;
 }
 
+void tun_net_destruct(struct net_device *dev)
+{
+	if (dev) {
+		if (dev->priv) {
+			kfree(dev->priv);
+			dev->priv=NULL;
+			MOD_DEC_USE_COUNT;
+		}
+	}
+}
+
 /* Character device part */
 
 /* Poll */
@@ -204,14 +223,14 @@
 	skb_reserve(skb, 2);
 	copy_from_user(skb_put(skb, len), ptr, len); 
 
-	skb->dev = &tun->dev;
+	skb->dev = tun->dev;
 	switch (tun->flags & TUN_TYPE_MASK) {
 	case TUN_TUN_DEV:
 		skb->mac.raw = skb->data;
 		skb->protocol = pi.proto;
 		break;
 	case TUN_TAP_DEV:
-		skb->protocol = eth_type_trans(skb, &tun->dev);
+		skb->protocol = eth_type_trans(skb, tun->dev);
 		break;
 	};
 
@@ -310,7 +329,7 @@
 			schedule();
 			continue;
 		}
-		netif_start_queue(&tun->dev);
+		netif_start_queue(tun->dev);
 
 		if (!verify_area(VERIFY_WRITE, buf, count))
 			ret = tun_put_user(tun, skb, buf, count);
@@ -357,8 +376,6 @@
 		init_waitqueue_head(&tun->read_wait);
 
 		tun->owner = -1;
-		tun->dev.init = tun_net_init;
-		tun->dev.priv = tun;
 
 		err = -EINVAL;
 
@@ -377,17 +394,21 @@
 		if (*ifr->ifr_name)
 			name = ifr->ifr_name;
 
-		if ((err = dev_alloc_name(&tun->dev, name)) < 0)
-			goto failed;
-		if ((err = register_netdevice(&tun->dev)))
-			goto failed;
-	
-		MOD_INC_USE_COUNT;
+		dev = dev_alloc(name, &err);
+		if (!dev) goto failed;
+		
+		tun->dev=dev;
+		dev->init = tun_net_init;
+		dev->priv = tun;
+		dev->destructor = tun_net_destruct;
+		dev->features |= NETIF_F_DYNALLOC;
+		tun->name = dev->name;
 
-		tun->name = tun->dev.name;
-	}
+	        err=register_netdevice(dev);
+		if (err<0) goto failed;
 
-	DBG(KERN_INFO "%s: tun_set_iff\n", tun->name);
+		MOD_INC_USE_COUNT;
+	}
 
 	if (ifr->ifr_flags & IFF_NO_PI)
 		tun->flags |= TUN_NO_PI;
@@ -402,6 +423,7 @@
 	return 0;
 
 failed:
+	kfree(dev);
 	kfree(tun);
 	return err;
 }
@@ -532,10 +554,8 @@
 	skb_queue_purge(&tun->readq);
 
 	if (!(tun->flags & TUN_PERSIST)) {
-		dev_close(&tun->dev);
-		unregister_netdevice(&tun->dev);
-		kfree(tun);
-		MOD_DEC_USE_COUNT;
+		dev_close(tun->dev);
+		unregister_netdevice(tun->dev);
 	}
 
 	rtnl_unlock();
--- linux/include/linux/if_tun.h.orig	Tue Jun 12 04:15:27 2001
+++ linux/include/linux/if_tun.h	Thu Aug  1 14:33:40 2002
@@ -40,7 +40,7 @@
 	wait_queue_head_t	read_wait;
 	struct sk_buff_head	readq;
 
-	struct net_device	dev;
+	struct net_device	*dev;
 	struct net_device_stats	stats;
 
 	struct fasync_struct    *fasync;

Re: "new style" netdevice allocation patch for TUN driver (2.4.18 kernel)

[Julian Anastasov]

	Hello,

Jacek Konieczny wrote:

> I had a lot of problem with tun devices created with both openvpn and
> vtund. When I wanted to shut down my system when the devices were in
> use (eg. TCP connection established on tun0 interface), even if the
> tunneling daemon was killed, it stopped while trying to deconfigure
> network. And "unregister_netdevice: waiting for tun0 to become free"
> message was displayed again and again. I tried to resolve this problem
> using Google, but I have only found out, that this is behaviour of 2.4
> kernels, and that it is proper
...
> This is patch against 2.4.18 sources.

	Don't waste time, now when 2.4.19 is out can you try with it? It 
has fix for such problem if you have multipath routes with such devices, 
Alexey already forgot about it :) As for tun in kernel it looks correct, 
unregister_netdevice() under rtnl_lock, only that dev_close is redundant.

Regards

--
Julian Anastasov <ja@ssi.bg>