netdev - Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 15 Aug 2013 14:45:30 -0400
From:	Vlad Yasevich <vyasevic@...hat.com>
To:	"Michael S. Tsirkin" <mst@...hat.com>
CC:	netdev@...r.kernel.org
Subject: Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD
 mask

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

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