Hi,
This RFC series suggests a solution for the following problem:
Bond interface and lower interface are both up with TLS RX/TX offloads on.
TX/RX csum offload is turned off for the upper, hence RX/TX TLS is turned off
for it as well.
Yet, although it indicates that feature is disabled, new connections are still
offloaded by the lower, as Bond has no way to impact that:
Return value of bond_sk_get_lower_dev() is agnostic to this change.
One way to solve this issue, is to bring back the Bond TLS operations callbacks,
i.e. provide implementation for struct tlsdev_ops in Bond.
This gives full control for the Bond over its features, making it aware of every
new TLS connection offload request.
This direction was proposed in the original Bond TLS implementation, but dropped
during ML review. Probably it's right to re-consider now.
Here I suggest another solution, which requires generic changes out of the bond
driver.
Fixes in patches 1 and 4 are needed anyway, independently to which solution
we choose. I'll probably submit them separately soon.
Regards,
Tariq
Tariq Toukan (6):
net: Fix features skip in for_each_netdev_feature()
net: Disable TX TLS device offload on lower devices if disabled on the
upper
net: Disable RX TLS device offload on lower devices if disabled on the
upper
net/bond: Enable RXCSUM feature for bond
net/bond: Allow explicit control of the TLS device offload features
net/bond: Do not turn on TLS features in bond_fix_features()
drivers/net/bonding/bond_main.c | 6 +++---
include/linux/netdev_features.h | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
--
2.21.0
The find_next_netdev_feature() macro gets the "remaining length",
not bit index.
Passing "bit - 1" for the following iteration is wrong as it skips
the adjacent bit. Pass "bit" instead.
Fixes: 3b89ea9c5902 ("net: Fix for_each_netdev_feature on Big endian")
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
include/linux/netdev_features.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -169,7 +169,7 @@ enum {#define NETIF_F_HW_HSR_FWD __NETIF_F(HW_HSR_FWD)#define NETIF_F_HW_HSR_DUP __NETIF_F(HW_HSR_DUP)-/* Finds the next feature with the highest number of the range of start till 0.+/* Finds the next feature with the highest number of the range of start-1 till 0.*/staticinlineintfind_next_netdev_feature(u64feature,unsignedlongstart){
@@ -188,7 +188,7 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)for((bit)=find_next_netdev_feature((mask_addr),\NETDEV_FEATURE_COUNT);\(bit)>=0;\-(bit)=find_next_netdev_feature((mask_addr),(bit)-1))+(bit)=find_next_netdev_feature((mask_addr),(bit)))/* Features valid for ethtool to change *//* = all defined minus driver/device-class-related */
In the control flow of the TLS device offload feature, the upper device
gives a pointer to the target lower device. All struct tlsdev_ops
are called directly against the lower device, bypassing the upper.
This means, the upper device has very limited means of blocking/disabling
the TLS device offload.
Today, for instance, disabling TX checksum offload of the upper dev
automatically disables the TX TLS device offload capability.
However, this does not affect the lower device at all, and it keeps
doing TLS device offload for all new connections.
Here we fix this, by propagating the disablement of the TLS TX device
offload features to all lower devices.
Fixes: ae0b04b238e2 ("net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled")
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
include/linux/netdev_features.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -239,7 +239,7 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)*Ifupper/masterdevicehasthesefeaturesdisabled,theymustbedisabled*onalllower/slavedevicesaswell.*/-#define NETIF_F_UPPER_DISABLES NETIF_F_LRO+#define NETIF_F_UPPER_DISABLES (NETIF_F_LRO | NETIF_F_HW_TLS_TX)/* changeable features with no special hardware requirements */#define NETIF_F_SOFT_FEATURES (NETIF_F_GSO | NETIF_F_GRO)
In the control flow of the TLS device offload feature, the upper device
gives a pointer to the target lower device. All struct tlsdev_ops
are called directly against the lower device, bypassing the upper.
This means, the upper device has very limited means of blocking/disabling
the TLS device offload.
Today, for instance, disabling RX checksum offload of the upper dev
automatically disables the RX TLS device offload capability.
However, this does not affect the lower device at all, and it keeps
doing TLS device offload for all new connections.
Here we fix this, by propagating the disablement of the TLS RX device
offload features to all lower devices.
Fixes: a3eb4e9d4c92 ("net: Disable NETIF_F_HW_TLS_RX when RXCSUM is disabled")
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
include/linux/netdev_features.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Allow direct control of the TLS device offload features on the bond.
Disabling a TLS offload feature is propagated to all lower devices.
This solves an issue in which the bond interface had no means of enforcing
disablement of a TLS offload, as it is bypassed by direct communication
with the lower device.
Fixes: 89df6a810470 ("net/bonding: Implement TLS TX device offload")
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
drivers/net/bonding/bond_main.c | 1 +
1 file changed, 1 insertion(+)
There is no more need to enforce TLS features in bond_fix_features()
when supported, as they became explicitly controllable for a bond
interface.
Fixes: 89df6a810470 ("net/bonding: Implement TLS TX device offload")
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
drivers/net/bonding/bond_main.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-05-27 00:47:18
On Wed, 26 May 2021 12:57:41 +0300 Tariq Toukan wrote:
This RFC series suggests a solution for the following problem:
Bond interface and lower interface are both up with TLS RX/TX offloads on.
TX/RX csum offload is turned off for the upper, hence RX/TX TLS is turned off
for it as well.
Yet, although it indicates that feature is disabled, new connections are still
offloaded by the lower, as Bond has no way to impact that:
Return value of bond_sk_get_lower_dev() is agnostic to this change.
One way to solve this issue, is to bring back the Bond TLS operations callbacks,
i.e. provide implementation for struct tlsdev_ops in Bond.
This gives full control for the Bond over its features, making it aware of every
new TLS connection offload request.
This direction was proposed in the original Bond TLS implementation, but dropped
during ML review. Probably it's right to re-consider now.
Here I suggest another solution, which requires generic changes out of the bond
driver.
Fixes in patches 1 and 4 are needed anyway, independently to which solution
we choose. I'll probably submit them separately soon.
No opinions here, semantics of bond features were always clear
as mud to me. What does it mean that bond survived 20 years without
rx-csum? And it so why would TLS offload be different from what one
may presume the semantics of rx-csum are today? 🤷🏻♂️
On Wed, 26 May 2021 12:57:41 +0300 Tariq Toukan wrote:
quoted
This RFC series suggests a solution for the following problem:
Bond interface and lower interface are both up with TLS RX/TX offloads on.
TX/RX csum offload is turned off for the upper, hence RX/TX TLS is turned off
for it as well.
Yet, although it indicates that feature is disabled, new connections are still
offloaded by the lower, as Bond has no way to impact that:
Return value of bond_sk_get_lower_dev() is agnostic to this change.
One way to solve this issue, is to bring back the Bond TLS operations callbacks,
i.e. provide implementation for struct tlsdev_ops in Bond.
This gives full control for the Bond over its features, making it aware of every
new TLS connection offload request.
This direction was proposed in the original Bond TLS implementation, but dropped
during ML review. Probably it's right to re-consider now.
Here I suggest another solution, which requires generic changes out of the bond
driver.
Fixes in patches 1 and 4 are needed anyway, independently to which solution
we choose. I'll probably submit them separately soon.
No opinions here, semantics of bond features were always clear
as mud to me. What does it mean that bond survived 20 years without
rx-csum? And it so why would TLS offload be different from what one
may presume the semantics of rx-csum are today? 🤷🏻♂️
Hi Jakub,
Advanced device offloads have basic logical dependencies, that are
applied for all kind of netdevs, agnostic to internal details of each
netdev.
Nothing special with TLS really.
TLS device offload behaves similarly to TSO (needs HW_CSUM), and GRO_HW
(needs RXCSUM).
For TLS TX:
Dependency problem doesn't exist for bond because HW_CSUM is already
supported. That's why TSO is available on bond.
Currently, TLS RX is blocked, as RXCSUM is cleared.
Why wouldn't RX side of bond act in a symmetric way to TX?
Moreover:
Today, bond *does* support NETIF_F_LRO (find it in BOND_VLAN_FEATURES).
It should mean that GRO_HW can be easily enabled as well. But no, it's
blocked by the missing RXCSUM.
Actually, I think that this code below that blocks GRO_HW if no RXCSUM
should be extended to block LRO as well. But this would make a
degradation in bond, unless RXCSUM.
if (!(features & NETIF_F_RXCSUM)) {
/* NETIF_F_GRO_HW implies doing RXCSUM since every packet
* successfully merged by hardware must also have the
* checksum verified by hardware. If the user does not
* want to enable RXCSUM, logically, we should disable GRO_HW.
*/
if (features & NETIF_F_GRO_HW) {
netdev_dbg(dev, "Dropping NETIF_F_GRO_HW since no
RXCSUM feature.\n");
features &= ~NETIF_F_GRO_HW;
}
}
More relevant code from netdev_fix_features():
if ((features & NETIF_F_TSO) && !(features & NETIF_F_HW_CSUM) &&
!(features & NETIF_F_IP_CSUM)) {
netdev_dbg(dev, "Dropping TSO features since no CSUM feature.\n");
features &= ~NETIF_F_TSO;
features &= ~NETIF_F_TSO_ECN;
}
if ((features & NETIF_F_TSO6) && !(features & NETIF_F_HW_CSUM) &&
!(features & NETIF_F_IPV6_CSUM)) {
netdev_dbg(dev, "Dropping TSO6 features since no CSUM feature.\n");
features &= ~NETIF_F_TSO6;
}
Thanks,
Tariq
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-05-27 17:56:35
On Thu, 27 May 2021 17:07:06 +0300 Tariq Toukan wrote:
On 5/27/2021 3:47 AM, Jakub Kicinski wrote:
quoted
On Wed, 26 May 2021 12:57:41 +0300 Tariq Toukan wrote:
quoted
This RFC series suggests a solution for the following problem:
Bond interface and lower interface are both up with TLS RX/TX offloads on.
TX/RX csum offload is turned off for the upper, hence RX/TX TLS is turned off
for it as well.
Yet, although it indicates that feature is disabled, new connections are still
offloaded by the lower, as Bond has no way to impact that:
Return value of bond_sk_get_lower_dev() is agnostic to this change.
One way to solve this issue, is to bring back the Bond TLS operations callbacks,
i.e. provide implementation for struct tlsdev_ops in Bond.
This gives full control for the Bond over its features, making it aware of every
new TLS connection offload request.
This direction was proposed in the original Bond TLS implementation, but dropped
during ML review. Probably it's right to re-consider now.
Here I suggest another solution, which requires generic changes out of the bond
driver.
Fixes in patches 1 and 4 are needed anyway, independently to which solution
we choose. I'll probably submit them separately soon.
No opinions here, semantics of bond features were always clear
as mud to me. What does it mean that bond survived 20 years without
rx-csum? And it so why would TLS offload be different from what one
may presume the semantics of rx-csum are today?
Advanced device offloads have basic logical dependencies, that are
applied for all kind of netdevs, agnostic to internal details of each
netdev.
Nothing special with TLS really.
TLS device offload behaves similarly to TSO (needs HW_CSUM), and GRO_HW
(needs RXCSUM).
[...]
Right, the inter-dependency between features is obvious enough.
What makes a feature be part of UPPER_DISABLES though?
On Thu, 27 May 2021 17:07:06 +0300 Tariq Toukan wrote:
quoted
On 5/27/2021 3:47 AM, Jakub Kicinski wrote:
quoted
On Wed, 26 May 2021 12:57:41 +0300 Tariq Toukan wrote:
quoted
This RFC series suggests a solution for the following problem:
Bond interface and lower interface are both up with TLS RX/TX offloads on.
TX/RX csum offload is turned off for the upper, hence RX/TX TLS is turned off
for it as well.
Yet, although it indicates that feature is disabled, new connections are still
offloaded by the lower, as Bond has no way to impact that:
Return value of bond_sk_get_lower_dev() is agnostic to this change.
One way to solve this issue, is to bring back the Bond TLS operations callbacks,
i.e. provide implementation for struct tlsdev_ops in Bond.
This gives full control for the Bond over its features, making it aware of every
new TLS connection offload request.
This direction was proposed in the original Bond TLS implementation, but dropped
during ML review. Probably it's right to re-consider now.
Here I suggest another solution, which requires generic changes out of the bond
driver.
Fixes in patches 1 and 4 are needed anyway, independently to which solution
we choose. I'll probably submit them separately soon.
No opinions here, semantics of bond features were always clear
as mud to me. What does it mean that bond survived 20 years without
rx-csum? And it so why would TLS offload be different from what one
may presume the semantics of rx-csum are today?
Advanced device offloads have basic logical dependencies, that are
applied for all kind of netdevs, agnostic to internal details of each
netdev.
Nothing special with TLS really.
TLS device offload behaves similarly to TSO (needs HW_CSUM), and GRO_HW
(needs RXCSUM).
[...]
Right, the inter-dependency between features is obvious enough.
What makes a feature be part of UPPER_DISABLES though?
Regarding UPPER_DISABLES:
I propose using it here as an attempt to give the bond device some
control over kTLS offloaded connections, to avoid cases where:
(*) UPPER.ktls_device_offload==OFF
(*) LOWER.ktls_device_offload==ON
(*) Newly created connections are offloaded!! Simply ignoring and
bypassing the UPPER device state (this is how .ndo_sk_get_lower_dev works).
This is not my preferred solution though.
I think we should reconsider introducing bond implementation for "struct
tlsdev_ops" callbacks, which gives bond interface full control and
awareness to new TLS connections.
On Thu, 27 May 2021 17:07:06 +0300 Tariq Toukan wrote:
quoted
On 5/27/2021 3:47 AM, Jakub Kicinski wrote:
quoted
On Wed, 26 May 2021 12:57:41 +0300 Tariq Toukan wrote:
quoted
This RFC series suggests a solution for the following problem:
Bond interface and lower interface are both up with TLS RX/TX
offloads on.
TX/RX csum offload is turned off for the upper, hence RX/TX TLS is
turned off
for it as well.
Yet, although it indicates that feature is disabled, new
connections are still
offloaded by the lower, as Bond has no way to impact that:
Return value of bond_sk_get_lower_dev() is agnostic to this change.
One way to solve this issue, is to bring back the Bond TLS
operations callbacks,
i.e. provide implementation for struct tlsdev_ops in Bond.
This gives full control for the Bond over its features, making it
aware of every
new TLS connection offload request.
This direction was proposed in the original Bond TLS
implementation, but dropped
during ML review. Probably it's right to re-consider now.
Here I suggest another solution, which requires generic changes out
of the bond
driver.
Fixes in patches 1 and 4 are needed anyway, independently to which
solution
we choose. I'll probably submit them separately soon.
No opinions here, semantics of bond features were always clear
as mud to me. What does it mean that bond survived 20 years without
rx-csum? And it so why would TLS offload be different from what one
may presume the semantics of rx-csum are today?
Advanced device offloads have basic logical dependencies, that are
applied for all kind of netdevs, agnostic to internal details of each
netdev.
Nothing special with TLS really.
TLS device offload behaves similarly to TSO (needs HW_CSUM), and GRO_HW
(needs RXCSUM).
[...]
Right, the inter-dependency between features is obvious enough.
What makes a feature be part of UPPER_DISABLES though?
Regarding UPPER_DISABLES:
I propose using it here as an attempt to give the bond device some
control over kTLS offloaded connections, to avoid cases where:
(*) UPPER.ktls_device_offload==OFF
(*) LOWER.ktls_device_offload==ON
(*) Newly created connections are offloaded!! Simply ignoring and
bypassing the UPPER device state (this is how .ndo_sk_get_lower_dev works).
This is not my preferred solution though.
I think we should reconsider introducing bond implementation for "struct
tlsdev_ops" callbacks, which gives bond interface full control and
awareness to new TLS connections.
If you're fine with the direction, I can prepare a new series that adds
"struct tlsdev_ops" callbacks implementation for bond, instead of the
"UPPER_DISABLES solution" above.
What do you think?
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-06-07 19:37:36
On Sun, 6 Jun 2021 17:02:49 +0300 Tariq Toukan wrote:
quoted
Regarding UPPER_DISABLES:
I propose using it here as an attempt to give the bond device some
control over kTLS offloaded connections, to avoid cases where:
(*) UPPER.ktls_device_offload==OFF
(*) LOWER.ktls_device_offload==ON
(*) Newly created connections are offloaded!! Simply ignoring and
bypassing the UPPER device state (this is how .ndo_sk_get_lower_dev works).
This is not my preferred solution though.
I think we should reconsider introducing bond implementation for "struct
tlsdev_ops" callbacks, which gives bond interface full control and
awareness to new TLS connections.
If you're fine with the direction, I can prepare a new series that adds
"struct tlsdev_ops" callbacks implementation for bond, instead of the
"UPPER_DISABLES solution" above.
What do you think?
I think a design which requires teaching middle layer drivers about
individual hardware offloads for ULPs is poor, brittle and hard to
reason about.
But historically we seem to have acted in an ad-hoc fashion, so I
won't hold it against you if you prefer to keep the whack-a-mole going.