diff mbox

[09/10] macvtap: Re-enable UFO support

Message ID 1418840455-22598-10-git-send-email-vyasevic@redhat.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Vladislav Yasevich Dec. 17, 2014, 6:20 p.m. UTC
Now that UFO is split into v4 and v6 parts, we can bring
back v4 support.  Continue to handle legacy applications
by selecting the ipv6 fagment id but do not change the
gso type.  This allows 2 legacy VMs to continue to communicate.

Based on original work from Ben Hutchings.

Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
CC: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/macvtap.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Comments

Michael S. Tsirkin Dec. 17, 2014, 10:41 p.m. UTC | #1
On Wed, Dec 17, 2014 at 01:20:54PM -0500, Vladislav Yasevich wrote:
> Now that UFO is split into v4 and v6 parts, we can bring
> back v4 support.  Continue to handle legacy applications
> by selecting the ipv6 fagment id but do not change the
> gso type.  This allows 2 legacy VMs to continue to communicate.
> 
> Based on original work from Ben Hutchings.
> 
> Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
> CC: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>  drivers/net/macvtap.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index 880cc09..75febd4 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -66,7 +66,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 | \
> -		      NETIF_F_TSO6)
> +		      NETIF_F_TSO6 | NETIF_F_UFO)
>  #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
>  #define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
>  
> @@ -570,11 +570,14 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
>  			gso_type = SKB_GSO_TCPV6;
>  			break;
>  		case VIRTIO_NET_HDR_GSO_UDP:
> -			pr_warn_once("macvtap: %s: using disabled UFO feature; please fix this program\n",
> -				     current->comm);
>  			gso_type = SKB_GSO_UDP;
> -			if (skb->protocol == htons(ETH_P_IPV6))
> +			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
> +				/* This is to support legacy appliacations.
> +				 * Do not change the gso_type as legacy apps
> +				 * may not know about the new type.
> +				 */
>  				ipv6_proxy_select_ident(skb);
> +			}
>  			break;
>  		default:
>  			return -EINVAL;
> @@ -619,6 +622,8 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>  		else if (sinfo->gso_type & SKB_GSO_TCPV6)
>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> +		else if (sinfo->gso_type & SKB_GSO_UDP)
> +			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP;
>  		else
>  			BUG();
>  		if (sinfo->gso_type & SKB_GSO_TCP_ECN)
> @@ -955,6 +960,9 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
>  			if (arg & TUN_F_TSO6)
>  				feature_mask |= NETIF_F_TSO6;
>  		}
> +
> +		if (arg & TUN_F_UFO)
> +			feature_mask |= NETIF_F_UFO;
>  	}
>  
>  	/* tun/tap driver inverts the usage for TSO offloads, where
> @@ -965,7 +973,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
>  	 * When user space turns off TSO, we turn off GSO/LRO so that
>  	 * user-space will not receive TSO frames.
>  	 */
> -	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6))
> +	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
>  		features |= RX_OFFLOADS;
>  	else
>  		features &= ~RX_OFFLOADS;

By the way this logic is completely broken, even without your patch,
isn't it?

It says: enable LRO+GRO if any of NETIF_F_TSO | NETIF_F_TSO6 |
NETIF_F_UFO set.

So what happens if I enable TSO only?
LRO gets enabled so I can still get TSO6 packets.


This really should be:

	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO) ==
			(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO)


fixing this probably should be a separate patch before your
series, and Cc stable.


> @@ -1066,7 +1074,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
>  	case TUNSETOFFLOAD:
>  		/* let the user check for future flags */
>  		if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
> -			    TUN_F_TSO_ECN))
> +			    TUN_F_TSO_ECN | TUN_F_UFO))
>  			return -EINVAL;
>  
>  		rtnl_lock();
> -- 
> 1.9.3
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vlad Yasevich Dec. 18, 2014, 2:43 a.m. UTC | #2
On 12/17/2014 05:41 PM, Michael S. Tsirkin wrote:
> On Wed, Dec 17, 2014 at 01:20:54PM -0500, Vladislav Yasevich wrote:
>> Now that UFO is split into v4 and v6 parts, we can bring
>> back v4 support.  Continue to handle legacy applications
>> by selecting the ipv6 fagment id but do not change the
>> gso type.  This allows 2 legacy VMs to continue to communicate.
>>
>> Based on original work from Ben Hutchings.
>>
>> Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
>> CC: Ben Hutchings <ben@decadent.org.uk>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> ---
>>  drivers/net/macvtap.c | 20 ++++++++++++++------
>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>> index 880cc09..75febd4 100644
>> --- a/drivers/net/macvtap.c
>> +++ b/drivers/net/macvtap.c
>> @@ -66,7 +66,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 | \
>> -		      NETIF_F_TSO6)
>> +		      NETIF_F_TSO6 | NETIF_F_UFO)
>>  #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
>>  #define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
>>  
>> @@ -570,11 +570,14 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
>>  			gso_type = SKB_GSO_TCPV6;
>>  			break;
>>  		case VIRTIO_NET_HDR_GSO_UDP:
>> -			pr_warn_once("macvtap: %s: using disabled UFO feature; please fix this program\n",
>> -				     current->comm);
>>  			gso_type = SKB_GSO_UDP;
>> -			if (skb->protocol == htons(ETH_P_IPV6))
>> +			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
>> +				/* This is to support legacy appliacations.
>> +				 * Do not change the gso_type as legacy apps
>> +				 * may not know about the new type.
>> +				 */
>>  				ipv6_proxy_select_ident(skb);
>> +			}
>>  			break;
>>  		default:
>>  			return -EINVAL;
>> @@ -619,6 +622,8 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
>>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>>  		else if (sinfo->gso_type & SKB_GSO_TCPV6)
>>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
>> +		else if (sinfo->gso_type & SKB_GSO_UDP)
>> +			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP;
>>  		else
>>  			BUG();
>>  		if (sinfo->gso_type & SKB_GSO_TCP_ECN)
>> @@ -955,6 +960,9 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
>>  			if (arg & TUN_F_TSO6)
>>  				feature_mask |= NETIF_F_TSO6;
>>  		}
>> +
>> +		if (arg & TUN_F_UFO)
>> +			feature_mask |= NETIF_F_UFO;
>>  	}
>>  
>>  	/* tun/tap driver inverts the usage for TSO offloads, where
>> @@ -965,7 +973,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
>>  	 * When user space turns off TSO, we turn off GSO/LRO so that
>>  	 * user-space will not receive TSO frames.
>>  	 */
>> -	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6))
>> +	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
>>  		features |= RX_OFFLOADS;
>>  	else
>>  		features &= ~RX_OFFLOADS;
> 
> By the way this logic is completely broken, even without your patch,
> isn't it?
> 
> It says: enable LRO+GRO if any of NETIF_F_TSO | NETIF_F_TSO6 |
> NETIF_F_UFO set.
> 
> So what happens if I enable TSO only?
> LRO gets enabled so I can still get TSO6 packets.
> 
> 
> This really should be:
> 
> 	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO) ==
> 			(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO)
> 
> 
> fixing this probably should be a separate patch before your
> series, and Cc stable.

Actually, all this LRO/GRO feature manipulation on the macvtap device is
rather pointless as those features aren't really checked at any point
on the macvtap receive path.  GRO calls are done from napi context on
the lowest device, so by the time a packet hits macvtap, GRO/LRO has
already been done.

So it doesn't really matter that we disable them incorrectly.  I
can send a separate patch to clean this up.

> 
> 
>> @@ -1066,7 +1074,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
>>  	case TUNSETOFFLOAD:
>>  		/* let the user check for future flags */
>>  		if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
>> -			    TUN_F_TSO_ECN))
>> +			    TUN_F_TSO_ECN | TUN_F_UFO))
>>  			return -EINVAL;
>>  
>>  		rtnl_lock();
>> -- 
>> 1.9.3

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael S. Tsirkin Dec. 18, 2014, 7:55 a.m. UTC | #3
On Wed, Dec 17, 2014 at 09:43:51PM -0500, Vlad Yasevich wrote:
> On 12/17/2014 05:41 PM, Michael S. Tsirkin wrote:
> > On Wed, Dec 17, 2014 at 01:20:54PM -0500, Vladislav Yasevich wrote:
> >> Now that UFO is split into v4 and v6 parts, we can bring
> >> back v4 support.  Continue to handle legacy applications
> >> by selecting the ipv6 fagment id but do not change the
> >> gso type.  This allows 2 legacy VMs to continue to communicate.
> >>
> >> Based on original work from Ben Hutchings.
> >>
> >> Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
> >> CC: Ben Hutchings <ben@decadent.org.uk>
> >> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> >> ---
> >>  drivers/net/macvtap.c | 20 ++++++++++++++------
> >>  1 file changed, 14 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >> index 880cc09..75febd4 100644
> >> --- a/drivers/net/macvtap.c
> >> +++ b/drivers/net/macvtap.c
> >> @@ -66,7 +66,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 | \
> >> -		      NETIF_F_TSO6)
> >> +		      NETIF_F_TSO6 | NETIF_F_UFO)
> >>  #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
> >>  #define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
> >>  
> >> @@ -570,11 +570,14 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
> >>  			gso_type = SKB_GSO_TCPV6;
> >>  			break;
> >>  		case VIRTIO_NET_HDR_GSO_UDP:
> >> -			pr_warn_once("macvtap: %s: using disabled UFO feature; please fix this program\n",
> >> -				     current->comm);
> >>  			gso_type = SKB_GSO_UDP;
> >> -			if (skb->protocol == htons(ETH_P_IPV6))
> >> +			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
> >> +				/* This is to support legacy appliacations.
> >> +				 * Do not change the gso_type as legacy apps
> >> +				 * may not know about the new type.
> >> +				 */
> >>  				ipv6_proxy_select_ident(skb);
> >> +			}
> >>  			break;
> >>  		default:
> >>  			return -EINVAL;
> >> @@ -619,6 +622,8 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
> >>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> >>  		else if (sinfo->gso_type & SKB_GSO_TCPV6)
> >>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> >> +		else if (sinfo->gso_type & SKB_GSO_UDP)
> >> +			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP;
> >>  		else
> >>  			BUG();
> >>  		if (sinfo->gso_type & SKB_GSO_TCP_ECN)
> >> @@ -955,6 +960,9 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
> >>  			if (arg & TUN_F_TSO6)
> >>  				feature_mask |= NETIF_F_TSO6;
> >>  		}
> >> +
> >> +		if (arg & TUN_F_UFO)
> >> +			feature_mask |= NETIF_F_UFO;
> >>  	}
> >>  
> >>  	/* tun/tap driver inverts the usage for TSO offloads, where
> >> @@ -965,7 +973,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
> >>  	 * When user space turns off TSO, we turn off GSO/LRO so that
> >>  	 * user-space will not receive TSO frames.
> >>  	 */
> >> -	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6))
> >> +	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
> >>  		features |= RX_OFFLOADS;
> >>  	else
> >>  		features &= ~RX_OFFLOADS;
> > 
> > By the way this logic is completely broken, even without your patch,
> > isn't it?
> > 
> > It says: enable LRO+GRO if any of NETIF_F_TSO | NETIF_F_TSO6 |
> > NETIF_F_UFO set.
> > 
> > So what happens if I enable TSO only?
> > LRO gets enabled so I can still get TSO6 packets.
> > 
> > 
> > This really should be:
> > 
> > 	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO) ==
> > 			(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO)
> > 
> > 
> > fixing this probably should be a separate patch before your
> > series, and Cc stable.
> 
> Actually, all this LRO/GRO feature manipulation on the macvtap device is
> rather pointless as those features aren't really checked at any point
> on the macvtap receive path.  GRO calls are done from napi context on
> the lowest device, so by the time a packet hits macvtap, GRO/LRO has
> already been done.
> 
> So it doesn't really matter that we disable them incorrectly.  I
> can send a separate patch to clean this up.

Hmm so if userspace says it can't handle TSO packets,
it might get them anyway?


> > 
> > 
> >> @@ -1066,7 +1074,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
> >>  	case TUNSETOFFLOAD:
> >>  		/* let the user check for future flags */
> >>  		if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
> >> -			    TUN_F_TSO_ECN))
> >> +			    TUN_F_TSO_ECN | TUN_F_UFO))
> >>  			return -EINVAL;
> >>  
> >>  		rtnl_lock();
> >> -- 
> >> 1.9.3
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vlad Yasevich Dec. 18, 2014, 3:15 p.m. UTC | #4
On 12/18/2014 02:55 AM, Michael S. Tsirkin wrote:
> On Wed, Dec 17, 2014 at 09:43:51PM -0500, Vlad Yasevich wrote:
>> On 12/17/2014 05:41 PM, Michael S. Tsirkin wrote:
>>> On Wed, Dec 17, 2014 at 01:20:54PM -0500, Vladislav Yasevich wrote:
>>>> Now that UFO is split into v4 and v6 parts, we can bring
>>>> back v4 support.  Continue to handle legacy applications
>>>> by selecting the ipv6 fagment id but do not change the
>>>> gso type.  This allows 2 legacy VMs to continue to communicate.
>>>>
>>>> Based on original work from Ben Hutchings.
>>>>
>>>> Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
>>>> CC: Ben Hutchings <ben@decadent.org.uk>
>>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>>>> ---
>>>>  drivers/net/macvtap.c | 20 ++++++++++++++------
>>>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>>>> index 880cc09..75febd4 100644
>>>> --- a/drivers/net/macvtap.c
>>>> +++ b/drivers/net/macvtap.c
>>>> @@ -66,7 +66,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 | \
>>>> -		      NETIF_F_TSO6)
>>>> +		      NETIF_F_TSO6 | NETIF_F_UFO)
>>>>  #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
>>>>  #define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
>>>>  
>>>> @@ -570,11 +570,14 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
>>>>  			gso_type = SKB_GSO_TCPV6;
>>>>  			break;
>>>>  		case VIRTIO_NET_HDR_GSO_UDP:
>>>> -			pr_warn_once("macvtap: %s: using disabled UFO feature; please fix this program\n",
>>>> -				     current->comm);
>>>>  			gso_type = SKB_GSO_UDP;
>>>> -			if (skb->protocol == htons(ETH_P_IPV6))
>>>> +			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
>>>> +				/* This is to support legacy appliacations.
>>>> +				 * Do not change the gso_type as legacy apps
>>>> +				 * may not know about the new type.
>>>> +				 */
>>>>  				ipv6_proxy_select_ident(skb);
>>>> +			}
>>>>  			break;
>>>>  		default:
>>>>  			return -EINVAL;
>>>> @@ -619,6 +622,8 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
>>>>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>>>>  		else if (sinfo->gso_type & SKB_GSO_TCPV6)
>>>>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
>>>> +		else if (sinfo->gso_type & SKB_GSO_UDP)
>>>> +			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP;
>>>>  		else
>>>>  			BUG();
>>>>  		if (sinfo->gso_type & SKB_GSO_TCP_ECN)
>>>> @@ -955,6 +960,9 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
>>>>  			if (arg & TUN_F_TSO6)
>>>>  				feature_mask |= NETIF_F_TSO6;
>>>>  		}
>>>> +
>>>> +		if (arg & TUN_F_UFO)
>>>> +			feature_mask |= NETIF_F_UFO;
>>>>  	}
>>>>  
>>>>  	/* tun/tap driver inverts the usage for TSO offloads, where
>>>> @@ -965,7 +973,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
>>>>  	 * When user space turns off TSO, we turn off GSO/LRO so that
>>>>  	 * user-space will not receive TSO frames.
>>>>  	 */
>>>> -	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6))
>>>> +	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
>>>>  		features |= RX_OFFLOADS;
>>>>  	else
>>>>  		features &= ~RX_OFFLOADS;
>>>
>>> By the way this logic is completely broken, even without your patch,
>>> isn't it?
>>>
>>> It says: enable LRO+GRO if any of NETIF_F_TSO | NETIF_F_TSO6 |
>>> NETIF_F_UFO set.
>>>
>>> So what happens if I enable TSO only?
>>> LRO gets enabled so I can still get TSO6 packets.
>>>
>>>
>>> This really should be:
>>>
>>> 	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO) ==
>>> 			(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO)
>>>
>>>
>>> fixing this probably should be a separate patch before your
>>> series, and Cc stable.
>>
>> Actually, all this LRO/GRO feature manipulation on the macvtap device is
>> rather pointless as those features aren't really checked at any point
>> on the macvtap receive path.  GRO calls are done from napi context on
>> the lowest device, so by the time a packet hits macvtap, GRO/LRO has
>> already been done.
>>
>> So it doesn't really matter that we disable them incorrectly.  I
>> can send a separate patch to clean this up.
> 
> Hmm so if userspace says it can't handle TSO packets,
> it might get them anyway?

No.  It will get individual segments because in macvtap_handle_frame
we use user specified features to decide if segmentation needs
to be performed or not.

-vlad

> 
> 
>>>
>>>
>>>> @@ -1066,7 +1074,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
>>>>  	case TUNSETOFFLOAD:
>>>>  		/* let the user check for future flags */
>>>>  		if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
>>>> -			    TUN_F_TSO_ECN))
>>>> +			    TUN_F_TSO_ECN | TUN_F_UFO))
>>>>  			return -EINVAL;
>>>>  
>>>>  		rtnl_lock();
>>>> -- 
>>>> 1.9.3

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 880cc09..75febd4 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -66,7 +66,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 | \
-		      NETIF_F_TSO6)
+		      NETIF_F_TSO6 | NETIF_F_UFO)
 #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
 #define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
 
@@ -570,11 +570,14 @@  static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
 			gso_type = SKB_GSO_TCPV6;
 			break;
 		case VIRTIO_NET_HDR_GSO_UDP:
-			pr_warn_once("macvtap: %s: using disabled UFO feature; please fix this program\n",
-				     current->comm);
 			gso_type = SKB_GSO_UDP;
-			if (skb->protocol == htons(ETH_P_IPV6))
+			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
+				/* This is to support legacy appliacations.
+				 * Do not change the gso_type as legacy apps
+				 * may not know about the new type.
+				 */
 				ipv6_proxy_select_ident(skb);
+			}
 			break;
 		default:
 			return -EINVAL;
@@ -619,6 +622,8 @@  static void macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
 			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
 		else if (sinfo->gso_type & SKB_GSO_TCPV6)
 			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
+		else if (sinfo->gso_type & SKB_GSO_UDP)
+			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP;
 		else
 			BUG();
 		if (sinfo->gso_type & SKB_GSO_TCP_ECN)
@@ -955,6 +960,9 @@  static int set_offload(struct macvtap_queue *q, unsigned long arg)
 			if (arg & TUN_F_TSO6)
 				feature_mask |= NETIF_F_TSO6;
 		}
+
+		if (arg & TUN_F_UFO)
+			feature_mask |= NETIF_F_UFO;
 	}
 
 	/* tun/tap driver inverts the usage for TSO offloads, where
@@ -965,7 +973,7 @@  static int set_offload(struct macvtap_queue *q, unsigned long arg)
 	 * When user space turns off TSO, we turn off GSO/LRO so that
 	 * user-space will not receive TSO frames.
 	 */
-	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6))
+	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
 		features |= RX_OFFLOADS;
 	else
 		features &= ~RX_OFFLOADS;
@@ -1066,7 +1074,7 @@  static long macvtap_ioctl(struct file *file, unsigned int cmd,
 	case TUNSETOFFLOAD:
 		/* let the user check for future flags */
 		if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
-			    TUN_F_TSO_ECN))
+			    TUN_F_TSO_ECN | TUN_F_UFO))
 			return -EINVAL;
 
 		rtnl_lock();