diff mbox series

[net-next] bond: Disable TLS features indication

Message ID 20221025105300.4718-1-tariqt@nvidia.com (mailing list archive)
State Accepted
Commit 28581b9c2c94cc912354eadc98c1146fdc7092e6
Delegated to: Netdev Maintainers
Headers show
Series [net-next] bond: Disable TLS features indication | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 11 this patch: 11
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 11 this patch: 11
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 83 lines checked
netdev/kdoc success Errors and warnings before: 2 this patch: 2
netdev/source_inline success Was 0 now: 0

Commit Message

Tariq Toukan Oct. 25, 2022, 10:53 a.m. UTC 
Bond agnostically interacts with TLS device-offload requests via the
.ndo_sk_get_lower_dev operation. Return value is true iff bond
guarantees fixed mapping between the TLS connection and a lower netdev.

Due to this nature, the bond TLS device offload features are not
explicitly controllable in the bond layer. As of today, these are
read-only values based on the evaluation of bond_sk_check().  However,
this indication might be incorrect and misleading, when the feature bits
are "fixed" by some dependency features.  For example,
NETIF_F_HW_TLS_TX/RX are forcefully cleared in case the corresponding
checksum offload is disabled. But in fact the bond ability to still
offload TLS connections to the lower device is not hurt.

This means that these bits can not be trusted, and hence better become
unused.

This patch revives some old discussion [1] and proposes a much simpler
solution: Clear the bond's TLS features bits. Everyone should stop
reading them.

[1] https://lore.kernel.org/netdev/20210526095747.22446-1-tariqt@nvidia.com/

Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
Reviewed-by: Gal Pressman <gal@nvidia.com>
---
 drivers/net/bonding/bond_main.c    | 13 +------------
 drivers/net/bonding/bond_options.c | 18 ------------------
 include/net/bonding.h              |  4 ----
 3 files changed, 1 insertion(+), 34 deletions(-)

Comments

Jakub Kicinski Oct. 26, 2022, 2:57 a.m. UTC | #1 
On Tue, 25 Oct 2022 13:53:00 +0300 Tariq Toukan wrote:
> Bond agnostically interacts with TLS device-offload requests via the
> .ndo_sk_get_lower_dev operation. Return value is true iff bond
> guarantees fixed mapping between the TLS connection and a lower netdev.
> 
> Due to this nature, the bond TLS device offload features are not
> explicitly controllable in the bond layer. As of today, these are
> read-only values based on the evaluation of bond_sk_check().  However,
> this indication might be incorrect and misleading, when the feature bits
> are "fixed" by some dependency features.  For example,
> NETIF_F_HW_TLS_TX/RX are forcefully cleared in case the corresponding
> checksum offload is disabled. But in fact the bond ability to still
> offload TLS connections to the lower device is not hurt.
> 
> This means that these bits can not be trusted, and hence better become
> unused.
> 
> This patch revives some old discussion [1] and proposes a much simpler
> solution: Clear the bond's TLS features bits. Everyone should stop
> reading them.

Acked-by: Jakub Kicinski <kuba@kernel.org>
patchwork-bot+netdevbpf@kernel.org Oct. 27, 2022, 10:50 a.m. UTC | #2 
Hello:

This patch was applied to netdev/net-next.git (master)
by Paolo Abeni <pabeni@redhat.com>:

On Tue, 25 Oct 2022 13:53:00 +0300 you wrote:
> Bond agnostically interacts with TLS device-offload requests via the
> .ndo_sk_get_lower_dev operation. Return value is true iff bond
> guarantees fixed mapping between the TLS connection and a lower netdev.
> 
> Due to this nature, the bond TLS device offload features are not
> explicitly controllable in the bond layer. As of today, these are
> read-only values based on the evaluation of bond_sk_check().  However,
> this indication might be incorrect and misleading, when the feature bits
> are "fixed" by some dependency features.  For example,
> NETIF_F_HW_TLS_TX/RX are forcefully cleared in case the corresponding
> checksum offload is disabled. But in fact the bond ability to still
> offload TLS connections to the lower device is not hurt.
> 
> [...]

Here is the summary with links:
  - [net-next] bond: Disable TLS features indication
    https://git.kernel.org/netdev/net-next/c/28581b9c2c94

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e84c49bf4d0c..1cd4e71916f8 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -307,7 +307,7 @@  netdev_tx_t bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb,
 	return dev_queue_xmit(skb);
 }
 
-bool bond_sk_check(struct bonding *bond)
+static bool bond_sk_check(struct bonding *bond)
 {
 	switch (BOND_MODE(bond)) {
 	case BOND_MODE_8023AD:
@@ -1398,13 +1398,6 @@  static netdev_features_t bond_fix_features(struct net_device *dev,
 	netdev_features_t mask;
 	struct slave *slave;
 
-#if IS_ENABLED(CONFIG_TLS_DEVICE)
-	if (bond_sk_check(bond))
-		features |= BOND_TLS_FEATURES;
-	else
-		features &= ~BOND_TLS_FEATURES;
-#endif
-
 	mask = features;
 
 	features &= ~NETIF_F_ONE_FOR_ALL;
@@ -5806,10 +5799,6 @@  void bond_setup(struct net_device *bond_dev)
 	if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP)
 		bond_dev->features |= BOND_XFRM_FEATURES;
 #endif /* CONFIG_XFRM_OFFLOAD */
-#if IS_ENABLED(CONFIG_TLS_DEVICE)
-	if (bond_sk_check(bond))
-		bond_dev->features |= BOND_TLS_FEATURES;
-#endif
 }
 
 /* Destroy a bonding device.
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 3498db1c1b3c..f71d5517f829 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -842,19 +842,6 @@  static bool bond_set_xfrm_features(struct bonding *bond)
 	return true;
 }
 
-static bool bond_set_tls_features(struct bonding *bond)
-{
-	if (!IS_ENABLED(CONFIG_TLS_DEVICE))
-		return false;
-
-	if (bond_sk_check(bond))
-		bond->dev->wanted_features |= BOND_TLS_FEATURES;
-	else
-		bond->dev->wanted_features &= ~BOND_TLS_FEATURES;
-
-	return true;
-}
-
 static int bond_option_mode_set(struct bonding *bond,
 				const struct bond_opt_value *newval)
 {
@@ -885,7 +872,6 @@  static int bond_option_mode_set(struct bonding *bond,
 		bool update = false;
 
 		update |= bond_set_xfrm_features(bond);
-		update |= bond_set_tls_features(bond);
 
 		if (update)
 			netdev_update_features(bond->dev);
@@ -1418,10 +1404,6 @@  static int bond_option_xmit_hash_policy_set(struct bonding *bond,
 		   newval->string, newval->value);
 	bond->params.xmit_policy = newval->value;
 
-	if (bond->dev->reg_state == NETREG_REGISTERED)
-		if (bond_set_tls_features(bond))
-			netdev_update_features(bond->dev);
-
 	return 0;
 }
 
diff --git a/include/net/bonding.h b/include/net/bonding.h
index e999f851738b..ea36ab7f9e72 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -92,8 +92,6 @@ 
 #define BOND_XFRM_FEATURES (NETIF_F_HW_ESP | NETIF_F_HW_ESP_TX_CSUM | \
 			    NETIF_F_GSO_ESP)
 
-#define BOND_TLS_FEATURES (NETIF_F_HW_TLS_TX | NETIF_F_HW_TLS_RX)
-
 #ifdef CONFIG_NET_POLL_CONTROLLER
 extern atomic_t netpoll_block_tx;
 
@@ -280,8 +278,6 @@  struct bond_vlan_tag {
 	unsigned short	vlan_id;
 };
 
-bool bond_sk_check(struct bonding *bond);
-
 /**
  * Returns NULL if the net_device does not belong to any of the bond's slaves
  *