[net,V2] net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled

With NETIF_F_HW_TLS_TX packets are encrypted in HW. This cannot be
logically done when HW_CSUM offload is off.

Fixes: 2342a8512a1e ("net: Add TLS TX offload features")
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
Reviewed-by: Boris Pismenny <borisp@nvidia.com>
---
 Documentation/networking/tls-offload.rst | 4 ++++
 net/core/dev.c                           | 5 +++++
 2 files changed, 9 insertions(+)

Hi,

Please queue to -stable >= v4.18.
Thanks.

v2:
- Documented the change in tls-offload.rst.

On Sun,  8 Nov 2020 16:43:09 +0200 Tariq Toukan wrote:
> @@ -528,3 +528,7 @@ Drivers should ignore the changes to TLS the device feature flags.
>  These flags will be acted upon accordingly by the core ``ktls`` code.
>  TLS device feature flags only control adding of new TLS connection
>  offloads, old connections will remain active after flags are cleared.
> +
> +The TLS encryption cannot be offloaded to device if checksum calculation
> +is not, hence the TLS TX device feature flag is cleared when HW_CSUM is
> +disabled.

This makes it sound like the driver will fall back to software crypto
if L4 csum offload gets disabled, is this your intention?

Seems at odds with the paragraph above it.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 9499a414d67e..26c9b059cade 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9584,6 +9584,11 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>  		}
>  	}
>  
> +	if ((features & NETIF_F_HW_TLS_TX) && !(features & NETIF_F_HW_CSUM)) {
> +		netdev_dbg(dev, "Dropping TLS TX HW offload feature since no CSUM feature.\n");
> +		features &= ~NETIF_F_HW_TLS_TX;
> +	}
> +
>  	return features;
>  }
>

On 11/11/2020 1:44 AM, Jakub Kicinski wrote:
> On Sun,  8 Nov 2020 16:43:09 +0200 Tariq Toukan wrote:
>> @@ -528,3 +528,7 @@ Drivers should ignore the changes to TLS the device feature flags.
>>   These flags will be acted upon accordingly by the core ``ktls`` code.
>>   TLS device feature flags only control adding of new TLS connection
>>   offloads, old connections will remain active after flags are cleared.
>> +
>> +The TLS encryption cannot be offloaded to device if checksum calculation
>> +is not, hence the TLS TX device feature flag is cleared when HW_CSUM is
>> +disabled.
> 
> This makes it sound like the driver will fall back to software crypto
> if L4 csum offload gets disabled, is this your intention?
> 
> Seems at odds with the paragraph above it.
> 

Actually, TLS feature bit acts on new connections, while CSUM feature 
bit acts immediately, so for old connections we still have a gap.

I think of adding logic in netif_skb_features or tls_validate_xmit_skb, 
but it's not trivial.

I'll resubmit when i figure out a clean way that covers all cases and is 
consistent with TLS feature bit behavior.

Regards,
Tariq


>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 9499a414d67e..26c9b059cade 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -9584,6 +9584,11 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>>   		}
>>   	}
>>   
>> +	if ((features & NETIF_F_HW_TLS_TX) && !(features & NETIF_F_HW_CSUM)) {
>> +		netdev_dbg(dev, "Dropping TLS TX HW offload feature since no CSUM feature.\n");
>> +		features &= ~NETIF_F_HW_TLS_TX;
>> +	}
>> +
>>   	return features;
>>   }
>>   
>

On Wed, 11 Nov 2020 14:25:53 +0200 Tariq Toukan wrote:
> On 11/11/2020 1:44 AM, Jakub Kicinski wrote:
> > On Sun,  8 Nov 2020 16:43:09 +0200 Tariq Toukan wrote:  
> >> @@ -528,3 +528,7 @@ Drivers should ignore the changes to TLS the device feature flags.
> >>   These flags will be acted upon accordingly by the core ``ktls`` code.
> >>   TLS device feature flags only control adding of new TLS connection
> >>   offloads, old connections will remain active after flags are cleared.
> >> +
> >> +The TLS encryption cannot be offloaded to device if checksum calculation
> >> +is not, hence the TLS TX device feature flag is cleared when HW_CSUM is
> >> +disabled.  
> > 
> > This makes it sound like the driver will fall back to software crypto
> > if L4 csum offload gets disabled, is this your intention?
> > 
> > Seems at odds with the paragraph above it.
> >   
> 
> Actually, TLS feature bit acts on new connections, while CSUM feature 
> bit acts immediately, so for old connections we still have a gap.

Well, for your implementation. I'm pretty sure NFP will always recompute
the TCP csum on TLS segments :)

> I think of adding logic in netif_skb_features or tls_validate_xmit_skb, 
> but it's not trivial.
> 
> I'll resubmit when i figure out a clean way that covers all cases and is 
> consistent with TLS feature bit behavior.

Sounds like the way forward you're thinking of is changing the way both
feature flags work, and not just stopping new connections? Fine by me,
TLS fallback is quite a bit less efficient than the normal SW
implementation (can't use AES-NI), my concern was that users will be
surprised they don't get the performance they are expecting from SW.