Messages in this thread | | | Date | Thu, 6 Jun 2013 09:59:05 +0300 | From | "Michael S. Tsirkin" <> | Subject | Re: [net-next rfc V3 7/9] macvtap: allow TUNSETIFF to create multiqueue device |
| |
On Thu, Jun 06, 2013 at 11:13:29AM +0800, Jason Wang wrote: > On 06/05/2013 06:43 PM, Michael S. Tsirkin wrote: > > On Wed, Jun 05, 2013 at 02:36:30PM +0800, Jason Wang wrote: > >> Though the queue were in fact created by open(), we still need to add this check > >> to be compatible with tuntap which can let mgmt software use a single API to > >> manage queues. This patch only validates the device name and moves the TUNSETIFF > >> to a helper. > >> > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > > The patch is OK, the description is confusing. > > What you mean is simply: > > > > Allow IFF_MULTI_QUEUE in TUNSETIFF for macvtap, to match > > tun behaviour. > > > > And if you put it like this, I would say make this > > the last patch in the series, so userspace > > can use IFF_MULTI_QUEUE to detect new versus old > > behaviour. > > Make sense, thanks. > > > >> --- > >> drivers/net/macvtap.c | 51 ++++++++++++++++++++++++++++++++++++++---------- > >> 1 files changed, 40 insertions(+), 11 deletions(-) > >> > >> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c > >> index 5ccba99..14764cc 100644 > >> --- a/drivers/net/macvtap.c > >> +++ b/drivers/net/macvtap.c > >> @@ -869,6 +869,7 @@ out: > >> return ret; > >> } > >> > >> + > >> static struct macvlan_dev *macvtap_get_vlan(struct macvtap_queue *q) > >> { > >> struct macvlan_dev *vlan; > > Please don't. > > Ok. > > > >> @@ -887,6 +888,44 @@ static void macvtap_put_vlan(struct macvlan_dev *vlan) > >> dev_put(vlan->dev); > >> } > >> > >> +static int macvtap_set_iff(struct file *file, struct ifreq __user *ifr_u) > >> +{ > >> + struct macvtap_queue *q = file->private_data; > >> + struct net *net = current->nsproxy->net_ns; > >> + struct inode *inode = file_inode(file); > >> + struct net_device *dev, *dev2; > >> + struct ifreq ifr; > >> + > >> + if (copy_from_user(&ifr, ifr_u, sizeof(struct ifreq))) > >> + return -EFAULT; > >> + > >> + /* To keep the same behavior of tuntap, validate ifr_name */ > > So I'm not sure - why is it important to validate ifr_name here? > > We ignore the name for all other flags - why is IFF_MULTI_QUEUE > > special? > > It raises another question, why not validate ifname like tuntap? We > should warn userspace about their error, otherwise they may create > queues on the wrong device. In fact I want validate for both, but keep > the behaviour w/o IFF_MULTI_QUEUE for backward compatibility.
Basically macvtap ignores ifr_name because it doesn't need it. Making it ignore it without IFF_MULTI_QUEUE but not with IFF_MULTI_QUEUE seems ugly.
Do you think we'll need ifr_name at some point? Why not validate then, when we actually do?
> > > >> + if (ifr.ifr_flags & IFF_MULTI_QUEUE) { > >> + dev = __dev_get_by_name(net, ifr.ifr_name); > >> + if (!dev) > >> + return -EINVAL; > >> + > >> + dev2 = dev_get_by_macvtap_minor(iminor(inode)); > >> + if (!dev2) > >> + return -EINVAL; > >> + > >> + if (dev != dev2) { > >> + dev_put(dev2); > >> + return -EINVAL; > >> + } > >> + > >> + dev_put(dev2); > >> + } > >> + > >> + if ((ifr.ifr_flags & ~(IFF_VNET_HDR | IFF_MULTI_QUEUE)) != > >> + (IFF_NO_PI | IFF_TAP)) > >> + return -EINVAL; > >> + else > >> + q->flags = ifr.ifr_flags; > >> + > >> + return 0; > >> +} > >> + > >> /* > >> * provide compatibility with generic tun/tap interface > >> */ > >> @@ -905,17 +944,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd, > >> > >> switch (cmd) { > >> case TUNSETIFF: > >> - /* ignore the name, just look at flags */ > > This is actually a useful comment that you've removed. > > Will get it back. > > > >> - if (get_user(u, &ifr->ifr_flags)) > >> - return -EFAULT; > >> - > >> - ret = 0; > >> - if ((u & ~IFF_VNET_HDR) != (IFF_NO_PI | IFF_TAP)) > >> - ret = -EINVAL; > >> - else > >> - q->flags = u; > >> - > >> - return ret; > >> + return macvtap_set_iff(file, ifr); > >> > >> case TUNGETIFF: > >> vlan = macvtap_get_vlan(q); > >> -- > >> 1.7.1
| |