[PATCH v3 0/2] Correctly perform offloads when VNET_HDR is disabled

[PATCH v3 0/2] Correctly perform offloads when VNET_HDR is disabled

[Vlad Yasevich]

This is v3 of the patch.  It as been spit into 2 sperate patches as requested.
The asymetry of behavior has been removed.  Now it doesn't matter which
order the VNET_HDR and TUNSETOFFLOAD are performed.  The TUNSETOFFLOAD
ioctl always modifies the ioctl, and IFF_VNET_HDR can be turned on and off
independed of that.  What is different is that when computing features for
GSO, the value of IFF_VNET_HDR is also consulted and a different mask is used.
This way, the following sequence:
	clear IFF_VNET_HDR
	change TUNSETOFFLOAD
	set IFF_VNET_HDR
works and packet packets are delivered in correct format at all times.

Vlad Yasevich (2):
  macvtap: include all checksum offloads in TUN_OFFLOAD mask
  macvtap: Correctly set tap features when IFF_VNET_HDR is disabled.

 drivers/net/macvtap.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

-- 
1.8.1.4

[PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask

[Vlad Yasevich]

The features of the macvlan are based on the features of lower
device and thus can have checksum featurs other then IFF_F_HW_CSUM
set.  However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM.  Thus
when performing gso segmentation during macvtap_forward(),
it is possbile to end up with skbs that have ip_summed set
to CHECKSUM_PARTIAL.  This is incorrect when the user
turns off checksum offloading.

Include all possible checksum offload values so that
we'll properly mask them off when performing GSO.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 drivers/net/macvtap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index b51db2a..8121358 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
 
 static const struct proto_ops macvtap_socket_ops;
 
-#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
+#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
 		      NETIF_F_TSO6 | NETIF_F_UFO)
 #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
 /*
-- 
1.8.1.4

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

[Vlad Yasevich]

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
+ * 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;
+}
+
 /*
  * 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

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

[Michael S. Tsirkin]

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.

>  /*
>   * 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

Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask

[Michael S. Tsirkin]

On Thu, Aug 15, 2013 at 01:02:52PM -0400, Vlad Yasevich wrote:
> The features of the macvlan are based on the features of lower
> device and thus can have checksum featurs other then IFF_F_HW_CSUM

s/featurs/features/

:set spell spelllang=en_us

or whatever's the equivalent in your editor of choice.

> set.  However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM.  Thus
> when performing gso segmentation during macvtap_forward(),
> it is possbile to end up with skbs that have ip_summed set
> to CHECKSUM_PARTIAL.  This is incorrect when the user
> turns off checksum offloading.
> 
> Include all possible checksum offload values so that
> we'll properly mask them off when performing GSO.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>  drivers/net/macvtap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index b51db2a..8121358 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
>  
>  static const struct proto_ops macvtap_socket_ops;
>  
> -#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> +#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
>  		      NETIF_F_TSO6 | NETIF_F_UFO)
>  #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
>  /*

Okay so you are talking about hardware that sets some other
checksum bit besides HW_CSUM, e.g. IP_CSUM, so

        vlan->tap_features = vlan->dev->features &
                            (feature_mask | ~TUN_OFFLOADS);

will not clear IP_CSUM even if feature_mask is 0.

Maybe mention this in the changelog, in case user
will wonder whether his hardware is affected.

So I agree, that's a bug, but if you make this change the reverse will hold
(on this hardware):
if user sets TUN_F_CSUM, we won't set NETIF_F_IP_CSUM
so checksum offloading won't work.

So I think you want s/NETIF_F_HW_CSUM/NETIF_F_ALL_CSUM/ everywhere
and not just in this place.

Also - Cc stable?


> -- 
> 1.8.1.4

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

[Vlad Yasevich]

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

>>   /*
>>    * 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

Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask

[Vlad Yasevich]

On 08/15/2013 02:24 PM, Michael S. Tsirkin wrote:
> On Thu, Aug 15, 2013 at 01:02:52PM -0400, Vlad Yasevich wrote:
>> The features of the macvlan are based on the features of lower
>> device and thus can have checksum featurs other then IFF_F_HW_CSUM
>
> s/featurs/features/
>
> :set spell spelllang=en_us
>
> or whatever's the equivalent in your editor of choice.
>
>> set.  However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM.  Thus
>> when performing gso segmentation during macvtap_forward(),
>> it is possbile to end up with skbs that have ip_summed set
>> to CHECKSUM_PARTIAL.  This is incorrect when the user
>> turns off checksum offloading.
>>
>> Include all possible checksum offload values so that
>> we'll properly mask them off when performing GSO.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>>   drivers/net/macvtap.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>> index b51db2a..8121358 100644
>> --- a/drivers/net/macvtap.c
>> +++ b/drivers/net/macvtap.c
>> @@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
>>
>>   static const struct proto_ops macvtap_socket_ops;
>>
>> -#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
>> +#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
>>   		      NETIF_F_TSO6 | NETIF_F_UFO)
>>   #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
>>   /*
>
> Okay so you are talking about hardware that sets some other
> checksum bit besides HW_CSUM, e.g. IP_CSUM, so
>
>          vlan->tap_features = vlan->dev->features &
>                              (feature_mask | ~TUN_OFFLOADS);
>
> will not clear IP_CSUM even if feature_mask is 0.
>
> Maybe mention this in the changelog, in case user
> will wonder whether his hardware is affected.
>
> So I agree, that's a bug, but if you make this change the reverse will hold
> (on this hardware):
> if user sets TUN_F_CSUM, we won't set NETIF_F_IP_CSUM
> so checksum offloading won't work.
>
> So I think you want s/NETIF_F_HW_CSUM/NETIF_F_ALL_CSUM/ everywhere
> and not just in this place.

Yes.  I thought about that, but forgot to do in this patch set.

>
> Also - Cc stable?
>

The offloads work went into 3.11, so I don't think there is a need for 
stable.

-vlad
>
>> --
>> 1.8.1.4

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

[Michael S. Tsirkin]

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


> >>  /*
> >>   * 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

Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask

[Michael S. Tsirkin]

On Thu, Aug 15, 2013 at 02:45:30PM -0400, Vlad Yasevich wrote:
> On 08/15/2013 02:24 PM, Michael S. Tsirkin wrote:
> >On Thu, Aug 15, 2013 at 01:02:52PM -0400, Vlad Yasevich wrote:
> >>The features of the macvlan are based on the features of lower
> >>device and thus can have checksum featurs other then IFF_F_HW_CSUM
> >
> >s/featurs/features/
> >
> >:set spell spelllang=en_us
> >
> >or whatever's the equivalent in your editor of choice.
> >
> >>set.  However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM.  Thus
> >>when performing gso segmentation during macvtap_forward(),
> >>it is possbile to end up with skbs that have ip_summed set
> >>to CHECKSUM_PARTIAL.  This is incorrect when the user
> >>turns off checksum offloading.
> >>
> >>Include all possible checksum offload values so that
> >>we'll properly mask them off when performing GSO.
> >>
> >>Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >>---
> >>  drivers/net/macvtap.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >>index b51db2a..8121358 100644
> >>--- a/drivers/net/macvtap.c
> >>+++ b/drivers/net/macvtap.c
> >>@@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
> >>
> >>  static const struct proto_ops macvtap_socket_ops;
> >>
> >>-#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> >>+#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> >>  		      NETIF_F_TSO6 | NETIF_F_UFO)
> >>  #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
> >>  /*
> >
> >Okay so you are talking about hardware that sets some other
> >checksum bit besides HW_CSUM, e.g. IP_CSUM, so
> >
> >         vlan->tap_features = vlan->dev->features &
> >                             (feature_mask | ~TUN_OFFLOADS);
> >
> >will not clear IP_CSUM even if feature_mask is 0.
> >
> >Maybe mention this in the changelog, in case user
> >will wonder whether his hardware is affected.
> >
> >So I agree, that's a bug, but if you make this change the reverse will hold
> >(on this hardware):
> >if user sets TUN_F_CSUM, we won't set NETIF_F_IP_CSUM
> >so checksum offloading won't work.
> >
> >So I think you want s/NETIF_F_HW_CSUM/NETIF_F_ALL_CSUM/ everywhere
> >and not just in this place.
> 
> Yes.  I thought about that, but forgot to do in this patch set.
> 
> >
> >Also - Cc stable?
> >
> 
> The offloads work went into 3.11, so I don't think there is a need
> for stable.

Right. Thanks for the correction.

> -vlad
> >
> >>--
> >>1.8.1.4

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

[Vlad Yasevich]

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

Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask

[Michael S. Tsirkin]

On Thu, Aug 15, 2013 at 02:45:30PM -0400, Vlad Yasevich wrote:
> On 08/15/2013 02:24 PM, Michael S. Tsirkin wrote:
> >On Thu, Aug 15, 2013 at 01:02:52PM -0400, Vlad Yasevich wrote:
> >>The features of the macvlan are based on the features of lower
> >>device and thus can have checksum featurs other then IFF_F_HW_CSUM
> >
> >s/featurs/features/
> >
> >:set spell spelllang=en_us
> >
> >or whatever's the equivalent in your editor of choice.
> >
> >>set.  However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM.  Thus
> >>when performing gso segmentation during macvtap_forward(),
> >>it is possbile to end up with skbs that have ip_summed set
> >>to CHECKSUM_PARTIAL.  This is incorrect when the user
> >>turns off checksum offloading.
> >>
> >>Include all possible checksum offload values so that
> >>we'll properly mask them off when performing GSO.
> >>
> >>Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >>---
> >>  drivers/net/macvtap.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >>index b51db2a..8121358 100644
> >>--- a/drivers/net/macvtap.c
> >>+++ b/drivers/net/macvtap.c
> >>@@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
> >>
> >>  static const struct proto_ops macvtap_socket_ops;
> >>
> >>-#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> >>+#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> >>  		      NETIF_F_TSO6 | NETIF_F_UFO)
> >>  #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
> >>  /*
> >
> >Okay so you are talking about hardware that sets some other
> >checksum bit besides HW_CSUM, e.g. IP_CSUM, so
> >
> >         vlan->tap_features = vlan->dev->features &
> >                             (feature_mask | ~TUN_OFFLOADS);
> >
> >will not clear IP_CSUM even if feature_mask is 0.
> >
> >Maybe mention this in the changelog, in case user
> >will wonder whether his hardware is affected.
> >
> >So I agree, that's a bug, but if you make this change the reverse will hold
> >(on this hardware):
> >if user sets TUN_F_CSUM, we won't set NETIF_F_IP_CSUM
> >so checksum offloading won't work.
> >
> >So I think you want s/NETIF_F_HW_CSUM/NETIF_F_ALL_CSUM/ everywhere
> >and not just in this place.
> 
> Yes.  I thought about that, but forgot to do in this patch set.

In fact I wonder why do we do it like this at all.
tap_features have nothing to do with the device,
they only have to do with tap.
For example, you might have a dumb device with no
checksum support but userspace said it does not need a checksum,
we can have NETIF_F_HW_CSUM there anyway.

So why don't we just
        vlan->tap_features = feature_mask;
?
In fact, this is exactly set_features, so why don't
we just use set_features everywhere?
Then this patch won't be needed at all.

Kind of like this - compile-tested only - do you have a setup with
IP_CSUM which you can use to test?

-->

macvtap: fix up tap features

There's no apparent need to have separate tap features
masked according to the lowerdev config:
we only use them in software so hardware configuration
does not matter.
Drop tap_features and use set_features directly


This fixes bugs for some unusual hardware configurations,
for example, we only masked HW_CSUM so for hardware with
e.g. IP_CSUM the checksum bit remained set, which would cause
packets with CHECKSUM_PARTIAL to be sent to userspace
even when offloads are disabled.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index a98ed9f..26cea91 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -289,7 +289,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 = netif_skb_features(skb) & vlan->set_features;
 	if (netif_needs_gso(skb, features)) {
 		struct sk_buff *segs = __skb_gso_segment(skb, features, false);
 
@@ -382,11 +382,6 @@ static int macvtap_newlink(struct net *src_net,
 	struct macvlan_dev *vlan = netdev_priv(dev);
 	INIT_LIST_HEAD(&vlan->queue_list);
 
-	/* Since macvlan supports all offloads by default, make
-	 * tap support all offloads also.
-	 */
-	vlan->tap_features = TUN_OFFLOADS;
-
 	/* Don't put anything that may fail after macvlan_common_newlink
 	 * because we can't undo what it does.
 	 */
@@ -1027,7 +1022,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
 	features = vlan->dev->features;
 
 	if (arg & TUN_F_CSUM) {
-		feature_mask = NETIF_F_HW_CSUM;
+		feature_mask = NETIF_F_HW_CSUM;
 
 		if (arg & (TUN_F_TSO4 | TUN_F_TSO6)) {
 			if (arg & TUN_F_TSO_ECN)
@@ -1055,11 +1050,6 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
 	else
 		features &= ~RX_OFFLOADS;
 
-	/* 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->set_features = features;
 	netdev_update_features(vlan->dev);
 
diff --git a/include/linux/if_macvlan.h b/include/linux/if_macvlan.h
index ddd33fd..e446e82 100644
--- a/include/linux/if_macvlan.h
+++ b/include/linux/if_macvlan.h
@@ -76,7 +76,6 @@ struct macvlan_dev {
 	struct list_head	queue_list;
 	int			numvtaps;
 	int			numqueues;
-	netdev_features_t	tap_features;
 	int			minor;
 };
 

Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask

[Michael S. Tsirkin]

On Thu, Aug 15, 2013 at 10:09:45PM +0300, Michael S. Tsirkin wrote:
> On Thu, Aug 15, 2013 at 02:45:30PM -0400, Vlad Yasevich wrote:
> > On 08/15/2013 02:24 PM, Michael S. Tsirkin wrote:
> > >On Thu, Aug 15, 2013 at 01:02:52PM -0400, Vlad Yasevich wrote:
> > >>The features of the macvlan are based on the features of lower
> > >>device and thus can have checksum featurs other then IFF_F_HW_CSUM
> > >
> > >s/featurs/features/
> > >
> > >:set spell spelllang=en_us
> > >
> > >or whatever's the equivalent in your editor of choice.
> > >
> > >>set.  However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM.  Thus
> > >>when performing gso segmentation during macvtap_forward(),
> > >>it is possbile to end up with skbs that have ip_summed set
> > >>to CHECKSUM_PARTIAL.  This is incorrect when the user
> > >>turns off checksum offloading.
> > >>
> > >>Include all possible checksum offload values so that
> > >>we'll properly mask them off when performing GSO.
> > >>
> > >>Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> > >>---
> > >>  drivers/net/macvtap.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >>diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> > >>index b51db2a..8121358 100644
> > >>--- a/drivers/net/macvtap.c
> > >>+++ b/drivers/net/macvtap.c
> > >>@@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
> > >>
> > >>  static const struct proto_ops macvtap_socket_ops;
> > >>
> > >>-#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> > >>+#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> > >>  		      NETIF_F_TSO6 | NETIF_F_UFO)
> > >>  #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
> > >>  /*
> > >
> > >Okay so you are talking about hardware that sets some other
> > >checksum bit besides HW_CSUM, e.g. IP_CSUM, so
> > >
> > >         vlan->tap_features = vlan->dev->features &
> > >                             (feature_mask | ~TUN_OFFLOADS);
> > >
> > >will not clear IP_CSUM even if feature_mask is 0.
> > >
> > >Maybe mention this in the changelog, in case user
> > >will wonder whether his hardware is affected.
> > >
> > >So I agree, that's a bug, but if you make this change the reverse will hold
> > >(on this hardware):
> > >if user sets TUN_F_CSUM, we won't set NETIF_F_IP_CSUM
> > >so checksum offloading won't work.
> > >
> > >So I think you want s/NETIF_F_HW_CSUM/NETIF_F_ALL_CSUM/ everywhere
> > >and not just in this place.
> > 
> > Yes.  I thought about that, but forgot to do in this patch set.
> 
> In fact I wonder why do we do it like this at all.
> tap_features have nothing to do with the device,
> they only have to do with tap.
> For example, you might have a dumb device with no
> checksum support but userspace said it does not need a checksum,
> we can have NETIF_F_HW_CSUM there anyway.
> 
> So why don't we just
>         vlan->tap_features = feature_mask;
> ?
> In fact, this is exactly set_features, so why don't
> we just use set_features everywhere?
> Then this patch won't be needed at all.
> 
> Kind of like this - compile-tested only - do you have a setup with
> IP_CSUM which you can use to test?

Ugh this does not work even on a simple setup, sorry about the noise.
Maybe like this? Does this work for you?

-->

macvtap: fix up tap features

There's no apparent need to have tap features
masked according to the lowerdev config:
we only use them in software so hardware configuration
does not matter.

This patch fixes bugs for some unusual hardware configurations,
for example, we only masked HW_CSUM so for hardware with
e.g. IP_CSUM the checksum bit remained set, which would cause
packets with CHECKSUM_PARTIAL to be sent to userspace
even when offloads are disabled.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index a98ed9f..3c18f12 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -1058,8 +1058,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 = feature_mask;
 	vlan->set_features = features;
 	netdev_update_features(vlan->dev);
 

Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask

[Vlad Yasevich]

On 08/15/2013 03:20 PM, Michael S. Tsirkin wrote:
> On Thu, Aug 15, 2013 at 10:09:45PM +0300, Michael S. Tsirkin wrote:
>> On Thu, Aug 15, 2013 at 02:45:30PM -0400, Vlad Yasevich wrote:
>>> On 08/15/2013 02:24 PM, Michael S. Tsirkin wrote:
>>>> On Thu, Aug 15, 2013 at 01:02:52PM -0400, Vlad Yasevich wrote:
>>>>> The features of the macvlan are based on the features of lower
>>>>> device and thus can have checksum featurs other then IFF_F_HW_CSUM
>>>>
>>>> s/featurs/features/
>>>>
>>>> :set spell spelllang=en_us
>>>>
>>>> or whatever's the equivalent in your editor of choice.
>>>>
>>>>> set.  However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM.  Thus
>>>>> when performing gso segmentation during macvtap_forward(),
>>>>> it is possbile to end up with skbs that have ip_summed set
>>>>> to CHECKSUM_PARTIAL.  This is incorrect when the user
>>>>> turns off checksum offloading.
>>>>>
>>>>> Include all possible checksum offload values so that
>>>>> we'll properly mask them off when performing GSO.
>>>>>
>>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>>>> ---
>>>>>   drivers/net/macvtap.c | 2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>>>>> index b51db2a..8121358 100644
>>>>> --- a/drivers/net/macvtap.c
>>>>> +++ b/drivers/net/macvtap.c
>>>>> @@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
>>>>>
>>>>>   static const struct proto_ops macvtap_socket_ops;
>>>>>
>>>>> -#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
>>>>> +#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
>>>>>   		      NETIF_F_TSO6 | NETIF_F_UFO)
>>>>>   #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
>>>>>   /*
>>>>
>>>> Okay so you are talking about hardware that sets some other
>>>> checksum bit besides HW_CSUM, e.g. IP_CSUM, so
>>>>
>>>>          vlan->tap_features = vlan->dev->features &
>>>>                              (feature_mask | ~TUN_OFFLOADS);
>>>>
>>>> will not clear IP_CSUM even if feature_mask is 0.
>>>>
>>>> Maybe mention this in the changelog, in case user
>>>> will wonder whether his hardware is affected.
>>>>
>>>> So I agree, that's a bug, but if you make this change the reverse will hold
>>>> (on this hardware):
>>>> if user sets TUN_F_CSUM, we won't set NETIF_F_IP_CSUM
>>>> so checksum offloading won't work.
>>>>
>>>> So I think you want s/NETIF_F_HW_CSUM/NETIF_F_ALL_CSUM/ everywhere
>>>> and not just in this place.
>>>
>>> Yes.  I thought about that, but forgot to do in this patch set.
>>
>> In fact I wonder why do we do it like this at all.
>> tap_features have nothing to do with the device,
>> they only have to do with tap.
>> For example, you might have a dumb device with no
>> checksum support but userspace said it does not need a checksum,
>> we can have NETIF_F_HW_CSUM there anyway.
>>
>> So why don't we just
>>          vlan->tap_features = feature_mask;
>> ?
>> In fact, this is exactly set_features, so why don't
>> we just use set_features everywhere?
>> Then this patch won't be needed at all.
>>
>> Kind of like this - compile-tested only - do you have a setup with
>> IP_CSUM which you can use to test?
>
> Ugh this does not work even on a simple setup, sorry about the noise.
> Maybe like this? Does this work for you?
>

No, this is really no different then what I had to start with.

> -->
>
> macvtap: fix up tap features
>
> There's no apparent need to have tap features
> masked according to the lowerdev config:
> we only use them in software so hardware configuration
> does not matter.
>
> This patch fixes bugs for some unusual hardware configurations,
> for example, we only masked HW_CSUM so for hardware with
> e.g. IP_CSUM the checksum bit remained set, which would cause
> packets with CHECKSUM_PARTIAL to be sent to userspace
> even when offloads are disabled.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index a98ed9f..3c18f12 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -1058,8 +1058,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 = feature_mask;

This wouldn't work when tap_features = 0, since the skb features in 
forward would evaluate to 0.  This means that netif_needs_gso() will 
return false and a GSO packet will be queued to the socket without going 
though segmentation.

-vlad


>   	vlan->set_features = features;
>   	netdev_update_features(vlan->dev);
>
>

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

[Michael S. Tsirkin]

On Thu, Aug 15, 2013 at 02:59:01PM -0400, Vlad Yasevich wrote:
> 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.


Well, I think that's true but it would still be better
to disable RX offloads, no?

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

What exactly will break by call to netdev_update_features?

> 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

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

[Vlad Yasevich]

On 08/15/2013 03:27 PM, Michael S. Tsirkin wrote:
> On Thu, Aug 15, 2013 at 02:59:01PM -0400, Vlad Yasevich wrote:
>> 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.
>
>
> Well, I think that's true but it would still be better
> to disable RX offloads, no?

Disabled RX offloads doesn't do anything since rx offload
processing only happens on the lowest level device and not
macvlan.  I've toyed with disabling RX offloads on the lowest
device when all macvlans above it had RX disabled, but the
performance was lower then performing GSO.  Didn't dig into
the why.

>
>>   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.
>
> What exactly will break by call to netdev_update_features?

I was referring to the proposed patch, not to calling 
netdev_update_features.  Sorry for the confusion.

There macvlan device features aren't changing, so why call
netdev_update_features().

The v1 of the patch attempted to change the features upon
flag change, and you didn't like that.

The way I see it, we have 2 options:
   1) Don't bother with tracking the state of VNET_HDR, and let
      broken apps find out the hard way that they are broken.  This
      would reduce this patch to just removing the 'if' check from
      SETOFFLOADS ioctl.

   2) Try to be smart and handle the situation where VNET_HDR is
      turned off while offloads still set.  Then we need essentially
      ignore the state the offloads.  We could change them, but then
      there would be no way to restore them and we'll have asymmetric
      behavior (v2 of the patch).  This v3, removes that asymmetry.

I guess I'd rather we be smart and handle the situation for the clueless
user/app, but I am OK with option 1 as well.

-vlad

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

Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask

[Michael S. Tsirkin]

On Thu, Aug 15, 2013 at 03:26:24PM -0400, Vlad Yasevich wrote:
> On 08/15/2013 03:20 PM, Michael S. Tsirkin wrote:
> >On Thu, Aug 15, 2013 at 10:09:45PM +0300, Michael S. Tsirkin wrote:
> >>On Thu, Aug 15, 2013 at 02:45:30PM -0400, Vlad Yasevich wrote:
> >>>On 08/15/2013 02:24 PM, Michael S. Tsirkin wrote:
> >>>>On Thu, Aug 15, 2013 at 01:02:52PM -0400, Vlad Yasevich wrote:
> >>>>>The features of the macvlan are based on the features of lower
> >>>>>device and thus can have checksum featurs other then IFF_F_HW_CSUM
> >>>>
> >>>>s/featurs/features/
> >>>>
> >>>>:set spell spelllang=en_us
> >>>>
> >>>>or whatever's the equivalent in your editor of choice.
> >>>>
> >>>>>set.  However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM.  Thus
> >>>>>when performing gso segmentation during macvtap_forward(),
> >>>>>it is possbile to end up with skbs that have ip_summed set
> >>>>>to CHECKSUM_PARTIAL.  This is incorrect when the user
> >>>>>turns off checksum offloading.
> >>>>>
> >>>>>Include all possible checksum offload values so that
> >>>>>we'll properly mask them off when performing GSO.
> >>>>>
> >>>>>Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >>>>>---
> >>>>>  drivers/net/macvtap.c | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>>diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >>>>>index b51db2a..8121358 100644
> >>>>>--- a/drivers/net/macvtap.c
> >>>>>+++ b/drivers/net/macvtap.c
> >>>>>@@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
> >>>>>
> >>>>>  static const struct proto_ops macvtap_socket_ops;
> >>>>>
> >>>>>-#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> >>>>>+#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> >>>>>  		      NETIF_F_TSO6 | NETIF_F_UFO)
> >>>>>  #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
> >>>>>  /*
> >>>>
> >>>>Okay so you are talking about hardware that sets some other
> >>>>checksum bit besides HW_CSUM, e.g. IP_CSUM, so
> >>>>
> >>>>         vlan->tap_features = vlan->dev->features &
> >>>>                             (feature_mask | ~TUN_OFFLOADS);
> >>>>
> >>>>will not clear IP_CSUM even if feature_mask is 0.
> >>>>
> >>>>Maybe mention this in the changelog, in case user
> >>>>will wonder whether his hardware is affected.
> >>>>
> >>>>So I agree, that's a bug, but if you make this change the reverse will hold
> >>>>(on this hardware):
> >>>>if user sets TUN_F_CSUM, we won't set NETIF_F_IP_CSUM
> >>>>so checksum offloading won't work.
> >>>>
> >>>>So I think you want s/NETIF_F_HW_CSUM/NETIF_F_ALL_CSUM/ everywhere
> >>>>and not just in this place.
> >>>
> >>>Yes.  I thought about that, but forgot to do in this patch set.
> >>
> >>In fact I wonder why do we do it like this at all.
> >>tap_features have nothing to do with the device,
> >>they only have to do with tap.
> >>For example, you might have a dumb device with no
> >>checksum support but userspace said it does not need a checksum,
> >>we can have NETIF_F_HW_CSUM there anyway.
> >>
> >>So why don't we just
> >>         vlan->tap_features = feature_mask;
> >>?
> >>In fact, this is exactly set_features, so why don't
> >>we just use set_features everywhere?
> >>Then this patch won't be needed at all.
> >>
> >>Kind of like this - compile-tested only - do you have a setup with
> >>IP_CSUM which you can use to test?
> >
> >Ugh this does not work even on a simple setup, sorry about the noise.
> >Maybe like this? Does this work for you?
> >
> 
> No, this is really no different then what I had to start with.
> 
> >-->
> >
> >macvtap: fix up tap features
> >
> >There's no apparent need to have tap features
> >masked according to the lowerdev config:
> >we only use them in software so hardware configuration
> >does not matter.
> >
> >This patch fixes bugs for some unusual hardware configurations,
> >for example, we only masked HW_CSUM so for hardware with
> >e.g. IP_CSUM the checksum bit remained set, which would cause
> >packets with CHECKSUM_PARTIAL to be sent to userspace
> >even when offloads are disabled.
> >
> >Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >---
> >
> >diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >index a98ed9f..3c18f12 100644
> >--- a/drivers/net/macvtap.c
> >+++ b/drivers/net/macvtap.c
> >@@ -1058,8 +1058,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 = feature_mask;
> 
> This wouldn't work when tap_features = 0, since the skb features in
> forward would evaluate to 0.  This means that netif_needs_gso() will
> return false and a GSO packet will be queued to the socket without
> going though segmentation.
> 
> -vlad
> 

Hmm won't same thing happen if features == 0 on lowerdev?


> >  	vlan->set_features = features;
> >  	netdev_update_features(vlan->dev);
> >
> >

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

[David Miller]

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Thu, 15 Aug 2013 20:33:47 +0300

> 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
>  */

Not in the networking, we use:

	/* This
	 * style.
	 */

Your misguiding people on this issue makes it hard for me
to get people to do things properly.

Thanks.

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

[Michael S. Tsirkin]

On Thu, Aug 15, 2013 at 01:16:45PM -0700, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Thu, 15 Aug 2013 20:33:47 +0300
> 
> > 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
> >  */
> 
> Not in the networking, we use:
> 
> 	/* This
> 	 * style.
> 	 */
> 
> Your misguiding people on this issue makes it hard for me
> to get people to do things properly.
> 
> Thanks.

OK in that case let's fix the rest of macvtap so I can stop getting
confused?

--->

macvtap: fix style for multiline comments

In the networking, we use:
 
 	/* This
 	 * style.
 	 */

fix up macvtap to stop misguiding people on this issue.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index a98ed9f..04b7575 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -22,15 +22,13 @@
 #include <net/sock.h>
 #include <linux/virtio_net.h>
 
-/*
- * A macvtap queue is the central object of this driver, it connects
+/* A macvtap queue is the central object of this driver, it connects
  * an open character device to a macvlan interface. There can be
  * multiple queues on one interface, which map back to queues
  * implemented in hardware on the underlying device.
  *
  * macvtap_proto is used to allocate queues through the sock allocation
  * mechanism.
- *
  */
 struct macvtap_queue {
 	struct sock sk;
@@ -51,9 +49,7 @@ static struct proto macvtap_proto = {
 	.obj_size = sizeof (struct macvtap_queue),
 };
 
-/*
- * Variables for dealing with macvtaps device numbers.
- */
+/* Variables for dealing with macvtaps device numbers.  */
 static dev_t macvtap_major;
 #define MACVTAP_NUM_DEVS (1U << MINORBITS)
 static DEFINE_MUTEX(minor_lock);
@@ -68,8 +64,8 @@ static const struct proto_ops macvtap_socket_ops;
 #define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
 		      NETIF_F_TSO6 | NETIF_F_UFO)
 #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
-/*
- * RCU usage:
+
+/* RCU usage:
  * The macvtap_queue and the macvlan_dev are loosely coupled, the
  * pointers from one to the other can only be read while rcu_read_lock
  * or rtnl is held.
@@ -162,8 +158,7 @@ static int macvtap_disable_queue(struct macvtap_queue *q)
 	return 0;
 }
 
-/*
- * The file owning the queue got closed, give up both
+/* The file owning the queue got closed, give up both
  * the reference that the files holds as well as the
  * one from the macvlan_dev if that still exists.
  *
@@ -193,8 +188,7 @@ static void macvtap_put_queue(struct macvtap_queue *q)
 	sock_put(&q->sk);
 }
 
-/*
- * Select a queue based on the rxq of the device on which this packet
+/* Select a queue based on the rxq of the device on which this packet
  * arrived. If the incoming device is not mq, calculate a flow hash
  * to select a queue. If all fails, find the first available queue.
  * Cache vlan->numvtaps since it can become zero during the execution
@@ -238,8 +232,7 @@ out:
 	return tap;
 }
 
-/*
- * The net_device is going away, give up the reference
+/* The net_device is going away, give up the reference
  * that it holds on all queues and safely set the pointer
  * from the queues to NULL.
  */
@@ -269,8 +262,7 @@ static void macvtap_del_queues(struct net_device *dev)
 		sock_put(&qlist[j]->sk);
 }
 
-/*
- * Forward happens for data that gets sent from one macvlan
+/* Forward happens for data that gets sent from one macvlan
  * endpoint to another one in bridge mode. We just take
  * the skb and put it into the receive queue.
  */
@@ -322,8 +314,7 @@ drop:
 	return NET_RX_DROP;
 }
 
-/*
- * Receive is for data from the external interface (lowerdev),
+/* Receive is for data from the external interface (lowerdev),
  * in case of macvtap, we can treat that the same way as
  * forward, which macvlan cannot.
  */
@@ -462,8 +453,7 @@ static int macvtap_open(struct inode *inode, struct file *file)
 	q->flags = IFF_VNET_HDR | IFF_NO_PI | IFF_TAP;
 	q->vnet_hdr_sz = sizeof(struct virtio_net_hdr);
 
-	/*
-	 * so far only KVM virtio_net uses macvtap, enable zero copy between
+	/* so far only KVM virtio_net uses macvtap, enable zero copy between
 	 * guest kernel and host kernel when lower device supports zerocopy
 	 *
 	 * The macvlan supports zerocopy iff the lower device supports zero
@@ -616,8 +606,7 @@ static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
 	return 0;
 }
 
-/*
- * macvtap_skb_from_vnet_hdr and macvtap_skb_to_vnet_hdr should
+/* macvtap_skb_from_vnet_hdr and macvtap_skb_to_vnet_hdr should
  * be shared with the tun/tap driver.
  */
 static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
@@ -1066,8 +1055,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
 	return 0;
 }
 
-/*
- * provide compatibility with generic tun/tap interface
+/* provide compatibility with generic tun/tap interface
  */
 static long macvtap_ioctl(struct file *file, unsigned int cmd,
 			  unsigned long arg)
@@ -1225,7 +1213,8 @@ static const struct proto_ops macvtap_socket_ops = {
 /* Get an underlying socket object from tun file.  Returns error unless file is
  * attached to a device.  The returned object works like a packet socket, it
  * can be used for sock_sendmsg/sock_recvmsg.  The caller is responsible for
- * holding a reference to the file for as long as the socket is in use. */
+ * holding a reference to the file for as long as the socket is in use.
+ */
 struct socket *macvtap_get_socket(struct file *file)
 {
 	struct macvtap_queue *q;

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

[David Miller]

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Thu, 15 Aug 2013 23:52:45 +0300

> OK in that case let's fix the rest of macvtap so I can stop getting
> confused?

Sure, but after you guys sort out these bug fixes.