| Message ID | 20201104102141.3489-1-tariqt@nvidia.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for net |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | success | Errors and warnings before: 10 this patch: 10 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 11 lines checked |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 10 this patch: 10 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
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; > + }
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; >> + }
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.
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.
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; }
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.