[RFC net-next 2/7] net/mlx5e: kTLS, Resync TX fully after its WQEs were lost

[Tariq Toukan <tariqt@nvidia.com>]

The driver tracks the TIS state of a kTLS TX connection in software:
expected_seq is the TCP sequence that the TIS expects next, and
crypto_info.rec_seq is the record sequence of the last posted static
params. It assumes that the WQEs it posts for an skb execute, which does
not always hold:
- The skb is dropped after its kTLS WQEs were posted, or after
  expected_seq was advanced past its data: a failed resync, or a DMA
  mapping error of its SEND.
- An SQ error flushes the pending WQEs.
- Closing an SQ (channels reopen, TX timeout recovery, HTB leaf delete)
  destroys its pending WQEs.

The TIS state is then off from the software state, and the next skb is
not resynced:
- An in-order skb is encrypted with a stale record state, corrupting the
  records on the wire.
- If static params were lost, the TIS keeps the previous record
  sequence, and as rec_seq already matches, later resyncs skip posting
  them. When the first static params of a TIS taken from the pool are
  lost, the TIS still names the DEK and record sequence of the previous
  connection that used it, and the new connection's data is encrypted
  with them.

Mark the connection where the driver sees the loss: when it drops the
skb, when the skb completes with an error CQE, and when the skb is freed
with its SQ. The next skb of a marked connection resyncs it fully: post
the static params even if rec_seq matches, then the progress params, and
dump the record prefix. It takes the mark with a test-and-clear before
posting the resync, so a loss reported meanwhile marks the connection
again.

The skb holds the socket, so the connection's context is alive when it
is marked. A context that tls_device_down() releases is not marked: it
clears the netdev and waits for RCU readers first.

Pointed out by Rishikesh Jethwani for lost initial static params, while
discussing kTLS TX TIS reuse for TLS 1.3 rekey on netdev.

Reproduced on ConnectX-6 Dx with a test-only patch that fails the SEND
DMA mapping of a kTLS offloaded socket's skbs at a fixed interval, after
their kTLS WQEs were posted, while 32 connections stream pattern data
for 60 seconds to a software kTLS receiver that checks every byte, over
a single channel, so that no connection can move TX queue. On top of the
previous patch only, every drop broke its connection: 2186 of 2217
connections failed decryption. With this patch, 1838 drops broke none.

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     | 67 ++++++++++++++++---
 .../mellanox/mlx5/core/en_accel/ktls_txrx.h   | 15 +++++
 .../net/ethernet/mellanox/mlx5/core/en_tx.c   | 13 +++-
 3 files changed, 83 insertions(+), 12 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 31f61f2ba824..a4aa58ef1dda 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
@@ -87,11 +87,17 @@ static int mlx5e_ktls_destroy_tis_cb(struct mlx5_core_dev *mdev, u32 tisn,
 				out, outlen, callback, context);
 }
 
+/* Bits of mlx5e_ktls_offload_context_tx flags */
+enum {
+	MLX5E_KTLS_TX_RESYNC, /* WQEs lost, see mlx5e_ktls_tx_handle_lost() */
+};
+
 struct mlx5e_ktls_offload_context_tx {
 	/* fast path */
 	u32 expected_seq;
 	u32 tisn;
 	bool ctx_post_pending;
+	unsigned long flags; /* MLX5E_KTLS_TX_* */
 	/* control / resync */
 	struct list_head list_node; /* member of the pool */
 	union mlx5e_crypto_info crypto_info;
@@ -498,6 +504,7 @@ int mlx5e_ktls_add_tx(struct net_device *netdev, struct sock *sk,
 	mlx5e_set_ktls_tx_priv_ctx(tls_ctx, priv_tx);
 
 	priv_tx->ctx_post_pending = true;
+	priv_tx->flags = 0;
 	atomic64_inc(&priv_tx->sw_stats->tx_tls_ctx);
 
 	return 0;
@@ -676,7 +683,7 @@ tx_sync_info_get(struct mlx5e_ktls_offload_context_tx *priv_tx,
 static void
 tx_post_resync_params(struct mlx5e_txqsq *sq,
 		      struct mlx5e_ktls_offload_context_tx *priv_tx,
-		      u64 rcd_sn)
+		      u64 rcd_sn, bool full)
 {
 	__be64 rn_be = cpu_to_be64(rcd_sn);
 	bool skip_static_post;
@@ -704,7 +711,8 @@ tx_post_resync_params(struct mlx5e_txqsq *sq,
 		return;
 	}
 
-	skip_static_post = !memcmp(rec_seq, &rn_be, rec_seq_sz);
+	/* rec_seq is the record sequence of the last posted static params. */
+	skip_static_post = !full && !memcmp(rec_seq, &rn_be, rec_seq_sz);
 	if (!skip_static_post)
 		memcpy(rec_seq, &rn_be, rec_seq_sz);
 
@@ -772,7 +780,7 @@ static enum mlx5e_ktls_sync_retval
 mlx5e_ktls_tx_handle_ooo(struct mlx5e_ktls_offload_context_tx *priv_tx,
 			 struct mlx5e_txqsq *sq,
 			 int datalen,
-			 u32 seq)
+			 u32 seq, bool full)
 {
 	enum mlx5e_ktls_sync_retval ret;
 	struct tx_sync_info info = {};
@@ -786,7 +794,7 @@ mlx5e_ktls_tx_handle_ooo(struct mlx5e_ktls_offload_context_tx *priv_tx,
 		 */
 		return ret;
 
-	tx_post_resync_params(sq, priv_tx, info.rcd_sn);
+	tx_post_resync_params(sq, priv_tx, info.rcd_sn, full);
 
 	for (i = 0; i < info.nr_frags; i++) {
 		unsigned int orig_fsz, frag_offset = 0, n = 0;
@@ -826,6 +834,28 @@ mlx5e_ktls_tx_handle_ooo(struct mlx5e_ktls_offload_context_tx *priv_tx,
 	return MLX5E_KTLS_SYNC_FAIL;
 }
 
+/* The WQEs posted for an skb were not executed: the skb was dropped, or its
+ * WQEs were flushed by an SQ error or destroyed with their SQ. The TIS state
+ * of its connection may be off from ours, resync it fully on the next skb.
+ */
+void mlx5e_ktls_tx_handle_lost(struct mlx5e_txqsq *sq, struct sk_buff *skb)
+{
+	struct mlx5e_ktls_offload_context_tx *priv_tx;
+	struct tls_context *tls_ctx;
+
+	/* The skb holds the socket, and so its TLS context. tls_device_down()
+	 * clears the netdev, and waits for RCU readers before it releases the
+	 * driver context.
+	 */
+	tls_ctx = tls_get_ctx(skb->sk);
+	rcu_read_lock();
+	if (rcu_dereference(tls_ctx->netdev) == sq->netdev) {
+		priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);
+		set_bit(MLX5E_KTLS_TX_RESYNC, &priv_tx->flags);
+	}
+	rcu_read_unlock();
+}
+
 bool mlx5e_ktls_handle_tx_skb(struct net_device *netdev, struct mlx5e_txqsq *sq,
 			      struct sk_buff *skb,
 			      struct mlx5e_accel_tx_tls_state *state)
@@ -834,6 +864,7 @@ bool mlx5e_ktls_handle_tx_skb(struct net_device *netdev, struct mlx5e_txqsq *sq,
 	struct mlx5e_sq_stats *stats = sq->stats;
 	struct net_device *tls_netdev;
 	struct tls_context *tls_ctx;
+	bool ooo, resync;
 	int datalen;
 	u32 seq;
 
@@ -858,24 +889,37 @@ bool mlx5e_ktls_handle_tx_skb(struct net_device *netdev, struct mlx5e_txqsq *sq,
 		mlx5e_ktls_tx_post_param_wqes(sq, priv_tx, false, false);
 
 	seq = ntohl(tcp_hdr(skb)->seq);
-	if (unlikely(priv_tx->expected_seq != seq)) {
+	ooo = priv_tx->expected_seq != seq;
+	/* Take the loss mark before resyncing, a loss reported meanwhile
+	 * marks the connection again.
+	 */
+	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)) {
 		enum mlx5e_ktls_sync_retval ret =
-			mlx5e_ktls_tx_handle_ooo(priv_tx, sq, datalen, seq);
+			mlx5e_ktls_tx_handle_ooo(priv_tx, sq, datalen, seq,
+						 resync);
 
-		stats->tls_ooo++;
+		if (ooo)
+			stats->tls_ooo++;
 
 		switch (ret) {
 		case MLX5E_KTLS_SYNC_DONE:
 			break;
 		case MLX5E_KTLS_SYNC_SKIP_NO_DATA:
 			stats->tls_skip_no_sync_data++;
-			if (likely(!skb->decrypted))
+			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_out;
+			goto err_lost;
 		case MLX5E_KTLS_SYNC_FAIL:
 			stats->tls_drop_no_sync_data++;
-			goto err_out;
+			goto err_lost;
 		}
 	}
 
@@ -889,6 +933,9 @@ bool mlx5e_ktls_handle_tx_skb(struct net_device *netdev, struct mlx5e_txqsq *sq,
 out:
 	return true;
 
+err_lost:
+	/* Resync params may have moved the TIS state to the dropped skb. */
+	set_bit(MLX5E_KTLS_TX_RESYNC, &priv_tx->flags);
 err_out:
 	/* Parameter and DUMP WQEs may have been posted for the skb. */
 	mlx5e_tx_flush(sq, skb);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_txrx.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_txrx.h
index 93bd383a23d8..a6b15dcd32b8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_txrx.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_txrx.h
@@ -29,6 +29,15 @@ void mlx5e_ktls_handle_get_psv_completion(struct mlx5e_icosq_wqe_info *wi,
 void mlx5e_ktls_tx_handle_resync_dump_comp(struct mlx5e_txqsq *sq,
 					   struct mlx5e_tx_wqe_info *wi,
 					   u32 *dma_fifo_cc);
+void mlx5e_ktls_tx_handle_lost(struct mlx5e_txqsq *sq, struct sk_buff *skb);
+
+/* The WQEs posted for the skb may have not been executed. */
+static inline void
+mlx5e_ktls_tx_try_handle_lost(struct mlx5e_txqsq *sq, struct sk_buff *skb)
+{
+	if (tls_is_skb_tx_device_offloaded(skb))
+		mlx5e_ktls_tx_handle_lost(sq, skb);
+}
 
 void
 mlx5e_ktls_rx_resync_async_request_cancel(struct mlx5e_icosq_wqe_info *wi);
@@ -69,6 +78,12 @@ mlx5e_ktls_tx_try_handle_resync_dump_comp(struct mlx5e_txqsq *sq,
 	return false;
 }
 
+static inline void
+mlx5e_ktls_tx_handle_lost(struct mlx5e_txqsq *sq, struct sk_buff *skb) {}
+
+static inline void
+mlx5e_ktls_tx_try_handle_lost(struct mlx5e_txqsq *sq, struct sk_buff *skb) {}
+
 static inline bool
 mlx5e_ktls_rx_handle_resync_list(struct mlx5e_icosq *sq, int budget)
 {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
index eb8a7a89b120..bee6f0bf1eff 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
@@ -386,6 +386,7 @@ static void mlx5e_tx_drop(struct mlx5e_txqsq *sq, struct sk_buff *skb)
 	if (IS_ENABLED(CONFIG_MLX5_EN_TLS) &&
 	    tls_is_skb_tx_device_offloaded(skb)) {
 		/* kTLS WQEs may have been posted for the skb. */
+		mlx5e_ktls_tx_handle_lost(sq, skb);
 		mlx5e_tx_flush(sq, skb);
 		return;
 	}
@@ -815,13 +816,15 @@ bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
 	i = 0;
 	do {
 		struct mlx5e_tx_wqe_info *wi;
+		bool last_wqe, err_cqe;
 		u16 wqe_counter;
-		bool last_wqe;
 		u16 ci;
 
 		mlx5_cqwq_pop(&cq->wq);
 
 		wqe_counter = be16_to_cpu(cqe->wqe_counter);
+		/* WQEs covered by an error CQE may not have been executed. */
+		err_cqe = get_cqe_opcode(cqe) == MLX5_CQE_REQ_ERR;
 
 		do {
 			last_wqe = (sqcc == wqe_counter);
@@ -832,6 +835,10 @@ bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
 			sqcc += wi->num_wqebbs;
 
 			if (likely(wi->skb)) {
+				if (unlikely(err_cqe))
+					mlx5e_ktls_tx_try_handle_lost(sq,
+								      wi->skb);
+
 				if (unlikely(!wi->num_bytes)) {
 					/* Dropped, held by mlx5e_tx_flush() */
 					dev_kfree_skb_any(wi->skb);
@@ -859,7 +866,7 @@ bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
 			}
 		} while (!last_wqe);
 
-		if (unlikely(get_cqe_opcode(cqe) == MLX5_CQE_REQ_ERR)) {
+		if (unlikely(err_cqe)) {
 			if (!test_and_set_bit(MLX5E_SQ_STATE_RECOVERING,
 					      &sq->state)) {
 				mlx5e_dump_error_cqe(&sq->cq, sq->sqn,
@@ -913,6 +920,8 @@ void mlx5e_free_txqsq_descs(struct mlx5e_txqsq *sq)
 		sqcc += wi->num_wqebbs;
 
 		if (likely(wi->skb)) {
+			/* Destroyed with the SQ, not executed. */
+			mlx5e_ktls_tx_try_handle_lost(sq, wi->skb);
 			mlx5e_tx_wi_dma_unmap(sq, wi, &dma_fifo_cc);
 			dev_kfree_skb_any(wi->skb);
 
-- 
2.44.0