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

[Vlad Yasevich <vyasevic@redhat.com>]

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