[RFC net-next 3/7] net/mlx5e: kTLS, Post TX context params only for skbs that use them

[Tariq Toukan <tariqt@nvidia.com>]

A new kTLS TX context is installed by static and progress params that
the driver posts for the connection's first skb:
mlx5e_ktls_handle_tx_skb() posts them before it checks the skb's
sequence. That skb may not use the context though: a pass-through skb,
which ends before the offload start marker (SKIP_NO_DATA), e.g. TCP data
that was queued before the socket was offloaded.

A pass-through skb carries no data for the device to encrypt, and is not
tied to the TX queue of the connection's data. For example, when netem
on its TX queue delays it, skb_orphan_partial() releases its
sk_wmem_alloc charge, which lets the socket move to another TX queue
meanwhile. The params are then posted on one SQ and the connection's
data on another, with no ordering between them, and the data can be
encrypted before the TIS is installed, e.g. with the key of the previous
connection that used a TIS from the pool.

Post the params only for an skb that uses the context: an in-order skb
posts them as before, and a resynced one gets a full resync, which posts
the static params regardless of rec_seq, as it may match while the TIS
still holds the params of its previous user. A dropped skb leaves them
pending for the next skb.

A pass-through skb does not use the context at all. Only data that the
device encrypts is sent with skb->decrypted set, so skip kTLS handling
for an skb without it, before its sequence is looked at: a pass-through
skb held long enough can alias the sequence of a live record.

Pointed out by Rishikesh Jethwani for skbs that netem delays, while
discussing kTLS TX TIS reuse for TLS 1.3 rekey on netdev. He also
suggested the skb_is_decrypted() check.

Reproduced on ConnectX-6 Dx with netem, and a 10 Mb/s rate limit, on
half of the TX queues, and connections that send 1 MB of plaintext
before installing kTLS TX, then stream pattern data to a software kTLS
receiver that checks every byte, hopping between CPUs every 1 ms: 2 of
323 connections failed at the first record after the plaintext, and the
device failed another such record, which ran before its params, with a
local protection error. With this patch, none of 400 connections failed.
netem on all TX queues does not reproduce it: it fully orphans the
offloaded data, which validate_xmit_skb() then drops.

Fixes: d2ead1f360e8 ("net/mlx5e: Add kTLS TX HW offload support")
Reported-by: Rishikesh Jethwani <rjethwani@everpuredata.com>
Closes: https://lore.kernel.org/netdev/CAKaoeS1hj9XuM7ojLBCjGA=EXc2M-b+eRjC2Sk=8L_8XYeJ18w@mail.gmail.com/
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../mellanox/mlx5/core/en_accel/ktls_tx.c     | 40 +++++++++----------
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
index a4aa58ef1dda..0bd4d961e704 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
@@ -542,16 +542,6 @@ static void tx_fill_wi(struct mlx5e_txqsq *sq,
 	};
 }
 
-static bool
-mlx5e_ktls_tx_offload_test_and_clear_pending(struct mlx5e_ktls_offload_context_tx *priv_tx)
-{
-	bool ret = priv_tx->ctx_post_pending;
-
-	priv_tx->ctx_post_pending = false;
-
-	return ret;
-}
-
 static void
 post_static_params(struct mlx5e_txqsq *sq,
 		   struct mlx5e_ktls_offload_context_tx *priv_tx,
@@ -872,6 +862,12 @@ bool mlx5e_ktls_handle_tx_skb(struct net_device *netdev, struct mlx5e_txqsq *sq,
 	if (!datalen)
 		return true;
 
+	/* Only data that the device encrypts uses the TLS context. */
+	if (!skb_is_decrypted(skb)) {
+		stats->tls_skip_no_sync_data++;
+		return true;
+	}
+
 	mlx5e_tx_mpwqe_ensure_complete(sq);
 
 	tls_ctx = tls_get_ctx(skb->sk);
@@ -885,9 +881,9 @@ bool mlx5e_ktls_handle_tx_skb(struct net_device *netdev, struct mlx5e_txqsq *sq,
 
 	priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);
 
-	if (unlikely(mlx5e_ktls_tx_offload_test_and_clear_pending(priv_tx)))
-		mlx5e_ktls_tx_post_param_wqes(sq, priv_tx, false, false);
-
+	/* WQEs naming the TIS, pending context params included, are
+	 * posted only for an skb that uses it.
+	 */
 	seq = ntohl(tcp_hdr(skb)->seq);
 	ooo = priv_tx->expected_seq != seq;
 	/* Take the loss mark before resyncing, a loss reported meanwhile
@@ -896,31 +892,32 @@ bool mlx5e_ktls_handle_tx_skb(struct net_device *netdev, struct mlx5e_txqsq *sq,
 	resync = test_bit(MLX5E_KTLS_TX_RESYNC, &priv_tx->flags) &&
 		 test_and_clear_bit(MLX5E_KTLS_TX_RESYNC, &priv_tx->flags);
 	if (unlikely(ooo || resync)) {
+		/* A resync installs a pending context fully: rec_seq may match
+		 * while the TIS still holds the params of its previous user.
+		 */
+		bool full = resync || priv_tx->ctx_post_pending;
 		enum mlx5e_ktls_sync_retval ret =
 			mlx5e_ktls_tx_handle_ooo(priv_tx, sq, datalen, seq,
-						 resync);
+						 full);
 
 		if (ooo)
 			stats->tls_ooo++;
 
 		switch (ret) {
 		case MLX5E_KTLS_SYNC_DONE:
+			priv_tx->ctx_post_pending = false;
 			break;
 		case MLX5E_KTLS_SYNC_SKIP_NO_DATA:
 			stats->tls_skip_no_sync_data++;
-			if (likely(!skb->decrypted)) {
-				/* Nothing was posted, keep the mark. */
-				if (resync)
-					set_bit(MLX5E_KTLS_TX_RESYNC,
-						&priv_tx->flags);
-				goto out;
-			}
 			WARN_ON_ONCE(1);
 			goto err_lost;
 		case MLX5E_KTLS_SYNC_FAIL:
 			stats->tls_drop_no_sync_data++;
 			goto err_lost;
 		}
+	} else if (unlikely(priv_tx->ctx_post_pending)) {
+		priv_tx->ctx_post_pending = false;
+		mlx5e_ktls_tx_post_param_wqes(sq, priv_tx, false, false);
 	}
 
 	priv_tx->expected_seq = seq + datalen;
@@ -930,7 +927,6 @@ bool mlx5e_ktls_handle_tx_skb(struct net_device *netdev, struct mlx5e_txqsq *sq,
 	stats->tls_encrypted_packets += skb_is_gso(skb) ? skb_shinfo(skb)->gso_segs : 1;
 	stats->tls_encrypted_bytes   += datalen;
 
-out:
 	return true;
 
 err_lost:
-- 
2.44.0