All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/6] BOND TLS flags fixes
@ 2021-05-26  9:57 Tariq Toukan
  2021-05-26  9:57 ` [RFC PATCH 1/6] net: Fix features skip in for_each_netdev_feature() Tariq Toukan
                   ` (6 more replies)
  0 siblings, 7 replies; 13+ messages in thread
From: Tariq Toukan @ 2021-05-26  9:57 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: netdev, Moshe Shemesh, Boris Pismenny, Saeed Mahameed,
	Maxim Mikityanskiy, Tariq Toukan

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [RFC PATCH 1/6] net: Fix features skip in for_each_netdev_feature()
  2021-05-26  9:57 [RFC PATCH 0/6] BOND TLS flags fixes Tariq Toukan
@ 2021-05-26  9:57 ` Tariq Toukan
  2021-05-26  9:57 ` [RFC PATCH 2/6] net: Disable TX TLS device offload on lower devices if disabled on the upper Tariq Toukan
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Tariq Toukan @ 2021-05-26  9:57 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: netdev, Moshe Shemesh, Boris Pismenny, Saeed Mahameed,
	Maxim Mikityanskiy, Tariq Toukan

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(-)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 3de38d6a0aea..1a5f0c51bc99 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -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.
  */
 static inline int find_next_netdev_feature(u64 feature, unsigned long start)
 {
@@ -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 */
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH 2/6] net: Disable TX TLS device offload on lower devices if disabled on the upper
  2021-05-26  9:57 [RFC PATCH 0/6] BOND TLS flags fixes Tariq Toukan
  2021-05-26  9:57 ` [RFC PATCH 1/6] net: Fix features skip in for_each_netdev_feature() Tariq Toukan
@ 2021-05-26  9:57 ` Tariq Toukan
  2021-05-26  9:57 ` [RFC PATCH 3/6] net: Disable RX " Tariq Toukan
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Tariq Toukan @ 2021-05-26  9:57 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: netdev, Moshe Shemesh, Boris Pismenny, Saeed Mahameed,
	Maxim Mikityanskiy, Tariq Toukan

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(-)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 1a5f0c51bc99..0061c5b988c1 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -239,7 +239,7 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)
  * If upper/master device has these features disabled, they must be disabled
  * on all lower/slave devices as well.
  */
-#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)
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH 3/6] net: Disable RX TLS device offload on lower devices if disabled on the upper
  2021-05-26  9:57 [RFC PATCH 0/6] BOND TLS flags fixes Tariq Toukan
  2021-05-26  9:57 ` [RFC PATCH 1/6] net: Fix features skip in for_each_netdev_feature() Tariq Toukan
  2021-05-26  9:57 ` [RFC PATCH 2/6] net: Disable TX TLS device offload on lower devices if disabled on the upper Tariq Toukan
@ 2021-05-26  9:57 ` Tariq Toukan
  2021-05-26  9:57 ` [RFC PATCH 4/6] net/bond: Enable RXCSUM feature for bond Tariq Toukan
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Tariq Toukan @ 2021-05-26  9:57 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: netdev, Moshe Shemesh, Boris Pismenny, Saeed Mahameed,
	Maxim Mikityanskiy, Tariq Toukan

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(-)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 0061c5b988c1..a8b33313ad17 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -239,7 +239,7 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)
  * If upper/master device has these features disabled, they must be disabled
  * on all lower/slave devices as well.
  */
-#define NETIF_F_UPPER_DISABLES	(NETIF_F_LRO | NETIF_F_HW_TLS_TX)
+#define NETIF_F_UPPER_DISABLES	(NETIF_F_LRO | NETIF_F_HW_TLS_TX | NETIF_F_HW_TLS_RX)
 
 /* changeable features with no special hardware requirements */
 #define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO)
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH 4/6] net/bond: Enable RXCSUM feature for bond
  2021-05-26  9:57 [RFC PATCH 0/6] BOND TLS flags fixes Tariq Toukan
                   ` (2 preceding siblings ...)
  2021-05-26  9:57 ` [RFC PATCH 3/6] net: Disable RX " Tariq Toukan
@ 2021-05-26  9:57 ` Tariq Toukan
  2021-05-26  9:57 ` [RFC PATCH 5/6] net/bond: Allow explicit control of the TLS device offload features Tariq Toukan
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Tariq Toukan @ 2021-05-26  9:57 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: netdev, Moshe Shemesh, Boris Pismenny, Saeed Mahameed,
	Maxim Mikityanskiy, Tariq Toukan

Enable RXCSUM device offload on bond, otherwise the TLS RX device offload
will be blocked.

Fixes: dc5809f9e2b6 ("net/bonding: Declare TLS RX device offload support")
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/bonding/bond_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index c5a646d06102..9091db0d1540 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4903,6 +4903,7 @@ void bond_setup(struct net_device *bond_dev)
 				NETIF_F_HW_VLAN_CTAG_FILTER;
 
 	bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL;
+	bond_dev->hw_features |= NETIF_F_RXCSUM;
 	bond_dev->features |= bond_dev->hw_features;
 	bond_dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
 #ifdef CONFIG_XFRM_OFFLOAD
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH 5/6] net/bond: Allow explicit control of the TLS device offload features
  2021-05-26  9:57 [RFC PATCH 0/6] BOND TLS flags fixes Tariq Toukan
                   ` (3 preceding siblings ...)
  2021-05-26  9:57 ` [RFC PATCH 4/6] net/bond: Enable RXCSUM feature for bond Tariq Toukan
@ 2021-05-26  9:57 ` Tariq Toukan
  2021-05-26  9:57 ` [RFC PATCH 6/6] net/bond: Do not turn on TLS features in bond_fix_features() Tariq Toukan
  2021-05-27  0:47 ` [RFC PATCH 0/6] BOND TLS flags fixes Jakub Kicinski
  6 siblings, 0 replies; 13+ messages in thread
From: Tariq Toukan @ 2021-05-26  9:57 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: netdev, Moshe Shemesh, Boris Pismenny, Saeed Mahameed,
	Maxim Mikityanskiy, Tariq Toukan

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(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 9091db0d1540..34a72981df38 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4913,6 +4913,7 @@ void bond_setup(struct net_device *bond_dev)
 		bond_dev->features |= BOND_XFRM_FEATURES;
 #endif /* CONFIG_XFRM_OFFLOAD */
 #if IS_ENABLED(CONFIG_TLS_DEVICE)
+	bond_dev->hw_features |= BOND_TLS_FEATURES;
 	if (bond_sk_check(bond))
 		bond_dev->features |= BOND_TLS_FEATURES;
 #endif
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH 6/6] net/bond: Do not turn on TLS features in bond_fix_features()
  2021-05-26  9:57 [RFC PATCH 0/6] BOND TLS flags fixes Tariq Toukan
                   ` (4 preceding siblings ...)
  2021-05-26  9:57 ` [RFC PATCH 5/6] net/bond: Allow explicit control of the TLS device offload features Tariq Toukan
@ 2021-05-26  9:57 ` Tariq Toukan
  2021-05-27  0:47 ` [RFC PATCH 0/6] BOND TLS flags fixes Jakub Kicinski
  6 siblings, 0 replies; 13+ messages in thread
From: Tariq Toukan @ 2021-05-26  9:57 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: netdev, Moshe Shemesh, Boris Pismenny, Saeed Mahameed,
	Maxim Mikityanskiy, Tariq Toukan

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(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 34a72981df38..3c9466e6114a 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1229,9 +1229,7 @@ static netdev_features_t bond_fix_features(struct net_device *dev,
 	struct slave *slave;
 
 #if IS_ENABLED(CONFIG_TLS_DEVICE)
-	if (bond_sk_check(bond))
-		features |= BOND_TLS_FEATURES;
-	else
+	if (!bond_sk_check(bond) && (features & BOND_TLS_FEATURES))
 		features &= ~BOND_TLS_FEATURES;
 #endif
 
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 0/6] BOND TLS flags fixes
  2021-05-26  9:57 [RFC PATCH 0/6] BOND TLS flags fixes Tariq Toukan
                   ` (5 preceding siblings ...)
  2021-05-26  9:57 ` [RFC PATCH 6/6] net/bond: Do not turn on TLS features in bond_fix_features() Tariq Toukan
@ 2021-05-27  0:47 ` Jakub Kicinski
  2021-05-27 14:07   ` Tariq Toukan
  6 siblings, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2021-05-27  0:47 UTC (permalink / raw)
  To: Tariq Toukan
  Cc: David S. Miller, netdev, Moshe Shemesh, Boris Pismenny,
	Saeed Mahameed, Maxim Mikityanskiy

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? 🤷🏻‍♂️

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 0/6] BOND TLS flags fixes
  2021-05-27  0:47 ` [RFC PATCH 0/6] BOND TLS flags fixes Jakub Kicinski
@ 2021-05-27 14:07   ` Tariq Toukan
  2021-05-27 17:56     ` Jakub Kicinski
  0 siblings, 1 reply; 13+ messages in thread
From: Tariq Toukan @ 2021-05-27 14:07 UTC (permalink / raw)
  To: Jakub Kicinski, Tariq Toukan
  Cc: David S. Miller, netdev, Moshe Shemesh, Boris Pismenny,
	Saeed Mahameed, Maxim Mikityanskiy



On 5/27/2021 3:47 AM, Jakub Kicinski wrote:
> 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? 🤷🏻‍♂️
> 

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 0/6] BOND TLS flags fixes
  2021-05-27 14:07   ` Tariq Toukan
@ 2021-05-27 17:56     ` Jakub Kicinski
  2021-05-30 10:49       ` Tariq Toukan
  0 siblings, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2021-05-27 17:56 UTC (permalink / raw)
  To: Tariq Toukan
  Cc: Tariq Toukan, David S. Miller, netdev, Moshe Shemesh,
	Boris Pismenny, Saeed Mahameed, Maxim Mikityanskiy

On Thu, 27 May 2021 17:07:06 +0300 Tariq Toukan wrote:
> On 5/27/2021 3:47 AM, Jakub Kicinski wrote:
> > 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?
> 
> 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?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 0/6] BOND TLS flags fixes
  2021-05-27 17:56     ` Jakub Kicinski
@ 2021-05-30 10:49       ` Tariq Toukan
  2021-06-06 14:02         ` Tariq Toukan
  0 siblings, 1 reply; 13+ messages in thread
From: Tariq Toukan @ 2021-05-30 10:49 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Tariq Toukan, David S. Miller, netdev, Moshe Shemesh,
	Boris Pismenny, Saeed Mahameed, Maxim Mikityanskiy



On 5/27/2021 8:56 PM, Jakub Kicinski wrote:
> On Thu, 27 May 2021 17:07:06 +0300 Tariq Toukan wrote:
>> On 5/27/2021 3:47 AM, Jakub Kicinski wrote:
>>> 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?
>>
>> 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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 0/6] BOND TLS flags fixes
  2021-05-30 10:49       ` Tariq Toukan
@ 2021-06-06 14:02         ` Tariq Toukan
  2021-06-07 19:37           ` Jakub Kicinski
  0 siblings, 1 reply; 13+ messages in thread
From: Tariq Toukan @ 2021-06-06 14:02 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Tariq Toukan, David S. Miller, netdev, Moshe Shemesh,
	Boris Pismenny, Saeed Mahameed, Maxim Mikityanskiy



On 5/30/2021 1:49 PM, Tariq Toukan wrote:
> 
> 
> On 5/27/2021 8:56 PM, Jakub Kicinski wrote:
>> On Thu, 27 May 2021 17:07:06 +0300 Tariq Toukan wrote:
>>> On 5/27/2021 3:47 AM, Jakub Kicinski wrote:
>>>> 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?
>>>
>>> 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?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 0/6] BOND TLS flags fixes
  2021-06-06 14:02         ` Tariq Toukan
@ 2021-06-07 19:37           ` Jakub Kicinski
  0 siblings, 0 replies; 13+ messages in thread
From: Jakub Kicinski @ 2021-06-07 19:37 UTC (permalink / raw)
  To: Tariq Toukan
  Cc: Tariq Toukan, David S. Miller, netdev, Moshe Shemesh,
	Boris Pismenny, Saeed Mahameed, Maxim Mikityanskiy

On Sun, 6 Jun 2021 17:02:49 +0300 Tariq Toukan wrote:
> > 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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-06-07 19:37 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-26  9:57 [RFC PATCH 0/6] BOND TLS flags fixes Tariq Toukan
2021-05-26  9:57 ` [RFC PATCH 1/6] net: Fix features skip in for_each_netdev_feature() Tariq Toukan
2021-05-26  9:57 ` [RFC PATCH 2/6] net: Disable TX TLS device offload on lower devices if disabled on the upper Tariq Toukan
2021-05-26  9:57 ` [RFC PATCH 3/6] net: Disable RX " Tariq Toukan
2021-05-26  9:57 ` [RFC PATCH 4/6] net/bond: Enable RXCSUM feature for bond Tariq Toukan
2021-05-26  9:57 ` [RFC PATCH 5/6] net/bond: Allow explicit control of the TLS device offload features Tariq Toukan
2021-05-26  9:57 ` [RFC PATCH 6/6] net/bond: Do not turn on TLS features in bond_fix_features() Tariq Toukan
2021-05-27  0:47 ` [RFC PATCH 0/6] BOND TLS flags fixes Jakub Kicinski
2021-05-27 14:07   ` Tariq Toukan
2021-05-27 17:56     ` Jakub Kicinski
2021-05-30 10:49       ` Tariq Toukan
2021-06-06 14:02         ` Tariq Toukan
2021-06-07 19:37           ` Jakub Kicinski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.