| Message ID | 20201108144309.31699-1-tariqt@nvidia.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,V2] net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled | expand |
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.
diff --git a/Documentation/networking/tls-offload.rst b/Documentation/networking/tls-offload.rst index 37773da2bee5..f315feae3a65 100644 --- a/Documentation/networking/tls-offload.rst +++ b/Documentation/networking/tls-offload.rst @@ -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. 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; }
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.