2002-08-01 13:31:58

by Jacek Konieczny

[permalink] [raw]
Subject: "new style" netdevice allocation patch for TUN driver (2.4.18 kernel)

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 <[email protected]>
+ * 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;


2002-08-02 23:50:53

by Max Krasnyansky

[permalink] [raw]
Subject: Re: "new style" netdevice allocation patch for TUN driver (2.4.18 kernel)

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

2002-08-06 17:04:35

by Max Krasnyansky

[permalink] [raw]
Subject: Re: "new style" netdevice allocation patch for TUN driver (2.4.18 kernel)


>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

2002-08-06 17:17:05

by David Miller

[permalink] [raw]
Subject: Re: "new style" netdevice allocation patch for TUN driver (2.4.18 kernel)

From: "Maksim (Max) Krasnyanskiy" <[email protected]>
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.

2002-08-06 17:26:05

by Jacek Konieczny

[permalink] [raw]
Subject: Re: "new style" netdevice allocation patch for TUN driver (2.4.18 kernel)

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

2002-08-06 20:00:45

by Max Krasnyansky

[permalink] [raw]
Subject: Re: "new style" netdevice allocation patch for TUN driver (2.4.18 kernel)


> From: "Maksim (Max) Krasnyanskiy" <[email protected]>
> 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

2002-08-07 00:06:17

by Julian Anastasov

[permalink] [raw]
Subject: Re: "new style" netdevice allocation patch for TUN driver (2.4.18 kernel)


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 <[email protected]>