[RFC net-next 0/7] [Cover Letter] net/mlx5e: kTLS TX fixes for WQEs in flight

[RFC net-next 0/7] [Cover Letter] net/mlx5e: kTLS TX fixes for WQEs in flight

[Tariq Toukan]

Hi,

This series fixes kTLS TX offload cases where WQEs that a connection
posted are still in flight while its device state is released, changed
from another SQ, or assumed to be in sync. Records then go out wrongly
encrypted, or the TIS and DEK are released while still in use.

kTLS TX WQEs update the TIS crypto and record tracking state of a
connection as they execute, and the device orders WQEs only within an
SQ. The series makes sure that:

- An skb dropped after WQEs were posted for it is held until they
  complete, and a connection whose WQEs were lost is resynced fully
  (patches 1-2).
- The context params are posted only for an skb that uses them, on the
  SQ of the connection's data (patch 3).
- The old and new SQs never execute together across a channels switch
  (patch 4).
- The TX queue of an offloaded socket does not change while it has
  packets in flight, in the stack and in the mlx5e queue selection
  (patches 5-6). An skb that still reaches another SQ meanwhile, e.g.
  after tc skbedit queue_mapping, is dropped before anything is posted
  (patch 7).

Patches 1-6 are fixes, for net. Patch 7 is for net-next, on top of
them. TLS 1.3 rekey support for device offload builds on this series.

Tested on ConnectX-6 Dx, with a software kTLS receiver that checks
every byte. The commit messages describe the reproduction and the
results of each fix.

Link: https://lore.kernel.org/netdev/20261022181751.1349984-1-rjethwani@purestorage.com/

Regards,
Tariq

Tariq Toukan (7):
  net/mlx5e: kTLS, Hold a dropped skb until its posted WQEs complete
  net/mlx5e: kTLS, Resync TX fully after its WQEs were lost
  net/mlx5e: kTLS, Post TX context params only for skbs that use them
  net/mlx5e: Drain old TX SQs before activating new channels
  net: tls: keep the TX queue of a device-offloaded socket stable
  net/mlx5e: kTLS, Keep the TX queue of an offloaded socket stable
  net/mlx5e: kTLS, Keep a TX connection on one SQ at a time

 Documentation/admin-guide/sysctl/net.rst      |   3 +
 .../ethernet/mellanox/mlx5/counters.rst       |   7 +
 Documentation/networking/tls-offload.rst      |  20 +++
 .../net/ethernet/mellanox/mlx5/core/en/selq.c |  96 ++++++++--
 .../net/ethernet/mellanox/mlx5/core/en/txrx.h |   2 +
 .../mellanox/mlx5/core/en_accel/ktls.c        |   1 +
 .../mellanox/mlx5/core/en_accel/ktls_tx.c     | 165 +++++++++++++++---
 .../mellanox/mlx5/core/en_accel/ktls_txrx.h   |  21 +++
 .../net/ethernet/mellanox/mlx5/core/en_main.c |  88 +++++++++-
 .../ethernet/mellanox/mlx5/core/en_stats.c    |   4 +
 .../ethernet/mellanox/mlx5/core/en_stats.h    |   2 +
 .../net/ethernet/mellanox/mlx5/core/en_tx.c   |  98 +++++++++--
 include/net/sock.h                            |  33 +++-
 include/net/tls.h                             |  13 ++
 net/core/dev.c                                |   3 +-
 net/core/sock.c                               |   2 +-
 net/ipv4/tcp_output.c                         |   7 +-
 net/tls/tls_device.c                          |   3 +
 18 files changed, 503 insertions(+), 65 deletions(-)


base-commit: fa5196392691051963726787944db872420dfa75
-- 
2.44.0

[RFC net-next 1/7] net/mlx5e: kTLS, Hold a dropped skb until its posted WQEs complete

[Tariq Toukan]

mlx5e_ktls_del_tx() releases a TX connection's TIS and DEK with no
completion tracking: the DEK is destroyed, and the TIS is returned to
the recycling pool, right away. This relies on the socket lifetime:
tls_dev_del() is called only once the socket is destructed, i.e. after
sk_wmem_alloc drained, so every skb of the connection was completed,
and with it every WQE posted ahead of it on the same SQ; or from
tls_device_down(), after the channels were closed.

This does not cover the error path: an skb can be dropped after
parameter or DUMP WQEs were posted on its behalf, in the kTLS TX
handling, or when DMA mapping for its SEND fails. These WQEs request
no completion of their own and hold no socket memory, so freeing the
skb can let the socket be destructed while they are pending. The TIS
can then be handed to another connection, and the DEK destroyed,
before they execute.

Defer freeing such an skb to the completion of the NOP that
mlx5e_tx_flush() posts: completions are in order, so all WQEs posted
before the NOP are retired by then. The SEND error paths already post
this NOP; post it in the kTLS error path too. In the SEND error paths,
only skbs of kTLS offloaded sockets are held.

Found by LLM-assisted code inspection, while answering a question on
kTLS TX TIS reuse for TLS 1.3 rekey on netdev.

Reproduced on ConnectX-6 Dx with test-only code that fails the SEND DMA
mapping of every 2nd skb of short kTLS connections, which reset 0-1 s
after sending 32 KB, while bulk kTLS traffic at 10 Mb/s per TX queue
keeps the SQs backlogged, and that counts TX contexts released while
WQEs posted for them are still pending on their SQ: 32 in 60 seconds,
their TIS returned to the pool and their DEK destroyed early, against
none in the same run without the drops. With this patch alone, 11188
drops released none early.

Fixes: d2ead1f360e8 ("net/mlx5e: Add kTLS TX HW offload support")
Link: https://lore.kernel.org/netdev/20261022181751.1349984-1-rjethwani@purestorage.com/
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/en/txrx.h |  1 +
 .../mellanox/mlx5/core/en_accel/ktls_tx.c     |  3 +-
 .../net/ethernet/mellanox/mlx5/core/en_tx.c   | 44 +++++++++++++++----
 3 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h
index f2a8453d8dce..24e91279243e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h
@@ -377,6 +377,7 @@ mlx5e_tx_dma_unmap(struct device *pdev, struct mlx5e_sq_dma *dma)
 }
 
 void mlx5e_tx_mpwqe_ensure_complete(struct mlx5e_txqsq *sq);
+void mlx5e_tx_flush(struct mlx5e_txqsq *sq, struct sk_buff *skb);
 
 static inline bool mlx5e_tx_mpwqe_is_full(struct mlx5e_tx_mpwqe *session)
 {
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 29769fc99c4b..31f61f2ba824 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
@@ -890,7 +890,8 @@ bool mlx5e_ktls_handle_tx_skb(struct net_device *netdev, struct mlx5e_txqsq *sq,
 	return true;
 
 err_out:
-	dev_kfree_skb_any(skb);
+	/* Parameter and DUMP WQEs may have been posted for the skb. */
+	mlx5e_tx_flush(sq, skb);
 	return false;
 }
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
index 14479da98f42..eb8a7a89b120 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
@@ -348,7 +348,11 @@ static void mlx5e_tx_check_stop(struct mlx5e_txqsq *sq)
 	}
 }
 
-static void mlx5e_tx_flush(struct mlx5e_txqsq *sq)
+/* Post a NOP that requests a completion, and ring the doorbell. A given
+ * skb is held by the NOP until all WQEs posted ahead of it are retired:
+ * WQEs posted for it, e.g. kTLS ones, may need its socket to stay alive.
+ */
+void mlx5e_tx_flush(struct mlx5e_txqsq *sq, struct sk_buff *skb)
 {
 	struct mlx5e_tx_wqe_info *wi;
 	struct mlx5e_tx_wqe *wqe;
@@ -361,13 +365,35 @@ static void mlx5e_tx_flush(struct mlx5e_txqsq *sq)
 	wi = &sq->db.wqe_info[pi];
 
 	*wi = (struct mlx5e_tx_wqe_info) {
+		.skb = skb,
 		.num_wqebbs = 1,
 	};
 
 	wqe = mlx5e_post_nop(&sq->wq, sq->sqn, &sq->pc);
+
+	/* The NOP takes the room of the SEND that was not posted. As after a
+	 * SEND, stop the queue if room runs low; the completion wakes it.
+	 */
+	mlx5e_tx_check_stop(sq);
 	mlx5e_notify_hw(&sq->wq, sq->pc, sq->uar_map, &wqe->ctrl);
 }
 
+/* Drop an skb whose SEND could not be posted. */
+static void mlx5e_tx_drop(struct mlx5e_txqsq *sq, struct sk_buff *skb)
+{
+	sq->stats->dropped++;
+
+	if (IS_ENABLED(CONFIG_MLX5_EN_TLS) &&
+	    tls_is_skb_tx_device_offloaded(skb)) {
+		/* kTLS WQEs may have been posted for the skb. */
+		mlx5e_tx_flush(sq, skb);
+		return;
+	}
+
+	dev_kfree_skb_any(skb);
+	mlx5e_tx_flush(sq, NULL);
+}
+
 static inline void
 mlx5e_txwqe_complete(struct mlx5e_txqsq *sq, struct sk_buff *skb,
 		     const struct mlx5e_tx_attr *attr,
@@ -470,9 +496,7 @@ mlx5e_sq_xmit_wqe(struct mlx5e_txqsq *sq, struct sk_buff *skb,
 	return;
 
 err_drop:
-	stats->dropped++;
-	dev_kfree_skb_any(skb);
-	mlx5e_tx_flush(sq);
+	mlx5e_tx_drop(sq, skb);
 }
 
 static bool mlx5e_tx_skb_supports_mpwqe(struct sk_buff *skb, struct mlx5e_tx_attr *attr)
@@ -612,9 +636,7 @@ mlx5e_sq_xmit_mpwqe(struct mlx5e_txqsq *sq, struct sk_buff *skb,
 	return;
 
 err_unmap:
-	sq->stats->dropped++;
-	dev_kfree_skb_any(skb);
-	mlx5e_tx_flush(sq);
+	mlx5e_tx_drop(sq, skb);
 }
 
 void mlx5e_tx_mpwqe_ensure_complete(struct mlx5e_txqsq *sq)
@@ -810,6 +832,12 @@ bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
 			sqcc += wi->num_wqebbs;
 
 			if (likely(wi->skb)) {
+				if (unlikely(!wi->num_bytes)) {
+					/* Dropped, held by mlx5e_tx_flush() */
+					dev_kfree_skb_any(wi->skb);
+					continue;
+				}
+
 				mlx5e_tx_wi_dma_unmap(sq, wi, &dma_fifo_cc);
 				mlx5e_consume_skb(sq, wi->skb, cqe, napi_budget);
 
@@ -1007,6 +1035,6 @@ void mlx5i_sq_xmit(struct mlx5e_txqsq *sq, struct sk_buff *skb,
 err_drop:
 	stats->dropped++;
 	dev_kfree_skb_any(skb);
-	mlx5e_tx_flush(sq);
+	mlx5e_tx_flush(sq, NULL);
 }
 #endif
-- 
2.44.0

Re: [RFC net-next 1/7] net/mlx5e: kTLS, Hold a dropped skb until its posted WQEs complete

[sashiko-bot]

> mlx5e_ktls_del_tx() releases a TX connection's TIS and DEK with no
> completion tracking: the DEK is destroyed, and the TIS is returned to
> the recycling pool, right away. This relies on the socket lifetime:
> tls_dev_del() is called only once the socket is destructed, i.e. after
> sk_wmem_alloc drained, so every skb of the connection was completed,
> and with it every WQE posted ahead of it on the same SQ; or from
> tls_device_down(), after the channels were closed.
> 
> This does not cover the error path: an skb can be dropped after
> parameter or DUMP WQEs were posted on its behalf, in the kTLS TX
> handling, or when DMA mapping for its SEND fails. These WQEs request
> no completion of their own and hold no socket memory, so freeing the
> skb can let the socket be destructed while they are pending. The TIS
> can then be handed to another connection, and the DEK destroyed,
> before they execute.
> [ ... ]
> drops released none early.
> 
> Fixes: d2ead1f360e8 ("net/mlx5e: Add kTLS TX HW offload support")
> Link: https://lore.kernel.org/netdev/20261022181751.1349984-1-rjethwani@purestorage.com/
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109114734.2087878-1-tariqt@nvidia.com?part=1

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

[Tariq Toukan]

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

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

[sashiko-bot]

> 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
> [ ... ]
> 
> 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>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109114734.2087878-1-tariqt@nvidia.com?part=2

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

[Tariq Toukan]

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

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

[sashiko-bot]

> 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.
> [ ... ]
> 
> 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>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109114734.2087878-1-tariqt@nvidia.com?part=3

[RFC net-next 4/7] net/mlx5e: Drain old TX SQs before activating new channels

[Tariq Toukan]

On devices with tis_tir_td_order, commit 3b88a535a8e1 ("net/mlx5e:
Defer channels closure to reduce interface down time") closes the old
channels only after the new ones are activated. Their SQs are
deactivated before that, which rings a final doorbell, but nothing
waits for the HW to execute their pending WQEs: the completions of a
deactivated SQ are not polled, and the SQ is destroyed only when its
channel is closed. Meanwhile, the new SQ of the same TX queue starts
transmitting. A flow's WQEs still pending on the old SQ can then
execute after its newer WQEs on the new SQ, as WQE fences order only
within an SQ.

Besides reordering packets, this breaks kTLS TX offload. The WQEs of a
connection update its TIS crypto and record tracking state as they
execute. When old WQEs execute after new ones, e.g. because a PFC
pause held the old SQ back, the TIS state gets out of sync with the
TCP stream, and the connection's records go out corrupted.

Before activating the new channels, drain the TX SQs of the old ones:
reap their completions until all their WQEs have completed. If they
do not drain in time, e.g. under a long pause, warn, and close the old
channels before activating the new ones, as before that commit. Move
the deferred closure into mlx5e_switch_priv_channels(), next to that
decision.

Pointed out by Rishikesh Jethwani while discussing kTLS TX TIS reuse
for TLS 1.3 rekey on netdev.

Reproduced on ConnectX-6 Dx, with 248 channels, by changing the
channel count between 247 and 248 every 2 seconds while 32 kTLS TX
offloaded connections stream pattern data to a software kTLS receiver
that checks every byte. On this setup, the new channels are activated
about 230 ms after the old ones are deactivated, so the old SQs must
still hold WQEs by then: with every TX queue rate limited to 10 Mb/s,
9 of 41 connections failed decryption in 60 seconds, and 11 of 43 in
120 seconds. With this patch, no record failed in five such runs, 7
minutes in total; the drain timed out and fell back to closing the old
channels first 7 times. At 1 Gb/s per queue, the old SQs always
drained in time.

Fixes: 3b88a535a8e1 ("net/mlx5e: Defer channels closure to reduce interface down time")
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>
---
 .../net/ethernet/mellanox/mlx5/core/en/txrx.h |  1 +
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 88 ++++++++++++++++++-
 .../net/ethernet/mellanox/mlx5/core/en_tx.c   | 41 +++++++--
 3 files changed, 118 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h
index 24e91279243e..b5bd1b6e8601 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h
@@ -112,6 +112,7 @@ struct mlx5e_xmit_data_frags {
 
 netdev_tx_t mlx5e_xmit(struct sk_buff *skb, struct net_device *dev);
 bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget);
+bool mlx5e_txqsq_drain(struct mlx5e_txqsq *sq);
 void mlx5e_free_txqsq_descs(struct mlx5e_txqsq *sq);
 
 static inline bool
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index c3c2c5720c6f..a721e30bcaa8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -3421,6 +3421,82 @@ static int mlx5e_switch_priv_params(struct mlx5e_priv *priv,
 	return 0;
 }
 
+/* Time for the TX SQs of deactivated channels to complete their WQEs. */
+#define MLX5E_SQ_DRAIN_TIMEOUT 100 /* msecs */
+
+/* Returns 1 if the SQ still has pending WQEs, and records the first such SQ. */
+static int mlx5e_drain_txqsq(struct mlx5e_txqsq *sq,
+			     struct mlx5e_txqsq **pending)
+{
+	if (mlx5e_txqsq_drain(sq))
+		return 0;
+
+	if (!*pending)
+		*pending = sq;
+
+	return 1;
+}
+
+/* Returns the number of the channel's TX SQs that still have pending WQEs. */
+static int mlx5e_drain_channel_txqsqs(struct mlx5e_channel *c,
+				      struct mlx5e_txqsq **pending)
+{
+	struct mlx5e_txqsq __rcu **qos_sqs;
+	int num_pending = 0;
+	int i;
+
+	for (i = 0; i < c->num_tc; i++)
+		num_pending += mlx5e_drain_txqsq(&c->sq[i], pending);
+
+	qos_sqs = mlx5e_state_dereference(c->priv, c->qos_sqs);
+	if (!qos_sqs)
+		return num_pending;
+
+	for (i = 0; i < c->qos_sqs_size; i++) {
+		struct mlx5e_txqsq *sq;
+
+		sq = mlx5e_state_dereference(c->priv, qos_sqs[i]);
+		if (sq)
+			num_pending += mlx5e_drain_txqsq(sq, pending);
+	}
+
+	return num_pending;
+}
+
+/* Wait for the TX SQs of deactivated channels to complete all their WQEs,
+ * so that none of them executes concurrently with the new SQs.
+ * The PTP SQs are not waited for, they carry PTP event packets only.
+ */
+static bool mlx5e_drain_channels_txqsqs(struct mlx5e_channels *chs)
+{
+	unsigned long exp_time;
+
+	exp_time = jiffies + msecs_to_jiffies(MLX5E_SQ_DRAIN_TIMEOUT);
+	for (;;) {
+		struct mlx5e_txqsq *pending = NULL;
+		int num_pending = 0;
+		int i;
+
+		for (i = 0; i < chs->num; i++)
+			num_pending += mlx5e_drain_channel_txqsqs(chs->c[i],
+								  &pending);
+
+		if (!num_pending)
+			return true;
+
+		if (time_after(jiffies, exp_time)) {
+			netdev_warn(pending->netdev,
+				    "Old TX SQs not drained in %d msecs (%d pending, e.g. SQ 0x%x txq %d cc 0x%x pc 0x%x), closing the old channels first\n",
+				    MLX5E_SQ_DRAIN_TIMEOUT, num_pending,
+				    pending->sqn, pending->txq_ix,
+				    pending->cc, pending->pc);
+			return false;
+		}
+
+		usleep_range(10, 20);
+	}
+}
+
 static int mlx5e_switch_priv_channels(struct mlx5e_priv *priv,
 				      struct mlx5e_channels *old_chs,
 				      struct mlx5e_channels *new_chs,
@@ -3428,6 +3504,7 @@ static int mlx5e_switch_priv_channels(struct mlx5e_priv *priv,
 				      void *context)
 {
 	struct net_device *netdev = priv->netdev;
+	bool defer_close = false;
 	int carrier_ok;
 	int err = 0;
 
@@ -3450,7 +3527,10 @@ static int mlx5e_switch_priv_channels(struct mlx5e_priv *priv,
 	}
 
 	mlx5e_set_xdp_feature(priv);
-	if (!MLX5_CAP_GEN(priv->mdev, tis_tir_td_order))
+
+	defer_close = MLX5_CAP_GEN(priv->mdev, tis_tir_td_order) &&
+		      mlx5e_drain_channels_txqsqs(old_chs);
+	if (!defer_close)
 		mlx5e_close_channels(old_chs);
 	priv->profile->update_rx(priv);
 
@@ -3462,6 +3542,9 @@ static int mlx5e_switch_priv_channels(struct mlx5e_priv *priv,
 	if (carrier_ok)
 		netif_carrier_on(netdev);
 
+	if (defer_close)
+		mlx5e_close_channels(old_chs);
+
 	return err;
 }
 
@@ -3499,9 +3582,6 @@ int mlx5e_safe_switch_params(struct mlx5e_priv *priv,
 	if (err)
 		goto err_close;
 
-	if (MLX5_CAP_GEN(priv->mdev, tis_tir_td_order))
-		mlx5e_close_channels(old_chs);
-
 	kfree(new_chs);
 	kfree(old_chs);
 	return 0;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
index bee6f0bf1eff..eb20e3d14d46 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
@@ -780,10 +780,11 @@ void mlx5e_txqsq_wake(struct mlx5e_txqsq *sq)
 	}
 }
 
-bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
+/* Returns the number of polled CQEs. */
+static int mlx5e_poll_tx_cqes(struct mlx5e_txqsq *sq, int napi_budget)
 {
+	struct mlx5e_cq *cq = &sq->cq;
 	struct mlx5e_sq_stats *stats;
-	struct mlx5e_txqsq *sq;
 	struct mlx5_cqe64 *cqe;
 	u32 dma_fifo_cc;
 	u32 nbytes;
@@ -791,14 +792,9 @@ bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
 	u16 sqcc;
 	int i;
 
-	sq = container_of(cq, struct mlx5e_txqsq, cq);
-
-	if (unlikely(!test_bit(MLX5E_SQ_STATE_ENABLED, &sq->state)))
-		return false;
-
 	cqe = mlx5_cqwq_get_cqe(&cq->wq);
 	if (!cqe)
-		return false;
+		return 0;
 
 	stats = sq->stats;
 
@@ -891,11 +887,40 @@ bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
 
 	netdev_tx_completed_queue(sq->txq, npkts, nbytes);
 
+	return i;
+}
+
+bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
+{
+	struct mlx5e_txqsq *sq = container_of(cq, struct mlx5e_txqsq, cq);
+	int i;
+
+	if (unlikely(!test_bit(MLX5E_SQ_STATE_ENABLED, &sq->state)))
+		return false;
+
+	i = mlx5e_poll_tx_cqes(sq, napi_budget);
+	if (!i)
+		return false;
+
 	mlx5e_txqsq_wake(sq);
 
 	return (i == MLX5E_TX_CQ_POLL_BUDGET);
 }
 
+/* Reap the completions of a deactivated SQ, whose NAPI is disabled, without
+ * waking its TX queue. Returns true once all its WQEs have completed.
+ */
+bool mlx5e_txqsq_drain(struct mlx5e_txqsq *sq)
+{
+	int polled;
+
+	do {
+		polled = mlx5e_poll_tx_cqes(sq, 0);
+	} while (polled == MLX5E_TX_CQ_POLL_BUDGET);
+
+	return sq->cc == sq->pc;
+}
+
 static void mlx5e_tx_wi_kfree_fifo_skbs(struct mlx5e_txqsq *sq, struct mlx5e_tx_wqe_info *wi)
 {
 	int i;
-- 
2.44.0

Re: [RFC net-next 4/7] net/mlx5e: Drain old TX SQs before activating new channels

[Rishikesh Jethwani]

On Mon, Nov 9, 2026 at 3:49 AM Tariq Toukan <tariqt@nvidia.com> wrote:
>
> On devices with tis_tir_td_order, commit 3b88a535a8e1 ("net/mlx5e:
> Defer channels closure to reduce interface down time") closes the old
> channels only after the new ones are activated. Their SQs are
> deactivated before that, which rings a final doorbell, but nothing
> waits for the HW to execute their pending WQEs: the completions of a
> deactivated SQ are not polled, and the SQ is destroyed only when its
> channel is closed. Meanwhile, the new SQ of the same TX queue starts
> transmitting. A flow's WQEs still pending on the old SQ can then
> execute after its newer WQEs on the new SQ, as WQE fences order only
> within an SQ.
>
> Besides reordering packets, this breaks kTLS TX offload. The WQEs of a
> connection update its TIS crypto and record tracking state as they
> execute. When old WQEs execute after new ones, e.g. because a PFC
> pause held the old SQ back, the TIS state gets out of sync with the
> TCP stream, and the connection's records go out corrupted.
>
> Before activating the new channels, drain the TX SQs of the old ones:
> reap their completions until all their WQEs have completed. If they
> do not drain in time, e.g. under a long pause, warn, and close the old
> channels before activating the new ones, as before that commit. Move
> the deferred closure into mlx5e_switch_priv_channels(), next to that
> decision.
>
> Pointed out by Rishikesh Jethwani while discussing kTLS TX TIS reuse
> for TLS 1.3 rekey on netdev.
>
> Reproduced on ConnectX-6 Dx, with 248 channels, by changing the
> channel count between 247 and 248 every 2 seconds while 32 kTLS TX
> offloaded connections stream pattern data to a software kTLS receiver
> that checks every byte. On this setup, the new channels are activated
> about 230 ms after the old ones are deactivated, so the old SQs must
> still hold WQEs by then: with every TX queue rate limited to 10 Mb/s,
> 9 of 41 connections failed decryption in 60 seconds, and 11 of 43 in
> 120 seconds. With this patch, no record failed in five such runs, 7
> minutes in total; the drain timed out and fell back to closing the old
> channels first 7 times. At 1 Gb/s per queue, the old SQs always
> drained in time.
>
> Fixes: 3b88a535a8e1 ("net/mlx5e: Defer channels closure to reduce interface down time")
> 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>
> ---
>  .../net/ethernet/mellanox/mlx5/core/en/txrx.h |  1 +
>  .../net/ethernet/mellanox/mlx5/core/en_main.c | 88 ++++++++++++++++++-
>  .../net/ethernet/mellanox/mlx5/core/en_tx.c   | 41 +++++++--
>  3 files changed, 118 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h
> index 24e91279243e..b5bd1b6e8601 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h
> @@ -112,6 +112,7 @@ struct mlx5e_xmit_data_frags {
>
>  netdev_tx_t mlx5e_xmit(struct sk_buff *skb, struct net_device *dev);
>  bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget);
> +bool mlx5e_txqsq_drain(struct mlx5e_txqsq *sq);
>  void mlx5e_free_txqsq_descs(struct mlx5e_txqsq *sq);
>
>  static inline bool
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index c3c2c5720c6f..a721e30bcaa8 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -3421,6 +3421,82 @@ static int mlx5e_switch_priv_params(struct mlx5e_priv *priv,
>         return 0;
>  }
>
> +/* Time for the TX SQs of deactivated channels to complete their WQEs. */
> +#define MLX5E_SQ_DRAIN_TIMEOUT 100 /* msecs */
> +
> +/* Returns 1 if the SQ still has pending WQEs, and records the first such SQ. */
> +static int mlx5e_drain_txqsq(struct mlx5e_txqsq *sq,
> +                            struct mlx5e_txqsq **pending)
> +{
> +       if (mlx5e_txqsq_drain(sq))
> +               return 0;
> +
> +       if (!*pending)
> +               *pending = sq;
> +
> +       return 1;
> +}
> +
> +/* Returns the number of the channel's TX SQs that still have pending WQEs. */
> +static int mlx5e_drain_channel_txqsqs(struct mlx5e_channel *c,
> +                                     struct mlx5e_txqsq **pending)
> +{
> +       struct mlx5e_txqsq __rcu **qos_sqs;
> +       int num_pending = 0;
> +       int i;
> +
> +       for (i = 0; i < c->num_tc; i++)
> +               num_pending += mlx5e_drain_txqsq(&c->sq[i], pending);
> +
> +       qos_sqs = mlx5e_state_dereference(c->priv, c->qos_sqs);
> +       if (!qos_sqs)
> +               return num_pending;
> +
> +       for (i = 0; i < c->qos_sqs_size; i++) {
> +               struct mlx5e_txqsq *sq;
> +
> +               sq = mlx5e_state_dereference(c->priv, qos_sqs[i]);
> +               if (sq)
> +                       num_pending += mlx5e_drain_txqsq(sq, pending);
> +       }
> +
> +       return num_pending;
> +}
> +
> +/* Wait for the TX SQs of deactivated channels to complete all their WQEs,
> + * so that none of them executes concurrently with the new SQs.
> + * The PTP SQs are not waited for, they carry PTP event packets only.
> + */

The regular/QoS drain and the timeout ordering look right to me. The
PTP SQs can carry TLS data, though. Their indices are in
real_num_tx_queues and txq2sq, and egress skbedit can select them,
bypassing mlx5e_select_queue(). mlx5e_xmit() runs the kTLS handling on
any SQ, and does not reject a non-timestamped skb on a PTP SQ.

If a PTP SQ holds TLS WQEs during a channel replacement, the drain can
succeed while its TX queue index is reused, by the replacement PTP SQ
or by a new regular SQ after a channel count change. With one TC,
growing from 247 to 248 channels changes index 247 from PTP to the new
regular channel's SQ. A fixed skbedit rule still selects 247, and patch
7's owner-index comparison permits new TLS work there while the old
PTP SQ is still pending and not yet destroyed.

I'd prefer to include the PTP SQs in the drain/close ordering.
Rejecting TLS data on them before any connection-state change or TLS
WQE posting would also be safe, but with a fixed skbedit rule every
retransmit goes back to the same queue and is dropped again, stalling
the connection. Excluding their indices only in queue selection does
not cover skbedit.

> +static bool mlx5e_drain_channels_txqsqs(struct mlx5e_channels *chs)
> +{
> +       unsigned long exp_time;
> +
> +       exp_time = jiffies + msecs_to_jiffies(MLX5E_SQ_DRAIN_TIMEOUT);
> +       for (;;) {
> +               struct mlx5e_txqsq *pending = NULL;
> +               int num_pending = 0;
> +               int i;
> +
> +               for (i = 0; i < chs->num; i++)
> +                       num_pending += mlx5e_drain_channel_txqsqs(chs->c[i],
> +                                                                 &pending);
> +
> +               if (!num_pending)
> +                       return true;
> +
> +               if (time_after(jiffies, exp_time)) {
> +                       netdev_warn(pending->netdev,
> +                                   "Old TX SQs not drained in %d msecs (%d pending, e.g. SQ 0x%x txq %d cc 0x%x pc 0x%x), closing the old channels first\n",
> +                                   MLX5E_SQ_DRAIN_TIMEOUT, num_pending,
> +                                   pending->sqn, pending->txq_ix,
> +                                   pending->cc, pending->pc);
> +                       return false;
> +               }
> +
> +               usleep_range(10, 20);
> +       }
> +}
> +
>  static int mlx5e_switch_priv_channels(struct mlx5e_priv *priv,
>                                       struct mlx5e_channels *old_chs,
>                                       struct mlx5e_channels *new_chs,
> @@ -3428,6 +3504,7 @@ static int mlx5e_switch_priv_channels(struct mlx5e_priv *priv,
>                                       void *context)
>  {
>         struct net_device *netdev = priv->netdev;
> +       bool defer_close = false;
>         int carrier_ok;
>         int err = 0;
>
> @@ -3450,7 +3527,10 @@ static int mlx5e_switch_priv_channels(struct mlx5e_priv *priv,
>         }
>
>         mlx5e_set_xdp_feature(priv);
> -       if (!MLX5_CAP_GEN(priv->mdev, tis_tir_td_order))
> +
> +       defer_close = MLX5_CAP_GEN(priv->mdev, tis_tir_td_order) &&
> +                     mlx5e_drain_channels_txqsqs(old_chs);
> +       if (!defer_close)
>                 mlx5e_close_channels(old_chs);
>         priv->profile->update_rx(priv);
>
> @@ -3462,6 +3542,9 @@ static int mlx5e_switch_priv_channels(struct mlx5e_priv *priv,
>         if (carrier_ok)
>                 netif_carrier_on(netdev);
>
> +       if (defer_close)
> +               mlx5e_close_channels(old_chs);
> +
>         return err;
>  }
>
> @@ -3499,9 +3582,6 @@ int mlx5e_safe_switch_params(struct mlx5e_priv *priv,
>         if (err)
>                 goto err_close;
>
> -       if (MLX5_CAP_GEN(priv->mdev, tis_tir_td_order))
> -               mlx5e_close_channels(old_chs);
> -
>         kfree(new_chs);
>         kfree(old_chs);
>         return 0;
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> index bee6f0bf1eff..eb20e3d14d46 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> @@ -780,10 +780,11 @@ void mlx5e_txqsq_wake(struct mlx5e_txqsq *sq)
>         }
>  }
>
> -bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
> +/* Returns the number of polled CQEs. */
> +static int mlx5e_poll_tx_cqes(struct mlx5e_txqsq *sq, int napi_budget)
>  {
> +       struct mlx5e_cq *cq = &sq->cq;
>         struct mlx5e_sq_stats *stats;
> -       struct mlx5e_txqsq *sq;
>         struct mlx5_cqe64 *cqe;
>         u32 dma_fifo_cc;
>         u32 nbytes;
> @@ -791,14 +792,9 @@ bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
>         u16 sqcc;
>         int i;
>
> -       sq = container_of(cq, struct mlx5e_txqsq, cq);
> -
> -       if (unlikely(!test_bit(MLX5E_SQ_STATE_ENABLED, &sq->state)))
> -               return false;
> -
>         cqe = mlx5_cqwq_get_cqe(&cq->wq);
>         if (!cqe)
> -               return false;
> +               return 0;
>
>         stats = sq->stats;
>
> @@ -891,11 +887,40 @@ bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
>
>         netdev_tx_completed_queue(sq->txq, npkts, nbytes);
>
> +       return i;
> +}
> +
> +bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
> +{
> +       struct mlx5e_txqsq *sq = container_of(cq, struct mlx5e_txqsq, cq);
> +       int i;
> +
> +       if (unlikely(!test_bit(MLX5E_SQ_STATE_ENABLED, &sq->state)))
> +               return false;
> +
> +       i = mlx5e_poll_tx_cqes(sq, napi_budget);
> +       if (!i)
> +               return false;
> +
>         mlx5e_txqsq_wake(sq);
>
>         return (i == MLX5E_TX_CQ_POLL_BUDGET);
>  }
>
> +/* Reap the completions of a deactivated SQ, whose NAPI is disabled, without
> + * waking its TX queue. Returns true once all its WQEs have completed.
> + */
> +bool mlx5e_txqsq_drain(struct mlx5e_txqsq *sq)
> +{
> +       int polled;
> +
> +       do {
> +               polled = mlx5e_poll_tx_cqes(sq, 0);
> +       } while (polled == MLX5E_TX_CQ_POLL_BUDGET);
> +
> +       return sq->cc == sq->pc;
> +}
> +
>  static void mlx5e_tx_wi_kfree_fifo_skbs(struct mlx5e_txqsq *sq, struct mlx5e_tx_wqe_info *wi)
>  {
>         int i;
> --
> 2.44.0
>

Re: [RFC net-next 4/7] net/mlx5e: Drain old TX SQs before activating new channels

[Tariq Toukan]

On 10/11/2026 0:19, Rishikesh Jethwani wrote:
 > The regular/QoS drain and the timeout ordering look right to me. The
 > PTP SQs can carry TLS data, though. Their indices are in
 > real_num_tx_queues and txq2sq, and egress skbedit can select them,
 > bypassing mlx5e_select_queue().

[...]

 > I'd prefer to include the PTP SQs in the drain/close ordering.

Agreed, v2 drains the PTP SQs with the others, so a timeout closes the
old PTP channel first too, along with the old channels.

Thanks,
Tariq

Re: [RFC net-next 4/7] net/mlx5e: Drain old TX SQs before activating new channels

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] net/mlx5e: mlx5e_poll_tx_cqes() queues recover_work on
  deactivated SQs
--

commit fd69ab25361dddbeaf22971fdc4198ef4326e153
Author: Tariq Toukan <tariqt@nvidia.com>

net/mlx5e: Drain old TX SQs before activating new channels

This commit drains pending WQEs from deactivated TX SQs before activating
new channels when tis_tir_td_order is supported, falling back to closing
the old channels first if the drain times out.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> index bee6f0bf1eff5..eb20e3d14d461 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> @@ -780,10 +780,11 @@ void mlx5e_txqsq_wake(struct mlx5e_txqsq *sq)
>  	}
>  }
>  
> -bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
> +/* Returns the number of polled CQEs. */
> +static int mlx5e_poll_tx_cqes(struct mlx5e_txqsq *sq, int napi_budget)
>  {
> +	struct mlx5e_cq *cq = &sq->cq;
>  	struct mlx5e_sq_stats *stats;
> -	struct mlx5e_txqsq *sq;
>  	struct mlx5_cqe64 *cqe;
>  	u32 dma_fifo_cc;
>  	u32 nbytes;
> @@ -791,14 +792,9 @@ bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
>  	u16 sqcc;
>  	int i;
>  
> -	sq = container_of(cq, struct mlx5e_txqsq, cq);
> -
> -	if (unlikely(!test_bit(MLX5E_SQ_STATE_ENABLED, &sq->state)))
> -		return false;
> -
>  	cqe = mlx5_cqwq_get_cqe(&cq->wq);
>  	if (!cqe)
> -		return false;
> +		return 0;
>  
>  	stats = sq->stats;
>  
> @@ -891,11 +887,40 @@ bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
>  
>  	netdev_tx_completed_queue(sq->txq, npkts, nbytes);
>  
> +	return i;
> +}
> +
> +bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
> +{
> +	struct mlx5e_txqsq *sq = container_of(cq, struct mlx5e_txqsq, cq);
> +	int i;
> +
> +	if (unlikely(!test_bit(MLX5E_SQ_STATE_ENABLED, &sq->state)))
> +		return false;
> +
> +	i = mlx5e_poll_tx_cqes(sq, napi_budget);
> +	if (!i)
> +		return false;
> +
>  	mlx5e_txqsq_wake(sq);
>  
>  	return (i == MLX5E_TX_CQ_POLL_BUDGET);
>  }
>  
> +/* Reap the completions of a deactivated SQ, whose NAPI is disabled, without
> + * waking its TX queue. Returns true once all its WQEs have completed.
> + */
> +bool mlx5e_txqsq_drain(struct mlx5e_txqsq *sq)
> +{
> +	int polled;
> +
> +	do {
> +		polled = mlx5e_poll_tx_cqes(sq, 0);
> +	} while (polled == MLX5E_TX_CQ_POLL_BUDGET);
> +
> +	return sq->cc == sq->pc;
> +}

[Severity: High]
Can mlx5e_txqsq_drain() -> mlx5e_poll_tx_cqes() queue sq->recover_work on
a deactivated SQ when it polls an MLX5_CQE_REQ_ERR completion?

Before this patch, mlx5e_poll_tx_cq() returned early when
MLX5E_SQ_STATE_ENABLED was clear.  Now mlx5e_poll_tx_cqes() runs on the
deactivated SQs in old_chs during mlx5e_switch_priv_channels(), after
mlx5e_open_channels() -> mlx5e_health_channels_update() has already reset
priv->tx_reporter->health_state to DEVLINK_HEALTH_REPORTER_STATE_HEALTHY.
If an old SQ completes a WQE or the deactivation NOP with
MLX5_CQE_REQ_ERR, mlx5e_poll_tx_cqes() queues sq->recover_work on
cq->workqueue:

drivers/net/ethernet/mellanox/mlx5/core/en_tx.c:mlx5e_poll_tx_cqes() {
	...
		if (unlikely(err_cqe)) {
			if (!test_and_set_bit(MLX5E_SQ_STATE_RECOVERING,
					      &sq->state)) {
				mlx5e_dump_error_cqe(&sq->cq, sq->sqn,
						     (struct mlx5_err_cqe *)cqe);
				mlx5_wq_cyc_wqe_dump(&sq->wq, ci, wi->num_wqebbs);
				queue_work(cq->workqueue, &sq->recover_work);
			}
			stats->cqe_err++;
		}
	...
}

When mlx5e_safe_reopen_channels() is invoked from devlink health recovery
under devl_lock(devlink), this causes an ABBA deadlock with
cancel_work_sync(&sq->recover_work) when old_chs is closed:

CPU0                              CPU1 (kworker)
----                              --------------
devl_lock(devlink)
  ...
  mlx5e_switch_priv_channels()
    mlx5e_drain_channels_txqsqs()
      mlx5e_poll_tx_cqes()
        queue_work(..., &sq->recover_work)
                                  mlx5e_tx_err_cqe_work()
                                    mlx5e_reporter_tx_err_cqe()
                                      devlink_health_report()
                                        devl_lock(devlink)
    mlx5e_close_channels(old_chs)
      ...
      mlx5e_close_txqsq()
        cancel_work_sync(&sq->recover_work)

 *** DEADLOCK ***

Even when devl_lock(devlink) is not held by the caller (such as during an
ethtool channel change), queuing sq->recover_work for an SQ in old_chs
runs devlink_health_report() after mlx5e_open_channels() marked
tx_reporter healthy, which either leaves tx_reporter in an error state for
an already-closed SQ or starts a recovery graceful period that suppresses
real error recovery on new_chs.

Should mlx5e_poll_tx_cqes() only queue sq->recover_work when
MLX5E_SQ_STATE_ENABLED is set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109114734.2087878-1-tariqt@nvidia.com?part=4

[RFC net-next 5/7] net: tls: keep the TX queue of a device-offloaded socket stable

[Tariq Toukan]

A device that keeps per-connection TLS TX state (e.g. mlx5e kTLS, where
WQEs update the TIS crypto and record tracking state at execution time)
cannot order work posted on different TX queues. If the flow moves to
another TX queue while work is still in flight on the previous one, the
device state is accessed from both queues with no ordering, corrupting
the record stream on the wire.

The stack moves a socket's flow while packets are in flight: TCP sets
ooo_okay when the rtx queue is empty, a dst reset (e.g. on RTO or PLB
rehash) clears the cached TX queue, and txq_reselection_ms expires it.

Let a driver declare the constraint via a new tlsdev_ops flag,
TLSDEV_F_STICKY_TXQ, and mark sockets offloaded to such a device with a
new sock flag, SOCK_STICKY_TXQ. While such a socket holds sk_wmem_alloc,
keep its cached TX queue: do not clear it on dst changes, do not expire
it, and set ooo_okay from the wmem condition only.

This covers the stack's own queue selection. The rest is the driver's,
as documented: a ndo_select_queue that derives the queue from more than
the cached one must keep its choice stable too, changing it only for an
skb with ooo_okay set, and when TX queues are removed, the device must
be done with their work before the flows move. tc skbedit queue_mapping
bypasses queue selection, and is not supported for such sockets.

Packets in flight are tracked by sk_wmem_alloc, so the flag also
requires the driver not to free an skb, even a dropped one, before the
device is done with the work posted for it.

Found by LLM-assisted code inspection, while answering a question on
kTLS TX TIS reuse for TLS 1.3 rekey on netdev, and reproduced on
ConnectX-6 Dx with forced queue moves.

Reproduced on ConnectX-6 Dx, with 128 channels and 32 kTLS TX offloaded
connections streaming pattern data for 60 seconds to a software kTLS
receiver that checks every byte. Without this patch and the mlx5e one
that follows it, 1 of 33 connections failed decryption with the default
txq_reselection_ms, and 67 of 99 with txq_reselection_ms=1 and the
senders migrating between CPUs every millisecond. With both applied, no
record failed in either test, with no loss of throughput.

Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
Fixes: 726e9e8b94b9 ("tcp: refine skb->ooo_okay setting")
Fixes: 4a7708443dec ("net: allow busy connected flows to switch tx queues")
Link: https://lore.kernel.org/netdev/20261022181751.1349984-1-rjethwani@purestorage.com/
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 Documentation/admin-guide/sysctl/net.rst |  3 +++
 Documentation/networking/tls-offload.rst | 20 ++++++++++++++
 include/net/sock.h                       | 33 +++++++++++++++++++++---
 include/net/tls.h                        | 13 ++++++++++
 net/core/dev.c                           |  3 ++-
 net/core/sock.c                          |  2 +-
 net/ipv4/tcp_output.c                    |  7 ++++-
 net/tls/tls_device.c                     |  3 +++
 8 files changed, 78 insertions(+), 6 deletions(-)

diff --git a/Documentation/admin-guide/sysctl/net.rst b/Documentation/admin-guide/sysctl/net.rst
index fe43e8595958..88bfb4421843 100644
--- a/Documentation/admin-guide/sysctl/net.rst
+++ b/Documentation/admin-guide/sysctl/net.rst
@@ -449,6 +449,9 @@ But switching txq can introduce reorders, especially if the
 old queue is under high pressure. Modern TCP stacks deal
 well with reorders if they happen not too often.
 
+A flow offloaded to a TLS device that keeps per-flow TX state in
+the device switches txq only once it has no packets in flight.
+
 To disable this feature, set the value to 0.
 
 Default : 1000
diff --git a/Documentation/networking/tls-offload.rst b/Documentation/networking/tls-offload.rst
index cf0e599c06a1..f03e48643503 100644
--- a/Documentation/networking/tls-offload.rst
+++ b/Documentation/networking/tls-offload.rst
@@ -190,6 +190,26 @@ TLS handling and confirms the sequence number matches its expectation.
 The device performs encryption and authentication of the record data.
 It replaces the authentication tag and TCP checksum with correct values.
 
+A device may keep the TLS state of a connection in the device, and update it
+as it processes the TX queues, with no ordering between the queues. Its driver
+sets ``TLSDEV_F_STICKY_TXQ`` in ``tlsdev_ops->flags``, and the stack then keeps
+the TX queue selection of an offloaded socket stable while the socket has
+packets in flight (``SOCK_STICKY_TXQ``): the cached queue is neither cleared on
+route changes nor expired by ``txq_reselection_ms``, and TCP sets
+``skb->ooo_okay`` only when no packet of the socket is in flight. Packets in
+flight are tracked by ``sk_wmem_alloc``, so the driver must not free an skb
+before the device is done with all the work it posted for it, even when the skb
+is dropped.
+
+The rest is up to the driver. If its ``ndo_select_queue`` derives the queue from
+more than the socket's cached queue, it must keep its choice stable as well, and
+may change it only for an skb with ``ooo_okay`` set. A queue change driven by
+the skb's priority or traffic class then waits until the socket has no packets
+in flight, which under sustained traffic may never happen. When TX queues are
+removed, the device must be done with their work before the flows that move from
+them start transmitting on other queues. ``tc`` ``skbedit queue_mapping``
+bypasses queue selection and is not supported for such sockets.
+
 RX
 --
 
diff --git a/include/net/sock.h b/include/net/sock.h
index 9bba1e793f1e..ab4b5a5a74f5 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1026,6 +1026,11 @@ enum sock_flags {
 	SOCK_RCVMARK, /* Receive SO_MARK  ancillary data with packet */
 	SOCK_RCVPRIORITY, /* Receive SO_PRIORITY ancillary data with packet */
 	SOCK_TIMESTAMPING_ANY, /* Copy of sk_tsflags & TSFLAGS_ANY */
+	SOCK_STICKY_TXQ, /* Keep the TX queue selection while packets are in
+			  * flight; set for sockets whose TX offload keeps
+			  * per-flow device state updated by queue execution
+			  * (see TLSDEV_F_STICKY_TXQ).
+			  */
 };
 
 #define SK_FLAGS_TIMESTAMP ((1UL << SOCK_TIMESTAMP) | (1UL << SOCK_TIMESTAMPING_RX_SOFTWARE))
@@ -2067,6 +2072,29 @@ static inline void sk_tx_queue_clear(struct sock *sk)
 	WRITE_ONCE(sk->sk_tx_queue_mapping, NO_QUEUE_MAPPING);
 }
 
+#define SK_WMEM_ALLOC_BIAS 1
+
+/* A SOCK_STICKY_TXQ socket keeps its TX queue while it has packets in
+ * flight: a stateful TX offload must not see the flow on a new queue
+ * while its work may still be pending on the previous one.
+ */
+static inline bool sk_tx_queue_pinned(const struct sock *sk)
+{
+	return sock_flag(sk, SOCK_STICKY_TXQ) &&
+	       refcount_read(&sk->sk_wmem_alloc) - SK_WMEM_ALLOC_BIAS >=
+	       SKB_TRUESIZE(1);
+}
+
+/* Like sk_tx_queue_clear(), but keeps a pinned mapping, see
+ * sk_tx_queue_pinned(). Lifecycle resets (allocation, cloning) must use
+ * sk_tx_queue_clear() directly.
+ */
+static inline void sk_tx_queue_maybe_clear(struct sock *sk)
+{
+	if (!sk_tx_queue_pinned(sk))
+		sk_tx_queue_clear(sk);
+}
+
 int sk_tx_queue_get(const struct sock *sk);
 
 static inline void __sk_rx_queue_set(struct sock *sk,
@@ -2237,7 +2265,7 @@ __sk_dst_set(struct sock *sk, struct dst_entry *dst)
 {
 	struct dst_entry *old_dst;
 
-	sk_tx_queue_clear(sk);
+	sk_tx_queue_maybe_clear(sk);
 	WRITE_ONCE(sk->sk_dst_pending_confirm, 0);
 	old_dst = rcu_dereference_protected(sk->sk_dst_cache,
 					    lockdep_sock_is_held(sk));
@@ -2250,7 +2278,7 @@ sk_dst_set(struct sock *sk, struct dst_entry *dst)
 {
 	struct dst_entry *old_dst;
 
-	sk_tx_queue_clear(sk);
+	sk_tx_queue_maybe_clear(sk);
 	WRITE_ONCE(sk->sk_dst_pending_confirm, 0);
 	old_dst = unrcu_pointer(xchg(&sk->sk_dst_cache, RCU_INITIALIZER(dst)));
 	dst_release(old_dst);
@@ -2369,7 +2397,6 @@ static inline int skb_copy_to_page_nocache(struct sock *sk, struct iov_iter *fro
 	return 0;
 }
 
-#define SK_WMEM_ALLOC_BIAS 1
 /**
  * sk_wmem_alloc_get - returns write allocations
  * @sk: socket
diff --git a/include/net/tls.h b/include/net/tls.h
index e57bef58851e..80fd68c9801f 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -276,7 +276,20 @@ enum tls_offload_ctx_dir {
 	TLS_OFFLOAD_CTX_DIR_TX,
 };
 
+/* TX offload state (e.g. crypto and record tracking state of a device
+ * connection context) is updated by TX queue execution, and the device
+ * provides no cross-queue ordering. The stack keeps the socket's TX queue
+ * selection stable while the socket has packets in flight
+ * (SOCK_STICKY_TXQ), and the driver keeps its own part of the selection
+ * stable too, see Documentation/networking/tls-offload.rst. Packets in
+ * flight are tracked by sk_wmem_alloc, so the driver must not free an skb
+ * before the device is done with all work it posted for it, even when the
+ * skb is dropped.
+ */
+#define TLSDEV_F_STICKY_TXQ	BIT(0)
+
 struct tlsdev_ops {
+	u32 flags; /* TLSDEV_F_* */
 	int (*tls_dev_add)(struct net_device *netdev, struct sock *sk,
 			   enum tls_offload_ctx_dir direction,
 			   struct tls_crypto_info *crypto_info,
diff --git a/net/core/dev.c b/net/core/dev.c
index 3b60b15e2175..18f7b9159ec5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4894,7 +4894,8 @@ int sk_tx_queue_get(const struct sock *sk)
 
 	resel = READ_ONCE(sock_net(sk)->core.sysctl_txq_reselection);
 	if (resel && time_is_before_jiffies(
-			READ_ONCE(sk->sk_tx_queue_mapping_jiffies) + resel))
+			READ_ONCE(sk->sk_tx_queue_mapping_jiffies) + resel) &&
+	    !sk_tx_queue_pinned(sk))
 		return -1;
 
 	return val;
diff --git a/net/core/sock.c b/net/core/sock.c
index e2401d735c5a..d90a4f69ddda 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -606,7 +606,7 @@ struct dst_entry *__sk_dst_check(struct sock *sk, u32 cookie)
 	if (dst && READ_ONCE(dst->obsolete) &&
 	    INDIRECT_CALL_INET(dst->ops->check, ip6_dst_check, ipv4_dst_check,
 			       dst, cookie) == NULL) {
-		sk_tx_queue_clear(sk);
+		sk_tx_queue_maybe_clear(sk);
 		WRITE_ONCE(sk->sk_dst_pending_confirm, 0);
 		RCU_INIT_POINTER(sk->sk_dst_cache, NULL);
 		dst_release(dst);
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 08d0d4a639fe..690f73e9de41 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1661,9 +1661,14 @@ static int __tcp_transmit_skb(struct sock *sk, struct sk_buff *skb,
 	 * 2) Or rtx queue is empty.
 	 *    This mitigates above case if ACK packets for
 	 *    all prior packets were already processed.
+	 * A SOCK_STICKY_TXQ socket (stateful TX offload) relies on 1) only:
+	 * a spurious retransmit clone may still hold wmem and reference
+	 * device state from the old queue when the rtx queue is already
+	 * empty.
 	 */
 	skb->ooo_okay = sk_wmem_alloc_get(sk) < SKB_TRUESIZE(1) ||
-			tcp_rtx_queue_empty(sk);
+			(tcp_rtx_queue_empty(sk) &&
+			 !sock_flag(sk, SOCK_STICKY_TXQ));
 
 	/* If we had to use memory reserve to allocate this skb,
 	 * this might cause drops if packet is looped back :
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index 4cdfbb57ecff..048b6a912d41 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -1197,6 +1197,9 @@ int tls_set_device_offload(struct sock *sk)
 	tls_device_attach(ctx, sk, netdev);
 	up_read(&device_offload_lock);
 
+	if (netdev->tlsdev_ops->flags & TLSDEV_F_STICKY_TXQ)
+		sock_set_flag(sk, SOCK_STICKY_TXQ);
+
 	/* following this assignment tls_is_skb_tx_device_offloaded
 	 * will return true and the context might be accessed
 	 * by the netdev's xmit function.
-- 
2.44.0

Re: [RFC net-next 5/7] net: tls: keep the TX queue of a device-offloaded socket stable

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] net: netdev_pick_tx() skips sk_tx_queue_set() when
  sk_dst_cache is NULL
--

commit 32c68c0d3ebf3235b8df4cd0c9b9ac85b4fd2c5f
Author: Tariq Toukan <tariqt@nvidia.com>

net: tls: keep the TX queue of a device-offloaded socket stable

This commit adds TLSDEV_F_STICKY_TXQ and SOCK_STICKY_TXQ so that sockets
offloaded to stateful TLS TX devices keep their cached TX queue mapping and
avoid setting skb->ooo_okay while packets are in flight.

Link: https://lore.kernel.org/netdev/20261022181751.1349984-1-rjethwani@purestorage.com/

> diff --git a/include/net/sock.h b/include/net/sock.h
> index 9bba1e793f1ee..ab4b5a5a74f5d 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
[ ... ]
> @@ -2067,6 +2072,29 @@ static inline void sk_tx_queue_clear(struct sock *sk)
>  	WRITE_ONCE(sk->sk_tx_queue_mapping, NO_QUEUE_MAPPING);
>  }
>  
> +#define SK_WMEM_ALLOC_BIAS 1
> +
> +/* A SOCK_STICKY_TXQ socket keeps its TX queue while it has packets in
> + * flight: a stateful TX offload must not see the flow on a new queue
> + * while its work may still be pending on the previous one.
> + */
> +static inline bool sk_tx_queue_pinned(const struct sock *sk)
> +{
> +	return sock_flag(sk, SOCK_STICKY_TXQ) &&
> +	       refcount_read(&sk->sk_wmem_alloc) - SK_WMEM_ALLOC_BIAS >=
> +	       SKB_TRUESIZE(1);
> +}
> +
> +/* Like sk_tx_queue_clear(), but keeps a pinned mapping, see
> + * sk_tx_queue_pinned(). Lifecycle resets (allocation, cloning) must use
> + * sk_tx_queue_clear() directly.
> + */
> +static inline void sk_tx_queue_maybe_clear(struct sock *sk)
> +{
> +	if (!sk_tx_queue_pinned(sk))
> +		sk_tx_queue_clear(sk);
> +}
> +
>  int sk_tx_queue_get(const struct sock *sk);
>  
>  static inline void __sk_rx_queue_set(struct sock *sk,
> @@ -2237,7 +2265,7 @@ __sk_dst_set(struct sock *sk, struct dst_entry *dst)
>  {
>  	struct dst_entry *old_dst;
>  
> -	sk_tx_queue_clear(sk);
> +	sk_tx_queue_maybe_clear(sk);
>  	WRITE_ONCE(sk->sk_dst_pending_confirm, 0);
>  	old_dst = rcu_dereference_protected(sk->sk_dst_cache,
>  					    lockdep_sock_is_held(sk));
> @@ -2250,7 +2278,7 @@ sk_dst_set(struct sock *sk, struct dst_entry *dst)
>  {
>  	struct dst_entry *old_dst;
>  
> -	sk_tx_queue_clear(sk);
> +	sk_tx_queue_maybe_clear(sk);
>  	WRITE_ONCE(sk->sk_dst_pending_confirm, 0);
>  	old_dst = unrcu_pointer(xchg(&sk->sk_dst_cache, RCU_INITIALIZER(dst)));
>  	dst_release(old_dst);

[Severity: Medium]
Can netdev_pick_tx() transmit a packet on a newly selected TX queue without
updating sk->sk_tx_queue_mapping when sk->sk_dst_cache is NULL?

net/core/dev.c:netdev_pick_tx() {
	...
	if (queue_index < 0 || skb->ooo_okay ||
	    queue_index >= dev->real_num_tx_queues) {
		int new_index = get_xps_queue(dev, sb_dev, skb);

		if (new_index < 0)
			new_index = skb_tx_hash(dev, sb_dev, skb);

		if (sk && sk_fullsock(sk) &&
		    rcu_access_pointer(sk->sk_dst_cache))
			sk_tx_queue_set(sk, new_index);

		queue_index = new_index;
	}
	...
}

In __tcp_transmit_skb(), when a SOCK_STICKY_TXQ socket with no packets in
flight sends an skb, it sets skb->ooo_okay = 1 and charges skb->truesize to
sk->sk_wmem_alloc before calling queue_xmit, which makes
sk_tx_queue_pinned(sk) true.  If sk->sk_dst_cache is cleared before that skb
reaches netdev_pick_tx() (for example by a concurrent sk_dst_reset() on a
redirect/PMTU update, or by an egress cgroup BPF program updating SO_MARK),
sk_tx_queue_maybe_clear() leaves the previous sk->sk_tx_queue_mapping intact
while setting sk->sk_dst_cache to NULL.

When that skb enters netdev_pick_tx(), skb->ooo_okay picks a new queue
new_index, but sk_tx_queue_set() is skipped because sk->sk_dst_cache is NULL.
A subsequent packet sent while the first is still in flight has
skb->ooo_okay = 0, repopulates sk->sk_dst_cache without clearing the old
sk->sk_tx_queue_mapping (since sk_tx_queue_pinned(sk) is true), and is sent
on the old queue while the first packet is still in flight on new_index,
corrupting per-connection device TLS state.

Allowing netdev_pick_tx() to also call sk_tx_queue_set() when
sock_flag(sk, SOCK_STICKY_TXQ) is set would keep sk->sk_tx_queue_mapping in
sync with the queue actually used.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109114734.2087878-1-tariqt@nvidia.com?part=5

[RFC net-next 6/7] net/mlx5e: kTLS, Keep the TX queue of an offloaded socket stable

[Tariq Toukan]

kTLS TX WQEs update the TIS crypto and record tracking state at
execution time, and WQEs of different SQs are not ordered. A connection
must not move to another SQ while WQEs that it posted to its SQ may be
pending.

With TLSDEV_F_STICKY_TXQ, the stack keeps the socket's TX queue while
the socket has packets in flight. mlx5e_select_queue() uses that queue
to pick the channel only: with multiple TCs, it adds the offset of the
skb's UP, derived from its DSCP or VLAN priority, and with HTB, it picks
the QoS queue of the skb's priority. A connection whose packets change
priority moves to another SQ at any time.

For a SOCK_STICKY_TXQ socket, cache the final queue in the socket, and
reuse it while TCP does not allow a move (ooo_okay is clear). A priority
change then takes effect once no packet of the connection is in flight.
TCP sets ooo_okay when it transmits the skb, and it may be stale by the
time the skb is selected, e.g. after an upper device's qdisc reordered
the socket's skbs, so a move also requires that no other skb of the
socket is in flight. A cached PTP queue is not reused, these are
selected for PTP event packets only.

The remaining moves are covered by the net fixes "net/mlx5e: Drain old
TX SQs before activating new channels", for channel replacement and the
TX queue count changes that come with it, and "net/mlx5e: kTLS, Hold a
dropped skb until its posted WQEs complete", which keeps sk_wmem_alloc
held while WQEs of a dropped skb may be pending. When an HTB leaf is
deleted or becomes an inner class, its queue is removed or given to
another class only after its queued packets were dropped and its SQ
closed, so its connections have nothing in flight and pick a queue anew.
An skb that netem partially orphaned holds no sk_wmem_alloc charge, but
it carries no data for the device, and does not post WQEs that name the
TIS since "net/mlx5e: kTLS, Post TX context params only for skbs that
use them".

Opt in to TLSDEV_F_STICKY_TXQ.

Found by LLM-assisted code inspection, while answering a question on
kTLS TX TIS reuse for TLS 1.3 rekey on netdev, and reproduced on
ConnectX-6 Dx with forced queue moves.

Reproduced on ConnectX-6 Dx, with 128 channels, mqprio with 8 TCs and
DSCP trust mapping DSCP 46 to priority 5, by flipping the IP_TOS of 32
kTLS TX offloaded connections between 0 and DSCP 46 every 5 ms while
they stream pattern data for 60 seconds to a software kTLS receiver that
checks every byte. Without this patch, 115 of 147 connections failed
decryption. With it, none did, and the connections still reached the TC5
queues.

Fixes: d2ead1f360e8 ("net/mlx5e: Add kTLS TX HW offload support")
Link: https://lore.kernel.org/netdev/20261022181751.1349984-1-rjethwani@purestorage.com/
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/en/selq.c | 92 ++++++++++++++++---
 .../mellanox/mlx5/core/en_accel/ktls.c        |  1 +
 2 files changed, 81 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c b/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
index 8318a26bc263..dfaee2c3624e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
@@ -5,6 +5,7 @@
 #include <linux/slab.h>
 #include <linux/netdevice.h>
 #include <linux/rcupdate.h>
+#include <net/sock.h>
 #include "en.h"
 #include "en/ptp.h"
 #include "en/htb.h"
@@ -190,22 +191,12 @@ static int mlx5e_select_htb_queue(struct mlx5e_priv *priv, struct sk_buff *skb,
 	return mlx5e_htb_get_txq_by_classid(priv->htb, classid);
 }
 
-u16 mlx5e_select_queue(struct net_device *dev, struct sk_buff *skb,
-		       struct net_device *sb_dev)
+static u16 __mlx5e_select_queue(struct net_device *dev, struct sk_buff *skb,
+				struct mlx5e_selq_params *selq)
 {
 	struct mlx5e_priv *priv = netdev_priv(dev);
-	struct mlx5e_selq_params *selq;
 	int txq_ix, up;
 
-	selq = rcu_dereference_bh(priv->selq.active);
-
-	/* This is a workaround needed only for the mlx5e_netdev_change_profile
-	 * flow that zeroes out the whole priv without unregistering the netdev
-	 * and without preventing ndo_select_queue from being called.
-	 */
-	if (unlikely(!selq))
-		return 0;
-
 	if (likely(!selq->is_special_queues)) {
 		/* No special queues, netdev_pick_tx returns one of the regular ones. */
 
@@ -266,3 +257,80 @@ u16 mlx5e_select_queue(struct net_device *dev, struct sk_buff *skb,
 
 	return txq_ix + up * selq->num_channels;
 }
+
+static bool mlx5e_sk_sticky_txq(const struct sock *sk)
+{
+	return IS_ENABLED(CONFIG_MLX5_EN_TLS) && sk && sk_fullsock(sk) &&
+	       sock_flag(sk, SOCK_STICKY_TXQ);
+}
+
+/* TCP sets skb->ooo_okay when it transmits the skb, and it may be stale by
+ * now, e.g. after an upper device's qdisc reordered the socket's skbs.
+ * Allow a move only if no other skb of the socket is in flight: a
+ * decrypted skb holds its own sk_wmem_alloc charge, any other skb may not.
+ */
+static bool mlx5e_sk_txq_may_move(const struct sk_buff *skb)
+{
+	int own = skb_is_decrypted(skb) ? skb->truesize : 0;
+
+	return skb->ooo_okay && sk_wmem_alloc_get(skb->sk) <= own;
+}
+
+/* A SOCK_STICKY_TXQ socket must not move to another TX queue while it has
+ * packets in flight, see TLSDEV_F_STICKY_TXQ. netdev_pick_tx() keeps the
+ * socket's queue, but the UP offset and the HTB queue are derived from each
+ * skb. Reuse the final queue of the socket's previous skb, unless TCP allows
+ * a move.
+ */
+static int mlx5e_select_sticky_queue(struct net_device *dev,
+				     struct sk_buff *skb,
+				     struct mlx5e_selq_params *selq)
+{
+	int txq_ix;
+
+	if (mlx5e_sk_txq_may_move(skb))
+		return -1;
+
+	txq_ix = sk_tx_queue_get(skb->sk);
+	if (txq_ix < 0 || txq_ix >= dev->real_num_tx_queues)
+		return -1;
+
+	/* The PTP queues are selected for PTP event packets only. */
+	if (selq->is_ptp && txq_ix >= selq->num_regular_queues &&
+	    txq_ix < selq->num_regular_queues + selq->num_tcs)
+		return -1;
+
+	return txq_ix;
+}
+
+u16 mlx5e_select_queue(struct net_device *dev, struct sk_buff *skb,
+		       struct net_device *sb_dev)
+{
+	struct mlx5e_priv *priv = netdev_priv(dev);
+	struct mlx5e_selq_params *selq;
+	struct sock *sk = skb->sk;
+	int txq_ix;
+
+	selq = rcu_dereference_bh(priv->selq.active);
+
+	/* This is a workaround needed only for the mlx5e_netdev_change_profile
+	 * flow that zeroes out the whole priv without unregistering the netdev
+	 * and without preventing ndo_select_queue from being called.
+	 */
+	if (unlikely(!selq))
+		return 0;
+
+	if (likely(!mlx5e_sk_sticky_txq(sk)))
+		return __mlx5e_select_queue(dev, skb, selq);
+
+	txq_ix = mlx5e_select_sticky_queue(dev, skb, selq);
+	if (txq_ix >= 0)
+		return txq_ix;
+
+	txq_ix = __mlx5e_select_queue(dev, skb, selq);
+	/* Cache the final queue, as netdev_pick_tx() does with its own pick. */
+	if (rcu_access_pointer(sk->sk_dst_cache))
+		sk_tx_queue_set(sk, txq_ix);
+
+	return txq_ix;
+}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls.c
index 337201f12895..248ccae9f8e8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls.c
@@ -87,6 +87,7 @@ static int mlx5e_ktls_resync(struct net_device *netdev,
 }
 
 static const struct tlsdev_ops mlx5e_ktls_ops = {
+	.flags = TLSDEV_F_STICKY_TXQ,
 	.tls_dev_add = mlx5e_ktls_add,
 	.tls_dev_del = mlx5e_ktls_del,
 	.tls_dev_resync = mlx5e_ktls_resync,
-- 
2.44.0

Re: [RFC net-next 6/7] net/mlx5e: kTLS, Keep the TX queue of an offloaded socket stable

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] net/mlx5e: missing sk_tx_queue_set() in mlx5e_select_queue()
  without dst
- [Medium] net/mlx5e: un-normalized queue cached in sk_tx_queue_mapping
  in selq
- [Medium] net/mlx5e: mlx5e_select_sticky_queue() reuses reassigned HTB
  txq_ix
- [Medium] net/mlx5e: mlx5e_sk_txq_may_move() fails on TSQ and pure ACK
  wmem charges
--

commit a5df807906b676e83f48b3d983daeac0ca981f49
Author: Tariq Toukan <tariqt@nvidia.com>

net/mlx5e: kTLS, Keep the TX queue of an offloaded socket stable

This commit opts in to TLSDEV_F_STICKY_TXQ and caches the final selected TX
queue in sk->sk_tx_queue_mapping for SOCK_STICKY_TXQ sockets so that priority
or HTB changes do not move an offloaded kTLS connection to another SQ while
packets are in flight.

Link: https://lore.kernel.org/netdev/20261022181751.1349984-1-rjethwani@purestorage.com/

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c b/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
> index 8318a26bc263b..dfaee2c3624e0 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c

[ ... ]

> @@ -266,3 +257,80 @@ u16 mlx5e_select_queue(struct net_device *dev, struct sk_buff *skb,
>  
>  	return txq_ix + up * selq->num_channels;
>  }
> +
> +static bool mlx5e_sk_sticky_txq(const struct sock *sk)
> +{
> +	return IS_ENABLED(CONFIG_MLX5_EN_TLS) && sk && sk_fullsock(sk) &&
> +	       sock_flag(sk, SOCK_STICKY_TXQ);
> +}
> +
> +/* TCP sets skb->ooo_okay when it transmits the skb, and it may be stale by
> + * now, e.g. after an upper device's qdisc reordered the socket's skbs.
> + * Allow a move only if no other skb of the socket is in flight: a
> + * decrypted skb holds its own sk_wmem_alloc charge, any other skb may not.
> + */
> +static bool mlx5e_sk_txq_may_move(const struct sk_buff *skb)
> +{
> +	int own = skb_is_decrypted(skb) ? skb->truesize : 0;
> +
> +	return skb->ooo_okay && sk_wmem_alloc_get(skb->sk) <= own;
> +}

[Severity: Medium]
Should mlx5e_sk_txq_may_move() check sk_wmem_alloc_get(skb->sk) < own +
SKB_TRUESIZE(1) to match __tcp_transmit_skb() and sk_tx_queue_pinned()?

When TCP transmits from tcp_tsq_workfn(), tcp_wfree() retains a +1
reference in sk->sk_wmem_alloc until sk_free(sk) runs after
tcp_tsq_handler(sk), and in-flight pure ACKs charge truesize == 2 via
skb_set_tcp_pure_ack() without carrying TLS data.  __tcp_transmit_skb()
therefore sets skb->ooo_okay = 1 when sk_wmem_alloc_get(sk) <
SKB_TRUESIZE(1), raising sk_wmem_alloc_get(sk) to at least own + 1 after
charging skb->truesize.  Because sk_wmem_alloc_get(skb->sk) <= own is then
false, a socket transmitting via TSQ or with a pure ACK in flight cannot
move to its updated UP/DSCP or HTB queue even when no data packet is in
flight (and in the follow-up commit "net/mlx5e: kTLS, Keep a TX connection
on one SQ at a time", mlx5e_ktls_tx_own_txq() drops the skb if queue
selection falls back to __mlx5e_select_queue() during tcp_tsq_workfn()).

> +
> +/* A SOCK_STICKY_TXQ socket must not move to another TX queue while it has
> + * packets in flight, see TLSDEV_F_STICKY_TXQ. netdev_pick_tx() keeps the
> + * socket's queue, but the UP offset and the HTB queue are derived from each
> + * skb. Reuse the final queue of the socket's previous skb, unless TCP allows
> + * a move.
> + */
> +static int mlx5e_select_sticky_queue(struct net_device *dev,
> +				     struct sk_buff *skb,
> +				     struct mlx5e_selq_params *selq)
> +{
> +	int txq_ix;
> +
> +	if (mlx5e_sk_txq_may_move(skb))
> +		return -1;
> +
> +	txq_ix = sk_tx_queue_get(skb->sk);
> +	if (txq_ix < 0 || txq_ix >= dev->real_num_tx_queues)
> +		return -1;
> +
> +	/* The PTP queues are selected for PTP event packets only. */
> +	if (selq->is_ptp && txq_ix >= selq->num_regular_queues &&
> +	    txq_ix < selq->num_regular_queues + selq->num_tcs)
> +		return -1;
> +
> +	return txq_ix;
> +}

[Severity: Medium]
Can mlx5e_select_sticky_queue() reuse an HTB queue index after that queue
was reassigned to another HTB class?

When an HTB leaf is deleted or converted into an inner class in
mlx5e_htb_leaf_to_inner(), mlx5e_htb_leaf_del(), or
mlx5e_htb_leaf_del_last(), its QoS qid is reused for another class while
txq_ix stays below dev->real_num_tx_queues and sk->sk_tx_queue_mapping is
not cleared.  If the first packet sent after the change is a non-decrypted
TCP packet such as a pure ACK, __tcp_transmit_skb() charges skb->truesize
to sk->sk_wmem_alloc while own is 0, so mlx5e_sk_txq_may_move() returns
false, mlx5e_select_sticky_queue() returns the reassigned txq_ix, and
subsequent TLS records sent while that ACK is in flight see ooo_okay == 0
and stay pinned to the wrong HTB class.

In addition, htb_delete() and htb_change_class() invoke the driver offload
callback before qdisc_purge_queue() runs (while mlx5e_htb_leaf_to_inner()
does not call mlx5e_reset_qdisc(), and mlx5e_htb_leaf_del() passes the
0-based moved_qid instead of mlx5e_qid_from_qos() to mlx5e_reset_qdisc()),
and skbs that already passed __tcp_transmit_skb() with ooo_okay == 0 and
are queued in an upper device's qdisc also keep sk->sk_wmem_alloc elevated
and reuse the reassigned txq_ix when they reach mlx5e_select_sticky_queue().

> +
> +u16 mlx5e_select_queue(struct net_device *dev, struct sk_buff *skb,
> +		       struct net_device *sb_dev)
> +{
> +	struct mlx5e_priv *priv = netdev_priv(dev);
> +	struct mlx5e_selq_params *selq;
> +	struct sock *sk = skb->sk;
> +	int txq_ix;
> +
> +	selq = rcu_dereference_bh(priv->selq.active);
> +
> +	/* This is a workaround needed only for the mlx5e_netdev_change_profile
> +	 * flow that zeroes out the whole priv without unregistering the netdev
> +	 * and without preventing ndo_select_queue from being called.
> +	 */
> +	if (unlikely(!selq))
> +		return 0;
> +
> +	if (likely(!mlx5e_sk_sticky_txq(sk)))
> +		return __mlx5e_select_queue(dev, skb, selq);

[Severity: Medium]
Does calling netdev_pick_tx() inside __mlx5e_select_queue() leave an
unadjusted queue index in sk->sk_tx_queue_mapping when SOCK_STICKY_TXQ is
later enabled?

Before setsockopt(SOL_TLS, TLS_TX) sets SOCK_STICKY_TXQ in
tls_set_device_offload(), mlx5e_sk_sticky_txq(sk) is false and
__mlx5e_select_queue() is called without updating sk->sk_tx_queue_mapping
afterwards.  Inside __mlx5e_select_queue(), netdev_pick_tx() caches its
raw queue index in sk->sk_tx_queue_mapping (for example, a TC 0 queue in
[0, num_channels) when DSCP trust or VLAN priority maps the packet to a
higher UP, or an un-normalized HTB queue index before
mlx5e_txq_to_ch_ix_htb()) while __mlx5e_select_queue() returns the UP- or
HTB-adjusted queue.  If a pre-offload packet such as the TLS handshake
Finished message is still in flight when the first offloaded record is
sent (so ooo_okay == 0), mlx5e_select_sticky_queue() returns that raw
cached queue from sk->sk_tx_queue_mapping, moving the flow across TX
queues while packets are in flight and pinning it to the wrong TC or
queue.  Even after SOCK_STICKY_TXQ is set, netdev_pick_tx() transiently
writes the unadjusted queue into sk->sk_tx_queue_mapping before
mlx5e_select_queue() overwrites it with txq_ix.

> +
> +	txq_ix = mlx5e_select_sticky_queue(dev, skb, selq);
> +	if (txq_ix >= 0)
> +		return txq_ix;
> +
> +	txq_ix = __mlx5e_select_queue(dev, skb, selq);
> +	/* Cache the final queue, as netdev_pick_tx() does with its own pick. */
> +	if (rcu_access_pointer(sk->sk_dst_cache))
> +		sk_tx_queue_set(sk, txq_ix);

[Severity: Medium]
Can gating sk_tx_queue_set(sk, txq_ix) on sk->sk_dst_cache leave a stale
queue in sk->sk_tx_queue_mapping after a queue move?

If skb1 is transmitted with ooo_okay == 1 and no other packets in flight,
__tcp_transmit_skb() charges skb1->truesize to sk->sk_wmem_alloc before
skb1 is routed and queued in an upper device's qdisc or neigh->arp_queue.
If sk_dst_reset() or __sk_dst_check() runs before skb1 reaches
mlx5e_select_queue(), sk_tx_queue_maybe_clear() sees
sk_tx_queue_pinned(sk) == true and preserves the old queue in
sk->sk_tx_queue_mapping while clearing sk->sk_dst_cache.  When skb1 then
runs __mlx5e_select_queue() and selects a new queue txq_ix,
rcu_access_pointer(sk->sk_dst_cache) is NULL, so sk_tx_queue_set(sk,
txq_ix) is skipped.  Any subsequent packet sent while skb1 is still in
flight then reuses the old queue via mlx5e_select_sticky_queue(), putting
in-flight kTLS WQEs on two SQs at once (or causing
mlx5e_ktls_handle_tx_skb() to warn and drop the packet at the end of the
series).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109114734.2087878-1-tariqt@nvidia.com?part=6

[RFC net-next 7/7] net/mlx5e: kTLS, Keep a TX connection on one SQ at a time

[Tariq Toukan]

A kTLS TX connection must not post WQEs to another SQ while WQEs that it
posted to its SQ may be pending: both SQs would update its TIS state
with no ordering, corrupting the records on the wire. The stack and the
driver keep the TX queue of such a connection stable
(TLSDEV_F_STICKY_TXQ), but some paths bypass or defeat that: tc skbedit
queue_mapping skips queue selection, sk_tx_queue_mapping is shared with
the other devices on the route, and ooo_okay can be stale after an
upper device's qdisc reordered the socket's skbs.

Enforce it where the WQEs are posted. Each connection records the TX
queue that it posts on. An skb on another TX queue moves the connection
only if no other skb of its socket is in flight. An skb is freed only
once the WQEs posted for it completed, or were destroyed with their SQ,
so the connection then has no WQE pending on any SQ. An SQ that
replaces another on the same TX queue was drained or destroyed first.
Otherwise the skb is dropped before anything is posted, and counted in
a new tx[i]_tls_drop_txq_move counter. TCP retransmits it.

The move is made under the old TX queue's lock, taken with a trylock,
under which the connection's skbs are posted there, so it cannot race
with them. A connection that stays on its TX queue only compares the
queue index.

A move that the TX queue selection allows, when the socket has nothing
else in flight, is made at selection. Otherwise the socket's next skbs,
queued behind the first one on the new TX queue, would hold
sk_wmem_alloc when it reaches the driver, and it would be dropped,
although nothing of the connection is pending on the old SQ.

Tested on ConnectX-6 Dx, with 128 channels and 32 kTLS TX offloaded
connections streaming pattern data for 60 seconds to a software kTLS
receiver that checks every byte, while an egress tc skbedit
queue_mapping filter flips them between two TX queues every 20 ms.
Without this patch, 23 of 55 connections failed decryption. With it,
none did, and 7515 skbs were dropped over 2035 flips. Without the flips,
no skb was dropped.

Suggested-by: Rishikesh Jethwani <rjethwani@everpuredata.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../ethernet/mellanox/mlx5/counters.rst       |  7 ++
 .../net/ethernet/mellanox/mlx5/core/en/selq.c | 16 +++--
 .../mellanox/mlx5/core/en_accel/ktls_tx.c     | 71 +++++++++++++++++++
 .../mellanox/mlx5/core/en_accel/ktls_txrx.h   |  6 ++
 .../ethernet/mellanox/mlx5/core/en_stats.c    |  4 ++
 .../ethernet/mellanox/mlx5/core/en_stats.h    |  2 +
 6 files changed, 100 insertions(+), 6 deletions(-)

diff --git a/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst b/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst
index cc498895f92e..0a08ffddc834 100644
--- a/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst
+++ b/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst
@@ -711,6 +711,13 @@ the software port.
        data.
      - Acceleration
 
+   * - `tx[i]_tls_drop_txq_move`
+     - Number of TLS packets dropped on ring i because their connection still
+       had packets in flight on another ring. Posting them on ring i would
+       break the ordering of the connection's TLS state updates in the
+       device. TCP retransmits the dropped data.
+     - Error
+
    * - `ptp_cq[i]_abort`
      - Number of times a CQE has to be skipped in precision time protocol due to
        a skew between the port timestamp and CQE timestamp being greater than
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c b/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
index dfaee2c3624e..0bac217bbfec 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
@@ -9,6 +9,7 @@
 #include "en.h"
 #include "en/ptp.h"
 #include "en/htb.h"
+#include "en_accel/ktls_txrx.h"
 
 struct mlx5e_selq_params {
 	unsigned int num_regular_queues;
@@ -288,9 +289,6 @@ static int mlx5e_select_sticky_queue(struct net_device *dev,
 {
 	int txq_ix;
 
-	if (mlx5e_sk_txq_may_move(skb))
-		return -1;
-
 	txq_ix = sk_tx_queue_get(skb->sk);
 	if (txq_ix < 0 || txq_ix >= dev->real_num_tx_queues)
 		return -1;
@@ -309,6 +307,7 @@ u16 mlx5e_select_queue(struct net_device *dev, struct sk_buff *skb,
 	struct mlx5e_priv *priv = netdev_priv(dev);
 	struct mlx5e_selq_params *selq;
 	struct sock *sk = skb->sk;
+	bool may_move;
 	int txq_ix;
 
 	selq = rcu_dereference_bh(priv->selq.active);
@@ -323,14 +322,19 @@ u16 mlx5e_select_queue(struct net_device *dev, struct sk_buff *skb,
 	if (likely(!mlx5e_sk_sticky_txq(sk)))
 		return __mlx5e_select_queue(dev, skb, selq);
 
-	txq_ix = mlx5e_select_sticky_queue(dev, skb, selq);
-	if (txq_ix >= 0)
-		return txq_ix;
+	may_move = mlx5e_sk_txq_may_move(skb);
+	if (!may_move) {
+		txq_ix = mlx5e_select_sticky_queue(dev, skb, selq);
+		if (txq_ix >= 0)
+			return txq_ix;
+	}
 
 	txq_ix = __mlx5e_select_queue(dev, skb, selq);
 	/* Cache the final queue, as netdev_pick_tx() does with its own pick. */
 	if (rcu_access_pointer(sk->sk_dst_cache))
 		sk_tx_queue_set(sk, txq_ix);
+	if (may_move)
+		mlx5e_ktls_tx_select_txq(dev, skb, txq_ix);
 
 	return txq_ix;
 }
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 0bd4d961e704..9603ef4b3eb3 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
@@ -92,12 +92,15 @@ enum {
 	MLX5E_KTLS_TX_RESYNC, /* WQEs lost, see mlx5e_ktls_tx_handle_lost() */
 };
 
+#define MLX5E_KTLS_TX_NO_TXQ (-1)
+
 struct mlx5e_ktls_offload_context_tx {
 	/* fast path */
 	u32 expected_seq;
 	u32 tisn;
 	bool ctx_post_pending;
 	unsigned long flags; /* MLX5E_KTLS_TX_* */
+	int txq_ix; /* that it posts on, see mlx5e_ktls_tx_own_txq() */
 	/* control / resync */
 	struct list_head list_node; /* member of the pool */
 	union mlx5e_crypto_info crypto_info;
@@ -505,6 +508,7 @@ int mlx5e_ktls_add_tx(struct net_device *netdev, struct sock *sk,
 
 	priv_tx->ctx_post_pending = true;
 	priv_tx->flags = 0;
+	priv_tx->txq_ix = MLX5E_KTLS_TX_NO_TXQ;
 	atomic64_inc(&priv_tx->sw_stats->tx_tls_ctx);
 
 	return 0;
@@ -846,6 +850,59 @@ void mlx5e_ktls_tx_handle_lost(struct mlx5e_txqsq *sq, struct sk_buff *skb)
 	rcu_read_unlock();
 }
 
+/* A connection must not post WQEs to another SQ while WQEs that it posted to
+ * its SQ may be pending, see TLSDEV_F_STICKY_TXQ. Let it move to the TX queue
+ * of @sq only if no other skb of its socket is in flight: an skb is freed only
+ * once the WQEs posted for it are done. The move is made under the lock of the
+ * old TX queue, under which the connection posts there.
+ */
+static bool mlx5e_ktls_tx_own_txq(struct net_device *netdev,
+				  struct mlx5e_ktls_offload_context_tx *priv_tx,
+				  struct mlx5e_txqsq *sq, struct sk_buff *skb)
+{
+	int old = READ_ONCE(priv_tx->txq_ix);
+	struct netdev_queue *old_txq;
+	bool own;
+
+	if (old == MLX5E_KTLS_TX_NO_TXQ)
+		return cmpxchg(&priv_tx->txq_ix, old, sq->txq_ix) == old;
+
+	old_txq = netdev_get_tx_queue(netdev, old);
+	if (!__netif_tx_trylock(old_txq))
+		return false;
+
+	own = READ_ONCE(priv_tx->txq_ix) == old &&
+	      sk_wmem_alloc_get(skb->sk) <= skb->truesize;
+	if (own)
+		WRITE_ONCE(priv_tx->txq_ix, sq->txq_ix);
+	__netif_tx_unlock(old_txq);
+
+	return own;
+}
+
+/* The TX queue selection lets the socket of @skb move to @txq_ix only while
+ * no other skb of the socket is in flight, see mlx5e_select_queue(), so the
+ * connection has no WQE pending on any SQ: hand it the new TX queue now. Once
+ * @skb is queued there, the socket's next skbs queue up behind it and hold
+ * sk_wmem_alloc, and mlx5e_ktls_tx_own_txq() would refuse the move.
+ */
+void mlx5e_ktls_tx_select_txq(struct net_device *netdev, struct sk_buff *skb,
+			      u16 txq_ix)
+{
+	struct mlx5e_ktls_offload_context_tx *priv_tx;
+	struct tls_context *tls_ctx;
+
+	if (!tls_is_skb_tx_device_offloaded(skb))
+		return;
+
+	tls_ctx = tls_get_ctx(skb->sk);
+	if (rcu_dereference_bh(tls_ctx->netdev) != netdev)
+		return;
+
+	priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);
+	WRITE_ONCE(priv_tx->txq_ix, txq_ix);
+}
+
 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)
@@ -881,6 +938,10 @@ 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(READ_ONCE(priv_tx->txq_ix) != sq->txq_ix) &&
+	    !mlx5e_ktls_tx_own_txq(netdev, priv_tx, sq, skb))
+		goto err_move;
+
 	/* WQEs naming the TIS, pending context params included, are
 	 * posted only for an skb that uses it.
 	 */
@@ -929,6 +990,16 @@ bool mlx5e_ktls_handle_tx_skb(struct net_device *netdev, struct mlx5e_txqsq *sq,
 
 	return true;
 
+err_move:
+	/* Nothing was posted for the skb, TCP retransmits it. */
+	stats->tls_drop_txq_move++;
+	netdev_warn_once(netdev,
+			 "kTLS TX connection moved to TX queue %d with packets in flight, dropping\n",
+			 sq->txq_ix);
+	dev_kfree_skb_any(skb);
+	mlx5e_tx_flush(sq, NULL);
+	return false;
+
 err_lost:
 	/* Resync params may have moved the TIS state to the dropped skb. */
 	set_bit(MLX5E_KTLS_TX_RESYNC, &priv_tx->flags);
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 a6b15dcd32b8..acfd1b4025aa 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
@@ -30,6 +30,8 @@ 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);
+void mlx5e_ktls_tx_select_txq(struct net_device *netdev, struct sk_buff *skb,
+			      u16 txq_ix);
 
 /* The WQEs posted for the skb may have not been executed. */
 static inline void
@@ -84,6 +86,10 @@ 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 void
+mlx5e_ktls_tx_select_txq(struct net_device *netdev, struct sk_buff *skb,
+			 u16 txq_ix) {}
+
 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_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
index cd94bb44f6ab..d23790403714 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
@@ -132,6 +132,7 @@ static const struct counter_desc sw_stats_desc[] = {
 	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, tx_tls_skip_no_sync_data) },
 	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, tx_tls_drop_no_sync_data) },
 	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, tx_tls_drop_bypass_req) },
+	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, tx_tls_drop_txq_move) },
 #endif
 
 	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_lro_packets) },
@@ -441,6 +442,7 @@ static void mlx5e_stats_grp_sw_update_stats_sq(struct mlx5e_sw_stats *s,
 	s->tx_tls_skip_no_sync_data += sq_stats->tls_skip_no_sync_data;
 	s->tx_tls_drop_no_sync_data += sq_stats->tls_drop_no_sync_data;
 	s->tx_tls_drop_bypass_req   += sq_stats->tls_drop_bypass_req;
+	s->tx_tls_drop_txq_move += sq_stats->tls_drop_txq_move;
 #endif
 	s->tx_cqes                  += sq_stats->cqes;
 }
@@ -2303,6 +2305,7 @@ static const struct counter_desc sq_stats_desc[] = {
 	{ MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, tls_skip_no_sync_data) },
 	{ MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, tls_drop_no_sync_data) },
 	{ MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, tls_drop_bypass_req) },
+	{ MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, tls_drop_txq_move) },
 #endif
 	{ MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, csum_none) },
 	{ MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, stopped) },
@@ -2456,6 +2459,7 @@ static const struct counter_desc qos_sq_stats_desc[] = {
 	{ MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, tls_skip_no_sync_data) },
 	{ MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, tls_drop_no_sync_data) },
 	{ MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, tls_drop_bypass_req) },
+	{ MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, tls_drop_txq_move) },
 #endif
 	{ MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, csum_none) },
 	{ MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, stopped) },
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
index 09f155acb461..8f5fb0478f00 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
@@ -237,6 +237,7 @@ struct mlx5e_sw_stats {
 	u64 tx_tls_skip_no_sync_data;
 	u64 tx_tls_drop_no_sync_data;
 	u64 tx_tls_drop_bypass_req;
+	u64 tx_tls_drop_txq_move;
 
 	u64 rx_tls_decrypted_packets;
 	u64 rx_tls_decrypted_bytes;
@@ -434,6 +435,7 @@ struct mlx5e_sq_stats {
 	u64 tls_skip_no_sync_data;
 	u64 tls_drop_no_sync_data;
 	u64 tls_drop_bypass_req;
+	u64 tls_drop_txq_move;
 #endif
 	/* less likely accessed in data path */
 	u64 csum_none;
-- 
2.44.0

Re: [RFC net-next 7/7] net/mlx5e: kTLS, Keep a TX connection on one SQ at a time

[Rishikesh Jethwani]

On Mon, Nov 9, 2026 at 3:50 AM Tariq Toukan <tariqt@nvidia.com> wrote:
>
> A kTLS TX connection must not post WQEs to another SQ while WQEs that it
> posted to its SQ may be pending: both SQs would update its TIS state
> with no ordering, corrupting the records on the wire. The stack and the
> driver keep the TX queue of such a connection stable
> (TLSDEV_F_STICKY_TXQ), but some paths bypass or defeat that: tc skbedit
> queue_mapping skips queue selection, sk_tx_queue_mapping is shared with
> the other devices on the route, and ooo_okay can be stale after an
> upper device's qdisc reordered the socket's skbs.
>
> Enforce it where the WQEs are posted. Each connection records the TX
> queue that it posts on. An skb on another TX queue moves the connection
> only if no other skb of its socket is in flight. An skb is freed only
> once the WQEs posted for it completed, or were destroyed with their SQ,
> so the connection then has no WQE pending on any SQ. An SQ that
> replaces another on the same TX queue was drained or destroyed first.
> Otherwise the skb is dropped before anything is posted, and counted in
> a new tx[i]_tls_drop_txq_move counter. TCP retransmits it.
>
> The move is made under the old TX queue's lock, taken with a trylock,
> under which the connection's skbs are posted there, so it cannot race
> with them. A connection that stays on its TX queue only compares the
> queue index.
>
> A move that the TX queue selection allows, when the socket has nothing
> else in flight, is made at selection. Otherwise the socket's next skbs,
> queued behind the first one on the new TX queue, would hold
> sk_wmem_alloc when it reaches the driver, and it would be dropped,
> although nothing of the connection is pending on the old SQ.
>
> Tested on ConnectX-6 Dx, with 128 channels and 32 kTLS TX offloaded
> connections streaming pattern data for 60 seconds to a software kTLS
> receiver that checks every byte, while an egress tc skbedit
> queue_mapping filter flips them between two TX queues every 20 ms.
> Without this patch, 23 of 55 connections failed decryption. With it,
> none did, and 7515 skbs were dropped over 2035 flips. Without the flips,
> no skb was dropped.
>
> Suggested-by: Rishikesh Jethwani <rjethwani@everpuredata.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ---
>  .../ethernet/mellanox/mlx5/counters.rst       |  7 ++
>  .../net/ethernet/mellanox/mlx5/core/en/selq.c | 16 +++--
>  .../mellanox/mlx5/core/en_accel/ktls_tx.c     | 71 +++++++++++++++++++
>  .../mellanox/mlx5/core/en_accel/ktls_txrx.h   |  6 ++
>  .../ethernet/mellanox/mlx5/core/en_stats.c    |  4 ++
>  .../ethernet/mellanox/mlx5/core/en_stats.h    |  2 +
>  6 files changed, 100 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst b/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst
> index cc498895f92e..0a08ffddc834 100644
> --- a/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst
> +++ b/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst
> @@ -711,6 +711,13 @@ the software port.
>         data.
>       - Acceleration
>
> +   * - `tx[i]_tls_drop_txq_move`
> +     - Number of TLS packets dropped on ring i because their connection still
> +       had packets in flight on another ring. Posting them on ring i would
> +       break the ordering of the connection's TLS state updates in the
> +       device. TCP retransmits the dropped data.
> +     - Error
> +
>     * - `ptp_cq[i]_abort`
>       - Number of times a CQE has to be skipped in precision time protocol due to
>         a skew between the port timestamp and CQE timestamp being greater than
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c b/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
> index dfaee2c3624e..0bac217bbfec 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
> @@ -9,6 +9,7 @@
>  #include "en.h"
>  #include "en/ptp.h"
>  #include "en/htb.h"
> +#include "en_accel/ktls_txrx.h"
>
>  struct mlx5e_selq_params {
>         unsigned int num_regular_queues;
> @@ -288,9 +289,6 @@ static int mlx5e_select_sticky_queue(struct net_device *dev,
>  {
>         int txq_ix;
>
> -       if (mlx5e_sk_txq_may_move(skb))
> -               return -1;
> -
>         txq_ix = sk_tx_queue_get(skb->sk);
>         if (txq_ix < 0 || txq_ix >= dev->real_num_tx_queues)
>                 return -1;
> @@ -309,6 +307,7 @@ u16 mlx5e_select_queue(struct net_device *dev, struct sk_buff *skb,
>         struct mlx5e_priv *priv = netdev_priv(dev);
>         struct mlx5e_selq_params *selq;
>         struct sock *sk = skb->sk;
> +       bool may_move;
>         int txq_ix;
>
>         selq = rcu_dereference_bh(priv->selq.active);
> @@ -323,14 +322,19 @@ u16 mlx5e_select_queue(struct net_device *dev, struct sk_buff *skb,
>         if (likely(!mlx5e_sk_sticky_txq(sk)))
>                 return __mlx5e_select_queue(dev, skb, selq);
>
> -       txq_ix = mlx5e_select_sticky_queue(dev, skb, selq);
> -       if (txq_ix >= 0)
> -               return txq_ix;
> +       may_move = mlx5e_sk_txq_may_move(skb);
> +       if (!may_move) {
> +               txq_ix = mlx5e_select_sticky_queue(dev, skb, selq);
> +               if (txq_ix >= 0)
> +                       return txq_ix;
> +       }
>
>         txq_ix = __mlx5e_select_queue(dev, skb, selq);
>         /* Cache the final queue, as netdev_pick_tx() does with its own pick. */
>         if (rcu_access_pointer(sk->sk_dst_cache))
>                 sk_tx_queue_set(sk, txq_ix);
> +       if (may_move)
> +               mlx5e_ktls_tx_select_txq(dev, skb, txq_ix);
>
>         return txq_ix;
>  }
> 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 0bd4d961e704..9603ef4b3eb3 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
> @@ -92,12 +92,15 @@ enum {
>         MLX5E_KTLS_TX_RESYNC, /* WQEs lost, see mlx5e_ktls_tx_handle_lost() */
>  };
>
> +#define MLX5E_KTLS_TX_NO_TXQ (-1)
> +
>  struct mlx5e_ktls_offload_context_tx {
>         /* fast path */
>         u32 expected_seq;
>         u32 tisn;
>         bool ctx_post_pending;
>         unsigned long flags; /* MLX5E_KTLS_TX_* */
> +       int txq_ix; /* that it posts on, see mlx5e_ktls_tx_own_txq() */
>         /* control / resync */
>         struct list_head list_node; /* member of the pool */
>         union mlx5e_crypto_info crypto_info;
> @@ -505,6 +508,7 @@ int mlx5e_ktls_add_tx(struct net_device *netdev, struct sock *sk,
>
>         priv_tx->ctx_post_pending = true;
>         priv_tx->flags = 0;
> +       priv_tx->txq_ix = MLX5E_KTLS_TX_NO_TXQ;
>         atomic64_inc(&priv_tx->sw_stats->tx_tls_ctx);
>
>         return 0;
> @@ -846,6 +850,59 @@ void mlx5e_ktls_tx_handle_lost(struct mlx5e_txqsq *sq, struct sk_buff *skb)
>         rcu_read_unlock();
>  }
>
> +/* A connection must not post WQEs to another SQ while WQEs that it posted to
> + * its SQ may be pending, see TLSDEV_F_STICKY_TXQ. Let it move to the TX queue
> + * of @sq only if no other skb of its socket is in flight: an skb is freed only
> + * once the WQEs posted for it are done. The move is made under the lock of the
> + * old TX queue, under which the connection posts there.
> + */
> +static bool mlx5e_ktls_tx_own_txq(struct net_device *netdev,
> +                                 struct mlx5e_ktls_offload_context_tx *priv_tx,
> +                                 struct mlx5e_txqsq *sq, struct sk_buff *skb)
> +{
> +       int old = READ_ONCE(priv_tx->txq_ix);
> +       struct netdev_queue *old_txq;
> +       bool own;
> +
> +       if (old == MLX5E_KTLS_TX_NO_TXQ)
> +               return cmpxchg(&priv_tx->txq_ix, old, sq->txq_ix) == old;
> +
> +       old_txq = netdev_get_tx_queue(netdev, old);
> +       if (!__netif_tx_trylock(old_txq))
> +               return false;
> +
> +       own = READ_ONCE(priv_tx->txq_ix) == old &&
> +             sk_wmem_alloc_get(skb->sk) <= skb->truesize;

This serializes the ownership check and update with posting on the old
queue. But selection bypasses it:

> +       if (own)
> +               WRITE_ONCE(priv_tx->txq_ix, sq->txq_ix);
> +       __netif_tx_unlock(old_txq);
> +
> +       return own;
> +}
> +
> +/* The TX queue selection lets the socket of @skb move to @txq_ix only while
> + * no other skb of the socket is in flight, see mlx5e_select_queue(), so the
> + * connection has no WQE pending on any SQ: hand it the new TX queue now. Once
> + * @skb is queued there, the socket's next skbs queue up behind it and hold
> + * sk_wmem_alloc, and mlx5e_ktls_tx_own_txq() would refuse the move.
> + */
> +void mlx5e_ktls_tx_select_txq(struct net_device *netdev, struct sk_buff *skb,
> +                             u16 txq_ix)
> +{
> +       struct mlx5e_ktls_offload_context_tx *priv_tx;
> +       struct tls_context *tls_ctx;
> +
> +       if (!tls_is_skb_tx_device_offloaded(skb))
> +               return;
> +
> +       tls_ctx = tls_get_ctx(skb->sk);
> +       if (rcu_dereference_bh(tls_ctx->netdev) != netdev)
> +               return;
> +
> +       priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);
> +       WRITE_ONCE(priv_tx->txq_ix, txq_ix);
> +}
> +

This write relies on an earlier may_move check in mlx5e_select_queue(),
without locking the old queue or rechecking wmem.

With independent bond child qdiscs and skbedit selecting different upper
TX queues for A and B:

  1. A is dequeued asynchronously and reaches the lower mlx5e selector.
     Only A holds wmem, so may_move=true.
  2. TCP emits B through another child qdisc. B sees both charges,
     reuses cached lower SQ0 and posts; its WQEs remain pending.
  3. A resumes, picks SQ1 and overwrites the owner. The xmit guard sees
     a matching index, so A posts on SQ1 while B's WQEs still reference
     the same TIS from SQ0.

A can be dequeued via a qdisc watchdog outside the socket lock. Lower
selection precedes the lower TX lock. Both packets use the socket's
offload slave. The same race exists when the owner is unclaimed and B
claims it first.

B must select and post between A's may_move check and owner write in
one mlx5e_select_queue() call. Hard IRQs, vCPU descheduling and
PREEMPT_RT preemption of BH-disabled sections can extend that short
window while B runs on another CPU.

Selection needs the same initial-claim CAS and locked handoff: recheck
the owner and current wmem under the old queue's lock, and leave
ownership unchanged on failure. Locking only the write leaves the wmem
check stale. Preserve zero self-charge for non-decrypted, possibly
partially orphaned skbs.

A failed transfer can fall back to the owner's queue, updating the
cache, if the index is in [0, real_num_tx_queues), still maps to a live
SQ and is not a PTP index. Keep the xmit ownership guard.

>  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)
> @@ -881,6 +938,10 @@ 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(READ_ONCE(priv_tx->txq_ix) != sq->txq_ix) &&
> +           !mlx5e_ktls_tx_own_txq(netdev, priv_tx, sq, skb))
> +               goto err_move;
> +
>         /* WQEs naming the TIS, pending context params included, are
>          * posted only for an skb that uses it.
>          */
> @@ -929,6 +990,16 @@ bool mlx5e_ktls_handle_tx_skb(struct net_device *netdev, struct mlx5e_txqsq *sq,
>
>         return true;
>
> +err_move:
> +       /* Nothing was posted for the skb, TCP retransmits it. */
> +       stats->tls_drop_txq_move++;
> +       netdev_warn_once(netdev,
> +                        "kTLS TX connection moved to TX queue %d with packets in flight, dropping\n",
> +                        sq->txq_ix);
> +       dev_kfree_skb_any(skb);
> +       mlx5e_tx_flush(sq, NULL);
> +       return false;
> +
>  err_lost:
>         /* Resync params may have moved the TIS state to the dropped skb. */
>         set_bit(MLX5E_KTLS_TX_RESYNC, &priv_tx->flags);
> 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 a6b15dcd32b8..acfd1b4025aa 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
> @@ -30,6 +30,8 @@ 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);
> +void mlx5e_ktls_tx_select_txq(struct net_device *netdev, struct sk_buff *skb,
> +                             u16 txq_ix);
>
>  /* The WQEs posted for the skb may have not been executed. */
>  static inline void
> @@ -84,6 +86,10 @@ 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 void
> +mlx5e_ktls_tx_select_txq(struct net_device *netdev, struct sk_buff *skb,
> +                        u16 txq_ix) {}
> +
>  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_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> index cd94bb44f6ab..d23790403714 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> @@ -132,6 +132,7 @@ static const struct counter_desc sw_stats_desc[] = {
>         { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, tx_tls_skip_no_sync_data) },
>         { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, tx_tls_drop_no_sync_data) },
>         { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, tx_tls_drop_bypass_req) },
> +       { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, tx_tls_drop_txq_move) },
>  #endif
>
>         { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_lro_packets) },
> @@ -441,6 +442,7 @@ static void mlx5e_stats_grp_sw_update_stats_sq(struct mlx5e_sw_stats *s,
>         s->tx_tls_skip_no_sync_data += sq_stats->tls_skip_no_sync_data;
>         s->tx_tls_drop_no_sync_data += sq_stats->tls_drop_no_sync_data;
>         s->tx_tls_drop_bypass_req   += sq_stats->tls_drop_bypass_req;
> +       s->tx_tls_drop_txq_move += sq_stats->tls_drop_txq_move;
>  #endif
>         s->tx_cqes                  += sq_stats->cqes;
>  }
> @@ -2303,6 +2305,7 @@ static const struct counter_desc sq_stats_desc[] = {
>         { MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, tls_skip_no_sync_data) },
>         { MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, tls_drop_no_sync_data) },
>         { MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, tls_drop_bypass_req) },
> +       { MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, tls_drop_txq_move) },
>  #endif
>         { MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, csum_none) },
>         { MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, stopped) },
> @@ -2456,6 +2459,7 @@ static const struct counter_desc qos_sq_stats_desc[] = {
>         { MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, tls_skip_no_sync_data) },
>         { MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, tls_drop_no_sync_data) },
>         { MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, tls_drop_bypass_req) },
> +       { MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, tls_drop_txq_move) },
>  #endif
>         { MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, csum_none) },
>         { MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, stopped) },
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> index 09f155acb461..8f5fb0478f00 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> @@ -237,6 +237,7 @@ struct mlx5e_sw_stats {
>         u64 tx_tls_skip_no_sync_data;
>         u64 tx_tls_drop_no_sync_data;
>         u64 tx_tls_drop_bypass_req;
> +       u64 tx_tls_drop_txq_move;
>
>         u64 rx_tls_decrypted_packets;
>         u64 rx_tls_decrypted_bytes;
> @@ -434,6 +435,7 @@ struct mlx5e_sq_stats {
>         u64 tls_skip_no_sync_data;
>         u64 tls_drop_no_sync_data;
>         u64 tls_drop_bypass_req;
> +       u64 tls_drop_txq_move;
>  #endif
>         /* less likely accessed in data path */
>         u64 csum_none;
> --
> 2.44.0
>

Re: [RFC net-next 7/7] net/mlx5e: kTLS, Keep a TX connection on one SQ at a time

[Tariq Toukan]

On 10/11/2026 0:15, Rishikesh Jethwani wrote:
 > This write relies on an earlier may_move check in mlx5e_select_queue(),
 > without locking the old queue or rechecking wmem.

Right, I assumed the socket lock serializes selection, which doesn't
hold with an upper device's qdisc in between.

 > Selection needs the same initial-claim CAS and locked handoff: recheck
 > the owner and current wmem under the old queue's lock, and leave
 > ownership unchanged on failure. Locking only the write leaves the wmem
 > check stale. Preserve zero self-charge for non-decrypted, possibly
 > partially orphaned skbs.
 >
 > A failed transfer can fall back to the owner's queue, updating the
 > cache, if the index is in [0, real_num_tx_queues), still maps to a live
 > SQ and is not a PTP index. Keep the xmit ownership guard.

Agreed, v2 does that. Selection and xmit move the connection through
one helper, as you describe, and a failed move falls back to the
owner's TX queue under those conditions. The xmit guard stays.

Thanks,
Tariq

Re: [RFC net-next 7/7] net/mlx5e: kTLS, Keep a TX connection on one SQ at a time

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] net/mlx5e: unlocked txq_ix update in mlx5e_ktls_tx_select_txq()
- [Medium] net/mlx5e: self-trylock failure in mlx5e_ktls_tx_own_txq() on
  txq_ix re-read
- [Medium] net/mlx5e: missed mlx5e_ktls_tx_select_txq() when sticky
  queue is invalid
- [Medium] net/mlx5e: mlx5e_ktls_tx_own_txq() drops software-segmented
  GSO packets
- [Medium] net/mlx5e: kTLS in-flight wmem checks ignore tcp_tsq_workfn()
  +1 reference
--

commit d7dc36218f27e50a58cf06b01d4aeb04325f18d6
Author: Tariq Toukan <tariqt@nvidia.com>

net/mlx5e: kTLS, Keep a TX connection on one SQ at a time

This commit tracks the TX queue index on each kTLS TX offload context and
restricts moving the connection to another TX queue while packets of the
socket are still in flight, dropping packets that would otherwise post WQEs
concurrently to multiple SQs.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c b/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
> index dfaee2c3624e0..0bac217bbfec8 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/selq.c
[ ... ]
> @@ -323,14 +322,19 @@ u16 mlx5e_select_queue(struct net_device *dev, struct sk_buff *skb,
>  	if (likely(!mlx5e_sk_sticky_txq(sk)))
>  		return __mlx5e_select_queue(dev, skb, selq);
>  
> -	txq_ix = mlx5e_select_sticky_queue(dev, skb, selq);
> -	if (txq_ix >= 0)
> -		return txq_ix;
> +	may_move = mlx5e_sk_txq_may_move(skb);
> +	if (!may_move) {
> +		txq_ix = mlx5e_select_sticky_queue(dev, skb, selq);
> +		if (txq_ix >= 0)
> +			return txq_ix;
> +	}
>  
>  	txq_ix = __mlx5e_select_queue(dev, skb, selq);
>  	/* Cache the final queue, as netdev_pick_tx() does with its own pick. */
>  	if (rcu_access_pointer(sk->sk_dst_cache))
>  		sk_tx_queue_set(sk, txq_ix);
> +	if (may_move)
> +		mlx5e_ktls_tx_select_txq(dev, skb, txq_ix);
>  
>  	return txq_ix;
>  }

[Severity: Medium]
Does skipping mlx5e_ktls_tx_select_txq() when !may_move cause multi-packet
bursts to be dropped after mlx5e_select_sticky_queue() rejects a stale queue?

When skb->ooo_okay is 0 (for example, because an earlier skb was still
completing when __tcp_transmit_skb() built this skb), may_move is false.  If
mlx5e_select_sticky_queue() returns -1 because the cached queue index is out
of range after reducing the channel count with ethtool -L,
mlx5e_select_queue() picks a new txq_ix and caches it via sk_tx_queue_set(),
but leaves priv_tx->txq_ix pointing at the old queue even if all prior skbs
have now completed (sk_wmem_alloc_get(sk) <= own).

Subsequent skbs in the same burst then take the newly cached queue in
mlx5e_select_sticky_queue() and queue up behind the first skb, raising
sk_wmem_alloc above skb->truesize.  When the first skb reaches
mlx5e_ktls_handle_tx_skb(), mlx5e_ktls_tx_own_txq() sees
sk_wmem_alloc_get(skb->sk) > skb->truesize and drops every skb in the burst
except the last one, triggering TCP retransmissions and a hardware kTLS OOO
resync.

> 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 0bd4d961e7044..9603ef4b3eb3f 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
[ ... ]
> @@ -846,6 +850,59 @@ void mlx5e_ktls_tx_handle_lost(struct mlx5e_txqsq *sq, struct sk_buff *skb)
>  	rcu_read_unlock();
>  }
>  
> +/* A connection must not post WQEs to another SQ while WQEs that it posted to
> + * its SQ may be pending, see TLSDEV_F_STICKY_TXQ. Let it move to the TX queue
> + * of @sq only if no other skb of its socket is in flight: an skb is freed only
> + * once the WQEs posted for it are done. The move is made under the lock of the
> + * old TX queue, under which the connection posts there.
> + */
> +static bool mlx5e_ktls_tx_own_txq(struct net_device *netdev,
> +				  struct mlx5e_ktls_offload_context_tx *priv_tx,
> +				  struct mlx5e_txqsq *sq, struct sk_buff *skb)
> +{
> +	int old = READ_ONCE(priv_tx->txq_ix);
> +	struct netdev_queue *old_txq;
> +	bool own;
> +
> +	if (old == MLX5E_KTLS_TX_NO_TXQ)
> +		return cmpxchg(&priv_tx->txq_ix, old, sq->txq_ix) == old;
> +
> +	old_txq = netdev_get_tx_queue(netdev, old);
> +	if (!__netif_tx_trylock(old_txq))
> +		return false;

[Severity: Medium]
Can re-reading priv_tx->txq_ix into old without checking old == sq->txq_ix
cause mlx5e_ktls_tx_own_txq() to trylock the current TX queue and drop the
packet?

mlx5e_ktls_handle_tx_skb() runs under HARD_TX_LOCK for sq->txq and calls
mlx5e_ktls_tx_own_txq() after observing READ_ONCE(priv_tx->txq_ix) !=
sq->txq_ix.  If another CPU updates priv_tx->txq_ix to sq->txq_ix (for
example via mlx5e_ktls_tx_select_txq()) before the second READ_ONCE() here,
old equals sq->txq_ix.  __netif_tx_trylock(old_txq) then tries to acquire
sq->txq->_xmit_lock, which is already held by this CPU, so it fails and drops
the skb with a false warning.

> +
> +	own = READ_ONCE(priv_tx->txq_ix) == old &&
> +	      sk_wmem_alloc_get(skb->sk) <= skb->truesize;

[Severity: Medium]
Does checking sk_wmem_alloc_get(skb->sk) <= skb->truesize drop the first N-1
segments when a GSO skb is software-segmented before mlx5e_xmit()?

When validate_xmit_skb() segments a GSO skb in software via
__skb_gso_segment() -> tcp_gso_segment(), all N resulting segments are
charged to sk->sk_wmem_alloc with destructor tcp_wfree before
dev_hard_start_xmit() loops over the segment list.  If the connection moves
to a new TX queue at xmit time (such as via tc skbedit queue_mapping),
sk_wmem_alloc_get(skb->sk) holds the sum of truesizes of all remaining
segments, so mlx5e_ktls_tx_own_txq() rejects and frees segments 1 through N-1
one by one until only segment N remains, transmitting segment N out of order
and forcing a kTLS resync.

> +	if (own)
> +		WRITE_ONCE(priv_tx->txq_ix, sq->txq_ix);
> +	__netif_tx_unlock(old_txq);
> +
> +	return own;
> +}

[Severity: Medium]
Do the sk_wmem_alloc_get(skb->sk) checks in mlx5e_ktls_tx_own_txq() and
mlx5e_sk_txq_may_move() fail for packets sent from the TCP Small Queues
tasklet?

When tcp_wfree() schedules the TSQ worker, it leaves a +1 reference in
sk->sk_wmem_alloc for sk_free() to release after tcp_tsq_handler() returns:

net/ipv4/tcp_output.c:tcp_tsq_workfn() {
    ...
			tcp_tsq_handler(sk);
		}
		bh_unlock_sock(sk);

		if (need_free)
			sk_free(sk);
    ...
}

Inside tcp_tsq_handler(), __tcp_transmit_skb() charges skb->truesize while
that +1 reference is still present, so sk_wmem_alloc_get(skb->sk) is
skb->truesize + 1 even when no other skb is in flight (which is why
__tcp_transmit_skb() and sk_tx_queue_pinned() test against SKB_TRUESIZE(1)
rather than 0).  As a result, both sk_wmem_alloc_get(skb->sk) <= own in
mlx5e_sk_txq_may_move() and sk_wmem_alloc_get(skb->sk) <= skb->truesize in
mlx5e_ktls_tx_own_txq() evaluate to false during TSQ transmissions, blocking
queue moves at selection time and dropping packets moved to another TX queue.

> +
> +/* The TX queue selection lets the socket of @skb move to @txq_ix only while
> + * no other skb of the socket is in flight, see mlx5e_select_queue(), so the
> + * connection has no WQE pending on any SQ: hand it the new TX queue now. Once
> + * @skb is queued there, the socket's next skbs queue up behind it and hold
> + * sk_wmem_alloc, and mlx5e_ktls_tx_own_txq() would refuse the move.
> + */
> +void mlx5e_ktls_tx_select_txq(struct net_device *netdev, struct sk_buff *skb,
> +			      u16 txq_ix)
> +{
> +	struct mlx5e_ktls_offload_context_tx *priv_tx;
> +	struct tls_context *tls_ctx;
> +
> +	if (!tls_is_skb_tx_device_offloaded(skb))
> +		return;
> +
> +	tls_ctx = tls_get_ctx(skb->sk);
> +	if (rcu_dereference_bh(tls_ctx->netdev) != netdev)
> +		return;
> +
> +	priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);
> +	WRITE_ONCE(priv_tx->txq_ix, txq_ix);
> +}

[Severity: High]
Can this unlocked WRITE_ONCE(priv_tx->txq_ix, txq_ix) race with a packet
transmitting on the old TX queue and allow both SQs to post WQEs for the same
kTLS connection concurrently?

In mlx5e_select_queue(), may_move = mlx5e_sk_txq_may_move(skb) is checked
before __mlx5e_select_queue() and sk_tx_queue_set() without holding any TX
queue lock, and mlx5e_ktls_tx_select_txq() then unconditionally overwrites
priv_tx->txq_ix without locking old_txq or re-checking sk_wmem_alloc_get():

CPU0                                    CPU1
mlx5e_select_queue(skb1)
  may_move = mlx5e_sk_txq_may_move(skb1)
    /* true: wmem <= truesize */
                                        mlx5e_select_queue(skb2)
                                          /* !may_move: picks sticky old_q */
                                        mlx5e_xmit(skb2) [on old_sq]
                                          HARD_TX_LOCK(old_txq)
                                          mlx5e_ktls_handle_tx_skb(skb2)
                                            priv_tx->txq_ix == old_sq->txq_ix
                                            /* starts posting WQEs on old_sq */
  txq_ix = __mlx5e_select_queue(...)
  sk_tx_queue_set(sk, new_q)
  mlx5e_ktls_tx_select_txq(skb1, new_q)
    WRITE_ONCE(priv_tx->txq_ix, new_q)
mlx5e_xmit(skb1) [on new_sq]
  HARD_TX_LOCK(new_txq)
  mlx5e_ktls_handle_tx_skb(skb1)
    priv_tx->txq_ix == new_sq->txq_ix
    /* skips mlx5e_ktls_tx_own_txq(),
     * posts WQEs on new_sq concurrently! */

This can happen when skb1 is delayed in an upper device's qdisc (such as a
bonding or VLAN device where the socket lock is no longer held), or when skb1
is a non-decrypted packet (e.g. a pure ACK or pre-offload packet) whose
sk_wmem_alloc was released by skb_orphan_partial() in an upper qdisc, making
mlx5e_sk_txq_may_move() see own == 0 and sk_wmem_alloc == 0 before CPU1 sends
encrypted data on old_sq.  Both SQs then update the connection's TIS state
and priv_tx fields (expected_seq, ctx_post_pending, rec_seq) without
serialization, corrupting TLS records on the wire.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109114734.2087878-1-tariqt@nvidia.com?part=7