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

[Vlad Yasevich <vyasevic@redhat.com>]

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...

-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