Re: [net-next rfc V3 7/9] macvtap: allow TUNSETIFF to create multiqueue device

["Michael S. Tsirkin" <mst@redhat.com>]

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