| Message ID | 20090703090355.GA25768@gondor.apana.org.au |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Herbert Xu <herbert@gondor.apana.org.au> writes: > On Fri, Jul 03, 2009 at 04:35:01PM +0800, Herbert Xu wrote: >> >> This just turns a wide-open race into a less likely one (that's >> why it appears to fix the problem). The crux of the issue is that >> __tun_get(tfile) != NULL has nothing to do with whether the device >> has been unregistered. Not so. unregister_netdevice rollback_registered tun_net_unit __tun_detach. Further we need rtnl_lock around __tun_detach. Eric > tun: Fix device unregister race > > It is currently possible for an asynchronous device unregister > to cause the same tun device to be unregistered twice. This > is because the unregister in tun_chr_close only checks whether > __tun_get(tfile) != NULL. This however has nothing to do with > whether the device has already been unregistered. All it tells > you is whether __tun_detach has been called. > > This patch fixes this by using the most obvious thing to test > whether the device has been unregistered. > > It also moves __tun_detach outside of rtnl_unlock since nothing > that it does requires that lock. > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 11a0ba4..b393536 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1324,20 +1324,22 @@ static int tun_chr_close(struct inode *inode, struct file *file) > struct tun_file *tfile = file->private_data; > struct tun_struct *tun; > > - > - rtnl_lock(); > tun = __tun_get(tfile); > if (tun) { > - DBG(KERN_INFO "%s: tun_chr_close\n", tun->dev->name); > + struct net_device *dev = tun->dev; > + > + DBG(KERN_INFO "%s: tun_chr_close\n", dev->name); > > __tun_detach(tun); > > /* If desireable, unregister the netdevice. */ > - if (!(tun->flags & TUN_PERSIST)) > - unregister_netdevice(tun->dev); > - > + if (!(tun->flags & TUN_PERSIST)) { > + rtnl_lock(); > + if (dev->reg_state == NETREG_REGISTERED) > + unregister_netdevice(dev); > + rtnl_unlock(); > + } > } > - rtnl_unlock(); > > tun = tfile->tun; > if (tun) > > Cheers, > -- > Visit Openswan at http://www.openswan.org/ > Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt > -- > 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 -- 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
Eric W. Biederman <ebiederm@xmission.com> wrote: > > Not so. > > unregister_netdevice > rollback_registered > tun_net_unit > __tun_detach. > > > Further we need rtnl_lock around __tun_detach. No we don't, tfile->count prevents this from occuring. The async path will only __tun_detach if the count hits zero, in which case the first if clause in tun_chr_close will fail. Conversely, if we're in the first if clause in tun_chr_close, then the async path either didn't execute at all or did not call __tun_detach. Cheers,
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Fri, 3 Jul 2009 23:25:47 +0800 > Eric W. Biederman <ebiederm@xmission.com> wrote: >> >> Not so. >> >> unregister_netdevice >> rollback_registered >> tun_net_unit >> __tun_detach. >> >> >> Further we need rtnl_lock around __tun_detach. > > No we don't, tfile->count prevents this from occuring. The async > path will only __tun_detach if the count hits zero, in which case > the first if clause in tun_chr_close will fail. Conversely, if > we're in the first if clause in tun_chr_close, then the async > path either didn't execute at all or did not call __tun_detach. I've applied Herbert's patch, thanks! -- 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 11a0ba4..b393536 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1324,20 +1324,22 @@ static int tun_chr_close(struct inode *inode, struct file *file) struct tun_file *tfile = file->private_data; struct tun_struct *tun; - - rtnl_lock(); tun = __tun_get(tfile); if (tun) { - DBG(KERN_INFO "%s: tun_chr_close\n", tun->dev->name); + struct net_device *dev = tun->dev; + + DBG(KERN_INFO "%s: tun_chr_close\n", dev->name); __tun_detach(tun); /* If desireable, unregister the netdevice. */ - if (!(tun->flags & TUN_PERSIST)) - unregister_netdevice(tun->dev); - + if (!(tun->flags & TUN_PERSIST)) { + rtnl_lock(); + if (dev->reg_state == NETREG_REGISTERED) + unregister_netdevice(dev); + rtnl_unlock(); + } } - rtnl_unlock(); tun = tfile->tun; if (tun)