[PATCH net-next 0/2] TLS TX HW offload for Bond

[PATCH net-next 0/2] TLS TX HW offload for Bond

[Tariq Toukan]

Hi,

This series opens TLS TX HW offload for bond interfaces.
This allows bond interfaces to benefit from capable slave devices.

The first patch adds real_dev field in TLS context structure, and aligns
usages in TLS module and supporting drivers.
The second patch opens the offload for bond interfaces.

For the configuration above, SW kTLS keeps picking the same slave
To keep simple track of the HW and SW TLS contexts, we bind each socket to
a specific slave for the socket's whole lifetime. This is logically valid
(and similar to the SW kTLS behavior) in the following bond configuration, 
so we restrict the offload support to it:

((mode == balance-xor) or (mode == 802.3ad))
and xmit_hash_policy == layer3+4.

Regards,
Tariq

Tariq Toukan (2):
  net/tls: Add real_dev field to TLS context
  bond: Add TLS TX offload support

 drivers/net/bonding/bond_main.c               | 203 +++++++++++++++++-
 drivers/net/bonding/bond_options.c            |  10 +-
 .../chelsio/inline_crypto/ch_ktls/chcr_ktls.c |   2 +-
 .../mellanox/mlx5/core/en_accel/tls_rxtx.c    |   2 +-
 include/net/bonding.h                         |   4 +
 include/net/tls.h                             |   1 +
 net/tls/tls_device.c                          |   2 +
 net/tls/tls_device_fallback.c                 |   2 +-
 8 files changed, 216 insertions(+), 10 deletions(-)

-- 
2.21.0

[PATCH net-next 1/2] net/tls: Add real_dev field to TLS context

[Tariq Toukan]

Save the real device for the TLS context.
Upper devices that support TLS offload should init real_dev
to point to the slave dev in tls_dev_add().
Lower device drivers should work only against real_dev.

Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
Reviewed-by: Maxim Mikityanskiy <maximmi@mellanox.com>
Reviewed-by: Boris Pismenny <borisp@nvidia.com>
---
 drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c | 2 +-
 drivers/net/ethernet/mellanox/mlx5/core/en_accel/tls_rxtx.c    | 2 +-
 include/net/tls.h                                              | 1 +
 net/tls/tls_device.c                                           | 2 ++
 net/tls/tls_device_fallback.c                                  | 2 +-
 5 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
index c24485c0d512..d70839f2f267 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
@@ -1987,7 +1987,7 @@ static int chcr_ktls_xmit(struct sk_buff *skb, struct net_device *dev)
 	mss = skb_is_gso(skb) ? skb_shinfo(skb)->gso_size : data_len;
 
 	tls_ctx = tls_get_ctx(skb->sk);
-	if (unlikely(tls_ctx->netdev != dev))
+	if (unlikely(tls_ctx->real_dev != dev))
 		goto out;
 
 	tx_ctx = chcr_get_ktls_tx_context(tls_ctx);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/tls_rxtx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/tls_rxtx.c
index f51c04284e4d..7f912ba18948 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/tls_rxtx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/tls_rxtx.c
@@ -273,7 +273,7 @@ bool mlx5e_tls_handle_tx_skb(struct net_device *netdev, struct mlx5e_txqsq *sq,
 	mlx5e_tx_mpwqe_ensure_complete(sq);
 
 	tls_ctx = tls_get_ctx(skb->sk);
-	if (WARN_ON_ONCE(tls_ctx->netdev != netdev))
+	if (WARN_ON_ONCE(tls_ctx->real_dev != netdev))
 		goto err_out;
 
 	if (mlx5_accel_is_ktls_tx(sq->channel->mdev))
diff --git a/include/net/tls.h b/include/net/tls.h
index baf1e99d8193..3f37443ac7e6 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -241,6 +241,7 @@ struct tls_context {
 	void *priv_ctx_rx;
 
 	struct net_device *netdev;
+	struct net_device *real_dev;
 
 	/* rw cache line */
 	struct cipher_context tx;
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index cec86229a6a0..f97a8aaacf14 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -970,6 +970,8 @@ static void tls_device_attach(struct tls_context *ctx, struct sock *sk,
 		refcount_set(&ctx->refcount, 1);
 		dev_hold(netdev);
 		ctx->netdev = netdev;
+		if (!ctx->real_dev)
+			ctx->real_dev = netdev;
 		spin_lock_irq(&tls_device_lock);
 		list_add_tail(&ctx->list, &tls_device_list);
 		spin_unlock_irq(&tls_device_lock);
diff --git a/net/tls/tls_device_fallback.c b/net/tls/tls_device_fallback.c
index 28895333701e..516db5ff41ee 100644
--- a/net/tls/tls_device_fallback.c
+++ b/net/tls/tls_device_fallback.c
@@ -423,7 +423,7 @@ struct sk_buff *tls_validate_xmit_skb(struct sock *sk,
 				      struct net_device *dev,
 				      struct sk_buff *skb)
 {
-	if (dev == tls_get_ctx(sk)->netdev)
+	if (dev == tls_get_ctx(sk)->netdev || dev == tls_get_ctx(sk)->real_dev)
 		return skb;
 
 	return tls_sw_fallback(sk, skb);
-- 
2.21.0

[PATCH net-next 2/2] bond: Add TLS TX offload support

[Tariq Toukan]

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 in
the tls_dev_add operation.
To acheive a behavior similar to SW kTLS, we support only balance-xor
and 802.3ad modes, with xmit_hash_policy=layer3+4.
For the above configuration, the SW implementation keeps picking the
same exact slave for all the socket's SKBs.

We keep the bond feature bit independent from the slaves bits.
In case a non-capable slave is picked, the socket falls-back to
SW kTLS.

Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
Reviewed-by: Maxim Mikityanskiy <maximmi@mellanox.com>
Reviewed-by: Boris Pismenny <borisp@nvidia.com>
---
 drivers/net/bonding/bond_main.c    | 203 ++++++++++++++++++++++++++++-
 drivers/net/bonding/bond_options.c |  10 +-
 include/net/bonding.h              |   4 +
 3 files changed, 210 insertions(+), 7 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 71c9677d135f..ed6d70e80ce9 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"
 
@@ -301,6 +304,17 @@ netdev_tx_t bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb,
 	return dev_queue_xmit(skb);
 }
 
+static u32 bond_hash_ip(u32 hash, struct flow_keys *flow)
+{
+	hash ^= (__force u32)flow_get_u32_dst(flow) ^
+		(__force u32)flow_get_u32_src(flow);
+	hash ^= (hash >> 16);
+	hash ^= (hash >> 8);
+
+	/* discard lowest hash bit to deal with the common even ports pattern */
+	return hash >> 1;
+}
+
 /*---------------------------------- VLAN -----------------------------------*/
 
 /* In the following 2 functions, bond_vlan_rx_add_vid and bond_vlan_rx_kill_vid,
@@ -467,6 +481,143 @@ static const struct xfrmdev_ops bond_xfrmdev_ops = {
 };
 #endif /* CONFIG_XFRM_OFFLOAD */
 
+/*---------------------------------- TLS ------------------------------------*/
+
+#if IS_ENABLED(CONFIG_TLS_DEVICE)
+/**
+ * bond_sk_hash_l34 - generate a hash value based on the socket's L3 and L4 fields
+ * @sk: socket to use for headers
+ *
+ * This function will extract the necessary field from the socket and use
+ * them to generate a hash based on the LAYER34 xmit_policy.
+ * Assumes that sk is a TCP or UDP socket.
+ */
+static u32 bond_sk_hash_l34(struct sock *sk)
+{
+	struct flow_keys flow = {};
+	u32 hash;
+
+	switch (sk->sk_family) {
+#if IS_ENABLED(CONFIG_IPV6)
+	case AF_INET6:
+		if (sk->sk_ipv6only ||
+		    ipv6_addr_type(&sk->sk_v6_daddr) != IPV6_ADDR_MAPPED) {
+			flow.control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
+			flow.addrs.v6addrs.src = inet6_sk(sk)->saddr;
+			flow.addrs.v6addrs.dst = sk->sk_v6_daddr;
+			break;
+		}
+		fallthrough;
+#endif
+	default: /* AF_INET */
+		flow.control.addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS;
+		flow.addrs.v4addrs.src = inet_sk(sk)->inet_rcv_saddr;
+		flow.addrs.v4addrs.dst = inet_sk(sk)->inet_daddr;
+		break;
+	}
+
+	flow.ports.src = inet_sk(sk)->inet_sport;
+	flow.ports.dst = inet_sk(sk)->inet_dport;
+
+	memcpy(&hash, &flow.ports.ports, sizeof(hash));
+
+	return bond_hash_ip(hash, &flow);
+}
+
+static struct slave *bond_sk_3ad_xor_slave_get(struct bonding *bond, struct sock *sk)
+{
+	struct bond_up_slave *slaves;
+	unsigned int count;
+	u32 hash;
+
+	if (bond->params.xmit_policy != BOND_XMIT_POLICY_LAYER34)
+		return NULL;
+
+	slaves = rcu_dereference(bond->usable_slaves);
+	count = slaves ? READ_ONCE(slaves->count) : 0;
+	if (unlikely(!count))
+		return NULL;
+
+	hash = bond_sk_hash_l34(sk);
+	return slaves->arr[hash % count];
+}
+
+static struct net_device *bond_sk_get_slave_dev(struct bonding *bond, struct sock *sk)
+{
+	struct slave *slave = NULL;
+
+	switch (BOND_MODE(bond)) {
+	case BOND_MODE_8023AD:
+	case BOND_MODE_XOR:
+		slave = bond_sk_3ad_xor_slave_get(bond, sk);
+		break;
+	default:
+		break;
+	}
+
+	return slave ? slave->dev : NULL;
+}
+
+static int bond_tls_add(struct net_device *bond_dev, struct sock *sk,
+			enum tls_offload_ctx_dir direction,
+			struct tls_crypto_info *crypto_info,
+			u32 start_offload_tcp_sn)
+{
+	struct tls_context *tls_ctx = tls_get_ctx(sk);
+	struct bonding *bond = netdev_priv(bond_dev);
+	struct net_device *slave_dev;
+
+	if (unlikely(direction != TLS_OFFLOAD_CTX_DIR_TX))
+		return -EOPNOTSUPP;
+
+	slave_dev = bond_sk_get_slave_dev(bond, sk);
+	if (unlikely(!slave_dev))
+		return -EINVAL;
+
+	if (!slave_dev->tlsdev_ops ||
+	    !slave_dev->tlsdev_ops->tls_dev_add ||
+	    !slave_dev->tlsdev_ops->tls_dev_del ||
+	    !slave_dev->tlsdev_ops->tls_dev_resync)
+		return -EINVAL;
+
+	if (!(slave_dev->features & NETIF_F_HW_TLS_TX))
+		return -EINVAL;
+
+	tls_ctx->real_dev = slave_dev;
+	return slave_dev->tlsdev_ops->tls_dev_add(slave_dev, sk, direction, crypto_info,
+						  start_offload_tcp_sn);
+}
+
+static void bond_tls_del(struct net_device *bond_dev, struct tls_context *tls_ctx,
+			 enum tls_offload_ctx_dir direction)
+{
+	struct net_device *slave_dev = tls_ctx->real_dev;
+
+	if (unlikely(direction != TLS_OFFLOAD_CTX_DIR_TX))
+		return;
+
+	slave_dev->tlsdev_ops->tls_dev_del(slave_dev, tls_ctx, direction);
+}
+
+static int bond_tls_resync(struct net_device *bond_dev,
+			   struct sock *sk, u32 seq, u8 *rcd_sn,
+			   enum tls_offload_ctx_dir direction)
+{
+	struct net_device *slave_dev = tls_get_ctx(sk)->real_dev;
+
+	if (unlikely(direction != TLS_OFFLOAD_CTX_DIR_TX))
+		return -EOPNOTSUPP;
+
+	return slave_dev->tlsdev_ops->tls_dev_resync(slave_dev, sk, seq, rcd_sn, direction);
+}
+
+static const struct tlsdev_ops bond_tls_ops = {
+	.tls_dev_add = bond_tls_add,
+	.tls_dev_del = bond_tls_del,
+	.tls_dev_resync = bond_tls_resync,
+};
+#endif
+
 /*------------------------------- Link status -------------------------------*/
 
 /* Set the carrier state for the master according to the state of its
@@ -1204,6 +1355,21 @@ static void bond_netpoll_cleanup(struct net_device *bond_dev)
 
 /*---------------------------------- IOCTL ----------------------------------*/
 
+#if IS_ENABLED(CONFIG_TLS_DEVICE)
+bool bond_state_support_tls(struct bonding *bond)
+{
+	switch (BOND_MODE(bond)) {
+	case BOND_MODE_8023AD:
+	case BOND_MODE_XOR:
+		if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34)
+			return true;
+		fallthrough;
+	default:
+		return false;
+	}
+}
+#endif
+
 static netdev_features_t bond_fix_features(struct net_device *dev,
 					   netdev_features_t features)
 {
@@ -1212,6 +1378,11 @@ 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 ((features & BOND_TLS_FEATURES) && !bond_state_support_tls(bond))
+		features &= ~BOND_TLS_FEATURES;
+#endif
+
 	mask = features;
 
 	features &= ~NETIF_F_ONE_FOR_ALL;
@@ -3544,12 +3715,8 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
 		else
 			memcpy(&hash, &flow.ports.ports, sizeof(hash));
 	}
-	hash ^= (__force u32)flow_get_u32_dst(&flow) ^
-		(__force u32)flow_get_u32_src(&flow);
-	hash ^= (hash >> 16);
-	hash ^= (hash >> 8);
 
-	return hash >> 1;
+	return bond_hash_ip(hash, &flow);
 }
 
 /*-------------------------- Device entry points ----------------------------*/
@@ -4522,6 +4689,16 @@ static struct net_device *bond_xmit_get_slave(struct net_device *master_dev,
 	return NULL;
 }
 
+#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)->real_dev)))
+		return bond_dev_queue_xmit(bond, skb, tls_get_ctx(skb->sk)->real_dev);
+	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);
@@ -4530,6 +4707,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);
@@ -4702,7 +4884,9 @@ void bond_setup(struct net_device *bond_dev)
 	bond_dev->xfrmdev_ops = &bond_xfrmdev_ops;
 	bond->xs = NULL;
 #endif /* CONFIG_XFRM_OFFLOAD */
-
+#if IS_ENABLED(CONFIG_TLS_DEVICE)
+	bond_dev->tlsdev_ops = &bond_tls_ops;
+#endif
 	/* don't acquire bond device's netif_tx_lock when transmitting */
 	bond_dev->features |= NETIF_F_LLTX;
 
@@ -4724,6 +4908,9 @@ void bond_setup(struct net_device *bond_dev)
 #ifdef CONFIG_XFRM_OFFLOAD
 	bond_dev->hw_features |= BOND_XFRM_FEATURES;
 #endif /* CONFIG_XFRM_OFFLOAD */
+#if IS_ENABLED(CONFIG_TLS_DEVICE)
+	bond_dev->hw_features |= BOND_TLS_FEATURES;
+#endif
 	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
@@ -4731,6 +4918,10 @@ 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_state_support_tls(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 9abfaae1c6f7..3ad97cc55421 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -772,13 +772,16 @@ static int bond_option_mode_set(struct bonding *bond,
 		bond->dev->wanted_features |= BOND_XFRM_FEATURES;
 	else
 		bond->dev->wanted_features &= ~BOND_XFRM_FEATURES;
-	netdev_change_features(bond->dev);
 #endif /* CONFIG_XFRM_OFFLOAD */
 
 	/* don't cache arp_validate between modes */
 	bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
 	bond->params.mode = newval->value;
 
+#if IS_ENABLED(CONFIG_TLS_DEVICE) || defined(CONFIG_XFRM_OFFLOAD)
+	netdev_change_features(bond->dev);
+#endif
+
 	return 0;
 }
 
@@ -1211,6 +1214,11 @@ static int bond_option_xmit_hash_policy_set(struct bonding *bond,
 		   newval->string, newval->value);
 	bond->params.xmit_policy = newval->value;
 
+#if IS_ENABLED(CONFIG_TLS_DEVICE)
+	if (bond->dev->wanted_features & BOND_TLS_FEATURES)
+		netdev_change_features(bond->dev);
+#endif
+
 	return 0;
 }
 
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 7d132cc1e584..00e2c06732c5 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -90,6 +90,10 @@
 #define BOND_XFRM_FEATURES (NETIF_F_HW_ESP | NETIF_F_HW_ESP_TX_CSUM | \
 			    NETIF_F_GSO_ESP)
 #endif /* CONFIG_XFRM_OFFLOAD */
+#if IS_ENABLED(CONFIG_TLS_DEVICE)
+#define BOND_TLS_FEATURES (NETIF_F_HW_TLS_TX)
+bool bond_state_support_tls(struct bonding *bond);
+#endif
 
 #ifdef CONFIG_NET_POLL_CONTROLLER
 extern atomic_t netpoll_block_tx;
-- 
2.21.0

Re: [PATCH net-next 0/2] TLS TX HW offload for Bond

[Jakub Kicinski]

On Sun, 15 Nov 2020 15:42:49 +0200 Tariq Toukan wrote:
> This series opens TLS TX HW offload for bond interfaces.
> This allows bond interfaces to benefit from capable slave devices.
> 
> The first patch adds real_dev field in TLS context structure, and aligns
> usages in TLS module and supporting drivers.
> The second patch opens the offload for bond interfaces.
> 
> For the configuration above, SW kTLS keeps picking the same slave
> To keep simple track of the HW and SW TLS contexts, we bind each socket to
> a specific slave for the socket's whole lifetime. This is logically valid
> (and similar to the SW kTLS behavior) in the following bond configuration, 
> so we restrict the offload support to it:
> 
> ((mode == balance-xor) or (mode == 802.3ad))
> and xmit_hash_policy == layer3+4.

This does not feel extremely clean, maybe you can convince me otherwise.

Can we extend netdev_get_xmit_slave() and figure out the output dev
(and if it's "stable") in a more generic way? And just feed that dev
into TLS handling? All non-crypto upper SW devs should be safe to cross
with .decrypted = 1 skbs, right?

Re: [PATCH net-next 0/2] TLS TX HW offload for Bond

[Tariq Toukan]

On 11/19/2020 2:02 AM, Jakub Kicinski wrote:
> On Sun, 15 Nov 2020 15:42:49 +0200 Tariq Toukan wrote:
>> This series opens TLS TX HW offload for bond interfaces.
>> This allows bond interfaces to benefit from capable slave devices.
>>
>> The first patch adds real_dev field in TLS context structure, and aligns
>> usages in TLS module and supporting drivers.
>> The second patch opens the offload for bond interfaces.
>>
>> For the configuration above, SW kTLS keeps picking the same slave
>> To keep simple track of the HW and SW TLS contexts, we bind each socket to
>> a specific slave for the socket's whole lifetime. This is logically valid
>> (and similar to the SW kTLS behavior) in the following bond configuration,
>> so we restrict the offload support to it:
>>
>> ((mode == balance-xor) or (mode == 802.3ad))
>> and xmit_hash_policy == layer3+4.
> 
> This does not feel extremely clean, maybe you can convince me otherwise.
> 
> Can we extend netdev_get_xmit_slave() and figure out the output dev
> (and if it's "stable") in a more generic way? And just feed that dev
> into TLS handling? 

Hi Jakub,

I don't see we go through netdev_get_xmit_slave(), but through 
.ndo_start_xmit (bond_start_xmit). Currently I have my check there to 
catch all skbs belonging to offloaded TLS sockets.

The TLS offload get_slave() logic decision is per socket, so the result 
cannot be saved in the bond memory. Currently I save the real_dev field 
in the TLS context structure.
One way to make it more generic is to save it on the sock structure. I 
agree that this replaces the TLS-specific logic, but demands increasing 
the sock struct, and has larger impact on all other flows...

What do you think?
If we decide to go with this, I can provide the patches.

> All non-crypto upper SW devs should be safe to cross
> with .decrypted = 1 skbs, right?
> 

AFAIU yes.

Re: [PATCH net-next 0/2] TLS TX HW offload for Bond

[Jakub Kicinski]

On Thu, 19 Nov 2020 17:59:38 +0200 Tariq Toukan wrote:
> On 11/19/2020 2:02 AM, Jakub Kicinski wrote:
> > On Sun, 15 Nov 2020 15:42:49 +0200 Tariq Toukan wrote:  
> >> This series opens TLS TX HW offload for bond interfaces.
> >> This allows bond interfaces to benefit from capable slave devices.
> >>
> >> The first patch adds real_dev field in TLS context structure, and aligns
> >> usages in TLS module and supporting drivers.
> >> The second patch opens the offload for bond interfaces.
> >>
> >> For the configuration above, SW kTLS keeps picking the same slave
> >> To keep simple track of the HW and SW TLS contexts, we bind each socket to
> >> a specific slave for the socket's whole lifetime. This is logically valid
> >> (and similar to the SW kTLS behavior) in the following bond configuration,
> >> so we restrict the offload support to it:
> >>
> >> ((mode == balance-xor) or (mode == 802.3ad))
> >> and xmit_hash_policy == layer3+4.  
> > 
> > This does not feel extremely clean, maybe you can convince me otherwise.
> > 
> > Can we extend netdev_get_xmit_slave() and figure out the output dev
> > (and if it's "stable") in a more generic way? And just feed that dev
> > into TLS handling?   
> 
> I don't see we go through netdev_get_xmit_slave(), but through 
> .ndo_start_xmit (bond_start_xmit).

I may be misunderstanding the purpose of netdev_get_xmit_slave(),
please correct me if I'm wrong. AFAIU it's supposed to return a
lower netdev that the skb should then be xmited on.

So what I was thinking was either construct an skb or somehow reshuffle
the netdev_get_xmit_slave() code to take a flow dissector output or
${insert other ideas}. Then add a helper in the core that would drill
down from the socket netdev to the "egress" netdev. Have TLS call
that helper, and talk to the "egress" netdev from the start, rather
than the socket's netdev. Then loosen the checks on software devices.

I'm probably missing the problem you're trying to explain to me :S

Side note - Jarod, I'd be happy to take a patch renaming
netdev_get_xmit_slave() and the ndo, if you have the cycles to send 
a patch. It's a recent addition, and in the core we should make more 
of an effort to avoid sensitive terms.

> Currently I have my check there to 
> catch all skbs belonging to offloaded TLS sockets.
> 
> The TLS offload get_slave() logic decision is per socket, so the result 
> cannot be saved in the bond memory. Currently I save the real_dev field 
> in the TLS context structure.

Right, but we could just have ctx->netdev be the "egress" netdev
always, right? Do we expect somewhere that it's going to be matching
the socket's dst?

> One way to make it more generic is to save it on the sock structure. I 
> agree that this replaces the TLS-specific logic, but demands increasing 
> the sock struct, and has larger impact on all other flows...

Re: [PATCH net-next 0/2] TLS TX HW offload for Bond

[Tariq Toukan]

On 11/19/2020 6:38 PM, Jakub Kicinski wrote:
> On Thu, 19 Nov 2020 17:59:38 +0200 Tariq Toukan wrote:
>> On 11/19/2020 2:02 AM, Jakub Kicinski wrote:
>>> On Sun, 15 Nov 2020 15:42:49 +0200 Tariq Toukan wrote:
>>>> This series opens TLS TX HW offload for bond interfaces.
>>>> This allows bond interfaces to benefit from capable slave devices.
>>>>
>>>> The first patch adds real_dev field in TLS context structure, and aligns
>>>> usages in TLS module and supporting drivers.
>>>> The second patch opens the offload for bond interfaces.
>>>>
>>>> For the configuration above, SW kTLS keeps picking the same slave
>>>> To keep simple track of the HW and SW TLS contexts, we bind each socket to
>>>> a specific slave for the socket's whole lifetime. This is logically valid
>>>> (and similar to the SW kTLS behavior) in the following bond configuration,
>>>> so we restrict the offload support to it:
>>>>
>>>> ((mode == balance-xor) or (mode == 802.3ad))
>>>> and xmit_hash_policy == layer3+4.
>>>
>>> This does not feel extremely clean, maybe you can convince me otherwise.
>>>
>>> Can we extend netdev_get_xmit_slave() and figure out the output dev
>>> (and if it's "stable") in a more generic way? And just feed that dev
>>> into TLS handling?
>>
>> I don't see we go through netdev_get_xmit_slave(), but through
>> .ndo_start_xmit (bond_start_xmit).
> 
> I may be misunderstanding the purpose of netdev_get_xmit_slave(),
> please correct me if I'm wrong. AFAIU it's supposed to return a
> lower netdev that the skb should then be xmited on.

That's true. It was recently added and used by the RDMA team. Not used 
or integrated in the Eth networking stack.

> So what I was thinking was either construct an skb or somehow reshuffle
> the netdev_get_xmit_slave() code to take a flow dissector output or
> ${insert other ideas}. Then add a helper in the core that would drill
> down from the socket netdev to the "egress" netdev. Have TLS call
> that helper, and talk to the "egress" netdev from the start, rather
> than the socket's netdev. Then loosen the checks on software devices.

As I understand it, best if we can even generalize this to apply to all 
kinds of traffic: bond driver won't do the xmit itself anymore, it just 
picks an egress dev and returns it. The core infrastructure will call 
the xmit function for the egress dev.

I like the idea, it can generalize code structures for all kinds of 
upper-devices and sockets, taking them into a common place in core, 
which reduces code duplications.

If we go only half the way, i.e. keep xmit logic in bond for 
non-TLS-offloaded traffic, then we have to let TLS module (and others in 
the future) act deferentially for different kinds of devs (upper/lower) 
which IMHO reduces generality.

I'm in favor of the deeper change. It will be on a larger scale, and 
totally orthogonal to the current TLS offload support in bond.

If we decide to apply the idea only to TLS sockets (or any subset of 
sockets) we're actually taking a generic one-flow (the xmit patch of a 
bond dev) and turning it into two (or potentially more) flows, depending 
on the socket type. This also reduces generality.

> 
> I'm probably missing the problem you're trying to explain to me :S
> 

I kept the patch minimal, and kept the TLS offload logic internal to the 
bond driver, just like it is internal to the device drivers (mlx5e, and 
others), with no core infrastructure modification.

> Side note - Jarod, I'd be happy to take a patch renaming
> netdev_get_xmit_slave() and the ndo, if you have the cycles to send
> a patch. It's a recent addition, and in the core we should make more
> of an effort to avoid sensitive terms.
> 
>> Currently I have my check there to
>> catch all skbs belonging to offloaded TLS sockets.
>>
>> The TLS offload get_slave() logic decision is per socket, so the result
>> cannot be saved in the bond memory. Currently I save the real_dev field
>> in the TLS context structure.
> 
> Right, but we could just have ctx->netdev be the "egress" netdev
> always, right? Do we expect somewhere that it's going to be matching
> the socket's dst?
> 

So once the offload context is established we totally bypass the bond 
dev? and lose interaction or reference to it?
What if the egress dev is detached form the bond? We must then be 
notified somehow.

>> One way to make it more generic is to save it on the sock structure. I
>> agree that this replaces the TLS-specific logic, but demands increasing
>> the sock struct, and has larger impact on all other flows...
> 

Re: [PATCH net-next 0/2] TLS TX HW offload for Bond

[Jakub Kicinski]

On Sun, 22 Nov 2020 14:48:04 +0200 Tariq Toukan wrote:
> On 11/19/2020 6:38 PM, Jakub Kicinski wrote:
> > On Thu, 19 Nov 2020 17:59:38 +0200 Tariq Toukan wrote:  
> >> On 11/19/2020 2:02 AM, Jakub Kicinski wrote:  
> >>> On Sun, 15 Nov 2020 15:42:49 +0200 Tariq Toukan wrote:  
> >>>> This series opens TLS TX HW offload for bond interfaces.
> >>>> This allows bond interfaces to benefit from capable slave devices.
> >>>>
> >>>> The first patch adds real_dev field in TLS context structure, and aligns
> >>>> usages in TLS module and supporting drivers.
> >>>> The second patch opens the offload for bond interfaces.
> >>>>
> >>>> For the configuration above, SW kTLS keeps picking the same slave
> >>>> To keep simple track of the HW and SW TLS contexts, we bind each socket to
> >>>> a specific slave for the socket's whole lifetime. This is logically valid
> >>>> (and similar to the SW kTLS behavior) in the following bond configuration,
> >>>> so we restrict the offload support to it:
> >>>>
> >>>> ((mode == balance-xor) or (mode == 802.3ad))
> >>>> and xmit_hash_policy == layer3+4.  
> >>>
> >>> This does not feel extremely clean, maybe you can convince me otherwise.
> >>>
> >>> Can we extend netdev_get_xmit_slave() and figure out the output dev
> >>> (and if it's "stable") in a more generic way? And just feed that dev
> >>> into TLS handling?  
> >>
> >> I don't see we go through netdev_get_xmit_slave(), but through
> >> .ndo_start_xmit (bond_start_xmit).  
> > 
> > I may be misunderstanding the purpose of netdev_get_xmit_slave(),
> > please correct me if I'm wrong. AFAIU it's supposed to return a
> > lower netdev that the skb should then be xmited on.  
> 
> That's true. It was recently added and used by the RDMA team. Not used 
> or integrated in the Eth networking stack.
> 
> > So what I was thinking was either construct an skb or somehow reshuffle
> > the netdev_get_xmit_slave() code to take a flow dissector output or
> > ${insert other ideas}. Then add a helper in the core that would drill
> > down from the socket netdev to the "egress" netdev. Have TLS call
> > that helper, and talk to the "egress" netdev from the start, rather
> > than the socket's netdev. Then loosen the checks on software devices.  
> 
> As I understand it, best if we can even generalize this to apply to all 
> kinds of traffic: bond driver won't do the xmit itself anymore, it just 
> picks an egress dev and returns it. The core infrastructure will call 
> the xmit function for the egress dev.

I think you went way further than I was intending :) I was only
considering the control path. Leave the datapath unchanged.

AFAIK you're making 3 changes:
 - forwarding tls ops
 - pinning flows
 - handling features

Pinning of the TLS device to a leg of the bond looks like ~15LoC.
I think we can live with that.

It's the 150 LoC of forwarding TLS ops and duplicating dev selection
logic in bond_sk_hash_l34() that I'd rather avoid.

Handling features is probably fine, too, I haven't thought about that
much.

> I like the idea, it can generalize code structures for all kinds of 
> upper-devices and sockets, taking them into a common place in core, 
> which reduces code duplications.
> 
> If we go only half the way, i.e. keep xmit logic in bond for 
> non-TLS-offloaded traffic, then we have to let TLS module (and others in 
> the future) act deferentially for different kinds of devs (upper/lower) 
> which IMHO reduces generality.

How so? I was expecting TLS to just do something like:

	netdev = sk_get_xmit_dev_lowest(sk);

which would recursively call get_xmit_slave(CONST) until it reaches
a device which doesn't resolve further.

BTW is the flow pinning to bond legs actually a must-do? I don't know
much about bonding but wouldn't that mean that if the selected leg goes
down we'd lose connectivity, rather than falling back to SW crypto?

> I'm in favor of the deeper change. It will be on a larger scale, and 
> totally orthogonal to the current TLS offload support in bond.
> 
> If we decide to apply the idea only to TLS sockets (or any subset of 
> sockets) we're actually taking a generic one-flow (the xmit patch of a 
> bond dev) and turning it into two (or potentially more) flows, depending 
> on the socket type. This also reduces generality.

I don't follow this part.

> > I'm probably missing the problem you're trying to explain to me :S
> 
> I kept the patch minimal, and kept the TLS offload logic internal to the 
> bond driver, just like it is internal to the device drivers (mlx5e, and 
> others), with no core infrastructure modification.
> 
> > Side note - Jarod, I'd be happy to take a patch renaming
> > netdev_get_xmit_slave() and the ndo, if you have the cycles to send
> > a patch. It's a recent addition, and in the core we should make more
> > of an effort to avoid sensitive terms.
> >   
> >> Currently I have my check there to
> >> catch all skbs belonging to offloaded TLS sockets.
> >>
> >> The TLS offload get_slave() logic decision is per socket, so the result
> >> cannot be saved in the bond memory. Currently I save the real_dev field
> >> in the TLS context structure.  
> > 
> > Right, but we could just have ctx->netdev be the "egress" netdev
> > always, right? Do we expect somewhere that it's going to be matching
> > the socket's dst?
> 
> So once the offload context is established we totally bypass the bond 
> dev? and lose interaction or reference to it?

Yup, I don't think we need it.

> What if the egress dev is detached form the bond? We must then be 
> notified somehow.

Do we notify TLS when routing changes? I think it's a separate topic. 

If we have the code to "un-offload" a flow we could handle clearing
features better and notify from sk_validate_xmit_skb that the flow
started hitting unexpected dev, hence it should be re-offloaded.

I don't think we need an explicit invalidation from the particular
drivers here.

Re: [PATCH net-next 0/2] TLS TX HW offload for Bond

[Tariq Toukan]

On 11/23/2020 8:20 PM, Jakub Kicinski wrote:
> On Sun, 22 Nov 2020 14:48:04 +0200 Tariq Toukan wrote:
>> On 11/19/2020 6:38 PM, Jakub Kicinski wrote:
>>> On Thu, 19 Nov 2020 17:59:38 +0200 Tariq Toukan wrote:
>>>> On 11/19/2020 2:02 AM, Jakub Kicinski wrote:
>>>>> On Sun, 15 Nov 2020 15:42:49 +0200 Tariq Toukan wrote:
>>>>>> This series opens TLS TX HW offload for bond interfaces.
>>>>>> This allows bond interfaces to benefit from capable slave devices.
>>>>>>
>>>>>> The first patch adds real_dev field in TLS context structure, and aligns
>>>>>> usages in TLS module and supporting drivers.
>>>>>> The second patch opens the offload for bond interfaces.
>>>>>>
>>>>>> For the configuration above, SW kTLS keeps picking the same slave
>>>>>> To keep simple track of the HW and SW TLS contexts, we bind each socket to
>>>>>> a specific slave for the socket's whole lifetime. This is logically valid
>>>>>> (and similar to the SW kTLS behavior) in the following bond configuration,
>>>>>> so we restrict the offload support to it:
>>>>>>
>>>>>> ((mode == balance-xor) or (mode == 802.3ad))
>>>>>> and xmit_hash_policy == layer3+4.
>>>>>
>>>>> This does not feel extremely clean, maybe you can convince me otherwise.
>>>>>
>>>>> Can we extend netdev_get_xmit_slave() and figure out the output dev
>>>>> (and if it's "stable") in a more generic way? And just feed that dev
>>>>> into TLS handling?
>>>>
>>>> I don't see we go through netdev_get_xmit_slave(), but through
>>>> .ndo_start_xmit (bond_start_xmit).
>>>
>>> I may be misunderstanding the purpose of netdev_get_xmit_slave(),
>>> please correct me if I'm wrong. AFAIU it's supposed to return a
>>> lower netdev that the skb should then be xmited on.
>>
>> That's true. It was recently added and used by the RDMA team. Not used
>> or integrated in the Eth networking stack.
>>
>>> So what I was thinking was either construct an skb or somehow reshuffle
>>> the netdev_get_xmit_slave() code to take a flow dissector output or
>>> ${insert other ideas}. Then add a helper in the core that would drill
>>> down from the socket netdev to the "egress" netdev. Have TLS call
>>> that helper, and talk to the "egress" netdev from the start, rather
>>> than the socket's netdev. Then loosen the checks on software devices.
>>
>> As I understand it, best if we can even generalize this to apply to all
>> kinds of traffic: bond driver won't do the xmit itself anymore, it just
>> picks an egress dev and returns it. The core infrastructure will call
>> the xmit function for the egress dev.
> 
> I think you went way further than I was intending :) I was only
> considering the control path. Leave the datapath unchanged.
> 
> AFAIK you're making 3 changes:
>   - forwarding tls ops
>   - pinning flows
>   - handling features
> 
> Pinning of the TLS device to a leg of the bond looks like ~15LoC.
> I think we can live with that.
> 

Good.
You mean the changes under __bond_start_xmit ?

> It's the 150 LoC of forwarding TLS ops and duplicating dev selection
> logic in bond_sk_hash_l34() that I'd rather avoid.
> 

I see.
But there are several issues with this:

1. The .ndo_get_xmit_slave acts on an SKB, not a socket. Hence, it 
doesn't fit for the stage of calling tls_dev_add, unless the ndo goes 
through some refactoring before the feature itself.

2. Existing hash logic acts on an SKB. We must have one that acts on a 
socket to be used inside get_slave(sk). Hence, I don't really see how 
the logic under bond_sk_hash_l34() are going to disappear, maybe just 
move around to a new place.


> Handling features is probably fine, too, I haven't thought about that
> much.
> 

Good.

>> I like the idea, it can generalize code structures for all kinds of
>> upper-devices and sockets, taking them into a common place in core,
>> which reduces code duplications.
>>
>> If we go only half the way, i.e. keep xmit logic in bond for
>> non-TLS-offloaded traffic, then we have to let TLS module (and others in
>> the future) act deferentially for different kinds of devs (upper/lower)
>> which IMHO reduces generality.
> 
> How so? I was expecting TLS to just do something like:
> 
> 	netdev = sk_get_xmit_dev_lowest(sk);
> 
> which would recursively call get_xmit_slave(CONST) until it reaches
> a device which doesn't resolve further.
> 
> BTW is the flow pinning to bond legs actually a must-do? I don't know
> much about bonding but wouldn't that mean that if the selected leg goes
> down we'd lose connectivity, rather than falling back to SW crypto?
> 

Right. As long as we don't have logic for un-offloading.
Currently in TLS, the device-offloaded connections has some 
"independence" once they are created, it's hard to modify them and apply 
configuration modifications to them (example: interaction with tx csum 
offload).
So I think there is a missing un-offloading mechanism in TLS that should 
address all of these together.

This fits with your comments below.

>> I'm in favor of the deeper change. It will be on a larger scale, and
>> totally orthogonal to the current TLS offload support in bond.
>>
>> If we decide to apply the idea only to TLS sockets (or any subset of
>> sockets) we're actually taking a generic one-flow (the xmit patch of a
>> bond dev) and turning it into two (or potentially more) flows, depending
>> on the socket type. This also reduces generality.
> 
> I don't follow this part.
> 
>>> I'm probably missing the problem you're trying to explain to me :S
>>
>> I kept the patch minimal, and kept the TLS offload logic internal to the
>> bond driver, just like it is internal to the device drivers (mlx5e, and
>> others), with no core infrastructure modification.
>>
>>> Side note - Jarod, I'd be happy to take a patch renaming
>>> netdev_get_xmit_slave() and the ndo, if you have the cycles to send
>>> a patch. It's a recent addition, and in the core we should make more
>>> of an effort to avoid sensitive terms.
>>>    
>>>> Currently I have my check there to
>>>> catch all skbs belonging to offloaded TLS sockets.
>>>>
>>>> The TLS offload get_slave() logic decision is per socket, so the result
>>>> cannot be saved in the bond memory. Currently I save the real_dev field
>>>> in the TLS context structure.
>>>
>>> Right, but we could just have ctx->netdev be the "egress" netdev
>>> always, right? Do we expect somewhere that it's going to be matching
>>> the socket's dst?
>>
>> So once the offload context is established we totally bypass the bond
>> dev? and lose interaction or reference to it?
> 
> Yup, I don't think we need it.
> 
>> What if the egress dev is detached form the bond? We must then be
>> notified somehow.
> 
> Do we notify TLS when routing changes? I think it's a separate topic.
> 
> If we have the code to "un-offload" a flow we could handle clearing
> features better and notify from sk_validate_xmit_skb that the flow
> started hitting unexpected dev, hence it should be re-offloaded.
> 
> I don't think we need an explicit invalidation from the particular
> drivers here.
> 

Agree.

Re: [PATCH net-next 0/2] TLS TX HW offload for Bond

[Boris Pismenny]

On 23/11/2020 20:20, Jakub Kicinski wrote:
> On Sun, 22 Nov 2020 14:48:04 +0200 Tariq Toukan wrote:
>>
>> As I understand it, best if we can even generalize this to apply to all 
>> kinds of traffic: bond driver won't do the xmit itself anymore, it just 
>> picks an egress dev and returns it. The core infrastructure will call 
>> the xmit function for the egress dev.
> I think you went way further than I was intending :) I was only
> considering the control path. Leave the datapath unchanged.
>
> AFAIK you're making 3 changes:
>  - forwarding tls ops
>  - pinning flows
>  - handling features
>
> Pinning of the TLS device to a leg of the bond looks like ~15LoC.
> I think we can live with that.
>
> It's the 150 LoC of forwarding TLS ops and duplicating dev selection
> logic in bond_sk_hash_l34() that I'd rather avoid.
>
> Handling features is probably fine, too, I haven't thought about that
> much.

Sorry for jumping in late, but I'd like to present an argument in favor of the approach in the original patch-set, as it may have been overlooked.

The forwarding of TLS ops approach is very flexible, and it will enable support for per-SKB hashing in the future (high-availability): This will require taking ooo_okay into consideration and offloading the context to more than one NIC. But, I think its doable. Even though this approach requires more lines of code, it is already used by other offloads. For instance, XFRM offload in bond_main.c.


>> I like the idea, it can generalize code structures for all kinds of 
>> upper-devices and sockets, taking them into a common place in core, 
>> which reduces code duplications.
>>
>> If we go only half the way, i.e. keep xmit logic in bond for 
>> non-TLS-offloaded traffic, then we have to let TLS module (and others in 
>> the future) act deferentially for different kinds of devs (upper/lower) 
>> which IMHO reduces generality.
> How so? I was expecting TLS to just do something like:
>
> 	netdev = sk_get_xmit_dev_lowest(sk);
>
> which would recursively call get_xmit_slave(CONST) until it reaches
> a device which doesn't resolve further.
>
> BTW is the flow pinning to bond legs actually a must-do? I don't know
> much about bonding but wouldn't that mean that if the selected leg goes
> down we'd lose connectivity, rather than falling back to SW crypto?

It is definitely not a must, and I think we should remove it in the future, once the use-case presents itself.


>> What if the egress dev is detached form the bond? We must then be 
>> notified somehow.
> Do we notify TLS when routing changes? I think it's a separate topic. 
>
> If we have the code to "un-offload" a flow we could handle clearing
> features better and notify from sk_validate_xmit_skb that the flow
> started hitting unexpected dev, hence it should be re-offloaded.
>
> I don't think we need an explicit invalidation from the particular
> drivers here.

Even though re-offload is not exercised, it is possible:
if packets are not using offload by the old netdev, then remove offload from it, and add offload to the new netdev.
A resync, will likely follow, after which offload continue on the new netdev.

The question is who identifies/decides when to re-offload. One option is that the bond driver will trigger it.