[PATCH net] net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled

[PATCH net] net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled

[Tariq Toukan]

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>
---
 net/core/dev.c | 5 +++++
 1 file changed, 5 insertions(+)

Hi,

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

diff --git a/net/core/dev.c b/net/core/dev.c
index 82dc6b48e45f..5f72ea17d3f7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9588,6 +9588,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;
 }
 
-- 
2.21.0

Re: [PATCH net] net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled

[Jakub Kicinski]

On Wed,  4 Nov 2020 12:21:41 +0200 Tariq Toukan wrote:
> With NETIF_F_HW_TLS_TX packets are encrypted in HW. This cannot be
> logically done when HW_CSUM offload is off.

Right. Do you expect drivers to nack clearing NETIF_F_HW_TLS_TX when
there are active connections, then?  I don't think NFP does.  We either
gotta return -EBUSY when there are offloaded connections, or at least
clearly document the expected behavior.

> Fixes: 2342a8512a1e ("net: Add TLS TX offload features")
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> Reviewed-by: Boris Pismenny <borisp@nvidia.com>

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 82dc6b48e45f..5f72ea17d3f7 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9588,6 +9588,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;
> +	}

Re: [PATCH net] net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled

[Tariq Toukan]

On 11/4/2020 11:25 PM, Jakub Kicinski wrote:
> On Wed,  4 Nov 2020 12:21:41 +0200 Tariq Toukan wrote:
>> With NETIF_F_HW_TLS_TX packets are encrypted in HW. This cannot be
>> logically done when HW_CSUM offload is off.
> 
> Right. Do you expect drivers to nack clearing NETIF_F_HW_TLS_TX when
> there are active connections, then?  I don't think NFP does.  We either
> gotta return -EBUSY when there are offloaded connections, or at least
> clearly document the expected behavior.
> 

As I see from code, today drivers and TLS stack allow clearing 
NETIF_F_HW_TLS_TX without doing anything to change behavior in existing 
sockets, so they continue to do HW offload. Only new sockets will be 
affected.
I think the same behavior should apply when NETIF_F_HW_TLS_TX is cleared 
implicitly (due to clearing HW_CSUM).

If the existing behavior is not expected, and we should force fallback 
to SW kTLS for existing sockets, then I think this should be fixed 
independently to this patch, as it introduces no new regression.

What do you think?

>> Fixes: 2342a8512a1e ("net: Add TLS TX offload features")
>> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
>> Reviewed-by: Boris Pismenny <borisp@nvidia.com>
> 
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 82dc6b48e45f..5f72ea17d3f7 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -9588,6 +9588,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;
>> +	}

Re: [PATCH net] net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled

[Jakub Kicinski]

On Thu, 5 Nov 2020 15:22:53 +0200 Tariq Toukan wrote:
> On 11/4/2020 11:25 PM, Jakub Kicinski wrote:
> > On Wed,  4 Nov 2020 12:21:41 +0200 Tariq Toukan wrote:  
> >> With NETIF_F_HW_TLS_TX packets are encrypted in HW. This cannot be
> >> logically done when HW_CSUM offload is off.  
> > 
> > Right. Do you expect drivers to nack clearing NETIF_F_HW_TLS_TX when
> > there are active connections, then?  I don't think NFP does.  We either
> > gotta return -EBUSY when there are offloaded connections, or at least
> > clearly document the expected behavior.
> 
> As I see from code, today drivers and TLS stack allow clearing 
> NETIF_F_HW_TLS_TX without doing anything to change behavior in existing 
> sockets, so they continue to do HW offload. Only new sockets will be 
> affected.

Right, we want to let users turn off the offload when it misbehaves,
and we don't have a way of un-offloading connections.

> I think the same behavior should apply when NETIF_F_HW_TLS_TX is cleared 
> implicitly (due to clearing HW_CSUM).

Right, I don't mind either way. My thinking with the tls feature was
that offload is likely to be broken, at least initially. Checksum
offload should work, one would hope, so its less of a risk.

The question is perhaps - do we care more about consistency with the
behavior of the TLS feature, or current expectation that csum offload
off will actually turn it off.

> If the existing behavior is not expected, and we should force fallback 
> to SW kTLS for existing sockets, then I think this should be fixed 
> independently to this patch, as it introduces no new regression.
> 
> What do you think?

The current behavior of the TLS features is documented in
tls-offload.rst, you can do what you're doing in this patch, 
but you need to document it there.

Re: [PATCH net] net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled

[Tariq Toukan]

On 11/5/2020 6:00 PM, Jakub Kicinski wrote:
> On Thu, 5 Nov 2020 15:22:53 +0200 Tariq Toukan wrote:
>> On 11/4/2020 11:25 PM, Jakub Kicinski wrote:
>>> On Wed,  4 Nov 2020 12:21:41 +0200 Tariq Toukan wrote:
>>>> With NETIF_F_HW_TLS_TX packets are encrypted in HW. This cannot be
>>>> logically done when HW_CSUM offload is off.
>>>
>>> Right. Do you expect drivers to nack clearing NETIF_F_HW_TLS_TX when
>>> there are active connections, then?  I don't think NFP does.  We either
>>> gotta return -EBUSY when there are offloaded connections, or at least
>>> clearly document the expected behavior.
>>
>> As I see from code, today drivers and TLS stack allow clearing
>> NETIF_F_HW_TLS_TX without doing anything to change behavior in existing
>> sockets, so they continue to do HW offload. Only new sockets will be
>> affected.
> 
> Right, we want to let users turn off the offload when it misbehaves,
> and we don't have a way of un-offloading connections.
> 
>> I think the same behavior should apply when NETIF_F_HW_TLS_TX is cleared
>> implicitly (due to clearing HW_CSUM).
> 
> Right, I don't mind either way. My thinking with the tls feature was
> that offload is likely to be broken, at least initially. Checksum
> offload should work, one would hope, so its less of a risk.
> 
> The question is perhaps - do we care more about consistency with the
> behavior of the TLS feature, or current expectation that csum offload
> off will actually turn it off.
> 
>> If the existing behavior is not expected, and we should force fallback
>> to SW kTLS for existing sockets, then I think this should be fixed
>> independently to this patch, as it introduces no new regression.
>>
>> What do you think?
> 
> The current behavior of the TLS features is documented in
> tls-offload.rst, you can do what you're doing in this patch,
> but you need to document it there.
> 

I documented this in tls-offload.rst.
The preceding paragraph (with the note about old connections) is still 
valid.