diff mbox series

[net-next,V2,5/8] net/bonding: Implement TLS TX device offload

Message ID 20210114180135.11556-6-tariqt@nvidia.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series TLS device offload for Bond | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 13 this patch: 13
netdev/kdoc success Errors and warnings before: 2 this patch: 2
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: line length of 82 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 13 this patch: 13
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Tariq Toukan Jan. 14, 2021, 6:01 p.m. UTC 
Implement TLS TX device offload for bonding interfaces.
This allows kTLS sockets running on a bond to benefit from the
device offload on capable slaves.

To allow a simple and fast maintenance of the TLS context in SW and
slaves devices, we bind the TLS socket to a specific slave.
To achieve a behavior similar to SW kTLS, we support only balance-xor
and 802.3ad modes, with xmit_hash_policy=layer3+4. This is enforced
in bond_sk_check(), done in a previous patch.

For the above configuration, the SW implementation keeps picking the
same exact slave for all the socket's SKBs. The device offload behaves
similarly, making the decision once at the connection creation.

Per socket, the TLS module should work directly with the lowest netdev
in chain, to call the tls_dev_ops operations.

As the bond interface is being bypassed by the TLS module, interacting
directly against the slaves, there is no way for the bond interface to
disable its device offload capabilities, as long as the mode/policy
config allows it.
Hence, the feature flag is not directly controllable, but just reflects
the current offload status based on the logic under bond_sk_check().

Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
Reviewed-by: Boris Pismenny <borisp@nvidia.com>
---
 drivers/net/bonding/bond_main.c    | 30 ++++++++++++++++++++++++++++++
 drivers/net/bonding/bond_options.c | 27 +++++++++++++++++++++++++--
 include/net/bonding.h              |  2 ++
 3 files changed, 57 insertions(+), 2 deletions(-)

Comments

Jakub Kicinski Jan. 17, 2021, 2:54 a.m. UTC | #1 
On Thu, 14 Jan 2021 20:01:32 +0200 Tariq Toukan wrote:
> As the bond interface is being bypassed by the TLS module, interacting
> directly against the slaves, there is no way for the bond interface to
> disable its device offload capabilities, as long as the mode/policy
> config allows it.
> Hence, the feature flag is not directly controllable, but just reflects
> the current offload status based on the logic under bond_sk_check().

In that case why set it in ->hw_features ?
IIRC features set only in ->features but not ->hw_features show up to
userspace as "fixed" which I gather is what we want here, no?

> +#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
Tariq Toukan Jan. 17, 2021, 11:04 a.m. UTC | #2 
On 1/17/2021 4:54 AM, Jakub Kicinski wrote:
> On Thu, 14 Jan 2021 20:01:32 +0200 Tariq Toukan wrote:
>> As the bond interface is being bypassed by the TLS module, interacting
>> directly against the slaves, there is no way for the bond interface to
>> disable its device offload capabilities, as long as the mode/policy
>> config allows it.
>> Hence, the feature flag is not directly controllable, but just reflects
>> the current offload status based on the logic under bond_sk_check().
> 
> In that case why set it in ->hw_features ?
> IIRC features set only in ->features but not ->hw_features show up to
> userspace as "fixed" which I gather is what we want here, no?
> 

On one hand, by showing "off [Fixed]" we might hide the fact that bond 
driver now does support the TLS offload feature, you simply need to 
choose the proper mode/xmit_policy.

On the other hand, as the feature flag toggling has totally no impact, I 
don't see a point in opening it for toggling.

So yeah, I'll fix.


>> +#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
diff mbox series

Patch

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 717ce6164780..4bec38fcc95e 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -83,6 +83,9 @@ 
 #include <net/bonding.h>
 #include <net/bond_3ad.h>
 #include <net/bond_alb.h>
+#if IS_ENABLED(CONFIG_TLS_DEVICE)
+#include <net/tls.h>
+#endif
 
 #include "bonding_priv.h"
 
@@ -1225,6 +1228,13 @@  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;
@@ -4645,6 +4655,16 @@  static struct net_device *bond_sk_get_slave(struct net_device *master_dev,
 	return slave_dev;
 }
 
+#if IS_ENABLED(CONFIG_TLS_DEVICE)
+static netdev_tx_t bond_tls_device_xmit(struct bonding *bond, struct sk_buff *skb,
+					struct net_device *dev)
+{
+	if (likely(bond_get_slave_by_dev(bond, tls_get_ctx(skb->sk)->netdev)))
+		return bond_dev_queue_xmit(bond, skb, tls_get_ctx(skb->sk)->netdev);
+	return bond_tx_drop(dev, skb);
+}
+#endif
+
 static netdev_tx_t __bond_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct bonding *bond = netdev_priv(dev);
@@ -4653,6 +4673,11 @@  static netdev_tx_t __bond_start_xmit(struct sk_buff *skb, struct net_device *dev
 	    !bond_slave_override(bond, skb))
 		return NETDEV_TX_OK;
 
+#if IS_ENABLED(CONFIG_TLS_DEVICE)
+	if (skb->sk && tls_is_sk_tx_device_offloaded(skb->sk))
+		return bond_tls_device_xmit(bond, skb, dev);
+#endif
+
 	switch (BOND_MODE(bond)) {
 	case BOND_MODE_ROUNDROBIN:
 		return bond_xmit_roundrobin(skb, dev);
@@ -4853,6 +4878,11 @@  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)
+	bond_dev->hw_features |= BOND_TLS_FEATURES;
+	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 7f0ad97926de..8fcbf7f9c7b2 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -758,6 +758,19 @@  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)
 {
@@ -784,9 +797,15 @@  static int bond_option_mode_set(struct bonding *bond,
 	bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
 	bond->params.mode = newval->value;
 
-	if (bond->dev->reg_state == NETREG_REGISTERED)
-		if (bond_set_xfrm_features(bond))
+	if (bond->dev->reg_state == NETREG_REGISTERED) {
+		bool update = false;
+
+		update |= bond_set_xfrm_features(bond);
+		update |= bond_set_tls_features(bond);
+
+		if (update)
 			netdev_update_features(bond->dev);
+	}
 
 	return 0;
 }
@@ -1220,6 +1239,10 @@  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 21497193c4a4..97fbec02df2d 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -89,6 +89,8 @@ 
 #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)
+
 #ifdef CONFIG_NET_POLL_CONTROLLER
 extern atomic_t netpoll_block_tx;