Message ID | 20090416110818.GA20950@gondor.apana.org.au |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
Am Thursday 16 April 2009 13:08:18 schrieb Herbert Xu: > On Wed, Apr 15, 2009 at 10:38:34PM +0800, Herbert Xu wrote: > > > > So how about this? We replace the dev destructor with our own that > > doesn't immediately call free_netdev. We only call free_netdev once > > all tun fd's attached to the device have been closed. > > Here's the patch. I'd appreciate if everyone can review it > and see if they can recreate the original race by > > 1) Starting a process using tun and polls on it; > 2) Doing ip tun del tunXXX while the process is polling. > > tun: Only free a netdev when all tun descriptors are closed > > The commit c70f182940f988448f3c12a209d18b1edc276e33 ("tun: Fix > races between tun_net_close and free_netdev") fixed a race where > an asynchronous deletion of a tun device can hose a poll(2) on > a tun fd attached to that device. Sorry for the late reply, but this patch creates another regression: Now TUNSETIFF returns EBUSY on a persistent tap device: open("/dev/net/tun", O_RDWR) = 11 ioctl(11, 0x400454ca, 0x3ffff8e2270) = -1 EBUSY (Device or resource busy) Some debugging (thanks to Carsten Otte) showed that tun_set_iff calls tun_attach (the first call inside the if(dev)). tun_attach now checks for tun->tfile but this is already set. Looks like we need another fix on top :-( Christian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 24, 2009 at 10:55:49AM +0200, Christian Borntraeger wrote: > Am Thursday 16 April 2009 13:08:18 schrieb Herbert Xu: > > > Here's the patch. I'd appreciate if everyone can review it > > and see if they can recreate the original race by > > > > 1) Starting a process using tun and polls on it; > > 2) Doing ip tun del tunXXX while the process is polling. > > > > tun: Only free a netdev when all tun descriptors are closed > > > > The commit c70f182940f988448f3c12a209d18b1edc276e33 ("tun: Fix > > races between tun_net_close and free_netdev") fixed a race where > > an asynchronous deletion of a tun device can hose a poll(2) on > > a tun fd attached to that device. > > Sorry for the late reply, but this patch creates another regression: > Now TUNSETIFF returns EBUSY on a persistent tap device: > > open("/dev/net/tun", O_RDWR) = 11 > ioctl(11, 0x400454ca, 0x3ffff8e2270) = -1 EBUSY (Device or resource busy) The patch you qouted has been superceded by many versions :) Can you please test the latest that's in davem's tree? Thanks,
Am Friday 24 April 2009 14:11:56 schrieb Herbert Xu: > The patch you qouted has been superceded by many versions :) Yes, I got lost in this mail thread... > Can you please test the latest that's in davem's tree? Done. With http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fdavem%2Fnet-2.6.git;a=commitdiff_plain;h=9c3fea6ab04a7bd9298e635bf29b4a5379f6c476 http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fdavem%2Fnet-2.6.git;a=commitdiff_plain;h=c40af84a6726f63e35740d26f841992e8f31f92c Everything works fine. Thank you and sorry for the noise. Christian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index a1b0697..540f829 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -156,6 +156,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file) tfile->tun = tun; tun->tfile = tfile; dev_hold(tun->dev); + sock_hold(tun->sk); atomic_inc(&tfile->count); out: @@ -165,11 +166,8 @@ out: static void __tun_detach(struct tun_struct *tun) { - struct tun_file *tfile = tun->tfile; - /* Detach from net device */ netif_tx_lock_bh(tun->dev); - tfile->tun = NULL; tun->tfile = NULL; netif_tx_unlock_bh(tun->dev); @@ -339,6 +337,13 @@ static void tun_net_uninit(struct net_device *dev) } } +static void tun_free_netdev(struct net_device *dev) +{ + struct tun_struct *tun = netdev_priv(dev); + + sock_put(tun->sk); +} + /* Net device open. */ static int tun_net_open(struct net_device *dev) { @@ -810,7 +815,7 @@ static void tun_setup(struct net_device *dev) tun->group = -1; dev->ethtool_ops = &tun_ethtool_ops; - dev->destructor = free_netdev; + dev->destructor = tun_free_netdev; } /* Trivial set of netlink ops to allow deleting tun or tap @@ -847,7 +852,7 @@ static void tun_sock_write_space(struct sock *sk) static void tun_sock_destruct(struct sock *sk) { - dev_put(container_of(sk, struct tun_sock, sk)->tun->dev); + free_netdev(container_of(sk, struct tun_sock, sk)->tun->dev); } static struct proto tun_proto = { @@ -919,8 +924,6 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) if (!sk) goto err_free_dev; - /* This ref count is for tun->sk. */ - dev_hold(dev); sock_init_data(&tun->socket, sk); sk->sk_write_space = tun_sock_write_space; sk->sk_destruct = tun_sock_destruct; @@ -1275,20 +1278,18 @@ static int tun_chr_close(struct inode *inode, struct file *file) struct tun_file *tfile = file->private_data; struct tun_struct *tun = __tun_get(tfile); - if (tun) { - DBG(KERN_INFO "%s: tun_chr_close\n", tun->dev->name); - - rtnl_lock(); - __tun_detach(tun); - /* If desireable, unregister the netdevice. */ - if (!(tun->flags & TUN_PERSIST)) { - sock_put(tun->sk); - unregister_netdevice(tun->dev); - } + if (!(tun->flags & TUN_PERSIST)) + unregister_netdev(tun->dev); + else + tun_put(tun); + } else + tun = tfile->tun; - rtnl_unlock(); + if (tun) { + DBG(KERN_INFO "%s: tun_chr_close\n", tun->dev->name); + sock_put(tun->sk); } put_net(tfile->net);