Re: [PATCH net v2] tap: fix incorrect variable used for USO check in set_offload()

Willem de Bruijn wrote:
> Rongguang Wei wrote:
>> From: Rongguang Wei <[email protected]>
>>
>> The USO features in set_offload() incorrectly uses feature_mask and
>> features argument.
>>
>> The USO feature was written to the local features variable instead of
>> feature_mask. All other offload bits (TSO, TSO_ECN) are stored in
>> feature_mask which becomes tap->tap_features and is used by
>> tap_handle_frame() for GSO segmentation. Without NETIF_F_GSO_UDP_L4
>> in tap->tap_features, making USO on tap effectively non-functional.
>>
>> Keeping the USO handling inside the TUN_F_CSUM block avoids enabling
>> GRO/LRO when userspace requests USO without CSUM.
>>
>> Fixes: 399e0827642f ("driver/net/tun: Added features for USO.")
>> Signed-off-by: Rongguang Wei <[email protected]>
>> Reviewed-by: Willem de Bruijn <[email protected]>
> 
> Reviewed-by: Willem de Bruijn <[email protected]>
> 
> Substantially changed patch, please don't keep Reviewed-by tags across
> non-trivial changes. That said, I do agree, so adding it again.
> 
>> ---
>> v2: Fix more incorrect variable in USO check.
>>     https://lore.kernel.org/netdev/[email protected]/
>> v1: https://lore.kernel.org/netdev/[email protected]/
>> ---
>>  drivers/net/tap.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
>> index 5d2d34d24ce8..d4ca2fee538b 100644
>> --- a/drivers/net/tap.c
>> +++ b/drivers/net/tap.c
>> @@ -883,7 +883,7 @@ static int set_offload(struct tap_queue *q, unsigned long arg)
>>  
>>  		/* TODO: for now USO4 and USO6 should work simultaneously */
>>  		if ((arg & (TUN_F_USO4 | TUN_F_USO6)) == (TUN_F_USO4 | TUN_F_USO6))
>> -			features |= NETIF_F_GSO_UDP_L4;
>> +			feature_mask |= NETIF_F_GSO_UDP_L4;
> 
> Drivers/net/tun seems to have this same issue.
> 
> Do you want to take a look or shall I?
> 
Hi,
I have check the code in drivers/net/tun.c.
In tun.c, the tun's all offload bits go into features argument and
I think tun.c does not have this issue.
> 
>>  	}
>>  
>>  	/* tun/tap driver inverts the usage for TSO offloads, where
>> @@ -894,8 +894,7 @@ static int set_offload(struct tap_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) ||
>> -	    (feature_mask & (TUN_F_USO4 | TUN_F_USO6)) == (TUN_F_USO4 | TUN_F_USO6))
>> +	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_GSO_UDP_L4))
>>  		features |= RX_OFFLOADS;
>>  	else
>>  		features &= ~RX_OFFLOADS;