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 <jasow...@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
--
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/
