Re: [PATCH v3 2/2] macvtap: Allow tap features change when IFF_VNET_HDR is disabled.

[Vlad Yasevich <vyasevic@redhat.com>]

On 08/15/2013 02:48 PM, Michael S. Tsirkin wrote:
> On Thu, Aug 15, 2013 at 02:39:39PM -0400, Vlad Yasevich wrote:
>> On 08/15/2013 01:33 PM, Michael S. Tsirkin wrote:
>>> On Thu, Aug 15, 2013 at 01:02:53PM -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.  Also, take the setting of IFF_VNET_HDR into consideration
>>>> when determining the feature set to apply to incommig traffic.
>>>> If IFF_VNET_HDR is set, we use user-specfied feature set.  if the
>>>> IFF_VNET_HDR is clear, we mask off all tun-specific offload features,
>>>> since user can not be notified of the possbile offloads.
>>>>
>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>>> ---
>>>>   drivers/net/macvtap.c | 30 ++++++++++++++++++++++++------
>>>>   1 file changed, 24 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>>>> index 8121358..df45a59 100644
>>>> --- a/drivers/net/macvtap.c
>>>> +++ b/drivers/net/macvtap.c
>>>> @@ -269,6 +269,28 @@ static void macvtap_del_queues(struct net_device *dev)
>>>>   		sock_put(&qlist[j]->sk);
>>>>   }
>>>>
>>>> +/* Determine features to be applied to the packet as it
>>>
>>> /*
>>>   * Please use comments
>>>   * like this
>>>   */
>>>
>>>> + * gets sent to the tap.  The features depend on the offloads
>>>> + * set by user and by whether or not IFF_VNET_HDR is enabled.
>>>> + */
>>>> +static netdev_features_t macvtap_skb_features(struct sk_buff *skb,
>>>> +					      struct macvtap_queue *q)
>>>> +{
>>>> +	struct macvlan_dev *vlan = netdev_priv(skb->dev);
>>>> +	netdev_features_t features = netif_skb_features(skb);
>>>> +
>>>> +	/* If VNET_HDR is enabled, user can receive offload info so
>>>> +	 * use user-specified tap_features.
>>>> +	 * Otherwise, mask off all tap offload features.
>>>> +	 */
>>>> +	if (q->flags & IFF_VNET_HDR)
>>>> +		features &= vlan->tap_features;
>>>> +	else
>>>> +		features &= ~TUN_OFFLOADS;
>>>> +
>>>> +	return features;
>>>> +}
>>>> +
>>>
>>> Okay, but this means we need to do netdev_update_features
>>> on SETIFF since it changes q->flags.
>>
>> Why?
>>   The whole point it not to change features on SETIFF, so that
>> they are preserved for when the flags change again.  It just that
>> flag values either allow whatever offload user specified (correctly
>> masked via netdev_update_features) or turn off all offloads at the tap.
>>
>> -vlad
>
> That's the point. As it is, if flag value changes, we don't call
> netdev_update_features so offloads aren't masked/unmasked.
>
> Example:
>
> 1. SETOFFLOADS to enable
> 2. VNET = 0
>
> offloads are still enabled
>

But unused since since VNET=0.  I am trying to be careful not to
break things when ioctl calls get all inverted and reversed, but
may be I am trying too hard and we should just let things break.
I just feels kinds of silly since the app may now know all the
details, but the kernel does and can do the "right thing".

-vlad

>
>>>>   /*
>>>>    * Forward happens for data that gets sent from one macvlan
>>>>    * endpoint to another one in bridge mode. We just take
>>>> @@ -276,9 +298,9 @@ static void macvtap_del_queues(struct net_device *dev)
>>>>    */
>>>>   static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
>>>>   {
>>>> -	struct macvlan_dev *vlan = netdev_priv(dev);
>>>>   	struct macvtap_queue *q = macvtap_get_queue(dev, skb);
>>>>   	netdev_features_t features;
>>>> +
>>>>   	if (!q)
>>>>   		goto drop;
>>>>
>>>> @@ -289,7 +311,7 @@ static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
>>>>   	/* Apply the forward feature mask so that we perform segmentation
>>>>   	 * according to users wishes.
>>>>   	 */
>>>> -	features = netif_skb_features(skb) & vlan->tap_features;
>>>> +	features = macvtap_skb_features(skb, q);
>>>>   	if (netif_needs_gso(skb, features)) {
>>>>   		struct sk_buff *segs = __skb_gso_segment(skb, features, false);
>>>>
>>>> @@ -1161,10 +1183,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