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

[Vlad Yasevich <vyasevic@redhat.com>]

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