Re: [PATCH] macvtap: Correctly set tap features when IFF_VNET_HDR is disabled.

[Vlad Yasevich <vyasevic@redhat.com>]

On 08/14/2013 10:07 AM, Vlad Yasevich wrote:
> On 08/14/2013 08:10 AM, Michael S. Tsirkin wrote:
>> On Tue, Aug 13, 2013 at 01:55:58PM -0400, Vlad Yasevich wrote:
>>> When the user turns off IFF_VNET_HDR flag, attempts to change
>>> offload features via TUNSETOFFLOAD do not work.  This could cause
>>> GSO packets to be delivered to the user when the user is
>>> not prepared to handle them.
>>>
>>> To solve, allow processing of TUNSETOFFLOAD when IFF_VNET_HDR is
>>> disabled and make sure to turn off all offloads in this case.
>>> Also, when IFF_VNET_HDR is disabled, run throught the offload change
>>> as well to make sure that the functionality is not dependent on
>>> the order of the ioclt() calls.
>>>
>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>
>> Hmm, this seems to make it even messier.
>> So
>>     set TUNSETOFFLOAD
>>     clear IFF_VNET_HDR
>>     set IFF_VNET_HDR
>>
>> offloads are now clear?
>>
>> I'm also not sure userspace that sets offloads but clears
>> vnet hdr knows what it's doing.
>>
>> How about we fail attempts to clear vnet hdr
>> unless offloads are disabled?
>>
>
> OK.  That makes it simpler...

Spoke too soon.

What happens is that libvirt is typically the one that turns off
IFF_VNET_HDR, and qemu controls the offloads.

So, if we forbid turning off vnet_hdr while offloads are set, that
will break libvirt (this is because macvtap defaults to tap offloads 
being enabled).

-vlad

>
> -vlad
>
>>
>>
>>
>>> ---
>>>   drivers/net/macvtap.c | 27 +++++++++++++++++++--------
>>>   1 file changed, 19 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>>> index a98fb0e..076b9e7 100644
>>> --- a/drivers/net/macvtap.c
>>> +++ b/drivers/net/macvtap.c
>>> @@ -1019,6 +1019,7 @@ static int set_offload(struct macvtap_queue *q,
>>> unsigned long arg)
>>>       struct macvlan_dev *vlan;
>>>       netdev_features_t features;
>>>       netdev_features_t feature_mask = 0;
>>> +    netdev_features_t tap_mask = TUN_OFFLOADS;
>>>
>>>       vlan = rtnl_dereference(q->vlan);
>>>       if (!vlan)
>>> @@ -1026,7 +1027,12 @@ static int set_offload(struct macvtap_queue
>>> *q, unsigned long arg)
>>>
>>>       features = vlan->dev->features;
>>>
>>> -    if (arg & TUN_F_CSUM) {
>>> +    if (!(q->flags & IFF_VNET_HDR)) {
>>> +        /* Turn off all checsum offloading also if user does
>>> +         * not user vnet_hdr.
>>
>> Does not set?
>>
>>> +         */
>>> +        tap_mask |= NETIF_F_ALL_CSUM;
>>> +    } else if (arg & TUN_F_CSUM) {
>>>           feature_mask = NETIF_F_HW_CSUM;
>>>
>>>           if (arg & (TUN_F_TSO4 | TUN_F_TSO6)) {
>>> @@ -1058,8 +1064,7 @@ static int set_offload(struct macvtap_queue *q,
>>> unsigned long arg)
>>>       /* tap_features are the same as features on tun/tap and
>>>        * reflect user expectations.
>>>        */
>>> -    vlan->tap_features = vlan->dev->features &
>>> -                (feature_mask | ~TUN_OFFLOADS);
>>> +    vlan->tap_features = vlan->dev->features & (feature_mask |
>>> ~tap_mask);
>>>       vlan->set_features = features;
>>>       netdev_update_features(vlan->dev);
>>>
>>> @@ -1092,8 +1097,18 @@ static long macvtap_ioctl(struct file *file,
>>> unsigned int cmd,
>>>           if ((u & ~(IFF_VNET_HDR | IFF_MULTI_QUEUE)) !=
>>>               (IFF_NO_PI | IFF_TAP))
>>>               ret = -EINVAL;
>>> -        else
>>> +        else {
>>> +            if ((q->flags ^ u) & IFF_VNET_HDR) {
>>> +                /* vnet_hdr support impacts the offloads,
>>> +                 * so we need to run throught the offload
>>
>> throught ?
>>
>>> +                 * change.
>>> +                 */
>>> +                rtnl_lock();
>>> +                ret = set_offload(q, 0);
>>> +                rtnl_unlock();
>>> +            }
>>>               q->flags = u;
>>> +        }
>>>
>>>           return ret;
>>>
>>> @@ -1155,10 +1170,6 @@ static long macvtap_ioctl(struct file *file,
>>> unsigned int cmd,
>>>                   TUN_F_TSO_ECN | TUN_F_UFO))
>>>               return -EINVAL;
>>>
>>> -        /* TODO: only accept frames with the features that
>>> -             got enabled for forwarded frames */
>>> -        if (!(q->flags & IFF_VNET_HDR))
>>> -            return  -EINVAL;
>>>           rtnl_lock();
>>>           ret = set_offload(q, arg);
>>>           rtnl_unlock();
>>> --
>>> 1.8.1.4
>