tun: Extend RTNL lock coverage over whole ioctl
As it is, parts of the ioctl runs under the RTNL and parts of
it do not. The unlocked section is still protected by the BKL,
but there can be subtle races. For example, Eric Biederman and
Paul Moore observed that if two threads tried to create two tun
devices on the same file descriptor, then unexpected results
may occur.
As there isn't anything in the ioctl that is expected to sleep
indefinitely, we can prevent this from occurring by extending
the RTNL lock coverage.
This also allows to get rid of the BKL.
Finally, I changed tun_get_iff to take a tun device in order to
avoid calling tun_put which would dead-lock as it also tries to
take the RTNL lock.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: David S. Miller <davem@davemloft.net>
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 027f7ab..42b6c63 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1048,20 +1048,15 @@
return err;
}
-static int tun_get_iff(struct net *net, struct file *file, struct ifreq *ifr)
+static int tun_get_iff(struct net *net, struct tun_struct *tun,
+ struct ifreq *ifr)
{
- struct tun_struct *tun = tun_get(file);
-
- if (!tun)
- return -EBADFD;
-
DBG(KERN_INFO "%s: tun_get_iff\n", tun->dev->name);
strcpy(ifr->ifr_name, tun->dev->name);
ifr->ifr_flags = tun_flags(tun);
- tun_put(tun);
return 0;
}
@@ -1105,8 +1100,8 @@
return 0;
}
-static int tun_chr_ioctl(struct inode *inode, struct file *file,
- unsigned int cmd, unsigned long arg)
+static long tun_chr_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
{
struct tun_file *tfile = file->private_data;
struct tun_struct *tun;
@@ -1128,34 +1123,32 @@
(unsigned int __user*)argp);
}
+ rtnl_lock();
+
tun = __tun_get(tfile);
if (cmd == TUNSETIFF && !tun) {
- int err;
-
ifr.ifr_name[IFNAMSIZ-1] = '\0';
- rtnl_lock();
- err = tun_set_iff(tfile->net, file, &ifr);
- rtnl_unlock();
+ ret = tun_set_iff(tfile->net, file, &ifr);
- if (err)
- return err;
+ if (ret)
+ goto unlock;
if (copy_to_user(argp, &ifr, sizeof(ifr)))
- return -EFAULT;
- return 0;
+ ret = -EFAULT;
+ goto unlock;
}
-
+ ret = -EBADFD;
if (!tun)
- return -EBADFD;
+ goto unlock;
DBG(KERN_INFO "%s: tun_chr_ioctl cmd %d\n", tun->dev->name, cmd);
ret = 0;
switch (cmd) {
case TUNGETIFF:
- ret = tun_get_iff(current->nsproxy->net_ns, file, &ifr);
+ ret = tun_get_iff(current->nsproxy->net_ns, tun, &ifr);
if (ret)
break;
@@ -1201,7 +1194,6 @@
case TUNSETLINK:
/* Only allow setting the type when the interface is down */
- rtnl_lock();
if (tun->dev->flags & IFF_UP) {
DBG(KERN_INFO "%s: Linktype set failed because interface is up\n",
tun->dev->name);
@@ -1211,7 +1203,6 @@
DBG(KERN_INFO "%s: linktype set to %d\n", tun->dev->name, tun->dev->type);
ret = 0;
}
- rtnl_unlock();
break;
#ifdef TUN_DEBUG
@@ -1220,9 +1211,7 @@
break;
#endif
case TUNSETOFFLOAD:
- rtnl_lock();
ret = set_offload(tun->dev, arg);
- rtnl_unlock();
break;
case TUNSETTXFILTER:
@@ -1230,9 +1219,7 @@
ret = -EINVAL;
if ((tun->flags & TUN_TYPE_MASK) != TUN_TAP_DEV)
break;
- rtnl_lock();
ret = update_filter(&tun->txflt, (void __user *)arg);
- rtnl_unlock();
break;
case SIOCGIFHWADDR:
@@ -1248,9 +1235,7 @@
DBG(KERN_DEBUG "%s: set hw address: %pM\n",
tun->dev->name, ifr.ifr_hwaddr.sa_data);
- rtnl_lock();
ret = dev_set_mac_address(tun->dev, &ifr.ifr_hwaddr);
- rtnl_unlock();
break;
case TUNGETSNDBUF:
@@ -1273,7 +1258,10 @@
break;
};
- tun_put(tun);
+unlock:
+ rtnl_unlock();
+ if (tun)
+ tun_put(tun);
return ret;
}
@@ -1361,7 +1349,7 @@
.write = do_sync_write,
.aio_write = tun_chr_aio_write,
.poll = tun_chr_poll,
- .ioctl = tun_chr_ioctl,
+ .unlocked_ioctl = tun_chr_ioctl,
.open = tun_chr_open,
.release = tun_chr_close,
.fasync = tun_chr_fasync
