On Tue, Dec 11, 2012 at 07:03:47PM +0800, Jason Wang wrote: > Currently, when a file is attached to tuntap through TUNSETQUEUE, the uid/gid > and CAP_NET_ADMIN were checked, and we use this ioctl to create and destroy > queues. Sometimes, userspace such as qemu need to the ability to enable and > disable a specific queue without priveledge since guest operating system may > change the number of queues it want use. > > To support this kind of ability, this patch introduce a flag enabled which is > used to track whether the queue is enabled by userspace. And also restrict > that > only one deivce could be used for a queue to attach. With this patch, the DAC > checking when adding queues through IFF_ATTACH_QUEUE is still done and after > this, IFF_DETACH_QUEUE/IFF_ATTACH_QUEUE could be used to disable/enable this > queue. > > Signed-off-by: Jason Wang <jasow...@redhat.com> > --- > drivers/net/tun.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++----- > 1 files changed, 73 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index d593f56..43831a7 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -138,6 +138,7 @@ struct tun_file { > /* only used for fasnyc */ > unsigned int flags; > u16 queue_index; > + bool enabled; > }; > > struct tun_flow_entry { > @@ -345,9 +346,11 @@ unlock: > static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb) > { > struct tun_struct *tun = netdev_priv(dev); > + struct tun_file *tfile; > struct tun_flow_entry *e; > u32 txq = 0; > u32 numqueues = 0; > + int i; > > rcu_read_lock(); > numqueues = tun->numqueues; > @@ -366,6 +369,19 @@ static u16 tun_select_queue(struct net_device *dev, > struct sk_buff *skb) > txq -= numqueues; > } > > + tfile = rcu_dereference(tun->tfiles[txq]); > + if (unlikely(!tfile->enabled))
This unlikely tag is suspicious. It should be perfectly legal to use less queues than created. > + /* tun_detach() should make sure there's at least one queue > + * could be used to do the tranmission. > + */ > + for (i = 0; i < numqueues; i++) { > + tfile = rcu_dereference(tun->tfiles[i]); > + if (tfile->enabled) { > + txq = i; > + break; > + } > + } > + Worst case this will do a linear scan over all queueus on each packet. Instead, I think we need a list of all queues and only install the active ones in the array. > rcu_read_unlock(); > return txq; > } > @@ -386,6 +402,36 @@ static void tun_set_real_num_queues(struct tun_struct > *tun) > netif_set_real_num_rx_queues(tun->dev, tun->numqueues); > } > > +static int tun_enable(struct tun_file *tfile) > +{ > + if (tfile->enabled == true) simply if (tfile->enabled) > + return -EINVAL; Actually it's better to have operations be idempotent. If it's enabled, enabling should be a NOP not an error. > + > + tfile->enabled = true; > + return 0; > +} > + > +static int tun_disable(struct tun_file *tfile) > +{ > + struct tun_struct *tun = rcu_dereference_protected(tfile->tun, > + > lockdep_rtnl_is_held()); > + u16 index = tfile->queue_index; > + > + if (!tun) > + return -EINVAL; > + > + if (tun->numqueues == 1) > + return -EINVAL; So if there's a single queue we can't disable it, but if there are > 1 we can disable them all. This seems arbitrary. > + > + BUG_ON(index >= tun->numqueues); > + tfile->enabled = false; > + > + synchronize_net(); > + tun_flow_delete_by_queue(tun, index); > + > + return 0; > +} > + > static void __tun_detach(struct tun_file *tfile, bool clean) > { > struct tun_file *ntfile; > @@ -446,6 +492,7 @@ static void tun_detach_all(struct net_device *dev) > BUG_ON(!tfile); > wake_up_all(&tfile->wq.wait); > rcu_assign_pointer(tfile->tun, NULL); > + tfile->enabled = false; > --tun->numqueues; > } > BUG_ON(tun->numqueues != 0); > @@ -490,6 +537,7 @@ static int tun_attach(struct tun_struct *tun, struct file > *file) > rcu_assign_pointer(tun->tfiles[tun->numqueues], tfile); > sock_hold(&tfile->sk); > tun->numqueues++; > + tfile->enabled = true; > > tun_set_real_num_queues(tun); > > @@ -672,6 +720,10 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, > struct net_device *dev) > if (txq >= tun->numqueues) > goto drop; > > + /* Drop packet if the queue was not enabled */ > + if (!tfile->enabled) > + goto drop; > + > tun_debug(KERN_INFO, tun, "tun_net_xmit %d\n", skb->len); > > BUG_ON(!tfile); > @@ -1010,6 +1062,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, > struct tun_file *tfile, > bool zerocopy = false; > int err; > > + if (!tfile->enabled) > + return -EINVAL; > + > if (!(tun->flags & TUN_NO_PI)) { > if ((len -= sizeof(pi)) > total_len) > return -EINVAL; > @@ -1199,6 +1254,9 @@ static ssize_t tun_put_user(struct tun_struct *tun, > struct tun_pi pi = { 0, skb->protocol }; > ssize_t total = 0; > > + if (!tfile->enabled) > + return -EINVAL; > + > if (!(tun->flags & TUN_NO_PI)) { > if ((len -= sizeof(pi)) < 0) > return -EINVAL; > @@ -1769,15 +1827,21 @@ static int tun_set_queue(struct file *file, struct > ifreq *ifr) > if (dev->netdev_ops != &tap_netdev_ops && > dev->netdev_ops != &tun_netdev_ops) > ret = -EINVAL; > - else if (tun_not_capable(tun)) > - ret = -EPERM; > - /* TUNSETIFF is needed to do permission checking */ > - else if (tun->numqueues == 0) > - ret = -EPERM; > - else > - ret = tun_attach(tun, file); > + else { > + if (!rcu_dereference(tfile->tun)) { Should be rcu_dereference_protected. > + if (tun_not_capable(tun) || > + tun->numqueues == 0) > + ret = -EPERM; > + else > + ret = tun_attach(tun, file); > + } > + else { > + /* FIXME: permission check? */ > + ret = tun_enable(tfile); > + } > + } > } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) > - __tun_detach(tfile, false); > + tun_disable(tfile); > else > ret = -EINVAL; > > @@ -2085,6 +2149,7 @@ static int tun_chr_open(struct inode *inode, struct > file * file) > tfile->socket.file = file; > tfile->socket.ops = &tun_socket_ops; > > + tfile->enabled = false; > sock_init_data(&tfile->socket, &tfile->sk); > sk_change_net(&tfile->sk, tfile->net); > > -- > 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/