[PATCH net v3 0/6] net: mana: HW channel reliability and hardening fixes

[PATCH net v3 0/6] net: mana: HW channel reliability and hardening fixes

[Long Li]

This series fixes a set of latent bugs and robustness gaps in the MANA
Hardware Channel (HWC), the control path the driver uses to talk to the
device.  The issues range from a use-after-free of completion queues
during teardown to buffer mis-sizing, unsafe teardown ordering, missing
validation of device-supplied RX metadata, and stale-response handling
after a command timeout.

Patch overview:

  1  RCU-protect gc->cq_table lookups against concurrent CQ destroy
     The EQ interrupt handler dereferences CQ pointers from gc->cq_table
     while teardown can free them.  Put the table under RCU and wait a
     grace period before freeing, closing the use-after-free.

  2  fix HWC RQ/SQ buffer size swap
     init_queues() sized the RQ with max_req_msg_size and the SQ with
     max_resp_msg_size -- backwards.  A large response could overflow the
     RQ buffer and the RX slot-index divide used the wrong stride.  Also
     store the queue dimensions before creating the CQ so the RX handler
     never sees an uninitialised divisor.

  3  free HWC comp_buf after destroying the EQ
     Reorder teardown so the CQ/EQ are torn down (readers quiesced)
     before comp_buf is freed, preventing a late EQ-handler access to
     freed memory.

  4  validate hardware-supplied values in the HWC RX path
     Bounds-check the SGE, verify the recovered slot index and SGE
     address, and validate response length and msg_id before use, so
     malformed or hostile DMA metadata cannot cause wrong-slot completion
     or out-of-bounds access.

  5  fix HWC teardown safety with setup_active flag and destroy ordering
     Track setup activation explicitly, tear the CQ down before the
     TXQ/RXQ, and on an unrecoverable teardown failure leak the HWC
     resources rather than free memory the device may still DMA into.

  6  fix stale HWC response after command timeout
     Replace the inflight-slot semaphore with a bitmap + waitqueue and
     per-slot refcount/lock; latch the channel on timeout so no new slots
     are handed out, drop duplicate/late responses, and ignore a zero
     firmware-supplied timeout.

Follow-up feature work (net-next, sent separately)
--------------------------------------------------
The original series also contained two patches that are improvements, not
fixes:

  net: mana: support concurrent HWC requests
  net: mana: add dynamic HWC queue depth with reinit path

Per the netdev tree rules, fixes go to 'net' and features/improvements go
to 'net-next', and the two must not be combined in a single submission.
Those two patches build on the locking and teardown groundwork in this
series, so they will be posted as a separate net-next series only after
these fixes have propagated from net into net-next through the usual
periodic merge.

Changes since v2
----------------
 - Per maintainer feedback, split the original combined series: the six
   fixes here target 'net'; the two feature patches now go to 'net-next'
   and are sent separately (see above).  Rebased the fixes onto net.
 - Dropped the pcie_flr()-based reset fallback from the teardown path.
   pcie_flr() resets device config without the save/restore that
   pci_reset_function() provides, and cannot be used as a drop-in here.
   On an unrecoverable teardown failure the driver now leaks the HWC
   resources instead of touching memory the device may still DMA into.
 - patch 2: store the HWC queue dimensions before creating the CQ so the
   RX completion handler can never observe a zero max_resp_msg_size
   divisor or a stale num_inflight_msg bound.
 - Assorted commit-message and comment clarifications.

Long Li (6):
  net: mana: RCU-protect gc->cq_table lookups against concurrent CQ
    destroy
  net: mana: fix HWC RQ/SQ buffer size swap
  net: mana: free HWC comp_buf after destroying the EQ
  net: mana: validate hardware-supplied values in the HWC RX path
  net: mana: fix HWC teardown safety with setup_active flag and destroy
    ordering
  net: mana: fix stale HWC response after command timeout

 drivers/infiniband/hw/mana/cq.c               |  46 +-
 .../net/ethernet/microsoft/mana/gdma_main.c   |  25 +-
 .../net/ethernet/microsoft/mana/hw_channel.c  | 402 +++++++++++++++---
 drivers/net/ethernet/microsoft/mana/mana_en.c |  22 +-
 include/net/mana/gdma.h                       |  29 +-
 include/net/mana/hw_channel.h                 |  35 +-
 6 files changed, 475 insertions(+), 84 deletions(-)


base-commit: af39eb111ce6b5eba9c08513b62c4868eb7e7fd5
-- 
2.43.0

[PATCH net v3 1/6] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy

[Long Li]

The EQ interrupt handler (mana_gd_process_eqe) looks up the completing CQ
in gc->cq_table[cq_id] and runs its callback, concurrently with CQ
teardown on another CPU that clears the slot and frees the CQ.  cq_table
was a plain pointer array freed with no grace period, so the two race
into a use-after-free:

  CPU A (mana_gd_intr, hard IRQ)        CPU B (CQ destroy)
  ----------------------------------    ------------------------------
  cq = gc->cq_table[cq_id];  // valid
                                        gc->cq_table[id] = NULL;
                                        kfree(cq);          // freed
  cq->cq.callback(ctx, cq);  // use-after-free

The handler's existing rcu_read_lock() only guards the per-IRQ EQ list
traversal; cq_table was never under any RCU contract, and a read-side
lock is inert unless the freer also defers the free past a grace period.

Put cq_table under RCU: annotate the base pointer and entries __rcu, read
with rcu_dereference() in the handler, publish with rcu_assign_pointer(),
and on teardown clear the slot then synchronize_rcu() before freeing the
CQ.  The grace period blocks until every in-flight handler has dropped
the old pointer, so the kfree() can no longer race the callback.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/infiniband/hw/mana/cq.c               | 46 ++++++++++++++++---
 .../net/ethernet/microsoft/mana/gdma_main.c   | 25 ++++++++--
 .../net/ethernet/microsoft/mana/hw_channel.c  | 29 ++++++++----
 drivers/net/ethernet/microsoft/mana/mana_en.c | 22 +++++++--
 include/net/mana/gdma.h                       | 21 ++++++++-
 5 files changed, 119 insertions(+), 24 deletions(-)

diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
index f2547989f422..2bf4be21cede 100644
--- a/drivers/infiniband/hw/mana/cq.c
+++ b/drivers/infiniband/hw/mana/cq.c
@@ -131,12 +131,20 @@ static void mana_ib_cq_handler(void *ctx, struct gdma_queue *gdma_cq)
 int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
 {
 	struct gdma_context *gc = mdev_to_gc(mdev);
+	struct gdma_queue __rcu **cq_table;
 	struct gdma_queue *gdma_cq;
 
-	if (cq->queue.id >= gc->max_num_cqs)
+	/* No rcu_read_lock(): install/remove run within the IB device
+	 * lifetime, which mana_rdma_remove() (ib_unregister_device) drains
+	 * before the base cq_table can be freed.  See gdma_context::cq_table
+	 * in gdma.h for why "true" is sound.
+	 */
+	cq_table = rcu_dereference_protected(gc->cq_table, true);
+	if (!cq_table || cq->queue.id >= gc->max_num_cqs)
 		return -EINVAL;
+
 	/* Create CQ table entry, sharing a CQ between WQs is not supported */
-	if (gc->cq_table[cq->queue.id])
+	if (rcu_access_pointer(cq_table[cq->queue.id]))
 		return -EINVAL;
 	if (cq->queue.kmem)
 		gdma_cq = cq->queue.kmem;
@@ -149,23 +157,49 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
 	gdma_cq->type = GDMA_CQ;
 	gdma_cq->cq.callback = mana_ib_cq_handler;
 	gdma_cq->id = cq->queue.id;
-	gc->cq_table[cq->queue.id] = gdma_cq;
+	rcu_assign_pointer(cq_table[cq->queue.id], gdma_cq);
 	return 0;
 }
 
 void mana_ib_remove_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
 {
 	struct gdma_context *gc = mdev_to_gc(mdev);
+	struct gdma_queue __rcu **cq_table;
+	struct gdma_queue *gdma_cq;
 
-	if (cq->queue.id >= gc->max_num_cqs || cq->queue.id == INVALID_QUEUE_ID)
+	if (cq->queue.id == INVALID_QUEUE_ID)
 		return;
 
 	if (cq->queue.kmem)
 	/* Then it will be cleaned and removed by the mana */
 		return;
 
-	kfree(gc->cq_table[cq->queue.id]);
-	gc->cq_table[cq->queue.id] = NULL;
+	/* No rcu_read_lock(): like mana_ib_install_cq_cb(), this runs within
+	 * the IB device lifetime that mana_rdma_remove() drains before the
+	 * base cq_table can be freed.  See gdma_context::cq_table in gdma.h.
+	 */
+	cq_table = rcu_dereference_protected(gc->cq_table, true);
+	if (!cq_table || cq->queue.id >= gc->max_num_cqs)
+		return;
+	/* Removers for a given CQ are serialized by the IB core, so the slot
+	 * is read and cleared without rcu_read_lock() or atomicity: a CQ is
+	 * never torn down while a live QP references it (cq->usecnt), nor
+	 * while the QP-create that installed the entry is still running (that
+	 * create holds a reference on the CQ uobject across its error path,
+	 * before usecnt is taken).  Any double-remove is therefore sequential
+	 * -- the later caller sees the NULL stored below and returns.
+	 */
+	gdma_cq = rcu_dereference_protected(cq_table[cq->queue.id], true);
+	if (!gdma_cq)
+		return;  /* already removed by a prior teardown path */
+
+	rcu_assign_pointer(cq_table[cq->queue.id], NULL);
+
+	/* Wait for in-flight EQ handlers that may have loaded the old
+	 * pointer via rcu_dereference() to finish before freeing.
+	 */
+	synchronize_rcu();
+	kfree(gdma_cq);
 }
 
 int mana_ib_arm_cq(struct ib_cq *ibcq, enum ib_cq_notify_flags flags)
diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index e8b7ffb47eb9..fa6fcc2475c1 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -732,6 +732,7 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
 	union gdma_eqe_info eqe_info;
 	enum gdma_eqe_type type;
 	struct gdma_event event;
+	struct gdma_queue __rcu **cq_table;
 	struct gdma_queue *cq;
 	struct gdma_eqe *eqe;
 	u32 cq_id;
@@ -743,10 +744,11 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
 	switch (type) {
 	case GDMA_EQE_COMPLETION:
 		cq_id = eqe->details[0] & 0xFFFFFF;
-		if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
+		cq_table = rcu_dereference(gc->cq_table);
+		if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs || !cq_table))
 			break;
 
-		cq = gc->cq_table[cq_id];
+		cq = rcu_dereference(cq_table[cq_id]);
 		if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id))
 			break;
 
@@ -1053,15 +1055,28 @@ static void mana_gd_create_cq(const struct gdma_queue_spec *spec,
 static void mana_gd_destroy_cq(struct gdma_context *gc,
 			       struct gdma_queue *queue)
 {
+	struct gdma_queue __rcu **cq_table;
 	u32 id = queue->id;
 
-	if (id >= gc->max_num_cqs)
+	/* No rcu_read_lock() here: mana_gd_destroy_cq() runs only on the
+	 * CQ-destroy/teardown path, where the base cq_table is stable.  See
+	 * the lifecycle note on gdma_context::cq_table in gdma.h for why the
+	 * "true" predicate is sound.
+	 */
+	cq_table = rcu_dereference_protected(gc->cq_table, true);
+	if (!cq_table || id >= gc->max_num_cqs)
 		return;
 
-	if (!gc->cq_table[id])
+	if (!rcu_access_pointer(cq_table[id]))
 		return;
 
-	gc->cq_table[id] = NULL;
+	rcu_assign_pointer(cq_table[id], NULL);
+
+	/* Wait for in-flight EQ handlers that may have loaded the old
+	 * pointer via rcu_dereference() to finish before the caller
+	 * frees the CQ memory.
+	 */
+	synchronize_rcu();
 }
 
 int mana_gd_create_hwc_queue(struct gdma_dev *gd,
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index e3c24d50dad0..409e20caeccd 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -674,6 +674,7 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 	struct gdma_queue *sq = hwc->txq->gdma_wq;
 	struct gdma_queue *eq = hwc->cq->gdma_eq;
 	struct gdma_queue *cq = hwc->cq->gdma_cq;
+	struct gdma_queue __rcu **cq_table;
 	int err;
 
 	init_completion(&hwc->hwc_init_eqe_comp);
@@ -698,11 +699,15 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 	if (WARN_ON(cq->id >= gc->max_num_cqs))
 		return -EPROTO;
 
-	gc->cq_table = vcalloc(gc->max_num_cqs, sizeof(struct gdma_queue *));
-	if (!gc->cq_table)
+	cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
+	if (!cq_table)
 		return -ENOMEM;
 
-	gc->cq_table[cq->id] = cq;
+	rcu_assign_pointer(cq_table[cq->id], cq);
+	/* Publish the fully-initialised table last; pairs with the
+	 * rcu_dereference(gc->cq_table) in mana_gd_process_eqe().
+	 */
+	rcu_assign_pointer(gc->cq_table, cq_table);
 
 	return 0;
 }
@@ -811,6 +816,7 @@ int mana_hwc_create_channel(struct gdma_context *gc)
 void mana_hwc_destroy_channel(struct gdma_context *gc)
 {
 	struct hw_channel_context *hwc = gc->hwc.driver_data;
+	struct gdma_queue __rcu **old_cq_table;
 
 	if (!hwc)
 		return;
@@ -818,10 +824,8 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	/* gc->max_num_cqs is set in mana_hwc_init_event_handler(). If it's
 	 * non-zero, the HWC worked and we should tear down the HWC here.
 	 */
-	if (gc->max_num_cqs > 0) {
+	if (gc->max_num_cqs > 0)
 		mana_smc_teardown_hwc(&gc->shm_channel, false);
-		gc->max_num_cqs = 0;
-	}
 
 	if (hwc->txq)
 		mana_hwc_destroy_wq(hwc, hwc->txq);
@@ -832,6 +836,14 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	if (hwc->cq)
 		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
 
+	/* Reset only after mana_hwc_destroy_cq() above has run with a valid
+	 * max_num_cqs so mana_gd_destroy_cq() clears the CQ table slot and
+	 * waits out in-flight EQ handlers (synchronize_rcu) before the CQ is
+	 * freed.  Clearing it earlier would make that path early-return and
+	 * skip the slot clear, leaving a dangling cq_table entry.
+	 */
+	gc->max_num_cqs = 0;
+
 	kfree(hwc->caller_ctx);
 	hwc->caller_ctx = NULL;
 
@@ -848,8 +860,9 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	gc->hwc.driver_data = NULL;
 	gc->hwc.gdma_context = NULL;
 
-	vfree(gc->cq_table);
-	gc->cq_table = NULL;
+	old_cq_table = rcu_replace_pointer(gc->cq_table, NULL, true);
+	synchronize_rcu();
+	vfree(old_cq_table);
 }
 
 int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 92bb55935c1c..65b025e8f211 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -2496,6 +2496,7 @@ static int mana_create_txq(struct mana_port_context *apc,
 	struct mana_obj_spec cq_spec;
 	struct gdma_queue_spec spec;
 	struct gdma_context *gc;
+	struct gdma_queue __rcu **cq_table;
 	struct mana_txq *txq;
 	struct mana_cq *cq;
 	u32 txq_size;
@@ -2596,12 +2597,18 @@ static int mana_create_txq(struct mana_port_context *apc,
 
 		cq->gdma_id = cq->gdma_cq->id;
 
-		if (WARN_ON(cq->gdma_id >= gc->max_num_cqs)) {
+		/* No rcu_read_lock(): mana_create_txq runs under RTNL during
+		 * netdev bring-up, inside the netdev lifetime that
+		 * mana_remove() drains before the base cq_table can be freed.
+		 * See gdma_context::cq_table in gdma.h for why "true" is sound.
+		 */
+		cq_table = rcu_dereference_protected(gc->cq_table, true);
+		if (WARN_ON(!cq_table || cq->gdma_id >= gc->max_num_cqs)) {
 			err = -EINVAL;
 			goto out;
 		}
 
-		gc->cq_table[cq->gdma_id] = cq->gdma_cq;
+		rcu_assign_pointer(cq_table[cq->gdma_id], cq->gdma_cq);
 
 		mana_create_txq_debugfs(apc, i);
 
@@ -2821,6 +2828,7 @@ static struct mana_rxq *mana_create_rxq(struct mana_port_context *apc,
 	struct gdma_queue_spec spec;
 	struct mana_cq *cq = NULL;
 	struct gdma_context *gc;
+	struct gdma_queue __rcu **cq_table;
 	u32 cq_size, rq_size;
 	struct mana_rxq *rxq;
 	int err;
@@ -2905,12 +2913,18 @@ static struct mana_rxq *mana_create_rxq(struct mana_port_context *apc,
 	if (err)
 		goto out;
 
-	if (WARN_ON(cq->gdma_id >= gc->max_num_cqs)) {
+	/* No rcu_read_lock(): mana_create_rxq runs under RTNL during netdev
+	 * bring-up, inside the netdev lifetime that mana_remove() drains
+	 * before the base cq_table can be freed.  See gdma_context::cq_table
+	 * in gdma.h for why "true" is sound.
+	 */
+	cq_table = rcu_dereference_protected(gc->cq_table, true);
+	if (WARN_ON(!cq_table || cq->gdma_id >= gc->max_num_cqs)) {
 		err = -EINVAL;
 		goto out;
 	}
 
-	gc->cq_table[cq->gdma_id] = cq->gdma_cq;
+	rcu_assign_pointer(cq_table[cq->gdma_id], cq->gdma_cq);
 
 	netif_napi_add_weight_locked(ndev, &cq->napi, mana_poll, 1);
 
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 0c395917b214..0b48ded0aecd 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -418,7 +418,26 @@ struct gdma_context {
 
 	/* This maps a CQ index to the queue structure. */
 	unsigned int		max_num_cqs;
-	struct gdma_queue	**cq_table;
+	/* Both the base pointer and each entry are RCU-managed.  The fast
+	 * path (mana_gd_process_eqe) reads the base via rcu_dereference()
+	 * under rcu_read_lock(), so the table is freed with
+	 * rcu_assign_pointer(NULL) + synchronize_rcu() and an in-flight
+	 * reader can never observe freed memory.
+	 *
+	 * The slow paths -- mana_gd_destroy_cq() and the CQ install/remove
+	 * callers (mana_create_txq/_rxq, mana_ib_install/remove_cq_cb) --
+	 * instead read the base with rcu_dereference_protected(cq_table,
+	 * true).  The bare "true" is justified by teardown ordering, not by
+	 * a lock: the base table is replaced+freed only by
+	 * mana_hwc_destroy_channel() (and the create-time reinit), and every
+	 * teardown path first runs mana_remove() + mana_rdma_remove(), which
+	 * synchronously drain the netdev and the IB device
+	 * (unregister_netdevice / ib_unregister_device) that bound all
+	 * install/remove callers; the reinit case runs before either
+	 * consumer is probed.  So no slow-path caller can run while the base
+	 * table is being freed.
+	 */
+	struct gdma_queue	__rcu * __rcu *cq_table;
 
 	/* Protect eq_test_event and test_event_eq_id  */
 	struct mutex		eq_test_event_mutex;
-- 
2.43.0

[PATCH net v3 2/6] net: mana: fix HWC RQ/SQ buffer size swap

[Long Li]

The HWC RQ receives responses and the SQ sends requests, but
mana_hwc_init_queues() sized the RQ with max_req_msg_size and the SQ with
max_resp_msg_size -- backwards.  A response larger than the undersized RQ
buffer could overflow it, and mana_hwc_rx_event_handler() recovered the
RX slot index by dividing by the wrong size (max_req_msg_size).

Size the RQ by max_resp_msg_size and the SQ by max_req_msg_size, store
max_resp_msg_size in hw_channel_context, and use it as the RX slot
stride.

Store the queue dimensions before creating the CQ, which registers the
RX completion handler: that handler divides by max_resp_msg_size and
range-checks num_inflight_msg, so both must be set before it can run.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
 .../net/ethernet/microsoft/mana/hw_channel.c   | 18 ++++++++++++------
 include/net/mana/hw_channel.h                  |  1 +
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 409e20caeccd..cbb56c764787 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -263,7 +263,7 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
 
 	/* Select the RX work request for virtual address and for reposting. */
 	rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
-	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_req_msg_size;
+	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size;
 
 	if (rx_req_idx >= hwc_rxq->msg_buf->num_reqs) {
 		dev_err(hwc->dev, "HWC RX: wrong rx_req_idx=%llu, num_reqs=%u\n",
@@ -721,6 +721,15 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
 	if (err)
 		return err;
 
+	/* Set the queue dimensions before creating the CQ: doing so
+	 * registers mana_hwc_rx_event_handler(), which divides by
+	 * hwc->max_resp_msg_size and range-checks hwc->num_inflight_msg.
+	 * They must be valid before any RX completion can be delivered.
+	 */
+	hwc->num_inflight_msg = q_depth;
+	hwc->max_req_msg_size = max_req_msg_size;
+	hwc->max_resp_msg_size = max_resp_msg_size;
+
 	/* CQ is shared by SQ and RQ, so CQ's queue depth is the sum of SQ
 	 * queue depth and RQ queue depth.
 	 */
@@ -733,23 +742,20 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
 		goto out;
 	}
 
-	err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_req_msg_size,
+	err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_resp_msg_size,
 				 hwc->cq, &hwc->rxq);
 	if (err) {
 		dev_err(hwc->dev, "Failed to create HWC RQ: %d\n", err);
 		goto out;
 	}
 
-	err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_resp_msg_size,
+	err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_req_msg_size,
 				 hwc->cq, &hwc->txq);
 	if (err) {
 		dev_err(hwc->dev, "Failed to create HWC SQ: %d\n", err);
 		goto out;
 	}
 
-	hwc->num_inflight_msg = q_depth;
-	hwc->max_req_msg_size = max_req_msg_size;
-
 	return 0;
 out:
 	/* mana_hwc_create_channel() will do the cleanup.*/
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 16feb39616c1..73671f479399 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -181,6 +181,7 @@ struct hw_channel_context {
 
 	u16 num_inflight_msg;
 	u32 max_req_msg_size;
+	u32 max_resp_msg_size;
 
 	u16 hwc_init_q_depth_max;
 	u32 hwc_init_max_req_msg_size;
-- 
2.43.0

[PATCH net v3 3/6] net: mana: free HWC comp_buf after destroying the EQ

[Long Li]

mana_hwc_destroy_cq() freed hwc_cq->comp_buf before destroying the CQ and
EQ.  comp_buf is dereferenced by mana_hwc_comp_event(), which the EQ
interrupt handler invokes; freeing it while the EQ was still registered
let a late handler touch freed memory.

Destroy the CQ and EQ first -- the EQ teardown deregisters the IRQ and
fences in-flight handlers -- then free comp_buf and hwc_cq.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/net/ethernet/microsoft/mana/hw_channel.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index cbb56c764787..d701c427fe47 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -384,14 +384,20 @@ static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)
 
 static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
 {
-	kfree(hwc_cq->comp_buf);
-
 	if (hwc_cq->gdma_cq)
 		mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
 
+	/* comp_buf is reached only by mana_hwc_comp_event(), which the
+	 * EQ handler invokes via cq_table[id].  The CQ destroy above
+	 * already cleared that slot and ran synchronize_rcu(), so no
+	 * handler can reach comp_buf once it returns.  Destroying the EQ
+	 * here additionally tears down the IRQ (defense in depth) before
+	 * comp_buf and hwc_cq are freed below.
+	 */
 	if (hwc_cq->gdma_eq)
 		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
 
+	kfree(hwc_cq->comp_buf);
 	kfree(hwc_cq);
 }
 
-- 
2.43.0

[PATCH net v3 4/6] net: mana: validate hardware-supplied values in the HWC RX path

[Long Li]

mana_hwc_rx_event_handler() and mana_hwc_handle_resp() consumed lengths
and indices taken straight from device DMA without validation.  A buggy
firmware or a malicious host (in a confidential VM, where the DMA buffer
is shared) could drive a wrong or reused in-flight request to completion
or index out of bounds.  Validate before use:

  - bounds-check the SGE pointer located via the device-supplied
    inline_oob_size_div4 before dereferencing it, so a corrupted OOB
    size cannot push the read past the RQ buffer;
  - match the SGE address against the address the driver posted for that
    slot, not just an in-range index -- an in-range but wrong SGE would
    otherwise truncate onto a neighbouring slot and read a stale response;
  - require the response to cover a full gdma_resp_hdr before reading
    hwc_msg_id, so a short response cannot complete a slot with stale
    bytes left by the buffer's previous occupant;
  - bounds-check hwc_msg_id in mana_hwc_handle_resp() before indexing the
    inflight bitmap and caller_ctx;
  - reject a resp_len larger than the RX buffer.

Repost the RX WQE on every validation early-return so a rejected response
does not permanently shrink the posted RQ depth.  The two paths that
cannot identify the slot (an out-of-bounds SGE pointer, or an SGE address
matching no posted slot) intentionally leak a single WQE rather than risk
reposting the wrong one.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
 .../net/ethernet/microsoft/mana/hw_channel.c  | 82 ++++++++++++++++++-
 1 file changed, 78 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index d701c427fe47..e378b8ec97c9 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -83,6 +83,17 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
 	struct hwc_caller_ctx *ctx;
 	int err;
 
+	/* Validate msg_id is in range before using it to index bitmap
+	 * and caller_ctx array.  Malicious firmware could send
+	 * out-of-range msg_id causing out-of-bounds access.
+	 */
+	if (msg_id >= hwc->num_inflight_msg) {
+		dev_err(hwc->dev, "hwc_rx: msg_id %u >= max %u\n",
+			msg_id, hwc->num_inflight_msg);
+		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
+		return;
+	}
+
 	if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
 		dev_err(hwc->dev, "hwc_rx: invalid msg_id = %u\n", msg_id);
 		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
@@ -90,6 +101,18 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
 	}
 
 	ctx = hwc->caller_ctx + msg_id;
+
+	/* Reject responses larger than the RX DMA buffer — the SGE
+	 * limits what hardware can DMA, so an oversized resp_len
+	 * indicates a firmware bug.  Fail rather than silently
+	 * truncating.
+	 */
+	if (resp_len > rx_req->buf_len) {
+		dev_err(hwc->dev, "HWC RX: resp_len %u > buf_len %u\n",
+			resp_len, rx_req->buf_len);
+		resp_len = 0;
+	}
+
 	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
 	if (err)
 		goto out;
@@ -261,19 +284,69 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
 
 	sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 * 4);
 
-	/* Select the RX work request for virtual address and for reposting. */
+	/* inline_oob_size_div4 is read from the WQE in device-accessible RQ
+	 * memory, so a malicious host in a CVM (or buggy firmware) could set
+	 * it to push the SGE past the RQ buffer.  Bounds-check the SGE it
+	 * locates before dereferencing sge->address below.  A validly posted
+	 * WQE keeps the SGE inside the ring (worst case ends exactly at the
+	 * buffer boundary); reject anything that would read past it.  The
+	 * slot cannot be trusted here, so leak this RX WQE rather than repost
+	 * the wrong one -- as in the SGE-address mismatch path below.
+	 */
+	if ((u8 *)(sge + 1) > (u8 *)rq->queue_mem_ptr + rq->queue_size) {
+		dev_err(hwc->dev, "HWC RX: SGE past RQ buffer, oob_div4=%u\n",
+			dma_oob->inline_oob_size_div4);
+		return;
+	}
+
+	/* Recover the originating RX slot from the SGE address.  Of the three
+	 * terms here only sge->address lives in device-accessible RQ memory;
+	 * rq_base_addr and max_resp_msg_size are driver-private constants.  An
+	 * in-range but wrong/unaligned SGE (corrupted WQE, or a malicious host
+	 * in a CVM) would otherwise truncate onto a neighbouring slot, letting
+	 * us read a stale response that could complete the wrong, reused
+	 * in-flight request.  Require the index to be in range AND the address
+	 * to exactly match the value the driver posted for that slot.
+	 */
 	rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
 	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size;
 
-	if (rx_req_idx >= hwc_rxq->msg_buf->num_reqs) {
-		dev_err(hwc->dev, "HWC RX: wrong rx_req_idx=%llu, num_reqs=%u\n",
-			rx_req_idx, hwc_rxq->msg_buf->num_reqs);
+	if (rx_req_idx >= hwc_rxq->queue_depth) {
+		/* Cannot trust which WQE this is, so we cannot safely repost
+		 * it; leak one RX WQE and bail.  An out-of-range index means
+		 * a corrupted SGE from hardware (or host tampering), an
+		 * unrecoverable device error.
+		 */
+		dev_err(hwc->dev, "HWC RX: SGE idx %llu out of range\n",
+			rx_req_idx);
 		return;
 	}
 
 	rx_req = &hwc_rxq->msg_buf->reqs[rx_req_idx];
+	if (sge->address != (u64)rx_req->buf_sge_addr) {
+		/* In-range index but the address does not match what the
+		 * driver posted for that slot; the same unrecoverable case,
+		 * so leak this WQE rather than repost the wrong one.
+		 */
+		dev_err(hwc->dev, "HWC RX: invalid SGE address %llx (idx=%llu)\n",
+			sge->address, rx_req_idx);
+		return;
+	}
+
 	resp = (struct gdma_resp_hdr *)rx_req->buf_va;
 
+	/* Validate resp_len covers the response header before reading
+	 * hwc_msg_id.  A short response leaves stale data from the
+	 * previous buffer occupant, which could match a live slot and
+	 * complete the wrong request.
+	 */
+	if (rx_oob->tx_oob_data_size < sizeof(*resp)) {
+		dev_err(hwc->dev, "HWC RX: short resp_len=%u\n",
+			rx_oob->tx_oob_data_size);
+		mana_hwc_post_rx_wqe(hwc_rxq, rx_req);
+		return;
+	}
+
 	/* Read msg_id once from DMA buffer to prevent TOCTOU:
 	 * DMA memory is shared/unencrypted in CVMs - host can
 	 * modify it between reads.
@@ -281,6 +354,7 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
 	msg_id = READ_ONCE(resp->response.hwc_msg_id);
 	if (msg_id >= hwc->num_inflight_msg) {
 		dev_err(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
+		mana_hwc_post_rx_wqe(hwc_rxq, rx_req);
 		return;
 	}
 
-- 
2.43.0

[PATCH net v3 5/6] net: mana: fix HWC teardown safety with setup_active flag and destroy ordering

[Long Li]

Three teardown hazards let the hardware touch memory the driver freed.

First, once mana_smc_setup_hwc() succeeds the device has active MST
entries and can DMA into the HWC queue buffers.  If a later step in
mana_hwc_establish_channel() fails, the caller had no reliable way to
know teardown was required and could free those buffers while the
mappings were still live -- a DMA-after-free.  max_num_cqs was used as a
"HWC is up" proxy, but it is only set when the init EQE arrives.

Add a setup_active flag, set the moment setup_hwc activates MST entries.
establish_channel() now tears down on any later failure and clears
setup_active once teardown succeeds; destroy_channel() gates teardown on
setup_active.  max_num_cqs is no longer reset: it is an immutable bound
(see gdma.h) and cq_table == NULL is the sole teardown signal.

Second, destroy_channel() freed the TXQ/RXQ buffers while the HWC EQ was
still on the interrupt dispatch list, so an in-flight interrupt could run
the handler against freed buffers:

  CPU A (mana_gd_intr, hard IRQ)        CPU B (destroy_channel)
  ----------------------------------    ------------------------------
                                        free TXQ/RXQ DMA buffers
  handler accesses RQ/TXQ buffers       (EQ still registered)

Destroy the CQ first: mana_hwc_destroy_cq() -> mana_gd_deregister_irq()
removes the EQ via list_del_rcu() + synchronize_rcu(), after which no
handler can reach the queues; only then free the TXQ and RXQ.

Third, if mana_smc_teardown_hwc() itself fails the MST entries stay
live, yet destroy_channel() went on to free the CQ/RQ/TXQ buffers the
device can still DMA into -- a DMA-after-free on systems without an
IOMMU to fault the stale access.  Leak the HWC resources on teardown
failure instead of freeing memory the hardware can still reach, and
keep setup_active set so the failure is not mistaken for a clean
teardown.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
 .../net/ethernet/microsoft/mana/hw_channel.c  | 82 ++++++++++++++-----
 include/net/mana/gdma.h                       |  8 +-
 include/net/mana/hw_channel.h                 |  9 ++
 3 files changed, 77 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index e378b8ec97c9..1603968d7989 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -4,6 +4,7 @@
 #include <net/mana/gdma.h>
 #include <net/mana/mana.h>
 #include <net/mana/hw_channel.h>
+#include <linux/pci.h>
 #include <linux/vmalloc.h>
 
 static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
@@ -768,20 +769,33 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 	if (err)
 		return err;
 
-	if (!wait_for_completion_timeout(&hwc->hwc_init_eqe_comp, 60 * HZ))
-		return -ETIMEDOUT;
+	/* setup_hwc activated MST entries — hardware can now DMA into
+	 * our queue buffers.  If anything below fails, we must tear
+	 * down before returning so the caller doesn't need to track
+	 * whether setup_hwc succeeded.
+	 */
+	hwc->setup_active = true;
+
+	if (!wait_for_completion_timeout(&hwc->hwc_init_eqe_comp, 60 * HZ)) {
+		err = -ETIMEDOUT;
+		goto teardown;
+	}
 
 	*q_depth = hwc->hwc_init_q_depth_max;
 	*max_req_msg_size = hwc->hwc_init_max_req_msg_size;
 	*max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;
 
 	/* Both were set in mana_hwc_init_event_handler(). */
-	if (WARN_ON(cq->id >= gc->max_num_cqs))
-		return -EPROTO;
+	if (WARN_ON(cq->id >= gc->max_num_cqs)) {
+		err = -EPROTO;
+		goto teardown;
+	}
 
 	cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
-	if (!cq_table)
-		return -ENOMEM;
+	if (!cq_table) {
+		err = -ENOMEM;
+		goto teardown;
+	}
 
 	rcu_assign_pointer(cq_table[cq->id], cq);
 	/* Publish the fully-initialised table last; pairs with the
@@ -790,6 +804,16 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 	rcu_assign_pointer(gc->cq_table, cq_table);
 
 	return 0;
+
+teardown:
+	{
+		int td_err = mana_smc_teardown_hwc(&gc->shm_channel, false);
+
+		if (!td_err)
+			hwc->setup_active = false;
+
+		return td_err ? td_err : err;
+	}
 }
 
 static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
@@ -907,11 +931,38 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	if (!hwc)
 		return;
 
-	/* gc->max_num_cqs is set in mana_hwc_init_event_handler(). If it's
-	 * non-zero, the HWC worked and we should tear down the HWC here.
+	/* Tear down the HWC if setup_hwc previously activated MST entries.
+	 * This is the definitive flag — unlike max_num_cqs which depends
+	 * on the init EQE arriving.
+	 *
+	 * If teardown fails the device may still have active MST entries
+	 * and can DMA into the HWC queue buffers.  Freeing them would risk
+	 * memory corruption on systems without an IOMMU to fault the stale
+	 * DMA, so leak the HWC resources instead of handing the pages back
+	 * to the allocator.  Keep setup_active set so the failure is not
+	 * mistaken for a clean teardown.
 	 */
-	if (gc->max_num_cqs > 0)
-		mana_smc_teardown_hwc(&gc->shm_channel, false);
+	if (hwc->setup_active) {
+		int td_err = mana_smc_teardown_hwc(&gc->shm_channel, false);
+
+		if (td_err) {
+			dev_err(gc->dev,
+				"HWC teardown failed: %d, leaking resources\n",
+				td_err);
+			return;
+		}
+
+		hwc->setup_active = false;
+	}
+
+	/* Tear down the HWC CQ object first — mana_hwc_destroy_cq()
+	 * both unpublishes the CQ from cq_table (+synchronize_rcu) and
+	 * deregisters the HWC EQ from the interrupt handler list (via
+	 * mana_gd_deregister_irq + synchronize_rcu), guaranteeing no
+	 * interrupt handler can access RQ/TXQ buffers after this point.
+	 */
+	if (hwc->cq)
+		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
 
 	if (hwc->txq)
 		mana_hwc_destroy_wq(hwc, hwc->txq);
@@ -919,17 +970,6 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	if (hwc->rxq)
 		mana_hwc_destroy_wq(hwc, hwc->rxq);
 
-	if (hwc->cq)
-		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
-
-	/* Reset only after mana_hwc_destroy_cq() above has run with a valid
-	 * max_num_cqs so mana_gd_destroy_cq() clears the CQ table slot and
-	 * waits out in-flight EQ handlers (synchronize_rcu) before the CQ is
-	 * freed.  Clearing it earlier would make that path early-return and
-	 * skip the slot clear, leaving a dangling cq_table entry.
-	 */
-	gc->max_num_cqs = 0;
-
 	kfree(hwc->caller_ctx);
 	hwc->caller_ctx = NULL;
 
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 0b48ded0aecd..74eccc80cf6b 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -416,7 +416,13 @@ struct gdma_context {
 	/* L2 MTU */
 	u16 adapter_mtu;
 
-	/* This maps a CQ index to the queue structure. */
+	/* Size of cq_table, i.e. the largest valid CQ index + 1.  Set once
+	 * when cq_table is allocated and treated as immutable for the
+	 * table's lifetime (a bound only) -- it is never reset on teardown.
+	 * cq_table == NULL is the sole "table torn down" signal, so every
+	 * cq_table[id] access must guard with both !cq_table (gone) and
+	 * id >= max_num_cqs (out of bounds).
+	 */
 	unsigned int		max_num_cqs;
 	/* Both the base pointer and each entry are RCU-managed.  The fast
 	 * path (mana_gd_process_eqe) reads the base via rcu_dereference()
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 73671f479399..3d8543acb5cc 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -200,6 +200,15 @@ struct hw_channel_context {
 	u32 pf_dest_vrcq_id;
 	u32 hwc_timeout;
 
+	/* Set after mana_smc_setup_hwc() succeeds (hardware has active
+	 * MST entries).  Cleared only after mana_smc_teardown_hwc()
+	 * succeeds, on both the recoverable establish_channel path and the
+	 * terminal destroy_channel path.  If teardown fails it stays set:
+	 * establish_channel() skips its retry and destroy_channel() leaks
+	 * the HWC rather than free buffers the device may still DMA into.
+	 */
+	bool setup_active;
+
 	struct hwc_caller_ctx *caller_ctx;
 };
 
-- 
2.43.0

[PATCH net v3 6/6] net: mana: fix stale HWC response after command timeout

[Long Li]

The HWC freed a message slot (mana_hwc_put_msg_index) the instant
mana_hwc_send_request() timed out, while the hardware command was still
pending and caller_ctx.output_buf still pointed at the caller's response
buffer.  A late response then raced two ways:

  - handle_resp() runs in CQ interrupt context and memcpy()'d into
    output_buf after the sender had returned and its buffer was gone.

  - the freed slot was reused by the next request, so the stale
    response completed the wrong command with another request's data.

Give each caller_ctx a spinlock, a refcount and an -EINPROGRESS
sentinel:

  - The sender publishes output_buf under the slot lock and NULLs it
    under the same lock on timeout/exit, so handle_resp() (also under
    the lock) skips the copy once the sender is gone.

  - The slot is released only when both the sender and handle_resp()
    have dropped their reference, so a msg_id whose response is still
    outstanding is never handed to a new request.

  - A duplicate or replayed response for the same msg_id is dropped
    via a per-slot "responded" flag, so the response-side reference is
    consumed exactly once and cannot over-release the slot.

  - On a genuine timeout the channel is marked hwc_timed_out and further
    mana_hwc_get_msg_index() callers fail with -ETIMEDOUT instead of
    reusing a slot whose response may still arrive.

Replace the depth-1 semaphore with a waitqueue + bitmap so a slot held
past a timeout does not deadlock admission and timed-out waiters can be
released.

Because the timeout latch keys off wait_for_completion_timeout()
returning immediately, ignore a zero HWC_DATA_CFG_HWC_TIMEOUT reported
by the device: msecs_to_jiffies(0) would time out every command at once
and latch the whole channel.  Keep the positive default instead.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
 .../net/ethernet/microsoft/mana/hw_channel.c  | 201 +++++++++++++++---
 include/net/mana/hw_channel.h                 |  25 ++-
 2 files changed, 190 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 1603968d7989..d92032b466af 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -7,25 +7,49 @@
 #include <linux/pci.h>
 #include <linux/vmalloc.h>
 
+/* Acquire a free message slot from the inflight bitmap.  Returns
+ * -ETIMEDOUT if a prior HWC command has timed out (preserving the
+ * error code callers expect).
+ */
 static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
 {
 	struct gdma_resource *r = &hwc->inflight_msg_res;
 	unsigned long flags;
 	u32 index;
 
-	down(&hwc->sema);
+	for (;;) {
+		spin_lock_irqsave(&r->lock, flags);
 
-	spin_lock_irqsave(&r->lock, flags);
+		if (hwc->hwc_timed_out) {
+			spin_unlock_irqrestore(&r->lock, flags);
+			return -ETIMEDOUT;
+		}
 
-	index = find_first_zero_bit(hwc->inflight_msg_res.map,
-				    hwc->inflight_msg_res.size);
+		index = find_first_zero_bit(r->map, r->size);
+		if (index < r->size) {
+			struct hwc_caller_ctx *ctx;
+
+			bitmap_set(r->map, index, 1);
+			ctx = &hwc->caller_ctx[index];
+			reinit_completion(&ctx->comp_event);
+			refcount_set(&ctx->refcnt, 1);
+			ctx->responded = false;
+			ctx->msg_id = index;
+			ctx->error = -EINPROGRESS;
+			spin_unlock_irqrestore(&r->lock, flags);
+			break;
+		}
+		spin_unlock_irqrestore(&r->lock, flags);
 
-	bitmap_set(hwc->inflight_msg_res.map, index, 1);
+		wait_event(hwc->msg_waitq,
+			   hwc->hwc_timed_out ||
+			   !bitmap_full(r->map, r->size));
 
-	spin_unlock_irqrestore(&r->lock, flags);
+		if (hwc->hwc_timed_out)
+			return -ETIMEDOUT;
+	}
 
 	*msg_id = index;
-
 	return 0;
 }
 
@@ -35,10 +59,17 @@ static void mana_hwc_put_msg_index(struct hw_channel_context *hwc, u16 msg_id)
 	unsigned long flags;
 
 	spin_lock_irqsave(&r->lock, flags);
-	bitmap_clear(hwc->inflight_msg_res.map, msg_id, 1);
+	bitmap_clear(r->map, msg_id, 1);
 	spin_unlock_irqrestore(&r->lock, flags);
 
-	up(&hwc->sema);
+	wake_up(&hwc->msg_waitq);
+}
+
+static void hwc_ctx_put(struct hw_channel_context *hwc,
+			struct hwc_caller_ctx *ctx)
+{
+	if (refcount_dec_and_test(&ctx->refcnt))
+		mana_hwc_put_msg_index(hwc, ctx->msg_id);
 }
 
 static int mana_hwc_verify_resp_msg(const struct hwc_caller_ctx *caller_ctx,
@@ -114,22 +145,44 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
 		resp_len = 0;
 	}
 
-	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
-	if (err)
-		goto out;
+	spin_lock(&ctx->lock);
 
-	ctx->status_code = resp_msg->status;
+	if (ctx->responded) {
+		/* A response for this slot was already delivered; this is a
+		 * duplicate or replayed one.  Drop it so the hwc_ctx_put()
+		 * a first response performs is not done twice, which would
+		 * over-release the slot while the sender still owns it.
+		 */
+		spin_unlock(&ctx->lock);
+		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
+		return;
+	}
+	ctx->responded = true;
 
-	memcpy(ctx->output_buf, resp_msg, resp_len);
-out:
-	ctx->error = err;
+	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
 
-	/* Must post rx wqe before complete(), otherwise the next rx may
-	 * hit no_wqe error.
+	if (!err && ctx->output_buf) {
+		ctx->status_code = resp_msg->status;
+		memcpy(ctx->output_buf, resp_msg, resp_len);
+		ctx->error = 0;
+	} else if (ctx->output_buf) {
+		/* Only overwrite error if the sender hasn't timed out
+		 * or been force-completed by destroy.  When output_buf
+		 * is NULL, a terminal error (-ENODEV or timeout) has
+		 * already been set — preserve it so the sender doesn't
+		 * see a spurious success.
+		 */
+		ctx->error = err;
+	}
+
+	/* Post RX WQE before completing — the next response may arrive
+	 * immediately and needs a posted buffer.
 	 */
 	mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
-
 	complete(&ctx->comp_event);
+	spin_unlock(&ctx->lock);
+
+	hwc_ctx_put(hwc, ctx);
 }
 
 static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
@@ -216,7 +269,12 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
 
 		switch (type) {
 		case HWC_DATA_CFG_HWC_TIMEOUT:
-			hwc->hwc_timeout = val;
+			/* A zero timeout would make every command time out
+			 * immediately and latch hwc_timed_out, disabling the
+			 * channel.  Ignore it and keep the positive default.
+			 */
+			if (val)
+				hwc->hwc_timeout = val;
 			break;
 
 		case HWC_DATA_HW_LINK_CONNECT:
@@ -708,7 +766,7 @@ static int mana_hwc_init_inflight_msg(struct hw_channel_context *hwc,
 {
 	int err;
 
-	sema_init(&hwc->sema, num_msg);
+	init_waitqueue_head(&hwc->msg_waitq);
 
 	err = mana_gd_alloc_res_map(num_msg, &hwc->inflight_msg_res);
 	if (err)
@@ -738,8 +796,10 @@ static int mana_hwc_test_channel(struct hw_channel_context *hwc, u16 q_depth,
 	if (!ctx)
 		return -ENOMEM;
 
-	for (i = 0; i < q_depth; ++i)
+	for (i = 0; i < q_depth; ++i) {
+		spin_lock_init(&ctx[i].lock);
 		init_completion(&ctx[i].comp_event);
+	}
 
 	hwc->caller_ctx = ctx;
 
@@ -750,6 +810,9 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 				      u32 *max_req_msg_size,
 				      u32 *max_resp_msg_size)
 {
+	/* No RCU needed: called only from mana_hwc_create_channel
+	 * during init, before the channel is published to senders.
+	 */
 	struct hw_channel_context *hwc = gc->hwc.driver_data;
 	struct gdma_queue *rq = hwc->rxq->gdma_wq;
 	struct gdma_queue *sq = hwc->txq->gdma_wq;
@@ -999,13 +1062,17 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 	struct hwc_wq *txq = hwc->txq;
 	struct gdma_req_hdr *req_msg;
 	struct hwc_caller_ctx *ctx;
+	unsigned long flags;
 	u32 dest_vrcq = 0;
 	u32 dest_vrq = 0;
 	u32 command;
+	u32 status;
 	u16 msg_id;
 	int err;
 
-	mana_hwc_get_msg_index(hwc, &msg_id);
+	err = mana_hwc_get_msg_index(hwc, &msg_id);
+	if (err)
+		return err;
 
 	tx_wr = &txq->msg_buf->reqs[msg_id];
 
@@ -1017,8 +1084,11 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 	}
 
 	ctx = hwc->caller_ctx + msg_id;
+
+	spin_lock_irqsave(&ctx->lock, flags);
 	ctx->output_buf = resp;
 	ctx->output_buflen = resp_len;
+	spin_unlock_irqrestore(&ctx->lock, flags);
 
 	req_msg = (struct gdma_req_hdr *)tx_wr->buf_va;
 	if (req)
@@ -1034,8 +1104,14 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 		dest_vrcq = hwc->pf_dest_vrcq_id;
 	}
 
+	/* Take handle_resp's ref before posting — hardware can respond
+	 * immediately after the doorbell ring.
+	 */
+	refcount_inc(&ctx->refcnt);
+
 	err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
 	if (err) {
+		refcount_dec(&ctx->refcnt);
 		dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n", err);
 		goto out;
 	}
@@ -1046,31 +1122,86 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 			dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
 				command, hwc->hwc_timeout);
 
-		/* Reduce further waiting if HWC no response */
+		/* NULL out output_buf so a late handle_resp() won't write
+		 * into the caller's buffer after the sender returns, then
+		 * check whether handle_resp() already delivered a valid
+		 * response between the timeout firing and this lock
+		 * acquisition — ctx->error != -EINPROGRESS means it ran.
+		 */
+		spin_lock_irqsave(&ctx->lock, flags);
+		ctx->output_buf = NULL;
+		err = ctx->error;
+		status = ctx->status_code;
+		spin_unlock_irqrestore(&ctx->lock, flags);
+
+		if (err != -EINPROGRESS) {
+			/* handle_resp() delivered a valid response just after
+			 * the timeout fired.  The hardware is alive, so use
+			 * the response and leave the channel usable; do not
+			 * latch hwc_timed_out or degrade hwc_timeout for what
+			 * turned out to be a transient race.
+			 */
+			hwc_ctx_put(hwc, ctx);
+			goto check_status;
+		}
+
+		/* Genuine timeout: no response arrived.  Reduce further
+		 * waiting, and mark the channel timed out under the bitmap
+		 * lock so get_msg_index() cannot acquire new slots after this.
+		 */
 		if (hwc->hwc_timeout > 1)
 			hwc->hwc_timeout = 1;
 
+		spin_lock_irqsave(&hwc->inflight_msg_res.lock, flags);
+		hwc->hwc_timed_out = true;
+		spin_unlock_irqrestore(&hwc->inflight_msg_res.lock, flags);
+		wake_up_all(&hwc->msg_waitq);
+
 		err = -ETIMEDOUT;
-		goto out;
+		hwc_ctx_put(hwc, ctx);
+		goto done;
 	}
 
-	if (ctx->error) {
-		err = ctx->error;
-		goto out;
-	}
+	/* NULL output_buf so a late handle_resp() won't memcpy into
+	 * the caller's buffer after the sender exits.  Read error and
+	 * status_code under the same lock — after hwc_ctx_put the slot
+	 * may be reused and these fields overwritten.
+	 */
+	spin_lock_irqsave(&ctx->lock, flags);
+	ctx->output_buf = NULL;
+	err = ctx->error;
+	status = ctx->status_code;
+	spin_unlock_irqrestore(&ctx->lock, flags);
+	hwc_ctx_put(hwc, ctx);
+
+check_status:
+	if (err)
+		goto done;
 
-	if (ctx->status_code && ctx->status_code != GDMA_STATUS_MORE_ENTRIES) {
-		if (ctx->status_code == GDMA_STATUS_CMD_UNSUPPORTED) {
+	if (status && status != GDMA_STATUS_MORE_ENTRIES) {
+		if (status == GDMA_STATUS_CMD_UNSUPPORTED) {
 			err = -EOPNOTSUPP;
-			goto out;
+			goto done;
 		}
+
 		if (command != MANA_QUERY_PHY_STAT)
 			dev_err(hwc->dev, "Command 0x%x failed with status: 0x%x\n",
-				command, ctx->status_code);
+				command, status);
 		err = -EPROTO;
-		goto out;
+		goto done;
 	}
+
+	err = 0;
+	goto done;
 out:
-	mana_hwc_put_msg_index(hwc, msg_id);
+	/* Pre-post error paths: no WQE was submitted so handle_resp()
+	 * cannot race here.  refcount is 1 (no second ref taken).
+	 */
+	ctx = hwc->caller_ctx + msg_id;
+	spin_lock_irqsave(&ctx->lock, flags);
+	ctx->output_buf = NULL;
+	spin_unlock_irqrestore(&ctx->lock, flags);
+	hwc_ctx_put(hwc, ctx);
+done:
 	return err;
 }
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 3d8543acb5cc..5a55cedf0607 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -173,6 +173,23 @@ struct hwc_caller_ctx {
 
 	u32 error; /* Linux error code */
 	u32 status_code;
+
+	/* Protects output_buf against concurrent access from
+	 * handle_resp() (CQ interrupt) and the sender timeout path.
+	 */
+	spinlock_t lock;
+
+	/* Tracks sender + handle_resp ownership.  The last put
+	 * (refcount reaches 0) releases the bitmap slot.
+	 */
+	refcount_t refcnt;
+	u16 msg_id;
+
+	/* Set under lock by the first handle_resp() for this slot so a
+	 * duplicate or replayed response is dropped instead of consuming
+	 * the response-side reference a second time.
+	 */
+	bool responded;
 };
 
 struct hw_channel_context {
@@ -193,13 +210,19 @@ struct hw_channel_context {
 	struct hwc_wq *txq;
 	struct hwc_cq *cq;
 
-	struct semaphore sema;
 	struct gdma_resource inflight_msg_res;
+	/* Waitqueue for senders blocked on a full inflight bitmap. */
+	wait_queue_head_t msg_waitq;
 
 	u32 pf_dest_vrq_id;
 	u32 pf_dest_vrcq_id;
 	u32 hwc_timeout;
 
+	/* Set on first HWC timeout.  Causes get_msg_index() to return
+	 * -ETIMEDOUT instead of waiting, draining all queued senders.
+	 */
+	bool hwc_timed_out;
+
 	/* Set after mana_smc_setup_hwc() succeeds (hardware has active
 	 * MST entries).  Cleared only after mana_smc_teardown_hwc()
 	 * succeeds, on both the recoverable establish_channel path and the
-- 
2.43.0

Re: [PATCH net v3 1/6] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] A malicious host can trigger out-of-bounds array accesses in `cq_table` by dynamically inflating `gc->max_num_cqs`.
- [High] Pre-existing Use-After-Free in HWC channel teardown due to inverted destruction order.
--

--- Patch [1]: [PATCH net v3 1/6] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy ---
commit cb0751713ea0ba0da37493fd77c4af52cc7a9bc2
Author: Long Li <longli@microsoft.com>

net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy

This commit fixes a use-after-free by placing the cq_table under RCU
protection, synchronizing the fast-path EQ interrupt handlers against CQ
teardown on other CPUs.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index e3c24d50dad07..409e20caeccdc 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -818,10 +824,8 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
>  	/* gc->max_num_cqs is set in mana_hwc_init_event_handler(). If it's
>  	 * non-zero, the HWC worked and we should tear down the HWC here.
>  	 */
> -	if (gc->max_num_cqs > 0) {
> +	if (gc->max_num_cqs > 0)
>  		mana_smc_teardown_hwc(&gc->shm_channel, false);
> -		gc->max_num_cqs = 0;
> -	}
>  
>  	if (hwc->txq)
>  		mana_hwc_destroy_wq(hwc, hwc->txq);
> @@ -832,6 +836,14 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
>  	if (hwc->cq)
>  		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for an
in-flight EQ handler to access freed memory during teardown?

In mana_hwc_destroy_channel(), the work queues are destroyed and their
memory freed via mana_hwc_destroy_wq() before the completion queue and
event queue are torn down. The hwc->txq pointer is not cleared.

If a hardware interrupt fires or an in-flight EQ handler processes a
completion during this window, the handler might look up the still-valid
CQ and invoke mana_hwc_tx_event_handler().

Could mana_hwc_tx_event_handler() then dereference the freed hwc->txq
pointer?

    struct hwc_wq *hwc_txq = hwc->txq;

    WARN_ON_ONCE(!hwc_txq || hwc_txq->gdma_wq->id != gdma_txq_id);

Since the race window is open until mana_hwc_destroy_cq() runs, it seems
this could lead to a use-after-free.

[ ... ]

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 92bb55935c1c4..65b025e8f211c 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> @@ -2905,12 +2913,18 @@ static struct mana_rxq *mana_create_rxq(struct mana_port_context *apc,
>  	if (err)
>  		goto out;
>  
> -	if (WARN_ON(cq->gdma_id >= gc->max_num_cqs)) {
> +	/* No rcu_read_lock(): mana_create_rxq runs under RTNL during netdev
> +	 * bring-up, inside the netdev lifetime that mana_remove() drains
> +	 * before the base cq_table can be freed.  See gdma_context::cq_table
> +	 * in gdma.h for why "true" is sound.
> +	 */
> +	cq_table = rcu_dereference_protected(gc->cq_table, true);
> +	if (WARN_ON(!cq_table || cq->gdma_id >= gc->max_num_cqs)) {
>  		err = -EINVAL;
>  		goto out;
>  	}
>  
> -	gc->cq_table[cq->gdma_id] = cq->gdma_cq;
> +	rcu_assign_pointer(cq_table[cq->gdma_id], cq->gdma_cq);

[Severity: Critical]
This is a pre-existing issue, but could a malicious host trigger an
out-of-bounds write here by dynamically inflating gc->max_num_cqs?

If a compromised or malicious host sends a spoofed GDMA_EQE_HWC_INIT_DATA
event with HWC_INIT_DATA_MAX_NUM_CQS, mana_hwc_init_event_handler()
blindly updates gc->max_num_cqs at runtime without reallocating the
cq_table array:

    case HWC_INIT_DATA_MAX_NUM_CQS:
            gd->gdma_context->max_num_cqs = val;
            break;

When mana_create_rxq() creates a new queue later, the host could return
an out-of-bounds queue_index for cq->gdma_id.

Because gc->max_num_cqs was artificially inflated, the bounds check
cq->gdma_id >= gc->max_num_cqs would incorrectly pass. Does this allow
an attacker to write a kernel pointer past the end of the cq_table
allocation, potentially leading to guest kernel memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803234355.636038-1-longli@microsoft.com?part=1

Re: [PATCH net v3 3/6] net: mana: free HWC comp_buf after destroying the EQ

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Destroying the TX and RX queues before the CQ and EQ in mana_hwc_destroy_channel() causes a use-after-free if a late completion interrupt is processed.
--

--- Patch [3]: [PATCH net v3 3/6] net: mana: free HWC comp_buf after destroying the EQ ---
commit 7a0285e343813287658afb74760a65202a46b1ba
Author: Long Li <longli@microsoft.com>

net: mana: free HWC comp_buf after destroying the EQ

This commit fixes a use-after-free by destroying the CQ and EQ before
freeing the hwc_cq->comp_buf buffer. The EQ teardown deregisters the IRQ
and fences in-flight handlers, preventing late handlers from touching
freed memory.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index cbb56c7647876..d701c427fe47b 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -384,14 +384,20 @@ static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)
>  
>  static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
>  {
> -	kfree(hwc_cq->comp_buf);
> -
>  	if (hwc_cq->gdma_cq)
>  		mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
>  
> +	/* comp_buf is reached only by mana_hwc_comp_event(), which the
> +	 * EQ handler invokes via cq_table[id].  The CQ destroy above
> +	 * already cleared that slot and ran synchronize_rcu(), so no
> +	 * handler can reach comp_buf once it returns.  Destroying the EQ
> +	 * here additionally tears down the IRQ (defense in depth) before
> +	 * comp_buf and hwc_cq are freed below.
> +	 */
>  	if (hwc_cq->gdma_eq)
>  		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
>  
> +	kfree(hwc_cq->comp_buf);
>  	kfree(hwc_cq);
>  }

[Severity: High]
This is a pre-existing issue, but does the same use-after-free race still
exist for the TX and RX queues during channel teardown?

While this commit fixes the free order for comp_buf,
mana_hwc_destroy_channel() still frees the WQs before the CQ and EQ:

mana_hwc_destroy_channel()
{
        if (hwc->txq)
                mana_hwc_destroy_wq(hwc, hwc->txq);

        if (hwc->rxq)
                mana_hwc_destroy_wq(hwc, hwc->rxq);

        if (hwc->cq)
                mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
}

If a late EQ interrupt arrives or is already running in the window between
WQ destruction and CQ destruction, won't mana_gd_process_eqe() still find
the CQ in cq_table and invoke mana_hwc_comp_event()?

This would then call into mana_hwc_rx_event_handler() or
mana_hwc_tx_event_handler() and dereference the already-freed hwc->rxq
or hwc->txq memory:

mana_hwc_rx_event_handler()
{
        struct hwc_wq *hwc_rxq = hwc->rxq;
        ...
        if (WARN_ON_ONCE(hwc_rxq->gdma_wq->id != gdma_rxq_id))
                return;
        ...
}

Could the WQ destruction in mana_hwc_destroy_channel() be moved after the
CQ and EQ are destroyed to ensure all in-flight handlers are fenced before
the WQ memory is freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803234355.636038-1-longli@microsoft.com?part=3

Re: [PATCH net v3 2/6] net: mana: fix HWC RQ/SQ buffer size swap

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Out-of-Bounds Memory Read due to unvalidated `inline_oob_size_div4` from untrusted DMA memory.
- [High] Use-After-Free during hardware channel teardown due to incorrect destruction order and premature freeing of buffers.
- [High] Race condition / UAF in `mana_hwc_send_request` due to uncoordinated timeout handling.
--

--- Patch [2]: [PATCH net v3 2/6] net: mana: fix HWC RQ/SQ buffer size swap ---
commit 2b43add14b8efbca0def0e3777e0b101884e1da7
Author: Long Li <longli@microsoft.com>

net: mana: fix HWC RQ/SQ buffer size swap

This commit fixes an issue where the HWC RQ and SQ buffer sizes were
swapped during initialization. It ensures the RQ is sized with
max_resp_msg_size and the SQ with max_req_msg_size. The commit also
reorders the assignment of these queue dimensions before creating the CQ,
as the RX completion handler depends on them being set.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 409e20caeccdc..cbb56c7647876 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -263,7 +263,7 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
>  
>  	/* Select the RX work request for virtual address and for reposting. */
>  	rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
> -	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_req_msg_size;
> +	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size;

[Severity: High]
This is a pre-existing issue, but does this function have an out-of-bounds
memory read when parsing the WQE?

Looking at the code just before this modification in
mana_hwc_rx_event_handler():

	dma_oob = (struct gdma_wqe *)wqe;
	sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 * 4);
	...
	rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size;

Because inline_oob_size_div4 is read directly from the untrusted DMA buffer
without validation, could a compromised hypervisor provide a large value
that causes the sge pointer to point past the end of the mapped buffer,
resulting in a kernel panic when sge->address is dereferenced?

>  
>  	if (rx_req_idx >= hwc_rxq->msg_buf->num_reqs) {
>  		dev_err(hwc->dev, "HWC RX: wrong rx_req_idx=%llu, num_reqs=%u\n",

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk during
hardware channel teardown?

In mana_hwc_destroy_channel():

	if (hwc->txq)
		mana_hwc_destroy_wq(hwc, hwc->txq);

	if (hwc->rxq)
		mana_hwc_destroy_wq(hwc, hwc->rxq);

	if (hwc->cq)
		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);

The queues (txq and rxq) are destroyed and their memory is freed before the
CQ's RCU synchronization barrier is reached.

Inside mana_hwc_destroy_cq():

	kfree(hwc_cq->comp_buf);

	if (hwc_cq->gdma_cq)
		mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);

The comp_buf array is freed immediately, but the synchronize_rcu() barrier
sits inside mana_gd_destroy_queue(). Does this allow in-flight IRQ handlers
to access freed memory (txq, rxq, and comp_buf) before the RCU barrier
completes?

[Severity: High]
This is a pre-existing issue, but is there a race condition between the
timeout path and the response handler that could cause stack corruption?

In mana_hwc_send_request():

	if (!wait_for_completion_timeout(&ctx->comp_event,
					 (msecs_to_jiffies(hwc->hwc_timeout)))) {
		...
		err = -ETIMEDOUT;
		goto out;
	}
	...
out:
	mana_hwc_put_msg_index(hwc, msg_id);
	return err;

The timeout path exits and frees the message ID without synchronizing with
the receiver thread or clearing ctx->output_buf.

Meanwhile, in mana_hwc_handle_resp():

	if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
		...
	ctx = hwc->caller_ctx + msg_id;
	...
	memcpy(ctx->output_buf, resp_msg, resp_len);

The test_bit() check is done locklessly. If a timeout occurs right after
test_bit() succeeds, mana_hwc_send_request() will return and pop the
caller's stack frame. Will the response handler then memcpy() into a
dangling stack pointer (ctx->output_buf), corrupting the stack?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803234355.636038-1-longli@microsoft.com?part=2

Re: [PATCH net v3 5/6] net: mana: fix HWC teardown safety with setup_active flag and destroy ordering

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] An early return on HWC teardown failure leaves a stale pointer in `gc->cq_table`, which a malicious host can exploit to cause an out-of-bounds read.
- [Medium] Redundant HWC teardown in `mana_hwc_establish_channel()` causes a double hardware timeout (up to 120s stall) and masks the original initialization error code.

Pre-existing issues:
- [High] During HWC teardown, the CQ is destroyed and its memory freed before the EQ is deregistered, leading to Use-After-Free and WARN splats if an interrupt occurs.
--

--- Patch [5]: [PATCH net v3 5/6] net: mana: fix HWC teardown safety with setup_active flag and destroy ordering ---
commit cb8bd3f89f96cf2e7fd5d41275858b076d281d38
Author: Long Li <longli@microsoft.com>

net: mana: fix HWC teardown safety with setup_active flag and destroy ordering

Three teardown hazards let the hardware touch memory the driver freed.

[ ... ]

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -768,20 +769,33 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
[ ... ]
> +teardown:
> +	{
> +		int td_err = mana_smc_teardown_hwc(&gc->shm_channel, false);
> +
> +		if (!td_err)
> +			hwc->setup_active = false;
> +
> +		return td_err ? td_err : err;
> +	}
>  }

[Severity: Medium]
Does this teardown logic mask the original error code and cause a redundant
teardown attempt?

If mana_smc_teardown_hwc() fails, it returns td_err and leaves
hwc->setup_active = true. The caller mana_hwc_create_channel() handles the
returned error like this:

mana_hwc_create_channel() {
    ...
    err = mana_hwc_establish_channel(...);
    if (err) {
        dev_err(hwc->dev, "Failed to establish HWC: %d\n", err);
        goto out;
    }
    ...
out:
    mana_hwc_destroy_channel(gc);
    return err;
}

Since hwc->setup_active is still true, mana_hwc_destroy_channel() will
attempt to tear down the hardware a second time. This can cause the driver
to stall for twice the duration of a hardware timeout, while the original
initialization error code is lost.

> @@ -907,11 +931,38 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
[ ... ]
> +	if (hwc->setup_active) {
> +		int td_err = mana_smc_teardown_hwc(&gc->shm_channel, false);
> +
> +		if (td_err) {
> +			dev_err(gc->dev,
> +				"HWC teardown failed: %d, leaking resources\n",
> +				td_err);
> +			return;
> +		}

[Severity: High]
Could this early return leave a stale pointer in gc->cq_table?

If mana_smc_teardown_hwc() fails, this returns early without clearing
gc->cq_table. On resume or re-initialization, mana_hwc_create_channel()
creates a new hwc but reuses the same gc.

During mana_hwc_establish_channel(), while waiting for the initialization
event, the hardware could send an EQE updating gc->max_num_cqs to a large
value. Because the new cq_table is not assigned until after the wait
completes, gc->cq_table still points to the old, leaked table.

A malicious host could then inject a completion EQE with an out-of-bounds
cq_id. This would pass validation against the newly inflated gc->max_num_cqs
but read out of bounds from the smaller, old cq_table.

> +
> +		hwc->setup_active = false;
> +	}
> +
> +	/* Tear down the HWC CQ object first — mana_hwc_destroy_cq()
> +	 * both unpublishes the CQ from cq_table (+synchronize_rcu) and
> +	 * deregisters the HWC EQ from the interrupt handler list (via
> +	 * mana_gd_deregister_irq + synchronize_rcu), guaranteeing no
> +	 * interrupt handler can access RQ/TXQ buffers after this point.
> +	 */
> +	if (hwc->cq)
> +		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk during CQ
destruction?

Looking at mana_hwc_destroy_cq(), the CQ is destroyed and its memory freed
before the EQ:

mana_hwc_destroy_cq() {
    ...
    if (hwc_cq->gdma_cq)
        mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
    ...
    if (hwc_cq->gdma_eq)
        mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
}

Although mana_gd_destroy_queue() on the CQ clears the cq_table entry and
calls synchronize_rcu(), the EQ is still fully registered and on the
interrupt dispatch list. A pending interrupt could fire immediately after
the CQ is freed.

The interrupt handler (mana_gd_process_eqe) might then trigger a
WARN_ON_ONCE(!cq) for completions, or call mana_hwc_init_event_handler()
which accesses the freed memory:

mana_hwc_init_event_handler() {
    ...
    case HWC_INIT_DATA_CQID:
        hwc->cq->gdma_cq->id = val;
        break;
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803234355.636038-1-longli@microsoft.com?part=5

Re: [PATCH net v3 4/6] net: mana: validate hardware-supplied values in the HWC RX path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] A timeout in mana_hwc_send_request() can lead to stack corruption and use-after-free if the hardware response arrives concurrently or is delayed.
--

--- Patch [4]: [PATCH net v3 4/6] net: mana: validate hardware-supplied values in the HWC RX path ---
commit c5cc55c1f3f3c4968b8391ae19f32c3ab101adc9
Author: Long Li <longli@microsoft.com>

net: mana: validate hardware-supplied values in the HWC RX path

This commit validates lengths and indices from the device DMA in the HWC RX
path. It adds bounds checks and address matching to prevent out-of-bounds
accesses or reading stale responses. A reposting mechanism is added for
rejected responses to maintain posted RQ depth.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index d701c427fe47b..e378b8ec97c9a 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -83,6 +83,17 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
>  	struct hwc_caller_ctx *ctx;
>  	int err;
>  
[ ... ]
>  	ctx = hwc->caller_ctx + msg_id;
> +
> +	/* Reject responses larger than the RX DMA buffer — the SGE
> +	 * limits what hardware can DMA, so an oversized resp_len
> +	 * indicates a firmware bug.  Fail rather than silently
> +	 * truncating.
> +	 */
> +	if (resp_len > rx_req->buf_len) {
> +		dev_err(hwc->dev, "HWC RX: resp_len %u > buf_len %u\n",
> +			resp_len, rx_req->buf_len);
> +		resp_len = 0;
> +	}
> +
>  	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
>  	if (err)
>  		goto out;

[Severity: Critical]
This is a pre-existing issue, but I have a question about how timeouts are
handled when processing these contexts.

If a thread times out in mana_hwc_send_request() while waiting for
ctx->comp_event, does it return and free the stack frame where the
response buffer (ctx->output_buf) might be allocated?

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_send_request() {
    ...
	if (!wait_for_completion_timeout(&ctx->comp_event,
					 (msecs_to_jiffies(hwc->hwc_timeout)))) {
    ...
		err = -ETIMEDOUT;
		goto out;
	}
    ...
out:
	mana_hwc_put_msg_index(hwc, msg_id);
	return err;
}

Because the sender doesn't clear ctx->output_buf or synchronize with the RX
handler when returning on a timeout, could a delayed hardware response arriving
later lead to stack corruption when mana_hwc_handle_resp() executes the
memcpy() into the now-dangling ctx->output_buf pointer?

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_handle_resp() {
    ...
	if (!err && ctx->output_buf) {
		ctx->status_code = resp_msg->status;
		memcpy(ctx->output_buf, resp_msg, resp_len);
		ctx->error = 0;
	}
    ...
}

Additionally, since there is no reinit_completion() call when allocating a
message slot, could a newly reused slot immediately consume a stale completion
token from a delayed response and return prematurely?

Could a malicious host or firmware in a Confidential VM environment exploit
this by intentionally delaying responses to overwrite the guest kernel stack?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803234355.636038-1-longli@microsoft.com?part=4

Re: [PATCH net v3 6/6] net: mana: fix stale HWC response after command timeout

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] A spurious, duplicate, or maliciously early hardware response can prematurely free the message slot before the sender takes its reference, leading to a refcount use-after-free/saturation and data corruption.
--

--- Patch [6]: [PATCH net v3 6/6] net: mana: fix stale HWC response after command timeout ---
commit 374238ca2c81aa20a65060ecc749dc0a454b16f8
Author: Long Li <longli@microsoft.com>

net: mana: fix stale HWC response after command timeout

This commit introduces a refcounted spinlock mechanism for caller contexts
to prevent stale HWC responses from corrupting output buffers or completing
the wrong command after a timeout.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 1603968d7989a..d92032b466af2 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -1034,8 +1104,14 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
>  		dest_vrcq = hwc->pf_dest_vrcq_id;
>  	}
>  
> +	/* Take handle_resp's ref before posting — hardware can respond
> +	 * immediately after the doorbell ring.
> +	 */
> +	refcount_inc(&ctx->refcnt);

[Severity: High]
Could a spurious or duplicate hardware response arrive before this reference
is incremented?

Looking at mana_hwc_get_msg_index(), the slot is marked in the inflight
bitmap and the refcount is initialized to 1:

mana_hwc_get_msg_index()
    bitmap_set(r->map, index, 1);
    refcount_set(&ctx->refcnt, 1);
    ctx->responded = false;

Since the slot is immediately visible to the CQ interrupt handler upon being
added to the bitmap, if an unexpected response arrives before the sender
reaches this refcount_inc() call, mana_hwc_handle_resp() could process it.
It would see ctx->responded is false, set it to true, and call
hwc_ctx_put().

Because the sender hasn't taken its additional reference yet, hwc_ctx_put()
would drop the only reference and free the slot prematurely. When the sender
finally reaches this refcount_inc(), it would increment a zero-refcount,
triggering a warning for saturation and permanently leaking the HWC slot.

Should the refcount be initialized to 2 in mana_hwc_get_msg_index() to
account for both the sender and the response, rather than incrementing it
later during the request setup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803234355.636038-1-longli@microsoft.com?part=6

Re: [PATCH net v3 1/6] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy

This puts gc->cq_table under RCU: the base pointer and the entries are
annotated __rcu, the EQ interrupt handler reads them with
rcu_dereference(), publishers use rcu_assign_pointer(), and teardown
clears the slot and calls synchronize_rcu() before the CQ is freed.

A few questions on the teardown latency and on the lifetime rule the new
gdma.h comment documents.

> diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
> index f2547989f4229..2bf4be21cede7 100644
> --- a/drivers/infiniband/hw/mana/cq.c
> +++ b/drivers/infiniband/hw/mana/cq.c

[ ... ]

> @@ -149,23 +157,49 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)

[ ... ]

> +	rcu_assign_pointer(cq_table[cq->queue.id], NULL);
> +
> +	/* Wait for in-flight EQ handlers that may have loaded the old
> +	 * pointer via rcu_dereference() to finish before freeing.
> +	 */
> +	synchronize_rcu();
> +	kfree(gdma_cq);
>  }

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index e8b7ffb47eb98..fa6fcc2475c1a 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c

[ ... ]

> @@ -743,10 +744,11 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
>  	switch (type) {
>  	case GDMA_EQE_COMPLETION:
>  		cq_id = eqe->details[0] & 0xFFFFFF;
> -		if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
> +		cq_table = rcu_dereference(gc->cq_table);
> +		if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs || !cq_table))
>  			break;
>  
> -		cq = gc->cq_table[cq_id];
> +		cq = rcu_dereference(cq_table[cq_id]);
>  		if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id))
>  			break;

[Severity: High]

This isn't a bug introduced by this patch - the baseline had the same
"if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs)) break; cq = gc->cq_table[cq_id];"
pattern - but the patch rewrites these lines and documents the RCU scheme
as the complete safety argument for cq_table, while the bound that guards
every index stays outside the published object.

mana_hwc_establish_channel() freezes the element count at vcalloc() time
and publishes only the pointer, but gc->max_num_cqs keeps being written
straight from a device-supplied EQE payload:

hw_channel.c:mana_hwc_init_event_handler() {
	...
		case HWC_INIT_DATA_MAX_NUM_CQS:
			gd->gdma_context->max_num_cqs = val;
			break;
	...
}

That handler stays installed as the HWC EQ eq.callback for the whole
device lifetime, and mana_gd_process_eqe() dispatches any
GDMA_EQE_HWC_INIT_DATA to it from hard IRQ context, with no coupling to the
currently published table.

If the PF raises gc->max_num_cqs above the allocated element count, does
rcu_dereference(cq_table[cq_id]) then read past the end of the vcalloc'ed
array, and can the value read there be used as
cq->cq.callback(cq->cq.context, cq)?

Separately, the gc->max_num_cqs load carries no address dependency on the
rcu_dereference()d base pointer. Can a reader therefore pair a freshly
published table with a stale bound, producing a spurious hard-IRQ
WARN_ON_ONCE and a silently dropped completion?

Would storing the element count inside or alongside the published
allocation be more robust than trusting a device-writable, non-RCU field?

[ ... ]

> @@ -1053,15 +1055,28 @@ static void mana_gd_create_cq(const struct gdma_queue_spec *spec,
>  static void mana_gd_destroy_cq(struct gdma_context *gc,
>  			       struct gdma_queue *queue)
>  {
> +	struct gdma_queue __rcu **cq_table;
>  	u32 id = queue->id;
>  
> -	if (id >= gc->max_num_cqs)
> +	/* No rcu_read_lock() here: mana_gd_destroy_cq() runs only on the
> +	 * CQ-destroy/teardown path, where the base cq_table is stable.  See
> +	 * the lifecycle note on gdma_context::cq_table in gdma.h for why the
> +	 * "true" predicate is sound.
> +	 */
> +	cq_table = rcu_dereference_protected(gc->cq_table, true);
> +	if (!cq_table || id >= gc->max_num_cqs)
>  		return;
>  
> -	if (!gc->cq_table[id])
> +	if (!rcu_access_pointer(cq_table[id]))
>  		return;
>  
> -	gc->cq_table[id] = NULL;
> +	rcu_assign_pointer(cq_table[id], NULL);
> +
> +	/* Wait for in-flight EQ handlers that may have loaded the old
> +	 * pointer via rcu_dereference() to finish before the caller
> +	 * frees the CQ memory.
> +	 */
> +	synchronize_rcu();
>  }

[Severity: High]

Is one grace period per CQ acceptable on this path? mana_gd_destroy_cq()
is reached once per TX queue and once per RX queue during netdev teardown:

mana_en.c:mana_destroy_txq() {
	for (i = 0; i < apc->num_queues; i++) {
		...
		mana_deinit_cq(apc, &apc->tx_qp[i]->tx_cq);
}

mana_deinit_cq() -> mana_gd_destroy_queue() -> mana_gd_destroy_cq(), and
mana_destroy_rxq() does the same for &rxq->rx_cq.

With MANA_MAX_NUM_QUEUES == 64 that is up to about 128 serialized RCU
grace periods per ifdown, MTU change, ethtool -L, ethtool -G, XDP attach
or queue reset. Those loops run with rtnl_lock() held (mana_detach() has
ASSERT_RTNL(), and the same loops use napi_disable_locked() /
netif_napi_del_locked(), so the netdev instance lock is held too).

Since RTNL is system-wide, does this stall every other network
configuration operation on the machine for the accumulated duration?

The same question applies to mana_ib_remove_cq_cb() in
drivers/infiniband/hw/mana/cq.c, which now blocks for a full grace period
on each user-triggered ib_destroy_cq() and once per ind_tbl entry in the
mana_ib_create_qp_rss() unwind loop.

mana_ib_remove_cq_cb() frees the per-CQ struct gdma_queue itself, so would
kfree_rcu() give the same guarantee without blocking? For the netdev path,
could all slots be cleared first and a single grace period taken per
teardown batch instead of one per CQ?

The changelog describes the change as annotation plus a deferred free;
would it be worth mentioning the new teardown latency so backporters know
CQ-destroy and ifdown timing changes by orders of magnitude?

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index e3c24d50dad07..409e20caeccdc 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -698,11 +699,15 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
>  	if (WARN_ON(cq->id >= gc->max_num_cqs))
>  		return -EPROTO;
>  
> -	gc->cq_table = vcalloc(gc->max_num_cqs, sizeof(struct gdma_queue *));
> -	if (!gc->cq_table)
> +	cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
> +	if (!cq_table)
>  		return -ENOMEM;
>  
> -	gc->cq_table[cq->id] = cq;
> +	rcu_assign_pointer(cq_table[cq->id], cq);
> +	/* Publish the fully-initialised table last; pairs with the
> +	 * rcu_dereference(gc->cq_table) in mana_gd_process_eqe().
> +	 */
> +	rcu_assign_pointer(gc->cq_table, cq_table);

[Severity: High]

This is a pre-existing check-then-use pattern and not introduced by this
patch, but the patch rewrites exactly these lines.

cq->id and gc->max_num_cqs are validated once, then re-read separately -
one for the allocation size, one as the array index. Both fields are
written from the HWC EQ interrupt handler:

hw_channel.c:mana_hwc_init_event_handler() {
	...
		case HWC_INIT_DATA_CQID:
			hwc->cq->gdma_cq->id = val;
			break;
	...
		case HWC_INIT_DATA_MAX_NUM_CQS:
			gd->gdma_context->max_num_cqs = val;
			break;
	...
}

The HWC EQ is already armed and delivering EQEs here, since this code only
runs after GDMA_EQE_HWC_INIT_DONE completed hwc_init_eqe_comp.

If the device emits an extra or duplicate GDMA_EQE_HWC_INIT_DATA in the
window between the WARN_ON() check, the vcalloc() and the indexed store
(smaller MAX_NUM_CQS or larger CQID), do the validated expression and the
used expression diverge, writing the cq pointer at a device-chosen offset
past the end of the fresh vcalloc'ed table?

Would snapshotting both values into locals before the check and using only
the snapshot for the allocation size and the index close this?

[ ... ]

> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> index 0c395917b2144..0b48ded0aecda 100644
> --- a/include/net/mana/gdma.h
> +++ b/include/net/mana/gdma.h
> @@ -418,7 +418,26 @@ struct gdma_context {
>  
>  	/* This maps a CQ index to the queue structure. */
>  	unsigned int		max_num_cqs;
> -	struct gdma_queue	**cq_table;
> +	/* Both the base pointer and each entry are RCU-managed.  The fast
> +	 * path (mana_gd_process_eqe) reads the base via rcu_dereference()
> +	 * under rcu_read_lock(), so the table is freed with
> +	 * rcu_assign_pointer(NULL) + synchronize_rcu() and an in-flight
> +	 * reader can never observe freed memory.
> +	 *
> +	 * The slow paths -- mana_gd_destroy_cq() and the CQ install/remove
> +	 * callers (mana_create_txq/_rxq, mana_ib_install/remove_cq_cb) --
> +	 * instead read the base with rcu_dereference_protected(cq_table,
> +	 * true).  The bare "true" is justified by teardown ordering, not by
> +	 * a lock: the base table is replaced+freed only by
> +	 * mana_hwc_destroy_channel() (and the create-time reinit), and every
> +	 * teardown path first runs mana_remove() + mana_rdma_remove(), which
> +	 * synchronously drain the netdev and the IB device
> +	 * (unregister_netdevice / ib_unregister_device) that bound all
> +	 * install/remove callers; the reinit case runs before either
> +	 * consumer is probed.  So no slow-path caller can run while the base
> +	 * table is being freed.
> +	 */
> +	struct gdma_queue	__rcu * __rcu *cq_table;

[Severity: Medium]

Is the statement "every teardown path first runs mana_remove() +
mana_rdma_remove(), which synchronously drain the netdev and the IB device
(unregister_netdevice / ib_unregister_device)" accurate for the suspend
and shutdown paths?

mana_gd_suspend() (also reached from the reset service work via
mana_serv_reset()) and mana_gd_shutdown() both call
mana_remove(&gc->mana, true), and the suspending branch deliberately keeps
the netdev registered:

mana_en.c:mana_remove() {
	...
		if (suspending) {
			/* No need to unregister the ndev. */
			rtnl_unlock();
			continue;
		}

		unregister_netdevice(ndev);
	...
}

RTNL is then dropped and mana_gd_cleanup_device() ->
mana_hwc_destroy_channel() runs:

	old_cq_table = rcu_replace_pointer(gc->cq_table, NULL, true);
	synchronize_rcu();
	vfree(old_cq_table);

mana_detach(ndev, false) also calls netif_device_detach() while leaving
apc->port_st_save == true, so a concurrent RTNL reconfiguration in that
window (mana_change_mtu(), mana_set_channels(), mana_set_ringparam(), XDP
set) takes the "if (!from_close && !netif_device_present(ndev)) return 0;"
early return in mana_detach() and proceeds into mana_attach() ->
mana_alloc_queues() -> mana_create_txq(), which does:

	cq_table = rcu_dereference_protected(gc->cq_table, true);
	if (WARN_ON(!cq_table || cq->gdma_id >= gc->max_num_cqs)) {
	...
	rcu_assign_pointer(cq_table[cq->gdma_id], cq->gdma_cq);

These installers are writers, not RCU readers, so synchronize_rcu() does
not wait for them, and the !cq_table test is a check rather than
protection. Can this store land in the vfree()d table?

One step I could not confirm: mana_attach() first issues
mana_query_vport_cfg() over the HWC after mana_gd_deregister_device(), and
whether the PF still answers that is device behaviour; on the
mana_serv_reset() path hwc->hwc_timeout is 0 so the command times out, and
on PM suspend userspace is frozen, which leaves the reboot /
mana_gd_shutdown() window as the plausible case. The unsynchronized writer
predates this patch, but the lifetime rule stated here is new, so should
the invariant be corrected (serialize base replacement against the
installers, or run the installers under rcu_read_lock())?

One more on the same comment: mana_gd_destroy_cq() is listed as covered by
this rule, but it is called from inside mana_hwc_destroy_channel() itself
(mana_hwc_destroy_cq() -> mana_gd_destroy_queue() -> mana_gd_destroy_cq()),
a few statements before that function replaces and vfree()s the base
table. Should that intra-function ordering be spelled out here as well,
since it is what actually makes that caller's "true" predicate sound?

Re: [PATCH net v3 2/6] net: mana: fix HWC RQ/SQ buffer size swap

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mana: fix HWC RQ/SQ buffer size swap

This swaps the sizes used when creating the HWC RQ and SQ so the RQ is
sized by max_resp_msg_size and the SQ by max_req_msg_size, adds
max_resp_msg_size to struct hw_channel_context, and uses it as the RX
slot stride in mana_hwc_rx_event_handler().  It also hoists the
num_inflight_msg / max_req_msg_size / max_resp_msg_size assignments in
mana_hwc_init_queues() above the mana_hwc_create_cq() call.

Two questions, both about the description rather than the mechanics of
the swap.

First, about this part of the changelog:

>     The HWC RQ receives responses and the SQ sends requests, but
>     mana_hwc_init_queues() sized the RQ with max_req_msg_size and the SQ with
>     max_resp_msg_size -- backwards.  A response larger than the undersized RQ
>     buffer could overflow it, and mana_hwc_rx_event_handler() recovered the
>     RX slot index by dividing by the wrong size (max_req_msg_size).

[Severity: Medium]
Can the described overflow or mis-indexing actually happen in this tree?
mana_hwc_init_queues() has a single caller, mana_hwc_create_channel(),
which passes two constants that are equal:

	err = mana_hwc_init_queues(hwc, HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH,
				   HW_CHANNEL_MAX_REQUEST_SIZE,
				   HW_CHANNEL_MAX_RESPONSE_SIZE);

include/net/mana/hw_channel.h:

	#define HW_CHANNEL_MAX_REQUEST_SIZE  0x1000
	#define HW_CHANNEL_MAX_RESPONSE_SIZE 0x1000

The hardware-reported sizes that mana_hwc_establish_channel() returns are
only handed to mana_hwc_test_channel(); the queues are never re-created
with them, so max_req_msg_size == max_resp_msg_size on every path both
before and after this patch.

Also, mana_hwc_post_rx_wqe() advertises the real slot length to the
device:

	sge->address = (u64)req->buf_sge_addr;
	sge->mem_key = hwc_rxq->msg_buf->gpa_mkey;
	sge->size = req->buf_len;

so the device is not told it may write more than one slot regardless of
max_resp_msg_size.

Given that, would it be worth saying in the changelog that the two sizes
are currently identical and that there is no observable misbehaviour,
i.e. that this is a latent-correctness cleanup?  With the

	Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")

tag and the wording above, a stable backporter is likely to read this as
a live buffer overflow fix.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 409e20caeccdc..cbb56c7647876 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -263,7 +263,7 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
>  
>  	/* Select the RX work request for virtual address and for reposting. */
>  	rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
> -	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_req_msg_size;
> +	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size;
>  
>  	if (rx_req_idx >= hwc_rxq->msg_buf->num_reqs) {
>  		dev_err(hwc->dev, "HWC RX: wrong rx_req_idx=%llu, num_reqs=%u\n",

[ ... ]

> @@ -721,6 +721,15 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
>  	if (err)
>  		return err;
>  
> +	/* Set the queue dimensions before creating the CQ: doing so
> +	 * registers mana_hwc_rx_event_handler(), which divides by
> +	 * hwc->max_resp_msg_size and range-checks hwc->num_inflight_msg.
> +	 * They must be valid before any RX completion can be delivered.
> +	 */
> +	hwc->num_inflight_msg = q_depth;
> +	hwc->max_req_msg_size = max_req_msg_size;
> +	hwc->max_resp_msg_size = max_resp_msg_size;

[Severity: Low]
Does this comment claim more than the reordering provides?  It says the
scalars must be valid before mana_hwc_create_cq() registers
mana_hwc_rx_event_handler(), but in that same window hwc->rxq and
hwc->txq are still NULL, since they are only assigned by the
mana_hwc_create_wq() calls further down.

mana_hwc_rx_event_handler() touches hwc->rxq well before it reaches
either hoisted field:

	struct hwc_wq *hwc_rxq = hwc->rxq;
	...
	if (WARN_ON_ONCE(hwc_rxq->gdma_wq->id != gdma_rxq_id))
		return;
	...
	rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size;

and mana_hwc_handle_resp() uses hwc->caller_ctx, which is only allocated
later in mana_hwc_test_channel().  The EQ callback registered by the same
mana_hwc_create_cq() call, mana_hwc_init_event_handler(), likewise
dereferences hwc->cq, hwc->rxq and hwc->txq:

	hwc->cq->gdma_eq->id = eq_db.eq_id;
	...
	hwc->rxq->gdma_wq->id = val;
	...
	hwc->txq->msg_buf->gpa_mkey = val;

So if a completion really could be delivered while mana_hwc_create_cq()
runs, the hoisted scalars would not help.  The window looks not to be
live anyway, because the device only learns the queue DMA addresses via
mana_smc_setup_hwc() in mana_hwc_establish_channel(), which runs after
mana_hwc_init_queues() has published everything, matching the existing
comment in mana_hwc_create_channel():

	/* mana_hwc_init_queues() only creates the required data structures,
	 * and doesn't touch the HWC device.
	 */

Could the comment and the matching changelog paragraph be reworded to say
the assignments are simply hoisted for clarity, rather than asserting
that RX completions can be safely handled at that point?  As written, a
later reader may take this window to be safe for handler execution.

>  	/* CQ is shared by SQ and RQ, so CQ's queue depth is the sum of SQ
>  	 * queue depth and RQ queue depth.
>  	 */
> @@ -733,23 +742,20 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
>  		goto out;
>  	}
>  
> -	err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_req_msg_size,
> +	err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_resp_msg_size,
>  				 hwc->cq, &hwc->rxq);
>  	if (err) {
>  		dev_err(hwc->dev, "Failed to create HWC RQ: %d\n", err);
>  		goto out;
>  	}
>  
> -	err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_resp_msg_size,
> +	err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_req_msg_size,
>  				 hwc->cq, &hwc->txq);
>  	if (err) {
>  		dev_err(hwc->dev, "Failed to create HWC SQ: %d\n", err);
>  		goto out;
>  	}
>  
> -	hwc->num_inflight_msg = q_depth;
> -	hwc->max_req_msg_size = max_req_msg_size;
> -
>  	return 0;

[ ... ]

Re: [PATCH net v3 3/6] net: mana: free HWC comp_buf after destroying the EQ

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mana: free HWC comp_buf after destroying the EQ

This moves the kfree(hwc_cq->comp_buf) in mana_hwc_destroy_cq() to after
the CQ and EQ have been destroyed, so a late EQ handler calling
mana_hwc_comp_event() cannot dereference freed memory.  A comment is
added describing the fencing that makes the new order safe.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index cbb56c7647876..d701c427fe47b 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -384,14 +384,20 @@ static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)
>  
>  static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
>  {
> -	kfree(hwc_cq->comp_buf);
> -
>  	if (hwc_cq->gdma_cq)
>  		mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
>  
> +	/* comp_buf is reached only by mana_hwc_comp_event(), which the
> +	 * EQ handler invokes via cq_table[id].  The CQ destroy above
> +	 * already cleared that slot and ran synchronize_rcu(), so no
> +	 * handler can reach comp_buf once it returns.  Destroying the EQ
> +	 * here additionally tears down the IRQ (defense in depth) before
> +	 * comp_buf and hwc_cq are freed below.
> +	 */

[Severity: Low]
Does the EQ destroy really tear down the IRQ here?

mana_gd_destroy_queue() for GDMA_EQ calls mana_gd_destroy_eq(), whose whole
body is:

	if (flush_evenets) {
		err = mana_gd_test_eq(gc, queue);
		...
	}

	mana_gd_deregister_irq(queue);

	if (queue->eq.disable_needed)
		mana_gd_disable_queue(queue);

and mana_gd_deregister_irq() only unlinks the EQ from the shared IRQ's
list:

	spin_lock_irqsave(&gic->lock, flags);
	list_for_each_entry_rcu(eq, &gic->eq_list, entry) {
		if (queue == eq) {
			list_del_rcu(&eq->entry);
			break;
		}
	}
	spin_unlock_irqrestore(&gic->lock, flags);

	synchronize_rcu();

There is no free_irq() and no mana_gd_put_gic() on this path; the only
free_irq() in gdma_main.c lives in mana_gd_put_gic(), which the HWC path
reaches from mana_gd_remove_irqs() instead.  The MSI-X line stays
registered and keeps firing for the other EQs sharing it.

So the actual fence appears to be RCU in both steps (the cq_table slot
clear plus synchronize_rcu() in mana_gd_destroy_cq(), and the list_del_rcu()
plus synchronize_rcu() above), not IRQ teardown.

The same wording appears in the commit message:

    Destroy the CQ and EQ first -- the EQ teardown deregisters the IRQ and
    fences in-flight handlers -- then free comp_buf and hwc_cq.

Could the comment and the changelog be reworded to say the EQ is
deregistered from the shared IRQ's RCU list, rather than that the IRQ is
torn down?  The comment and the changelog also disagree on which step is
load bearing: the comment treats the EQ destroy as "defense in depth"
while the changelog presents it as the mechanism that fences handlers.

> +
>  	if (hwc_cq->gdma_eq)
>  		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
>  
> +	kfree(hwc_cq->comp_buf);
>  	kfree(hwc_cq);
>  }
>  

[Severity: Medium]
This isn't a bug introduced by this patch, but the same window the patch
closes for comp_buf still exists for hwc->rxq and hwc->txq, so the fix
looks asymmetric.  In mana_hwc_destroy_channel() the work queues are
freed before the CQ/EQ teardown that does the cq_table clear and the
synchronize_rcu() calls:

	if (hwc->txq)
		mana_hwc_destroy_wq(hwc, hwc->txq);

	if (hwc->rxq)
		mana_hwc_destroy_wq(hwc, hwc->rxq);

	if (hwc->cq)
		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);

mana_hwc_destroy_wq() releases the DMA buffer and the hwc_wq itself with
no fencing, and the GDMA_RQ/GDMA_SQ cases of mana_gd_destroy_queue() are
empty:

	mana_hwc_dealloc_dma_buf(hwc, hwc_wq->msg_buf);

	if (hwc_wq->gdma_wq)
		mana_gd_destroy_queue(hwc->gdma_dev->gdma_context,
				      hwc_wq->gdma_wq);

	kfree(hwc_wq);

In that window mana_hwc_comp_event() is still published in cq_table[id]
and reaches the event handlers with ctx == hwc:

		if (completions[i].is_sq)
			hwc_cq->tx_event_handler(hwc_cq->tx_event_ctx, ...);
		else
			hwc_cq->rx_event_handler(hwc_cq->rx_event_ctx, ...);

and mana_hwc_rx_event_handler() then dereferences the freed rxq:

	if (WARN_ON_ONCE(hwc_rxq->gdma_wq->id != gdma_rxq_id))
		return;

	rq = hwc_rxq->gdma_wq;
	...
	rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;

Can a completion delivered in that window read freed hwc_wq/hwc_dma_buf
memory, and repost into the already dma-unmapped RQ ring via
mana_hwc_post_rx_wqe()?  hwc->rxq and hwc->txq are also left set after
being freed.

For the record, this ordering does get corrected later in the same series
by "net: mana: fix HWC teardown safety with setup_active flag and destroy
ordering", where mana_hwc_destroy_channel() destroys the CQ before the
work queues.  Would it make sense to fold that reordering in here, or note
the dependency, so this patch does not land with only half of the window
closed?

Re: [PATCH net v3 4/6] net: mana: validate hardware-supplied values in the HWC RX path

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mana: validate hardware-supplied values in the HWC RX path

This adds validation of device-supplied lengths and indices in the HWC
receive path: the SGE pointer derived from inline_oob_size_div4 is bounds
checked, the SGE address must match the address posted for the slot, short
responses are rejected before hwc_msg_id is read, and hwc_msg_id and
resp_len are range checked in mana_hwc_handle_resp().  Most validation
early-returns now repost the RX WQE.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index d701c427fe47b..e378b8ec97c9a 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -83,6 +83,17 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
>  	struct hwc_caller_ctx *ctx;
>  	int err;
>  
> +	/* Validate msg_id is in range before using it to index bitmap
> +	 * and caller_ctx array.  Malicious firmware could send
> +	 * out-of-range msg_id causing out-of-bounds access.
> +	 */
> +	if (msg_id >= hwc->num_inflight_msg) {
> +		dev_err(hwc->dev, "hwc_rx: msg_id %u >= max %u\n",
> +			msg_id, hwc->num_inflight_msg);
> +		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> +		return;
> +	}
> +

[Severity: Low]
Is this branch reachable?  mana_hwc_handle_resp() has a single caller,
mana_hwc_rx_event_handler(), which already rejects the same condition
immediately before the call (unchanged context in the last hunk of this
patch):

	msg_id = READ_ONCE(resp->response.hwc_msg_id);
	if (msg_id >= hwc->num_inflight_msg) {
		dev_err(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
		return;
	}

	mana_hwc_handle_resp(hwc, rx_oob->tx_oob_data_size, rx_req, msg_id);

msg_id is passed by value, so there is no second read from DMA memory and
the bound tested is the same hwc->num_inflight_msg.

The changelog says mana_hwc_handle_resp() "consumed lengths and indices
taken straight from device DMA without validation" and could "index out of
bounds", and lists "bounds-check hwc_msg_id in mana_hwc_handle_resp()
before indexing the inflight bitmap and caller_ctx" as one of the fixes.

Given the caller's pre-existing check, could hwc->inflight_msg_res.map or
hwc->caller_ctx actually be indexed out of bounds before this patch?  If
not, could the changelog describe this hunk as defence in depth instead,
since the Fixes: tag makes it a backport candidate?

[ ... ]

> @@ -261,19 +284,69 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
>  
>  	sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 * 4);
>  
> -	/* Select the RX work request for virtual address and for reposting. */
> +	/* inline_oob_size_div4 is read from the WQE in device-accessible RQ
> +	 * memory, so a malicious host in a CVM (or buggy firmware) could set
> +	 * it to push the SGE past the RQ buffer.  Bounds-check the SGE it
> +	 * locates before dereferencing sge->address below.  A validly posted
> +	 * WQE keeps the SGE inside the ring (worst case ends exactly at the
> +	 * buffer boundary); reject anything that would read past it.  The
> +	 * slot cannot be trusted here, so leak this RX WQE rather than repost
> +	 * the wrong one -- as in the SGE-address mismatch path below.
> +	 */
> +	if ((u8 *)(sge + 1) > (u8 *)rq->queue_mem_ptr + rq->queue_size) {
> +		dev_err(hwc->dev, "HWC RX: SGE past RQ buffer, oob_div4=%u\n",
> +			dma_oob->inline_oob_size_div4);
> +		return;
> +	}

[Severity: Low]
Does this check assert the invariant the driver actually programs?  For
every HWC RQ WQE, mana_gd_post_work_request() in gdma_main.c forces the
layout:

	if (wq->type == GDMA_RQ) {
		if (client_oob_size != 0)
			return -EINVAL;

		client_oob_size = INLINE_OOB_SMALL_SIZE;

		max_wqe_size = GDMA_MAX_RQE_SIZE;
	}

so inline_oob_size_div4 is always 2 and the SGE always sits at wqe + 16
inside a single 32-byte GDMA_WQE_BU_SIZE WQE.

Since mana_gd_get_wqe_ptr() masks the offset into the ring, a WQE starts at
most at queue_size - 32, so the ring-wide comparison only rejects a
corrupted inline_oob_size_div4 when the completed WQE happens to be the
last BU of the ring.  For every other WQE all eight 3-bit values pass, and
sge->address is then read from somewhere in wqe + 8 ... wqe + 36, i.e. the
client OOB area, the real SGE's mem_key/size words, or the next WQE's
header.

Those bytes are then rejected by the index and address checks below, so the
visible effect is the leaked WQE discussed further down rather than
corruption.  Would checking inline_oob_size_div4 == 2 (or that the SGE lies
within this WQE's own 32-byte BU) be both stricter and cheaper here?

[Severity: Low]
Is the pointer that gets dereferenced provably the pointer that was
checked?  sge is computed from a plain load of dma_oob->inline_oob_size_div4,
and both dma_oob and sge point into the DMA-mapped RQ ring
(rq->queue_mem_ptr), which is shared host memory in a CVM.

The load is neither volatile nor barriered, and the field is already read a
second time for the dev_err() argument, so the compiler may rematerialize
the address computation at the later sge->address dereferences.  If the
host changed the field in between, the dereferenced SGE can sit past
queue_mem_ptr + queue_size, which is what the check was added to prevent.

sge->address is likewise loaded up to three times: for the rx_req_idx
computation, for the equality test, and for the dev_err().

A few lines below, the same function applies the opposite discipline to the
response header with an explicit comment about CVM TOCTOU:

	msg_id = READ_ONCE(resp->response.hwc_msg_id);

Would snapshotting inline_oob_size_div4 and sge->address once with
READ_ONCE() into locals, and checking and using only the locals, make the
two new checks consistent with that?

For the record, the index/address binding itself does look sound: since
mana_hwc_alloc_dma_buf() sets buf_sge_addr[i] == dma_handle +
i * max_resp_msg_size, the equality test can only pass for the slot that
matches the second load, so the double fetch cannot produce a mismatched
index/address pair.

> +
> +	/* Recover the originating RX slot from the SGE address.  Of the three
> +	 * terms here only sge->address lives in device-accessible RQ memory;
> +	 * rq_base_addr and max_resp_msg_size are driver-private constants.  An
> +	 * in-range but wrong/unaligned SGE (corrupted WQE, or a malicious host
> +	 * in a CVM) would otherwise truncate onto a neighbouring slot, letting
> +	 * us read a stale response that could complete the wrong, reused
> +	 * in-flight request.  Require the index to be in range AND the address
> +	 * to exactly match the value the driver posted for that slot.
> +	 */
>  	rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
>  	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size;
>  
> -	if (rx_req_idx >= hwc_rxq->msg_buf->num_reqs) {
> -		dev_err(hwc->dev, "HWC RX: wrong rx_req_idx=%llu, num_reqs=%u\n",
> -			rx_req_idx, hwc_rxq->msg_buf->num_reqs);
> +	if (rx_req_idx >= hwc_rxq->queue_depth) {
> +		/* Cannot trust which WQE this is, so we cannot safely repost
> +		 * it; leak one RX WQE and bail.  An out-of-range index means
> +		 * a corrupted SGE from hardware (or host tampering), an
> +		 * unrecoverable device error.
> +		 */
> +		dev_err(hwc->dev, "HWC RX: SGE idx %llu out of range\n",
> +			rx_req_idx);
>  		return;
>  	}
>  
>  	rx_req = &hwc_rxq->msg_buf->reqs[rx_req_idx];
> +	if (sge->address != (u64)rx_req->buf_sge_addr) {
> +		/* In-range index but the address does not match what the
> +		 * driver posted for that slot; the same unrecoverable case,
> +		 * so leak this WQE rather than repost the wrong one.
> +		 */
> +		dev_err(hwc->dev, "HWC RX: invalid SGE address %llx (idx=%llu)\n",
> +			sge->address, rx_req_idx);
> +		return;
> +	}
> +

[Severity: Medium]
Can these two new returns drain the RQ credits to zero?

The comments describe them as leaking "one RX WQE", but nothing bounds how
often they can be taken, and the trigger values
(dma_oob->inline_oob_size_div4, sge->address) live in device-accessible RQ
memory, which the changelog itself treats as host-writable in a CVM.

The HWC WQs are created with spec.monitor_avl_buf = false, and nothing ever
advances rq->tail for HWC queues (queue->tail is only initialised to 0 in
mana_gd_create_hwc_queue(); the only wq->tail += num_units in the driver is
the ethernet path in mana_en.c).  The device's view of posted RQ buffers
therefore follows wq->head, which only advances through
mana_hwc_post_rx_wqe() -> mana_gd_post_and_ring():

	wq->head += wqe_size / GDMA_WQE_BU_SIZE;

So each early return without a repost permanently reduces the posted depth,
while hwc_rxq->queue_depth and msg_buf->num_reqs keep reporting the full
depth.

The out-of-range-index path already behaved this way before the patch; this
adds two more such paths.  After HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH
occurrences the RQ is empty, no HWC response can be received, and every
command times out.  Since the timeout path in mana_hwc_send_request()
latches hwc->hwc_timeout = 1 and there is no HWC re-arm or reset path, is
there any way out of that state?

Would it be reasonable to count the leaked WQEs and signal the device as
unusable (or reset the channel) once the posted depth drops, rather than
silently continuing?

>  	resp = (struct gdma_resp_hdr *)rx_req->buf_va;
>  
> +	/* Validate resp_len covers the response header before reading
> +	 * hwc_msg_id.  A short response leaves stale data from the
> +	 * previous buffer occupant, which could match a live slot and
> +	 * complete the wrong request.
> +	 */
> +	if (rx_oob->tx_oob_data_size < sizeof(*resp)) {
> +		dev_err(hwc->dev, "HWC RX: short resp_len=%u\n",
> +			rx_oob->tx_oob_data_size);
> +		mana_hwc_post_rx_wqe(hwc_rxq, rx_req);
> +		return;
> +	}
> +

[Severity: High]
Does this early return leave the waiting sender uncompleted?

At this point the slot has been positively identified (sge->address was
matched exactly against rx_req->buf_sge_addr), but the function returns
before hwc_msg_id is read, so no hwc_caller_ctx is completed.

Before this patch the same completion reached mana_hwc_handle_resp(), which
applies the identical predicate through mana_hwc_verify_resp_msg():

	if (resp_len < sizeof(*resp_msg))
		return -EPROTO;

and then failed and woke the sender:

	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
	if (err)
		goto out;
	...
out:
	ctx->error = err;
	mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
	complete(&ctx->comp_event);

Post-patch the sender instead sits in mana_hwc_send_request() for the whole
hwc->hwc_timeout and then latches the channel:

	if (!wait_for_completion_timeout(&ctx->comp_event,
					 (msecs_to_jiffies(hwc->hwc_timeout)))) {
		...
		/* Reduce further waiting if HWC no response */
		if (hwc->hwc_timeout > 1)
			hwc->hwc_timeout = 1;

		err = -ETIMEDOUT;

hwc_timeout is only assigned at channel creation, by an
HWC_DATA_CFG_HWC_TIMEOUT reconfig event, or to 0 on destroy, so it is never
restored and every later command waits 1 ms.  At the end of this series the
same branch also sets hwc->hwc_timed_out = true, after which
mana_hwc_get_msg_index() fails all future commands with -ETIMEDOUT.

Would it be better to hand the short response to mana_hwc_handle_resp()
(or otherwise complete the identified slot with -EPROTO) so a single
malformed response does not convert into a channel-wide failure?

Also, the comment says a short response "could match a live slot and
complete the wrong request".  Is that accurate for the pre-patch code?
mana_hwc_verify_resp_msg() rejected the length before the memcpy(), so no
stale bytes were copied into a caller buffer; the pre-patch outcome looks
like a spurious -EPROTO on one command.

[ ... ]

Re: [PATCH net v3 5/6] net: mana: fix HWC teardown safety with setup_active flag and destroy ordering

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mana: fix HWC teardown safety with setup_active flag and destroy ordering

This adds a setup_active flag to struct hw_channel_context so the driver
knows when the device has live MST entries, reorders
mana_hwc_destroy_channel() to destroy the HWC CQ (and thereby deregister
the EQ) before freeing the TXQ/RXQ buffers, and leaks the HWC resources
when mana_smc_teardown_hwc() fails instead of freeing memory the device
may still DMA into.

A few questions on the new error paths below.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index e378b8ec97c9a..1603968d7989a 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -768,20 +769,33 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
>  	if (err)
>  		return err;
>  
> -	if (!wait_for_completion_timeout(&hwc->hwc_init_eqe_comp, 60 * HZ))
> -		return -ETIMEDOUT;
> +	/* setup_hwc activated MST entries — hardware can now DMA into
> +	 * our queue buffers.  If anything below fails, we must tear
> +	 * down before returning so the caller doesn't need to track
> +	 * whether setup_hwc succeeded.
> +	 */
> +	hwc->setup_active = true;

[Severity: High]
This isn't a bug introduced by this patch, but now that setup_active is
the only gate for sending DESTROY_HWC, is the flag armed late?

mana_smc_setup_hwc() in shm_channel.c publishes the EQ/CQ/RQ/SQ frame
addresses to the PF with the last of its writel()s, and only then polls
for the response:

	/* Write 256-message buffer to shared memory (final 32-bit write
	 * triggers HW to set possession bit to PF).
	 */
	dword = (u32 *)shm_buf;
	for (i = 0; i < SMC_APERTURE_DWORDS; i++)
		writel(*dword++, sc->base + i * SMC_BASIC_UNIT);
	...
	err = mana_smc_read_response(...);
	if (err) {
		dev_err(sc->dev, "Error when setting up HWC: %d\n", err);
		return err;
	}

So an error return here does not mean the request never reached the PF.
mana_smc_read_response() can return -ETIMEDOUT after the 20 s poll in
mana_smc_poll_register() (the PF still owns shmem while it processes the
establish) or -EPROTO on a non-zero status.

In that case mana_hwc_establish_channel() returns with setup_active still
false, and the cleanup path frees the very buffers whose addresses were
handed to the PF:

mana_hwc_create_channel()
  out: mana_hwc_destroy_channel()
         if (hwc->setup_active)      /* false, no DESTROY_HWC is sent */
         mana_hwc_destroy_cq()       /* frees EQ/CQ DMA buffers */
         mana_hwc_destroy_wq()       /* frees RQ/SQ DMA buffers */

Should the flag be set before mana_smc_setup_hwc() is called, or on an
ambiguous failure, so that DESTROY_HWC is still attempted in this window?

>  
>  	*q_depth = hwc->hwc_init_q_depth_max;
>  	*max_req_msg_size = hwc->hwc_init_max_req_msg_size;
>  	*max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;
>  
>  	/* Both were set in mana_hwc_init_event_handler(). */
> -	if (WARN_ON(cq->id >= gc->max_num_cqs))
> -		return -EPROTO;
> +	if (WARN_ON(cq->id >= gc->max_num_cqs)) {
> +		err = -EPROTO;
> +		goto teardown;
> +	}
>  
>  	cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
> -	if (!cq_table)
> -		return -ENOMEM;
> +	if (!cq_table) {
> +		err = -ENOMEM;
> +		goto teardown;
> +	}
>  
>  	rcu_assign_pointer(cq_table[cq->id], cq);
>  	/* Publish the fully-initialised table last; pairs with the
> @@ -790,6 +804,16 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
>  	rcu_assign_pointer(gc->cq_table, cq_table);
>  
>  	return 0;
> +
> +teardown:
> +	{
> +		int td_err = mana_smc_teardown_hwc(&gc->shm_channel, false);
> +
> +		if (!td_err)
> +			hwc->setup_active = false;
> +
> +		return td_err ? td_err : err;
> +	}
>  }

[ ... ]

> @@ -907,11 +931,38 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
>  	if (!hwc)
>  		return;
>  
> -	/* gc->max_num_cqs is set in mana_hwc_init_event_handler(). If it's
> -	 * non-zero, the HWC worked and we should tear down the HWC here.
> +	/* Tear down the HWC if setup_hwc previously activated MST entries.
> +	 * This is the definitive flag — unlike max_num_cqs which depends
> +	 * on the init EQE arriving.
> +	 *
> +	 * If teardown fails the device may still have active MST entries
> +	 * and can DMA into the HWC queue buffers.  Freeing them would risk
> +	 * memory corruption on systems without an IOMMU to fault the stale
> +	 * DMA, so leak the HWC resources instead of handing the pages back
> +	 * to the allocator.  Keep setup_active set so the failure is not
> +	 * mistaken for a clean teardown.
>  	 */
> -	if (gc->max_num_cqs > 0)
> -		mana_smc_teardown_hwc(&gc->shm_channel, false);
> +	if (hwc->setup_active) {
> +		int td_err = mana_smc_teardown_hwc(&gc->shm_channel, false);
> +
> +		if (td_err) {
> +			dev_err(gc->dev,
> +				"HWC teardown failed: %d, leaking resources\n",
> +				td_err);
> +			return;
> +		}

[Severity: Medium]
Does this early return also leak the objects the hardware cannot reach?

It skips the whole tail of mana_hwc_destroy_channel():

	kfree(hwc->caller_ctx);
	hwc->caller_ctx = NULL;
	mana_gd_free_res_map(&hwc->inflight_msg_res);
	...
	kfree(hwc);
	gc->hwc.driver_data = NULL;
	gc->hwc.gdma_context = NULL;

	old_cq_table = rcu_replace_pointer(gc->cq_table, NULL, true);
	synchronize_rcu();
	vfree(old_cq_table);

The comment in hw_channel.h calls this "the terminal destroy_channel
path", but mana_gd_cleanup_device() is also reached from
mana_gd_suspend(), from the mana_gd_probe() error path and from the
mana_gd_resume() error path, all on the same gdma_context.
mana_serv_reset() runs mana_gd_suspend() followed by mana_gd_resume()
exactly when the HWC is not responding (it sets hwc->hwc_timeout = 0),
which is when the DESTROY_HWC poll is most likely to time out.

On the following re-create, mana_hwc_create_channel() drops the last
pointer to the abandoned context:

	hwc = kzalloc_obj(*hwc);
	...
	gd->driver_data = hwc;

so hwc, hwc->caller_ctx and the inflight_msg bitmap become unreachable,
and mana_hwc_establish_channel() ends with a bare

	rcu_assign_pointer(gc->cq_table, cq_table);

which neither captures, synchronize_rcu()s nor vfree()s the table still
published by the previous instance. Would that leak one vcalloc'd
max_num_cqs table per failed reset cycle?

That also seems to differ from the contract this patch adds in gdma.h,
which says the base table is "replaced+freed only by
mana_hwc_destroy_channel() (and the create-time reinit)" — the
create-time reinit only replaces it. Would freeing these after
mana_gd_remove_irqs(), or at least having establish_channel() free or
warn on an already-published cq_table, keep the two consistent?

> +
> +		hwc->setup_active = false;
> +	}
> +
> +	/* Tear down the HWC CQ object first — mana_hwc_destroy_cq()
> +	 * both unpublishes the CQ from cq_table (+synchronize_rcu) and
> +	 * deregisters the HWC EQ from the interrupt handler list (via
> +	 * mana_gd_deregister_irq + synchronize_rcu), guaranteeing no
> +	 * interrupt handler can access RQ/TXQ buffers after this point.
> +	 */
> +	if (hwc->cq)
> +		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
>  
>  	if (hwc->txq)
>  		mana_hwc_destroy_wq(hwc, hwc->txq);
> @@ -919,17 +970,6 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
>  	if (hwc->rxq)
>  		mana_hwc_destroy_wq(hwc, hwc->rxq);
>  
> -	if (hwc->cq)
> -		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
> -
> -	/* Reset only after mana_hwc_destroy_cq() above has run with a valid
> -	 * max_num_cqs so mana_gd_destroy_cq() clears the CQ table slot and
> -	 * waits out in-flight EQ handlers (synchronize_rcu) before the CQ is
> -	 * freed.  Clearing it earlier would make that path early-return and
> -	 * skip the slot clear, leaving a dangling cq_table entry.
> -	 */
> -	gc->max_num_cqs = 0;
> -
>  	kfree(hwc->caller_ctx);
>  	hwc->caller_ctx = NULL;
>  
> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> index 0b48ded0aecda..74eccc80cf6b4 100644
> --- a/include/net/mana/gdma.h
> +++ b/include/net/mana/gdma.h
> @@ -416,7 +416,13 @@ struct gdma_context {
>  	/* L2 MTU */
>  	u16 adapter_mtu;
>  
> -	/* This maps a CQ index to the queue structure. */
> +	/* Size of cq_table, i.e. the largest valid CQ index + 1.  Set once
> +	 * when cq_table is allocated and treated as immutable for the
> +	 * table's lifetime (a bound only) -- it is never reset on teardown.
> +	 * cq_table == NULL is the sole "table torn down" signal, so every
> +	 * cq_table[id] access must guard with both !cq_table (gone) and
> +	 * id >= max_num_cqs (out of bounds).
> +	 */
>  	unsigned int		max_num_cqs;

[Severity: High]
This is a pre-existing issue, but is the "set once ... treated as
immutable" invariant actually enforced anywhere?

The only writer is mana_hwc_init_event_handler(), which stores the
device-supplied EQE payload unconditionally, with no check that
gc->cq_table is already non-NULL:

	case HWC_INIT_DATA_MAX_NUM_CQS:
		gd->gdma_context->max_num_cqs = val;
		break;

The size passed to vcalloc(gc->max_num_cqs, sizeof(*cq_table)) in
mana_hwc_establish_channel() is not recorded anywhere else, and the HWC
EQ callback stays registered for the channel's whole lifetime, so a later
GDMA_EQE_HWC_INIT_DATA event can raise the bound above the allocated
size. Then in mana_gd_process_eqe(), in hard IRQ context:

	cq_table = rcu_dereference(gc->cq_table);
	if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs || !cq_table))
		break;

	cq = rcu_dereference(cq_table[cq_id]);
	if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id))

Can this read past the end of the vmalloc'd array and then dereference
whatever pointer-sized value it found (cq->type, cq->id, and on success
the indirect call cq->cq.callback)? The same bound guards the
rcu_assign_pointer() writes in mana_create_txq()/mana_create_rxq() and in
mana_ib_install_cq_cb():

	cq_table = rcu_dereference_protected(gc->cq_table, true);
	if (!cq_table || cq->queue.id >= gc->max_num_cqs)
		return -EINVAL;

Given that this patch makes the bound load-bearing by removing its reset,
would it be worth latching the allocated size in its own field, or
ignoring HWC_INIT_DATA_MAX_NUM_CQS once cq_table is non-NULL, alongside
the new documentation?

> diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
> index 73671f479399a..3d8543acb5cc0 100644
> --- a/include/net/mana/hw_channel.h
> +++ b/include/net/mana/hw_channel.h
> @@ -200,6 +200,15 @@ struct hw_channel_context {
>  	u32 pf_dest_vrcq_id;
>  	u32 hwc_timeout;
>  
> +	/* Set after mana_smc_setup_hwc() succeeds (hardware has active
> +	 * MST entries).  Cleared only after mana_smc_teardown_hwc()
> +	 * succeeds, on both the recoverable establish_channel path and the
> +	 * terminal destroy_channel path.  If teardown fails it stays set:
> +	 * establish_channel() skips its retry and destroy_channel() leaks
> +	 * the HWC rather than free buffers the device may still DMA into.
> +	 */
> +	bool setup_active;

[Severity: Medium]
setup_active records device-scoped state (MST entries programmed for this
VF), but it lives in the per-instance hw_channel_context. Does the leak
path lose that record?

After the teardown failure, mana_hwc_destroy_channel() returns with
setup_active still true on a context that mana_gd_cleanup_device() then
walks away from (it also goes on to call mana_gd_remove_irqs()). On the
next attempt:

mana_gd_resume()
  mana_gd_setup()
    mana_hwc_create_channel()
      hwc = kzalloc_obj(*hwc);      /* setup_active == false */
      gd->driver_data = hwc;        /* old context dropped */
      mana_hwc_establish_channel()  /* second ESTABLISH_HWC */

Since shm_channel.c pairs ESTABLISH_HWC with DESTROY_HWC, does this leave
the first HWC's MST entries armed for the device's lifetime, with a later
clean teardown sending only one DESTROY_HWC? That looks like it defeats
the stated intent that "the failure is not mistaken for a clean
teardown".

Would keeping the flag in struct gdma_context (which survives
suspend/resume, as shm_channel does), or refusing to re-establish while a
previous HWC is known live, keep the driver's record in step with the
device?

Cross-instance finding from sashiko-gemini (b1a5d3a1fd2b96eddef89c85fad512dd15f2a00ad5dda70d873898d2dd806258):
[Severity: Medium]
Redundant HWC teardown in `mana_hwc_establish_channel()` causes a double hardware timeout (up to 120s stall) and masks the original initialization error code.

Re: [PATCH net v3 6/6] net: mana: fix stale HWC response after command timeout

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mana: fix stale HWC response after command timeout

This reworks the MANA HWC command slot lifetime so a late response cannot
memcpy into a caller buffer that is already gone, and cannot complete a
different command through a recycled msg_id.  Each caller_ctx gains a
spinlock, a refcount and an -EINPROGRESS sentinel, the semaphore is
replaced by a waitqueue plus the inflight bitmap, and a first timeout
latches a new hwc_timed_out flag.

A few questions about the new slot lifetime and the timeout latch below.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 1603968d7989a..d92032b466af2 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -7,25 +7,49 @@
>  #include <linux/pci.h>
>  #include <linux/vmalloc.h>
>  
> +/* Acquire a free message slot from the inflight bitmap.  Returns
> + * -ETIMEDOUT if a prior HWC command has timed out (preserving the
> + * error code callers expect).
> + */
>  static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
>  {
>  	struct gdma_resource *r = &hwc->inflight_msg_res;
>  	unsigned long flags;
>  	u32 index;
>  
> -	down(&hwc->sema);
> +	for (;;) {
> +		spin_lock_irqsave(&r->lock, flags);
>  
> -	spin_lock_irqsave(&r->lock, flags);
> +		if (hwc->hwc_timed_out) {
> +			spin_unlock_irqrestore(&r->lock, flags);
> +			return -ETIMEDOUT;
> +		}
>  
> -	index = find_first_zero_bit(hwc->inflight_msg_res.map,
> -				    hwc->inflight_msg_res.size);
> +		index = find_first_zero_bit(r->map, r->size);
> +		if (index < r->size) {
> +			struct hwc_caller_ctx *ctx;
> +
> +			bitmap_set(r->map, index, 1);
> +			ctx = &hwc->caller_ctx[index];
> +			reinit_completion(&ctx->comp_event);
> +			refcount_set(&ctx->refcnt, 1);
> +			ctx->responded = false;
> +			ctx->msg_id = index;
> +			ctx->error = -EINPROGRESS;
> +			spin_unlock_irqrestore(&r->lock, flags);
> +			break;
> +		}

[Severity: High]
bitmap_set() publishes the slot to the response path here, while the
response-side reference is only taken much later in
mana_hwc_send_request():

	/* Take handle_resp's ref before posting ... */
	refcount_inc(&ctx->refcnt);

In between, mana_hwc_handle_resp() already accepts the slot.  Its only
gates are the range check and a lockless test_bit():

	if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
		dev_err(hwc->dev, "hwc_rx: invalid msg_id = %u\n", msg_id);
		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
		return;
	}

and it ends with an unconditional hwc_ctx_put().  msg_id comes from
host-writable DMA memory (READ_ONCE()d out of the RX buffer), and
responded is false for a fresh allocation, so the new duplicate guard
does not catch a response naming a slot that is still in this pre-post
window.

Can such a response drop refcnt 1->0, clear the bitmap bit and wake
msg_waitq while the sender still owns the slot?  If so, does that give:

  - the sender's refcount_inc() running on 0, i.e. a "refcount_t:
    addition on 0; use-after-free" splat plus permanent saturation,
    leaking that slot for good;

  - another sender being handed the same msg_id, sharing both
    txq->msg_buf->reqs[msg_id] and the same hwc_caller_ctx, so one
    command completes with the other command's payload, which is the
    cross-completion this patch is meant to remove;

  - a spurious complete() letting wait_for_completion_timeout() succeed
    with ctx->error still -EINPROGRESS, so -115 is handed back to
    callers?

The same window also covers the req_len > tx_wr->buf_len path that jumps
to out:, whose new comment states "no WQE was submitted so handle_resp()
cannot race here" and "refcount is 1".  Can the slot already have been
released by then, making that hwc_ctx_put() a second release?

[Severity: Medium]
comp_event, refcnt, responded, msg_id and error are (re)initialised here
under inflight_msg_res.lock only, while mana_hwc_handle_resp() reads and
writes responded and error under ctx->lock only and never takes
inflight_msg_res.lock.  The intersection of the two locksets is empty,
even though the header comment names ctx->lock as the protecting lock.

bitmap_set() runs before these field writes in the same critical section
and handle_resp()'s visibility gate is a lockless test_bit(), so can a CQ
interrupt on another CPU observe the bit, take ctx->lock and interleave
with this initialisation?  It could then see the stale responded == true
from the previous generation and silently drop a real response, or set
responded/error and complete() while they are being reset, after which
its hwc_ctx_put() consumes the new generation's initial reference.

reinit_completion() also writes x->done = 0 without the completion's
wait.lock, while handle_resp() calls complete() under ctx->lock.  Can a
completion be lost or spuriously observed that way?

> +		spin_unlock_irqrestore(&r->lock, flags);
>  
> -	bitmap_set(hwc->inflight_msg_res.map, index, 1);
> +		wait_event(hwc->msg_waitq,
> +			   hwc->hwc_timed_out ||
> +			   !bitmap_full(r->map, r->size));
>  
> -	spin_unlock_irqrestore(&r->lock, flags);
> +		if (hwc->hwc_timed_out)
> +			return -ETIMEDOUT;
> +	}

[Severity: Low]
hwc_timed_out is written under inflight_msg_res.lock in
mana_hwc_send_request(), but read as a plain load both in this
wait_event() condition and in the check right after it.  bitmap_full()
also reads r->map outside r->lock while other CPUs modify the same words
with non-atomic bitmap_set()/bitmap_clear().  Would READ_ONCE()/
WRITE_ONCE() (or evaluating the bitmap under r->lock) be preferable here
to keep KCSAN quiet?

Related: the read-modify-write in the timeout path

	if (hwc->hwc_timeout > 1)
		hwc->hwc_timeout = 1;

is unlocked while mana_hwc_init_event_handler() also writes hwc_timeout
from EQ interrupt context.  That access is not introduced by this patch,
it is in the baseline, but the patch makes the value load-bearing since a
zero or degraded value now latches the whole channel.

>  
>  	*msg_id = index;
> -
>  	return 0;
>  }
>  
> @@ -35,10 +59,17 @@ static void mana_hwc_put_msg_index(struct hw_channel_context *hwc, u16 msg_id)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&r->lock, flags);
> -	bitmap_clear(hwc->inflight_msg_res.map, msg_id, 1);
> +	bitmap_clear(r->map, msg_id, 1);
>  	spin_unlock_irqrestore(&r->lock, flags);
>  
> -	up(&hwc->sema);
> +	wake_up(&hwc->msg_waitq);
> +}
> +
> +static void hwc_ctx_put(struct hw_channel_context *hwc,
> +			struct hwc_caller_ctx *ctx)
> +{
> +	if (refcount_dec_and_test(&ctx->refcnt))
> +		mana_hwc_put_msg_index(hwc, ctx->msg_id);
>  }
>  
>  static int mana_hwc_verify_resp_msg(const struct hwc_caller_ctx *caller_ctx,
> @@ -114,22 +145,44 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
>  		resp_len = 0;
>  	}
>  
> -	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
> -	if (err)
> -		goto out;
> +	spin_lock(&ctx->lock);
>  
> -	ctx->status_code = resp_msg->status;
> +	if (ctx->responded) {
> +		/* A response for this slot was already delivered; this is a
> +		 * duplicate or replayed one.  Drop it so the hwc_ctx_put()
> +		 * a first response performs is not done twice, which would
> +		 * over-release the slot while the sender still owns it.
> +		 */
> +		spin_unlock(&ctx->lock);
> +		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> +		return;
> +	}
> +	ctx->responded = true;
>  
> -	memcpy(ctx->output_buf, resp_msg, resp_len);
> -out:
> -	ctx->error = err;
> +	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
>  
> -	/* Must post rx wqe before complete(), otherwise the next rx may
> -	 * hit no_wqe error.
> +	if (!err && ctx->output_buf) {
> +		ctx->status_code = resp_msg->status;
> +		memcpy(ctx->output_buf, resp_msg, resp_len);
> +		ctx->error = 0;
> +	} else if (ctx->output_buf) {
> +		/* Only overwrite error if the sender hasn't timed out
> +		 * or been force-completed by destroy.  When output_buf
> +		 * is NULL, a terminal error (-ENODEV or timeout) has
> +		 * already been set — preserve it so the sender doesn't
> +		 * see a spurious success.
> +		 */
> +		ctx->error = err;
> +	}

[Severity: Low]
Is this comment accurate?  It says that when output_buf is NULL "a
terminal error (-ENODEV or timeout) has already been set".

On the timeout path mana_hwc_send_request() only NULLs ctx->output_buf
and assigns its local err; ctx->error keeps the -EINPROGRESS sentinel
written by mana_hwc_get_msg_index().  Grepping
drivers/net/ethernet/microsoft, the only ENODEV in hw_channel.c is inside
this comment.

The comment also mentions senders "force-completed by destroy", but
mana_hwc_destroy_channel() tears down the CQ and WQs and kfree()s
caller_ctx without ever completing ctx->comp_event or writing ctx->error.

Could the comment be reworded to describe what the code actually
guarantees?

> +
> +	/* Post RX WQE before completing — the next response may arrive
> +	 * immediately and needs a posted buffer.
>  	 */
>  	mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> -
>  	complete(&ctx->comp_event);
> +	spin_unlock(&ctx->lock);
> +
> +	hwc_ctx_put(hwc, ctx);
>  }
>  
>  static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
> @@ -216,7 +269,12 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
>  
>  		switch (type) {
>  		case HWC_DATA_CFG_HWC_TIMEOUT:
> -			hwc->hwc_timeout = val;
> +			/* A zero timeout would make every command time out
> +			 * immediately and latch hwc_timed_out, disabling the
> +			 * channel.  Ignore it and keep the positive default.
> +			 */
> +			if (val)
> +				hwc->hwc_timeout = val;
>  			break;

[Severity: Medium]
This guard covers the reconfig EQE, but is the other device-supplied
source of the same field covered?  mana_gd_query_hwc_timeout() still
does:

	*timeout_val = resp.timeout_ms;

and mana_gd_verify_vf_version() calls it as
mana_gd_query_hwc_timeout(pdev, &hwc->hwc_timeout), so a device-reported
timeout_ms of 0 reaches hwc->hwc_timeout unfiltered.

With the new code, does the next command then wait msecs_to_jiffies(0),
time out immediately and latch hwc_timed_out, after which every command
fails with -ETIMEDOUT out of mana_hwc_get_msg_index() without being
posted, for the remaining life of the channel?

>  
>  		case HWC_DATA_HW_LINK_CONNECT:
> @@ -708,7 +766,7 @@ static int mana_hwc_init_inflight_msg(struct hw_channel_context *hwc,
>  {
>  	int err;
>  
> -	sema_init(&hwc->sema, num_msg);
> +	init_waitqueue_head(&hwc->msg_waitq);

[Severity: Low]
The commit message says:

  Replace the depth-1 semaphore with a waitqueue + bitmap so a slot held
  past a timeout does not deadlock admission and timed-out waiters can be
  released.

Was the semaphore depth-1?  The removed line is sema_init(&hwc->sema,
num_msg), and mana_hwc_init_queues() calls
mana_hwc_init_inflight_msg(hwc, q_depth), so it admitted up to q_depth
concurrent senders, matching the bitmap size.

The reason a counting semaphore no longer fits looks like the fact that a
bitmap bit can now outlive its sender (held by handle_resp's reference),
which desynchronises the semaphore count from bitmap occupancy.  Could
the message be corrected, given this is a Fixes:-tagged patch headed for
stable?

>  
>  	err = mana_gd_alloc_res_map(num_msg, &hwc->inflight_msg_res);
>  	if (err)

[ ... ]

> @@ -999,13 +1062,17 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
>  	struct hwc_wq *txq = hwc->txq;
>  	struct gdma_req_hdr *req_msg;
>  	struct hwc_caller_ctx *ctx;
> +	unsigned long flags;
>  	u32 dest_vrcq = 0;
>  	u32 dest_vrq = 0;
>  	u32 command;
> +	u32 status;
>  	u16 msg_id;
>  	int err;
>  
> -	mana_hwc_get_msg_index(hwc, &msg_id);
> +	err = mana_hwc_get_msg_index(hwc, &msg_id);
> +	if (err)
> +		return err;
>  
>  	tx_wr = &txq->msg_buf->reqs[msg_id];
>  

[ ... ]

> @@ -1034,8 +1104,14 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
>  		dest_vrcq = hwc->pf_dest_vrcq_id;
>  	}
>  
> +	/* Take handle_resp's ref before posting — hardware can respond
> +	 * immediately after the doorbell ring.
> +	 */
> +	refcount_inc(&ctx->refcnt);
> +
>  	err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
>  	if (err) {
> +		refcount_dec(&ctx->refcnt);
>  		dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n", err);
>  		goto out;
>  	}
> @@ -1046,31 +1122,86 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
>  			dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
>  				command, hwc->hwc_timeout);
>  
> -		/* Reduce further waiting if HWC no response */
> +		/* NULL out output_buf so a late handle_resp() won't write
> +		 * into the caller's buffer after the sender returns, then
> +		 * check whether handle_resp() already delivered a valid
> +		 * response between the timeout firing and this lock
> +		 * acquisition — ctx->error != -EINPROGRESS means it ran.
> +		 */
> +		spin_lock_irqsave(&ctx->lock, flags);
> +		ctx->output_buf = NULL;
> +		err = ctx->error;
> +		status = ctx->status_code;
> +		spin_unlock_irqrestore(&ctx->lock, flags);
> +
> +		if (err != -EINPROGRESS) {
> +			/* handle_resp() delivered a valid response just after
> +			 * the timeout fired.  The hardware is alive, so use
> +			 * the response and leave the channel usable; do not
> +			 * latch hwc_timed_out or degrade hwc_timeout for what
> +			 * turned out to be a transient race.
> +			 */
> +			hwc_ctx_put(hwc, ctx);
> +			goto check_status;
> +		}
> +
> +		/* Genuine timeout: no response arrived.  Reduce further
> +		 * waiting, and mark the channel timed out under the bitmap
> +		 * lock so get_msg_index() cannot acquire new slots after this.
> +		 */
>  		if (hwc->hwc_timeout > 1)
>  			hwc->hwc_timeout = 1;
>  
> +		spin_lock_irqsave(&hwc->inflight_msg_res.lock, flags);
> +		hwc->hwc_timed_out = true;
> +		spin_unlock_irqrestore(&hwc->inflight_msg_res.lock, flags);
> +		wake_up_all(&hwc->msg_waitq);

[Severity: High]
Once hwc_timed_out is set here, is anything expected to clear it?
Grepping drivers/net/ethernet/microsoft and include/net/mana, this is the
only write besides the kzalloc zeroing, so the flag stays set for the
life of the hw_channel_context.

From then on mana_hwc_get_msg_index() bails out before a slot is even
allocated:

	if (hwc->hwc_timed_out) {
		spin_unlock_irqrestore(&r->lock, flags);
		return -ETIMEDOUT;
	}

so mana_hwc_post_tx_wqe() is never reached and no doorbell is rung for
any later command.  Every GDMA control command funnels through
mana_gd_send_request()->mana_hwc_send_request(), including the teardown
commands mana_gd_disable_queue(), mana_gd_destroy_dma_region() and
mana_gd_deregister_device(), all of which treat errors as non-fatal and
then dma_free_coherent() the backing pages anyway.

Before this patch those commands were still built, posted and the
doorbell rung (only the wait was shortened to 1 ms), so the device did
act on destroy and disable requests.  Can this leave the device holding
registered DMA regions that point at freed pages, which is the situation
the comment in mana_hwc_destroy_channel() describes as risking memory
corruption on systems without an IOMMU?

The commit message describes the latch only as:

  On a genuine timeout the channel is marked hwc_timed_out and further
  mana_hwc_get_msg_index() callers fail with -ETIMEDOUT instead of
  reusing a slot whose response may still arrive.

Could it state that the latch is channel-wide, unconditional and never
cleared, given the per-slot refcount already prevents reuse of a msg_id
whose response is outstanding?

[Severity: High]
Is the driver's own zero-timeout mode affected here too?  mana_serv_reset()
in gdma_main.c does:

	/* HWC is not responding in this case, so don't wait */
	hwc->hwc_timeout = 0;

	dev_info(&pdev->dev, "MANA reset cycle start\n");

	mana_gd_suspend(pdev, PMSG_SUSPEND);

and mana_gd_suspend() runs mana_rdma_remove() (which sends
mana_gd_deregister_device()), mana_remove(&gc->mana, true) and
mana_gd_cleanup_device(), all of which issue HWC commands.

For the first of those, wait_for_completion_timeout() with
msecs_to_jiffies(0) returns 0, ctx->error is still -EINPROGRESS, so this
branch runs: hwc->hwc_timeout > 1 is false and hwc_timed_out is set
unconditionally.  Do the remaining teardown commands then abort in
mana_hwc_get_msg_index() without ever reaching the SQ, turning the
intended fire-and-forget teardown into no teardown at all while the
buffers are still freed?

The patch already guards the device-reported value with if (val) for this
exact reason.  Should the in-driver zero assignment be handled as well,
for instance by not latching when hwc_timeout is 0, since mana_need_log()
already treats hwc_timeout == 0 as a legitimate "don't wait, don't log"
state?

>  
>  		err = -ETIMEDOUT;
> -		goto out;
> +		hwc_ctx_put(hwc, ctx);
> +		goto done;
>  	}
>  
> -	if (ctx->error) {
> -		err = ctx->error;
> -		goto out;
> -	}
> +	/* NULL output_buf so a late handle_resp() won't memcpy into
> +	 * the caller's buffer after the sender exits.  Read error and
> +	 * status_code under the same lock — after hwc_ctx_put the slot
> +	 * may be reused and these fields overwritten.
> +	 */
> +	spin_lock_irqsave(&ctx->lock, flags);
> +	ctx->output_buf = NULL;
> +	err = ctx->error;
> +	status = ctx->status_code;
> +	spin_unlock_irqrestore(&ctx->lock, flags);
> +	hwc_ctx_put(hwc, ctx);
> +
> +check_status:
> +	if (err)
> +		goto done;

[ ... ]

> +
> +	err = 0;
> +	goto done;
>  out:
> -	mana_hwc_put_msg_index(hwc, msg_id);
> +	/* Pre-post error paths: no WQE was submitted so handle_resp()
> +	 * cannot race here.  refcount is 1 (no second ref taken).
> +	 */
> +	ctx = hwc->caller_ctx + msg_id;
> +	spin_lock_irqsave(&ctx->lock, flags);
> +	ctx->output_buf = NULL;
> +	spin_unlock_irqrestore(&ctx->lock, flags);
> +	hwc_ctx_put(hwc, ctx);
> +done:
>  	return err;
>  }
> diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
> index 3d8543acb5cc0..5a55cedf0607a 100644
> --- a/include/net/mana/hw_channel.h
> +++ b/include/net/mana/hw_channel.h
> @@ -173,6 +173,23 @@ struct hwc_caller_ctx {
>  
>  	u32 error; /* Linux error code */
>  	u32 status_code;

[Severity: Low]
error stays declared as u32 while mana_hwc_get_msg_index() now stores
ctx->error = -EINPROGRESS into it, and mana_hwc_send_request() reads it
back into an int and compares it as signed with if (err != -EINPROGRESS).
The decision whether to latch the whole channel therefore depends on an
out-of-range u32 to int conversion rather than on the declared type.

Should the field become int error, matching its own "Linux error code"
comment, or the sentinel be made unsigned-safe?

> +
> +	/* Protects output_buf against concurrent access from
> +	 * handle_resp() (CQ interrupt) and the sender timeout path.
> +	 */
> +	spinlock_t lock;
> +
> +	/* Tracks sender + handle_resp ownership.  The last put
> +	 * (refcount reaches 0) releases the bitmap slot.
> +	 */
> +	refcount_t refcnt;
> +	u16 msg_id;
> +
> +	/* Set under lock by the first handle_resp() for this slot so a
> +	 * duplicate or replayed response is dropped instead of consuming
> +	 * the response-side reference a second time.
> +	 */
> +	bool responded;
>  };
>  
>  struct hw_channel_context {
> @@ -193,13 +210,19 @@ struct hw_channel_context {
>  	u32 hwc_timeout;
>  
> +	/* Set on first HWC timeout.  Causes get_msg_index() to return
> +	 * -ETIMEDOUT instead of waiting, draining all queued senders.
> +	 */
> +	bool hwc_timed_out;
> +

Could this comment also note that the state is never cleared, so it is
terminal for the lifetime of the channel?

RE: [EXTERNAL] Re: [PATCH net v3 6/6] net: mana: fix stale HWC response after command timeout

[Long Li]

> This is an AI-generated review of your patch. The human sending this email has
> considered the AI review valid, or at least plausible.
> ---
> net: mana: fix stale HWC response after command timeout
> 
> This reworks the MANA HWC command slot lifetime so a late response cannot
> memcpy into a caller buffer that is already gone, and cannot complete a
> different command through a recycled msg_id.  Each caller_ctx gains a spinlock,
> a refcount and an -EINPROGRESS sentinel, the semaphore is replaced by a
> waitqueue plus the inflight bitmap, and a first timeout latches a new
> hwc_timed_out flag.
> 
> A few questions about the new slot lifetime and the timeout latch below.
> 
> > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > index 1603968d7989a..d92032b466af2 100644
> > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > @@ -7,25 +7,49 @@
> >  #include <linux/pci.h>
> >  #include <linux/vmalloc.h>
> >
> > +/* Acquire a free message slot from the inflight bitmap.  Returns
> > + * -ETIMEDOUT if a prior HWC command has timed out (preserving the
> > + * error code callers expect).
> > + */
> >  static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16
> > *msg_id)  {
> >  	struct gdma_resource *r = &hwc->inflight_msg_res;
> >  	unsigned long flags;
> >  	u32 index;
> >
> > -	down(&hwc->sema);
> > +	for (;;) {
> > +		spin_lock_irqsave(&r->lock, flags);
> >
> > -	spin_lock_irqsave(&r->lock, flags);
> > +		if (hwc->hwc_timed_out) {
> > +			spin_unlock_irqrestore(&r->lock, flags);
> > +			return -ETIMEDOUT;
> > +		}
> >
> > -	index = find_first_zero_bit(hwc->inflight_msg_res.map,
> > -				    hwc->inflight_msg_res.size);
> > +		index = find_first_zero_bit(r->map, r->size);
> > +		if (index < r->size) {
> > +			struct hwc_caller_ctx *ctx;
> > +
> > +			bitmap_set(r->map, index, 1);
> > +			ctx = &hwc->caller_ctx[index];
> > +			reinit_completion(&ctx->comp_event);
> > +			refcount_set(&ctx->refcnt, 1);
> > +			ctx->responded = false;
> > +			ctx->msg_id = index;
> > +			ctx->error = -EINPROGRESS;
> > +			spin_unlock_irqrestore(&r->lock, flags);
> > +			break;
> > +		}
> 
> [Severity: High]
> bitmap_set() publishes the slot to the response path here, while the response-
> side reference is only taken much later in
> mana_hwc_send_request():
> 
> 	/* Take handle_resp's ref before posting ... */
> 	refcount_inc(&ctx->refcnt);
> 
> In between, mana_hwc_handle_resp() already accepts the slot.  Its only gates
> are the range check and a lockless test_bit():
> 
> 	if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
> 		dev_err(hwc->dev, "hwc_rx: invalid msg_id = %u\n", msg_id);
> 		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> 		return;
> 	}
> 
> and it ends with an unconditional hwc_ctx_put().  msg_id comes from host-
> writable DMA memory (READ_ONCE()d out of the RX buffer), and responded is
> false for a fresh allocation, so the new duplicate guard does not catch a
> response naming a slot that is still in this pre-post window.
> 
> Can such a response drop refcnt 1->0, clear the bitmap bit and wake msg_waitq
> while the sender still owns the slot?  If so, does that give:
> 
>   - the sender's refcount_inc() running on 0, i.e. a "refcount_t:
>     addition on 0; use-after-free" splat plus permanent saturation,
>     leaking that slot for good;
> 
>   - another sender being handed the same msg_id, sharing both
>     txq->msg_buf->reqs[msg_id] and the same hwc_caller_ctx, so one
>     command completes with the other command's payload, which is the
>     cross-completion this patch is meant to remove;
> 
>   - a spurious complete() letting wait_for_completion_timeout() succeed
>     with ctx->error still -EINPROGRESS, so -115 is handed back to
>     callers?
> 
> The same window also covers the req_len > tx_wr->buf_len path that jumps to
> out:, whose new comment states "no WQE was submitted so handle_resp()
> cannot race here" and "refcount is 1".  Can the slot already have been released
> by then, making that hwc_ctx_put() a second release?
> 
> [Severity: Medium]
> comp_event, refcnt, responded, msg_id and error are (re)initialised here under
> inflight_msg_res.lock only, while mana_hwc_handle_resp() reads and writes
> responded and error under ctx->lock only and never takes inflight_msg_res.lock.
> The intersection of the two locksets is empty, even though the header comment
> names ctx->lock as the protecting lock.
> 
> bitmap_set() runs before these field writes in the same critical section and
> handle_resp()'s visibility gate is a lockless test_bit(), so can a CQ interrupt on
> another CPU observe the bit, take ctx->lock and interleave with this
> initialisation?  It could then see the stale responded == true from the previous
> generation and silently drop a real response, or set responded/error and
> complete() while they are being reset, after which its hwc_ctx_put() consumes
> the new generation's initial reference.
> 
> reinit_completion() also writes x->done = 0 without the completion's wait.lock,
> while handle_resp() calls complete() under ctx->lock.  Can a completion be lost
> or spuriously observed that way?
> 
> > +		spin_unlock_irqrestore(&r->lock, flags);
> >
> > -	bitmap_set(hwc->inflight_msg_res.map, index, 1);
> > +		wait_event(hwc->msg_waitq,
> > +			   hwc->hwc_timed_out ||
> > +			   !bitmap_full(r->map, r->size));
> >
> > -	spin_unlock_irqrestore(&r->lock, flags);
> > +		if (hwc->hwc_timed_out)
> > +			return -ETIMEDOUT;
> > +	}
> 
> [Severity: Low]
> hwc_timed_out is written under inflight_msg_res.lock in
> mana_hwc_send_request(), but read as a plain load both in this
> wait_event() condition and in the check right after it.  bitmap_full() also reads
> r->map outside r->lock while other CPUs modify the same words with non-
> atomic bitmap_set()/bitmap_clear().  Would READ_ONCE()/
> WRITE_ONCE() (or evaluating the bitmap under r->lock) be preferable here to
> keep KCSAN quiet?
> 
> Related: the read-modify-write in the timeout path
> 
> 	if (hwc->hwc_timeout > 1)
> 		hwc->hwc_timeout = 1;
> 
> is unlocked while mana_hwc_init_event_handler() also writes hwc_timeout
> from EQ interrupt context.  That access is not introduced by this patch, it is in
> the baseline, but the patch makes the value load-bearing since a zero or
> degraded value now latches the whole channel.
> 
> >
> >  	*msg_id = index;
> > -
> >  	return 0;
> >  }
> >
> > @@ -35,10 +59,17 @@ static void mana_hwc_put_msg_index(struct
> hw_channel_context *hwc, u16 msg_id)
> >  	unsigned long flags;
> >
> >  	spin_lock_irqsave(&r->lock, flags);
> > -	bitmap_clear(hwc->inflight_msg_res.map, msg_id, 1);
> > +	bitmap_clear(r->map, msg_id, 1);
> >  	spin_unlock_irqrestore(&r->lock, flags);
> >
> > -	up(&hwc->sema);
> > +	wake_up(&hwc->msg_waitq);
> > +}
> > +
> > +static void hwc_ctx_put(struct hw_channel_context *hwc,
> > +			struct hwc_caller_ctx *ctx)
> > +{
> > +	if (refcount_dec_and_test(&ctx->refcnt))
> > +		mana_hwc_put_msg_index(hwc, ctx->msg_id);
> >  }
> >
> >  static int mana_hwc_verify_resp_msg(const struct hwc_caller_ctx
> > *caller_ctx, @@ -114,22 +145,44 @@ static void
> mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
> >  		resp_len = 0;
> >  	}
> >
> > -	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
> > -	if (err)
> > -		goto out;
> > +	spin_lock(&ctx->lock);
> >
> > -	ctx->status_code = resp_msg->status;
> > +	if (ctx->responded) {
> > +		/* A response for this slot was already delivered; this is a
> > +		 * duplicate or replayed one.  Drop it so the hwc_ctx_put()
> > +		 * a first response performs is not done twice, which would
> > +		 * over-release the slot while the sender still owns it.
> > +		 */
> > +		spin_unlock(&ctx->lock);
> > +		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> > +		return;
> > +	}
> > +	ctx->responded = true;
> >
> > -	memcpy(ctx->output_buf, resp_msg, resp_len);
> > -out:
> > -	ctx->error = err;
> > +	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
> >
> > -	/* Must post rx wqe before complete(), otherwise the next rx may
> > -	 * hit no_wqe error.
> > +	if (!err && ctx->output_buf) {
> > +		ctx->status_code = resp_msg->status;
> > +		memcpy(ctx->output_buf, resp_msg, resp_len);
> > +		ctx->error = 0;
> > +	} else if (ctx->output_buf) {
> > +		/* Only overwrite error if the sender hasn't timed out
> > +		 * or been force-completed by destroy.  When output_buf
> > +		 * is NULL, a terminal error (-ENODEV or timeout) has
> > +		 * already been set — preserve it so the sender doesn't
> > +		 * see a spurious success.
> > +		 */
> > +		ctx->error = err;
> > +	}
> 
> [Severity: Low]
> Is this comment accurate?  It says that when output_buf is NULL "a terminal
> error (-ENODEV or timeout) has already been set".
> 
> On the timeout path mana_hwc_send_request() only NULLs ctx->output_buf
> and assigns its local err; ctx->error keeps the -EINPROGRESS sentinel written by
> mana_hwc_get_msg_index().  Grepping drivers/net/ethernet/microsoft, the
> only ENODEV in hw_channel.c is inside this comment.
> 
> The comment also mentions senders "force-completed by destroy", but
> mana_hwc_destroy_channel() tears down the CQ and WQs and kfree()s
> caller_ctx without ever completing ctx->comp_event or writing ctx->error.
> 
> Could the comment be reworded to describe what the code actually
> guarantees?
> 
> > +
> > +	/* Post RX WQE before completing — the next response may arrive
> > +	 * immediately and needs a posted buffer.
> >  	 */
> >  	mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> > -
> >  	complete(&ctx->comp_event);
> > +	spin_unlock(&ctx->lock);
> > +
> > +	hwc_ctx_put(hwc, ctx);
> >  }
> >
> >  static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue
> > *q_self, @@ -216,7 +269,12 @@ static void
> > mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
> >
> >  		switch (type) {
> >  		case HWC_DATA_CFG_HWC_TIMEOUT:
> > -			hwc->hwc_timeout = val;
> > +			/* A zero timeout would make every command time
> out
> > +			 * immediately and latch hwc_timed_out, disabling the
> > +			 * channel.  Ignore it and keep the positive default.
> > +			 */
> > +			if (val)
> > +				hwc->hwc_timeout = val;
> >  			break;
> 
> [Severity: Medium]
> This guard covers the reconfig EQE, but is the other device-supplied source of
> the same field covered?  mana_gd_query_hwc_timeout() still
> does:
> 
> 	*timeout_val = resp.timeout_ms;
> 
> and mana_gd_verify_vf_version() calls it as
> mana_gd_query_hwc_timeout(pdev, &hwc->hwc_timeout), so a device-
> reported timeout_ms of 0 reaches hwc->hwc_timeout unfiltered.
> 
> With the new code, does the next command then wait msecs_to_jiffies(0), time
> out immediately and latch hwc_timed_out, after which every command fails
> with -ETIMEDOUT out of mana_hwc_get_msg_index() without being posted, for
> the remaining life of the channel?
> 
> >
> >  		case HWC_DATA_HW_LINK_CONNECT:
> > @@ -708,7 +766,7 @@ static int mana_hwc_init_inflight_msg(struct
> > hw_channel_context *hwc,  {
> >  	int err;
> >
> > -	sema_init(&hwc->sema, num_msg);
> > +	init_waitqueue_head(&hwc->msg_waitq);
> 
> [Severity: Low]
> The commit message says:
> 
>   Replace the depth-1 semaphore with a waitqueue + bitmap so a slot held
>   past a timeout does not deadlock admission and timed-out waiters can be
>   released.
> 
> Was the semaphore depth-1?  The removed line is sema_init(&hwc->sema,
> num_msg), and mana_hwc_init_queues() calls
> mana_hwc_init_inflight_msg(hwc, q_depth), so it admitted up to q_depth
> concurrent senders, matching the bitmap size.
> 
> The reason a counting semaphore no longer fits looks like the fact that a bitmap
> bit can now outlive its sender (held by handle_resp's reference), which
> desynchronises the semaphore count from bitmap occupancy.  Could the
> message be corrected, given this is a Fixes:-tagged patch headed for stable?
> 
> >
> >  	err = mana_gd_alloc_res_map(num_msg, &hwc->inflight_msg_res);
> >  	if (err)
> 
> [ ... ]
> 
> > @@ -999,13 +1062,17 @@ int mana_hwc_send_request(struct
> hw_channel_context *hwc, u32 req_len,
> >  	struct hwc_wq *txq = hwc->txq;
> >  	struct gdma_req_hdr *req_msg;
> >  	struct hwc_caller_ctx *ctx;
> > +	unsigned long flags;
> >  	u32 dest_vrcq = 0;
> >  	u32 dest_vrq = 0;
> >  	u32 command;
> > +	u32 status;
> >  	u16 msg_id;
> >  	int err;
> >
> > -	mana_hwc_get_msg_index(hwc, &msg_id);
> > +	err = mana_hwc_get_msg_index(hwc, &msg_id);
> > +	if (err)
> > +		return err;
> >
> >  	tx_wr = &txq->msg_buf->reqs[msg_id];
> >
> 
> [ ... ]
> 
> > @@ -1034,8 +1104,14 @@ int mana_hwc_send_request(struct
> hw_channel_context *hwc, u32 req_len,
> >  		dest_vrcq = hwc->pf_dest_vrcq_id;
> >  	}
> >
> > +	/* Take handle_resp's ref before posting — hardware can respond
> > +	 * immediately after the doorbell ring.
> > +	 */
> > +	refcount_inc(&ctx->refcnt);
> > +
> >  	err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
> >  	if (err) {
> > +		refcount_dec(&ctx->refcnt);
> >  		dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n",
> err);
> >  		goto out;
> >  	}
> > @@ -1046,31 +1122,86 @@ int mana_hwc_send_request(struct
> hw_channel_context *hwc, u32 req_len,
> >  			dev_err(hwc->dev, "Command 0x%x timed out: %u
> ms\n",
> >  				command, hwc->hwc_timeout);
> >
> > -		/* Reduce further waiting if HWC no response */
> > +		/* NULL out output_buf so a late handle_resp() won't write
> > +		 * into the caller's buffer after the sender returns, then
> > +		 * check whether handle_resp() already delivered a valid
> > +		 * response between the timeout firing and this lock
> > +		 * acquisition — ctx->error != -EINPROGRESS means it ran.
> > +		 */
> > +		spin_lock_irqsave(&ctx->lock, flags);
> > +		ctx->output_buf = NULL;
> > +		err = ctx->error;
> > +		status = ctx->status_code;
> > +		spin_unlock_irqrestore(&ctx->lock, flags);
> > +
> > +		if (err != -EINPROGRESS) {
> > +			/* handle_resp() delivered a valid response just after
> > +			 * the timeout fired.  The hardware is alive, so use
> > +			 * the response and leave the channel usable; do not
> > +			 * latch hwc_timed_out or degrade hwc_timeout for
> what
> > +			 * turned out to be a transient race.
> > +			 */
> > +			hwc_ctx_put(hwc, ctx);
> > +			goto check_status;
> > +		}
> > +
> > +		/* Genuine timeout: no response arrived.  Reduce further
> > +		 * waiting, and mark the channel timed out under the bitmap
> > +		 * lock so get_msg_index() cannot acquire new slots after this.
> > +		 */
> >  		if (hwc->hwc_timeout > 1)
> >  			hwc->hwc_timeout = 1;
> >
> > +		spin_lock_irqsave(&hwc->inflight_msg_res.lock, flags);
> > +		hwc->hwc_timed_out = true;
> > +		spin_unlock_irqrestore(&hwc->inflight_msg_res.lock, flags);
> > +		wake_up_all(&hwc->msg_waitq);
> 
> [Severity: High]
> Once hwc_timed_out is set here, is anything expected to clear it?
> Grepping drivers/net/ethernet/microsoft and include/net/mana, this is the only
> write besides the kzalloc zeroing, so the flag stays set for the life of the
> hw_channel_context.
> 
> From then on mana_hwc_get_msg_index() bails out before a slot is even
> allocated:
> 
> 	if (hwc->hwc_timed_out) {
> 		spin_unlock_irqrestore(&r->lock, flags);
> 		return -ETIMEDOUT;
> 	}
> 
> so mana_hwc_post_tx_wqe() is never reached and no doorbell is rung for any
> later command.  Every GDMA control command funnels through
> mana_gd_send_request()->mana_hwc_send_request(), including the teardown
> commands mana_gd_disable_queue(), mana_gd_destroy_dma_region() and
> mana_gd_deregister_device(), all of which treat errors as non-fatal and then
> dma_free_coherent() the backing pages anyway.
> 
> Before this patch those commands were still built, posted and the doorbell rung
> (only the wait was shortened to 1 ms), so the device did act on destroy and
> disable requests.  Can this leave the device holding registered DMA regions that
> point at freed pages, which is the situation the comment in
> mana_hwc_destroy_channel() describes as risking memory corruption on
> systems without an IOMMU?
> 
> The commit message describes the latch only as:
> 
>   On a genuine timeout the channel is marked hwc_timed_out and further
>   mana_hwc_get_msg_index() callers fail with -ETIMEDOUT instead of
>   reusing a slot whose response may still arrive.
> 
> Could it state that the latch is channel-wide, unconditional and never cleared,
> given the per-slot refcount already prevents reuse of a msg_id whose response
> is outstanding?
> 
> [Severity: High]
> Is the driver's own zero-timeout mode affected here too?  mana_serv_reset() in
> gdma_main.c does:
> 
> 	/* HWC is not responding in this case, so don't wait */
> 	hwc->hwc_timeout = 0;
> 
> 	dev_info(&pdev->dev, "MANA reset cycle start\n");
> 
> 	mana_gd_suspend(pdev, PMSG_SUSPEND);
> 
> and mana_gd_suspend() runs mana_rdma_remove() (which sends
> mana_gd_deregister_device()), mana_remove(&gc->mana, true) and
> mana_gd_cleanup_device(), all of which issue HWC commands.
> 
> For the first of those, wait_for_completion_timeout() with
> msecs_to_jiffies(0) returns 0, ctx->error is still -EINPROGRESS, so this branch
> runs: hwc->hwc_timeout > 1 is false and hwc_timed_out is set unconditionally.
> Do the remaining teardown commands then abort in
> mana_hwc_get_msg_index() without ever reaching the SQ, turning the
> intended fire-and-forget teardown into no teardown at all while the buffers are
> still freed?
> 
> The patch already guards the device-reported value with if (val) for this exact
> reason.  Should the in-driver zero assignment be handled as well, for instance
> by not latching when hwc_timeout is 0, since mana_need_log() already treats
> hwc_timeout == 0 as a legitimate "don't wait, don't log"
> state?
> 
> >
> >  		err = -ETIMEDOUT;
> > -		goto out;
> > +		hwc_ctx_put(hwc, ctx);
> > +		goto done;
> >  	}
> >
> > -	if (ctx->error) {
> > -		err = ctx->error;
> > -		goto out;
> > -	}
> > +	/* NULL output_buf so a late handle_resp() won't memcpy into
> > +	 * the caller's buffer after the sender exits.  Read error and
> > +	 * status_code under the same lock — after hwc_ctx_put the slot
> > +	 * may be reused and these fields overwritten.
> > +	 */
> > +	spin_lock_irqsave(&ctx->lock, flags);
> > +	ctx->output_buf = NULL;
> > +	err = ctx->error;
> > +	status = ctx->status_code;
> > +	spin_unlock_irqrestore(&ctx->lock, flags);
> > +	hwc_ctx_put(hwc, ctx);
> > +
> > +check_status:
> > +	if (err)
> > +		goto done;
> 
> [ ... ]
> 
> > +
> > +	err = 0;
> > +	goto done;
> >  out:
> > -	mana_hwc_put_msg_index(hwc, msg_id);
> > +	/* Pre-post error paths: no WQE was submitted so handle_resp()
> > +	 * cannot race here.  refcount is 1 (no second ref taken).
> > +	 */
> > +	ctx = hwc->caller_ctx + msg_id;
> > +	spin_lock_irqsave(&ctx->lock, flags);
> > +	ctx->output_buf = NULL;
> > +	spin_unlock_irqrestore(&ctx->lock, flags);
> > +	hwc_ctx_put(hwc, ctx);
> > +done:
> >  	return err;
> >  }
> > diff --git a/include/net/mana/hw_channel.h
> > b/include/net/mana/hw_channel.h index 3d8543acb5cc0..5a55cedf0607a
> > 100644
> > --- a/include/net/mana/hw_channel.h
> > +++ b/include/net/mana/hw_channel.h
> > @@ -173,6 +173,23 @@ struct hwc_caller_ctx {
> >
> >  	u32 error; /* Linux error code */
> >  	u32 status_code;
> 
> [Severity: Low]
> error stays declared as u32 while mana_hwc_get_msg_index() now stores
> ctx->error = -EINPROGRESS into it, and mana_hwc_send_request() reads it
> back into an int and compares it as signed with if (err != -EINPROGRESS).
> The decision whether to latch the whole channel therefore depends on an out-
> of-range u32 to int conversion rather than on the declared type.
> 
> Should the field become int error, matching its own "Linux error code"
> comment, or the sentinel be made unsigned-safe?
> 
> > +
> > +	/* Protects output_buf against concurrent access from
> > +	 * handle_resp() (CQ interrupt) and the sender timeout path.
> > +	 */
> > +	spinlock_t lock;
> > +
> > +	/* Tracks sender + handle_resp ownership.  The last put
> > +	 * (refcount reaches 0) releases the bitmap slot.
> > +	 */
> > +	refcount_t refcnt;
> > +	u16 msg_id;
> > +
> > +	/* Set under lock by the first handle_resp() for this slot so a
> > +	 * duplicate or replayed response is dropped instead of consuming
> > +	 * the response-side reference a second time.
> > +	 */
> > +	bool responded;
> >  };
> >
> >  struct hw_channel_context {
> > @@ -193,13 +210,19 @@ struct hw_channel_context {
> >  	u32 hwc_timeout;
> >
> > +	/* Set on first HWC timeout.  Causes get_msg_index() to return
> > +	 * -ETIMEDOUT instead of waiting, draining all queued senders.
> > +	 */
> > +	bool hwc_timed_out;
> > +
> 
> Could this comment also note that the state is never cleared, so it is terminal
> for the lifetime of the channel?

I will send v4 to address the comments. v4 also adds a 7th patch, "keep
max_num_cqs immutable once cq_table is allocated", referenced below.

There are the summary of v4 addressing / not addressing the comments:

Legend: [FIX] fixed in v4, [DESIGN] intentional/won't change,
[PRE] pre-existing (not introduced by this series).


== Patch 1: RCU-protect gc->cq_table ==

[FIX] "Per-CQ blocking grace periods added to hot teardown paths ...
mana_gd_destroy_cq() now ends with an unconditional synchronize_rcu()":
the netdev teardown paths will be reworked into a two-pass quiesce/free
that takes a single grace period instead of one per CQ.

[FIX] "The bound that guards every cq_table index lives outside the
RCU-published object" / "A malicious host can trigger out-of-bounds ...
by dynamically inflating gc->max_num_cqs": v4 snapshots cq->id and
max_num_cqs with READ_ONCE() so the same value sizes, bounds and indexes
the table, and a new patch keeps max_num_cqs immutable once cq_table is
allocated.

[FIX] "two *separate* loads of the same interrupt-mutable, device-
controlled values": collapsed to single READ_ONCE() snapshots.

[FIX] "read gc->cq_table with rcu_dereference_protected(..., true)" /
gdma.h lifetime comment: the gdma.h comment will be corrected to state
the actual lifetime rule (base pointer is written only at establish and
teardown, so the "true" predicate is sound).

[FIX] "Pre-existing Use-After-Free in HWC channel teardown due to
inverted destruction order": addressed by the teardown-ordering rework
(see P3/P5 below).


== Patch 2: fix HWC RQ/SQ buffer size swap ==

[FIX] "Commit message overstates the impact ... describes a reachable
buffer overflow": changelog reworded as a latent-correctness fix (both
sizes are 0x1000, so no overflow is observable).

[FIX] "the queue dimensions are hoisted above mana_hwc_create_cq()":
the hoist and its comment are dropped; the assignments stay at the end
of mana_hwc_init_queues().

(gemini's "OOB read from inline_oob_size_div4", "UAF during teardown",
and "race/UAF in send_request timeout" against this patch are addressed
in P4/P5/P6 respectively; see below.)


== Patch 3: free HWC comp_buf after destroying the EQ ==

[FIX] "hwc->txq and hwc->rxq ... are freed by mana_hwc_destroy_wq()
BEFORE mana_hwc_destroy_cq() unpublishes" / "Destroying the TX and RX
queues before the CQ and EQ ... causes a use-after-free": v4 destroys
the CQ first (which tears down the EQ + IRQ and unpublishes the
cq_table slot with a grace period) before freeing the RQ/TX WQs.

[FIX] "the comment ... misdescribe[s] the fencing mechanism": the
comment is corrected to describe the EQ-destroy -> IRQ-deregister ->
synchronize_rcu fence accurately.


== Patch 4: validate hardware-supplied values in the HWC RX path ==

[FIX] "short-response gate ... drops the completion without waking the
pending sender, turning a prompt -EPROTO ... into a full-timeout hang"
/ "a timeout ... can lead to stack corruption and use-after-free": the
short-response early return is removed; a malformed response reaches
verify_resp_msg() -> -EPROTO and completes the sender.

[FIX] "Unbounded, unrecoverable RX WQE (RQ credit) leak": the failure
paths now account leaked WQEs and trip hwc_timeout on RQ exhaustion.

[FIX] "a plain (non-READ_ONCE) load of device/host-writable DMA memory"
/ "SGE ... validates a much weaker invariant ... corrupted
inline_oob_size_div4": v4 snapshots inline_oob_size_div4 (read once
through its u32 flags word, since it is a bit-field) and sge->address
with READ_ONCE(), and rejects any value other than the exact one the
driver programs.

[FIX] "Commit message claims an out-of-bounds indexing fix ... that
does not exist": the msg_id check is reframed in the changelog as
defense in depth.


== Patch 5: HWC teardown safety (setup_active + destroy ordering) ==

[FIX] "makes setup_active the definitive signal while still arming it
too late": setup_active is armed immediately after mana_smc_setup_hwc()
succeeds.

[FIX] "Redundant HWC teardown in mana_hwc_establish_channel() causes a
double hardware timeout ... and masks the original error code": the
redundant teardown is removed; teardown happens once via the caller.

[FIX] "the CQ is destroyed and its memory freed before the EQ is
deregistered ... Use-After-Free and WARN splats": the EQ is destroyed
(IRQ deregistered, in-flight handlers drained) before the CQ.

[FIX] "removes the only reset of gc->max_num_cqs and codifies ... that
it is 'Set once'": complemented by the new immutability patch; the
field is never reset and never re-inflated.

[FIX] "An early return on HWC teardown failure leaves a stale pointer
in gc->cq_table, which a malicious host can exploit to cause an out-of-
bounds read": on teardown failure the HWC and its CQ are leaked (not
freed), so the cq_table slot is not dangling, and max_num_cqs
immutability prevents any OOB index.  No UAF/OOB results.

[DESIGN] "mana_hwc_destroy_channel() ... returns early when
mana_smc_teardown_hwc() fails [and] skips the entire tail" / "the leak
path abandons that instance": intentional.  If teardown fails the
device may still DMA into those buffers; freeing them without an IOMMU
to fault the stale DMA risks memory corruption, so the resources are
leaked on purpose (the code says so and keeps setup_active set).


== Patch 6: fix stale HWC response after command timeout ==

[FIX] "Refcount underflow / premature slot release: the response-side
reference is taken late" / "a maliciously early hardware response can
prematurely free the message slot before the sender takes its
reference": v4 takes both references up front in
mana_hwc_get_msg_index() (refcount initialised to 2), under the same
lock that publishes the slot, so an early/stale/forged response drops
only one reference (2->1) and cannot free the slot under the sender.
The pre-post error path latches ->responded so it cannot double-drop.

[FIX] "zero-value guard for hwc->hwc_timeout only in the ... reconfig
handler": v4 rejects a device-supplied zero in the query path too.

[FIX] "hwc_timed_out is written under inflight_msg_res.lock ... but
read as a plain load": all accesses use READ_ONCE()/WRITE_ONCE().

[FIX] "u32 error; /* Linux error code */ ... stores the negative
sentinel": caller_ctx::error is changed from u32 to int so it holds the
negative -EINPROGRESS sentinel and errno values directly.

[FIX] "the output_buf == NULL comment ... is inaccurate" and "'Replace
the depth-1 semaphore ...'": the comment and the changelog wording are
corrected.

[FIX + PRE] "Lockset mismatch: mana_hwc_get_msg_index() initialises
per-slot state under inflight_msg_res.lock, while handle_resp() uses
ctx->lock; the gate is a lockless test_bit()": the dangerous
consequence you flagged (a raced handle_resp releasing a slot the new
sender still owns) is eliminated by the refcount-initialised-to-2 change
above.  The remaining consequences (dropping a stale response, lost
completion) require a duplicate/forged/unsolicited response, i.e. the
pre-existing msg_id-reuse (ABA) window; the honest in-order path never
races the initialiser.  Fully closing the ABA window needs a per-request
generation token, which is a larger, separable change I plan to send on
top rather than fold in here.

[DESIGN] "Terminal, channel-wide hwc_timed_out latch": intentional.
Once the HWC stops responding the channel is unusable and further
commands should fail fast.  v4 only narrows the false positive: a valid
response racing the timeout (ctx->error != -EINPROGRESS) no longer
latches the channel.

[DESIGN] "mana_serv_reset() deliberately sets hwc->hwc_timeout = 0 ...
and then runs the whole suspend/teardown sequence": also intentional --
there the HWC is known unresponsive and the driver must not wait.  This
is distinct from a device-supplied zero, which is now rejected.

All changes were built per-commit (mana + mana_ib, W=0 E=0), checked
with sparse C=1 and checkpatch, and boot/traffic tested on Azure MANA
hardware.

Thanks,
Long

[PATCH net v4 0/7] net: mana: HW channel reliability and hardening fixes

[Long Li]

This series fixes a set of latent bugs and robustness gaps in the MANA
Hardware Channel (HWC), the control path the driver uses to talk to the
device.  The issues range from a use-after-free of completion queues
during teardown to buffer mis-sizing, unsafe teardown ordering, missing
validation of device-supplied RX metadata, and stale-response handling
after a command timeout.

Patch overview:

  1  RCU-protect gc->cq_table lookups against concurrent CQ destroy
     The EQ interrupt handler dereferences CQ pointers from gc->cq_table
     while teardown can free them.  Put the table under RCU and wait a
     grace period before freeing, closing the use-after-free.

  2  fix HWC RQ/SQ buffer size swap
     init_queues() sized the RQ with max_req_msg_size and the SQ with
     max_resp_msg_size -- backwards.  Correct the swap; both sizes are
     equal in practice, so this is a latent-correctness fix.

  3  free HWC comp_buf after destroying the EQ
     Reorder teardown so the EQ is destroyed (readers quiesced) before
     comp_buf and the CQ are freed, preventing a late EQ-handler access
     to freed memory.

  4  validate hardware-supplied values in the HWC RX path
     Bounds-check the SGE, verify the recovered slot index and SGE
     address, and validate response length and msg_id before use, so
     malformed or hostile DMA metadata cannot cause wrong-slot completion
     or out-of-bounds access.

  5  fix HWC teardown safety with setup_active flag and destroy ordering
     Track setup activation explicitly, tear the EQ/CQ down before the
     TXQ/RXQ, and on an unrecoverable teardown failure leak the HWC
     resources rather than free memory the device may still DMA into.

  6  fix stale HWC response after command timeout
     Replace the inflight-slot semaphore with a bitmap + waitqueue and
     per-slot refcount/lock; latch the channel on timeout so no new slots
     are handed out, drop duplicate/late responses, and ignore a zero
     firmware-supplied timeout.

  7  keep max_num_cqs immutable once cq_table is allocated
     gc->max_num_cqs is set once when cq_table is allocated and never
     reset, so a spoofed post-init HWC event cannot inflate the bound
     past the allocation and drive an out-of-bounds cq_table access.

Follow-up feature work (net-next, sent separately):

The original series also contained two patches that are improvements, not
fixes:

  net: mana: support concurrent HWC requests
  net: mana: add dynamic HWC queue depth with reinit path

Per the netdev tree rules, fixes go to 'net' and features/improvements go
to 'net-next', and the two must not be combined in a single submission.
Those two patches build on the locking and teardown groundwork in this
series, so they will be posted as a separate net-next series only after
these fixes have propagated from net into net-next through the usual
periodic merge.

Changes since v3:

Addressed the netdev-ai and sashiko.dev automated reviews of v3.

 - New patch 7 ("keep max_num_cqs immutable once cq_table is
   allocated"): gc->max_num_cqs is set once and never reset, so a
   spoofed post-init HWC event cannot inflate the bound past the
   allocation and cause an out-of-bounds cq_table access.
 - patch 1: replaced the per-CQ synchronize_rcu() in the netdev teardown
   paths with a two-pass quiesce/free that takes a single grace period
   per teardown; snapshot cq->id and max_num_cqs with READ_ONCE() so the
   same value sizes, bounds and indexes cq_table; corrected the
   gc->cq_table lifetime comment; rescoped the changelog to the
   use-after-free fix.
 - patch 2: reworded the changelog as a latent-correctness fix (both
   message sizes are 0x1000, so the swap has no observable overflow) and
   dropped the incorrect note about hoisting the queue dimensions.
 - patch 4: removed the short-response early return so a malformed
   response reaches verify_resp_msg() -> -EPROTO and completes the
   sender instead of hanging it; account leaked RX WQEs and trip
   hwc_timeout on RQ exhaustion; read the device-supplied
   inline_oob_size_div4 (through its u32 flags word, as it is a
   bit-field) and sge->address with READ_ONCE() and reject any value
   other than the one the driver programs; reframed the msg_id check as
   defense in depth.
 - patch 5: arm setup_active immediately after mana_smc_setup_hwc()
   succeeds; destroy the EQ (IRQ deregister + drain) before the CQ; drop
   the redundant teardown in mana_hwc_establish_channel() that caused a
   double hardware timeout and masked the original error.
 - patch 6: take both the sender and response-side references up front in
   mana_hwc_get_msg_index() (refcount initialised to 2, under the lock
   that publishes the slot) so an early/stale/forged response cannot free
   the slot before the sender posts; changed caller_ctx::error from u32
   to int; reject a zero firmware-supplied HWC timeout in the query path
   as well as the reconfig path; access hwc_timed_out with READ_ONCE()/
   WRITE_ONCE(); comment and changelog fixes.
 - Assorted comment and commit-message clarifications throughout.

Changes since v2:

 - Per maintainer feedback, split the original combined series: the
   fixes here target 'net'; the two feature patches now go to 'net-next'
   and are sent separately (see above).  Rebased the fixes onto net.
 - Dropped the pcie_flr()-based reset fallback from the teardown path.
   pcie_flr() resets device config without the save/restore that
   pci_reset_function() provides, and cannot be used as a drop-in here.
   On an unrecoverable teardown failure the driver now leaks the HWC
   resources instead of touching memory the device may still DMA into.
 - patch 2: store the HWC queue dimensions before creating the CQ so the
   RX completion handler can never observe a zero max_resp_msg_size
   divisor or a stale num_inflight_msg bound.
 - Assorted commit-message and comment clarifications.

Long Li (7):
  net: mana: RCU-protect gc->cq_table lookups against concurrent CQ
    destroy
  net: mana: fix HWC RQ/SQ buffer size swap
  net: mana: free HWC comp_buf after destroying the EQ
  net: mana: validate hardware-supplied values in the HWC RX path
  net: mana: fix HWC teardown safety with setup_active flag and destroy
    ordering
  net: mana: fix stale HWC response after command timeout
  net: mana: keep max_num_cqs immutable once cq_table is allocated

 drivers/infiniband/hw/mana/cq.c               |  46 +-
 .../net/ethernet/microsoft/mana/gdma_main.c   |  48 +-
 .../net/ethernet/microsoft/mana/hw_channel.c  | 452 +++++++++++++++---
 drivers/net/ethernet/microsoft/mana/mana_en.c | 136 ++++--
 include/net/mana/gdma.h                       |  40 +-
 include/net/mana/hw_channel.h                 |  44 +-
 6 files changed, 652 insertions(+), 114 deletions(-)


base-commit: dd057113ac7ba5bdd2aed3d9405305911152f911
-- 
2.43.0

[PATCH net v4 1/7] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy

[Long Li]

The EQ interrupt handler (mana_gd_process_eqe) looks up the completing CQ
in gc->cq_table[cq_id] and runs its callback, concurrently with CQ
teardown on another CPU that clears the slot and frees the CQ.  cq_table
was a plain pointer array freed with no grace period, so the two race
into a use-after-free:

  CPU A (mana_gd_intr, hard IRQ)        CPU B (CQ destroy)
  ----------------------------------    ------------------------------
  cq = gc->cq_table[cq_id];  // valid
                                        gc->cq_table[id] = NULL;
                                        kfree(cq);          // freed
  cq->cq.callback(ctx, cq);  // use-after-free

The handler's existing rcu_read_lock() only guards the per-IRQ EQ list
traversal; cq_table was never under any RCU contract, and a read-side
lock is inert unless the freer also defers the free past a grace period.

Put cq_table under RCU: annotate the base pointer and entries __rcu, read
with rcu_dereference() in the handler, publish with rcu_assign_pointer(),
and on teardown clear the slot then synchronize_rcu() before freeing the
CQ.  The grace period blocks until every in-flight handler has dropped
the old pointer, so the kfree() can no longer race the callback.

This fixes only the CQ lifetime (the use-after-free); it does not make
the cq_id bound trustworthy.  gc->max_num_cqs is still range-checked
outside the published table, and hardening that field against a spoofed
device value is a separate change.

netdev teardown destroys a CQ per TX and per RX queue, so one grace
period each in mana_gd_destroy_cq() would serialize up to
2 * MANA_MAX_NUM_QUEUES synchronize_rcu() calls under RTNL on every
ifdown, MTU change or ring/channel reconfigure.  Clear all of a port's
CQ slots first and take a single grace period per teardown instead:
mana_gd_unpublish_cq() clears a slot without waiting, and
mana_gd_destroy_cq() -- which still serves the single-CQ callers --
finds the slot already cleared and skips its own synchronize_rcu().

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v4:
 - Replaced the per-CQ synchronize_rcu() in the netdev teardown paths
   with a two-pass quiesce/free that takes one grace period per
   teardown; mana_gd_unpublish_cq() splits the slot-clear from the
   grace period.
 - Snapshot cq->id and max_num_cqs with READ_ONCE() in
   mana_hwc_establish_channel() so one value sizes, bounds and indexes
   cq_table.
 - Corrected the gc->cq_table lifetime comment in gdma.h; rescoped the
   changelog to the use-after-free fix (the bound is patch 7).

 drivers/infiniband/hw/mana/cq.c               |  46 +++++-
 .../net/ethernet/microsoft/mana/gdma_main.c   |  41 ++++--
 .../net/ethernet/microsoft/mana/hw_channel.c  |  29 ++--
 drivers/net/ethernet/microsoft/mana/mana_en.c | 136 ++++++++++++++----
 include/net/mana/gdma.h                       |  32 ++++-
 5 files changed, 234 insertions(+), 50 deletions(-)

diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
index f2547989f422..2bf4be21cede 100644
--- a/drivers/infiniband/hw/mana/cq.c
+++ b/drivers/infiniband/hw/mana/cq.c
@@ -131,12 +131,20 @@ static void mana_ib_cq_handler(void *ctx, struct gdma_queue *gdma_cq)
 int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
 {
 	struct gdma_context *gc = mdev_to_gc(mdev);
+	struct gdma_queue __rcu **cq_table;
 	struct gdma_queue *gdma_cq;
 
-	if (cq->queue.id >= gc->max_num_cqs)
+	/* No rcu_read_lock(): install/remove run within the IB device
+	 * lifetime, which mana_rdma_remove() (ib_unregister_device) drains
+	 * before the base cq_table can be freed.  See gdma_context::cq_table
+	 * in gdma.h for why "true" is sound.
+	 */
+	cq_table = rcu_dereference_protected(gc->cq_table, true);
+	if (!cq_table || cq->queue.id >= gc->max_num_cqs)
 		return -EINVAL;
+
 	/* Create CQ table entry, sharing a CQ between WQs is not supported */
-	if (gc->cq_table[cq->queue.id])
+	if (rcu_access_pointer(cq_table[cq->queue.id]))
 		return -EINVAL;
 	if (cq->queue.kmem)
 		gdma_cq = cq->queue.kmem;
@@ -149,23 +157,49 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
 	gdma_cq->type = GDMA_CQ;
 	gdma_cq->cq.callback = mana_ib_cq_handler;
 	gdma_cq->id = cq->queue.id;
-	gc->cq_table[cq->queue.id] = gdma_cq;
+	rcu_assign_pointer(cq_table[cq->queue.id], gdma_cq);
 	return 0;
 }
 
 void mana_ib_remove_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
 {
 	struct gdma_context *gc = mdev_to_gc(mdev);
+	struct gdma_queue __rcu **cq_table;
+	struct gdma_queue *gdma_cq;
 
-	if (cq->queue.id >= gc->max_num_cqs || cq->queue.id == INVALID_QUEUE_ID)
+	if (cq->queue.id == INVALID_QUEUE_ID)
 		return;
 
 	if (cq->queue.kmem)
 	/* Then it will be cleaned and removed by the mana */
 		return;
 
-	kfree(gc->cq_table[cq->queue.id]);
-	gc->cq_table[cq->queue.id] = NULL;
+	/* No rcu_read_lock(): like mana_ib_install_cq_cb(), this runs within
+	 * the IB device lifetime that mana_rdma_remove() drains before the
+	 * base cq_table can be freed.  See gdma_context::cq_table in gdma.h.
+	 */
+	cq_table = rcu_dereference_protected(gc->cq_table, true);
+	if (!cq_table || cq->queue.id >= gc->max_num_cqs)
+		return;
+	/* Removers for a given CQ are serialized by the IB core, so the slot
+	 * is read and cleared without rcu_read_lock() or atomicity: a CQ is
+	 * never torn down while a live QP references it (cq->usecnt), nor
+	 * while the QP-create that installed the entry is still running (that
+	 * create holds a reference on the CQ uobject across its error path,
+	 * before usecnt is taken).  Any double-remove is therefore sequential
+	 * -- the later caller sees the NULL stored below and returns.
+	 */
+	gdma_cq = rcu_dereference_protected(cq_table[cq->queue.id], true);
+	if (!gdma_cq)
+		return;  /* already removed by a prior teardown path */
+
+	rcu_assign_pointer(cq_table[cq->queue.id], NULL);
+
+	/* Wait for in-flight EQ handlers that may have loaded the old
+	 * pointer via rcu_dereference() to finish before freeing.
+	 */
+	synchronize_rcu();
+	kfree(gdma_cq);
 }
 
 int mana_ib_arm_cq(struct ib_cq *ibcq, enum ib_cq_notify_flags flags)
diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index e8b7ffb47eb9..7714040d1df4 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -732,6 +732,7 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
 	union gdma_eqe_info eqe_info;
 	enum gdma_eqe_type type;
 	struct gdma_event event;
+	struct gdma_queue __rcu **cq_table;
 	struct gdma_queue *cq;
 	struct gdma_eqe *eqe;
 	u32 cq_id;
@@ -743,10 +744,11 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
 	switch (type) {
 	case GDMA_EQE_COMPLETION:
 		cq_id = eqe->details[0] & 0xFFFFFF;
-		if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
+		cq_table = rcu_dereference(gc->cq_table);
+		if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs || !cq_table))
 			break;
 
-		cq = gc->cq_table[cq_id];
+		cq = rcu_dereference(cq_table[cq_id]);
 		if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id))
 			break;
 
@@ -1050,18 +1052,41 @@ static void mana_gd_create_cq(const struct gdma_queue_spec *spec,
 	queue->cq.callback = spec->cq.callback;
 }
 
-static void mana_gd_destroy_cq(struct gdma_context *gc,
-			       struct gdma_queue *queue)
+bool mana_gd_unpublish_cq(struct gdma_context *gc, struct gdma_queue *queue)
 {
+	struct gdma_queue __rcu **cq_table;
 	u32 id = queue->id;
 
-	if (id >= gc->max_num_cqs)
-		return;
+	/* No rcu_read_lock() here: unpublish runs only on the
+	 * CQ-destroy/teardown path, where the base cq_table is stable.  See
+	 * the lifecycle note on gdma_context::cq_table in gdma.h for why the
+	 * "true" predicate is sound.
+	 */
+	cq_table = rcu_dereference_protected(gc->cq_table, true);
+	if (!cq_table || id >= gc->max_num_cqs)
+		return false;
+
+	if (!rcu_access_pointer(cq_table[id]))
+		return false;
+
+	rcu_assign_pointer(cq_table[id], NULL);
+	return true;
+}
 
-	if (!gc->cq_table[id])
+static void mana_gd_destroy_cq(struct gdma_context *gc,
+			       struct gdma_queue *queue)
+{
+	/* A batched teardown may already have cleared the slot and taken the
+	 * grace period; then there is nothing left to wait for.
+	 */
+	if (!mana_gd_unpublish_cq(gc, queue))
 		return;
 
-	gc->cq_table[id] = NULL;
+	/* Wait for in-flight EQ handlers that may have loaded the old
+	 * pointer via rcu_dereference() to finish before the caller
+	 * frees the CQ memory.
+	 */
+	synchronize_rcu();
 }
 
 int mana_gd_create_hwc_queue(struct gdma_dev *gd,
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index e3c24d50dad0..409e20caeccd 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -674,6 +674,7 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 	struct gdma_queue *sq = hwc->txq->gdma_wq;
 	struct gdma_queue *eq = hwc->cq->gdma_eq;
 	struct gdma_queue *cq = hwc->cq->gdma_cq;
+	struct gdma_queue __rcu **cq_table;
 	int err;
 
 	init_completion(&hwc->hwc_init_eqe_comp);
@@ -698,11 +699,15 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 	if (WARN_ON(cq->id >= gc->max_num_cqs))
 		return -EPROTO;
 
-	gc->cq_table = vcalloc(gc->max_num_cqs, sizeof(struct gdma_queue *));
-	if (!gc->cq_table)
+	cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
+	if (!cq_table)
 		return -ENOMEM;
 
-	gc->cq_table[cq->id] = cq;
+	rcu_assign_pointer(cq_table[cq->id], cq);
+	/* Publish the fully-initialised table last; pairs with the
+	 * rcu_dereference(gc->cq_table) in mana_gd_process_eqe().
+	 */
+	rcu_assign_pointer(gc->cq_table, cq_table);
 
 	return 0;
 }
@@ -811,6 +816,7 @@ int mana_hwc_create_channel(struct gdma_context *gc)
 void mana_hwc_destroy_channel(struct gdma_context *gc)
 {
 	struct hw_channel_context *hwc = gc->hwc.driver_data;
+	struct gdma_queue __rcu **old_cq_table;
 
 	if (!hwc)
 		return;
@@ -818,10 +824,8 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	/* gc->max_num_cqs is set in mana_hwc_init_event_handler(). If it's
 	 * non-zero, the HWC worked and we should tear down the HWC here.
 	 */
-	if (gc->max_num_cqs > 0) {
+	if (gc->max_num_cqs > 0)
 		mana_smc_teardown_hwc(&gc->shm_channel, false);
-		gc->max_num_cqs = 0;
-	}
 
 	if (hwc->txq)
 		mana_hwc_destroy_wq(hwc, hwc->txq);
@@ -832,6 +836,14 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	if (hwc->cq)
 		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
 
+	/* Reset only after mana_hwc_destroy_cq() above has run with a valid
+	 * max_num_cqs so mana_gd_destroy_cq() clears the CQ table slot and
+	 * waits out in-flight EQ handlers (synchronize_rcu) before the CQ is
+	 * freed.  Clearing it earlier would make that path early-return and
+	 * skip the slot clear, leaving a dangling cq_table entry.
+	 */
+	gc->max_num_cqs = 0;
+
 	kfree(hwc->caller_ctx);
 	hwc->caller_ctx = NULL;
 
@@ -848,8 +860,9 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	gc->hwc.driver_data = NULL;
 	gc->hwc.gdma_context = NULL;
 
-	vfree(gc->cq_table);
-	gc->cq_table = NULL;
+	old_cq_table = rcu_replace_pointer(gc->cq_table, NULL, true);
+	synchronize_rcu();
+	vfree(old_cq_table);
 }
 
 int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 92bb55935c1c..5d215981bba8 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -2427,12 +2427,18 @@ static void mana_deinit_txq(struct mana_port_context *apc, struct mana_txq *txq)
 
 static void mana_destroy_txq(struct mana_port_context *apc)
 {
+	struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
 	struct napi_struct *napi;
 	int i;
 
 	if (!apc->tx_qp)
 		return;
 
+	/* Pass 1: quiesce each CQ on the device and clear its cq_table slot.
+	 * Taking one grace period below for the whole port avoids up to
+	 * apc->num_queues serialized synchronize_rcu() calls (one per CQ in
+	 * mana_gd_destroy_cq()) under RTNL on every teardown.
+	 */
 	for (i = 0; i < apc->num_queues; i++) {
 		if (!apc->tx_qp[i])
 			continue;
@@ -2448,8 +2454,24 @@ static void mana_destroy_txq(struct mana_port_context *apc)
 			apc->tx_qp[i]->txq.napi_initialized = false;
 		}
 
-		if (apc->tx_qp[i]->tx_object != INVALID_MANA_HANDLE)
-			mana_destroy_wq_obj(apc, GDMA_SQ, apc->tx_qp[i]->tx_object);
+		if (apc->tx_qp[i]->tx_object != INVALID_MANA_HANDLE) {
+			mana_destroy_wq_obj(apc, GDMA_SQ,
+					    apc->tx_qp[i]->tx_object);
+			apc->tx_qp[i]->tx_object = INVALID_MANA_HANDLE;
+		}
+
+		if (apc->tx_qp[i]->tx_cq.gdma_cq)
+			mana_gd_unpublish_cq(gc, apc->tx_qp[i]->tx_cq.gdma_cq);
+	}
+
+	synchronize_rcu();
+
+	/* Pass 2: the slots are clear, so mana_gd_destroy_cq() skips its own
+	 * grace period; free the CQ, the TXQ and the queue pair.
+	 */
+	for (i = 0; i < apc->num_queues; i++) {
+		if (!apc->tx_qp[i])
+			continue;
 
 		mana_deinit_cq(apc, &apc->tx_qp[i]->tx_cq);
 
@@ -2496,6 +2518,7 @@ static int mana_create_txq(struct mana_port_context *apc,
 	struct mana_obj_spec cq_spec;
 	struct gdma_queue_spec spec;
 	struct gdma_context *gc;
+	struct gdma_queue __rcu **cq_table;
 	struct mana_txq *txq;
 	struct mana_cq *cq;
 	u32 txq_size;
@@ -2596,12 +2619,18 @@ static int mana_create_txq(struct mana_port_context *apc,
 
 		cq->gdma_id = cq->gdma_cq->id;
 
-		if (WARN_ON(cq->gdma_id >= gc->max_num_cqs)) {
+		/* No rcu_read_lock(): mana_create_txq runs under RTNL during
+		 * netdev bring-up, inside the netdev lifetime that
+		 * mana_remove() drains before the base cq_table can be freed.
+		 * See gdma_context::cq_table in gdma.h for why "true" is sound.
+		 */
+		cq_table = rcu_dereference_protected(gc->cq_table, true);
+		if (WARN_ON(!cq_table || cq->gdma_id >= gc->max_num_cqs)) {
 			err = -EINVAL;
 			goto out;
 		}
 
-		gc->cq_table[cq->gdma_id] = cq->gdma_cq;
+		rcu_assign_pointer(cq_table[cq->gdma_id], cq->gdma_cq);
 
 		mana_create_txq_debugfs(apc, i);
 
@@ -2621,25 +2650,20 @@ static int mana_create_txq(struct mana_port_context *apc,
 	return err;
 }
 
-static void mana_destroy_rxq(struct mana_port_context *apc,
+/* Quiesce an RXQ's CQ on the device and clear its cq_table slot, without
+ * waiting for a grace period.  Split out of mana_destroy_rxq() so a batch
+ * teardown (mana_destroy_rxqs()) can quiesce every RXQ and then take a
+ * single synchronize_rcu() instead of one per RXQ.
+ */
+static void mana_quiesce_rxq(struct mana_port_context *apc,
 			     struct mana_rxq *rxq, bool napi_initialized)
-
 {
 	struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
-	struct mana_recv_buf_oob *rx_oob;
-	struct device *dev = gc->dev;
-	struct napi_struct *napi;
-	struct page *page;
-	int i;
-
-	if (!rxq)
-		return;
+	struct napi_struct *napi = &rxq->rx_cq.napi;
 
 	debugfs_remove_recursive(rxq->mana_rx_debugfs);
 	rxq->mana_rx_debugfs = NULL;
 
-	napi = &rxq->rx_cq.napi;
-
 	if (napi_initialized) {
 		napi_synchronize(napi);
 
@@ -2650,8 +2674,27 @@ static void mana_destroy_rxq(struct mana_port_context *apc,
 	if (xdp_rxq_info_is_reg(&rxq->xdp_rxq))
 		xdp_rxq_info_unreg(&rxq->xdp_rxq);
 
-	if (rxq->rxobj != INVALID_MANA_HANDLE)
+	if (rxq->rxobj != INVALID_MANA_HANDLE) {
 		mana_destroy_wq_obj(apc, GDMA_RQ, rxq->rxobj);
+		rxq->rxobj = INVALID_MANA_HANDLE;
+	}
+
+	if (rxq->rx_cq.gdma_cq)
+		mana_gd_unpublish_cq(gc, rxq->rx_cq.gdma_cq);
+}
+
+/* Free an RXQ once its cq_table slot has been cleared and a grace period
+ * has elapsed (see mana_quiesce_rxq()).  mana_deinit_cq() ->
+ * mana_gd_destroy_cq() finds the slot already NULL and skips its own
+ * synchronize_rcu().
+ */
+static void mana_free_rxq(struct mana_port_context *apc, struct mana_rxq *rxq)
+{
+	struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
+	struct mana_recv_buf_oob *rx_oob;
+	struct device *dev = gc->dev;
+	struct page *page;
+	int i;
 
 	mana_deinit_cq(apc, &rxq->rx_cq);
 
@@ -2685,6 +2728,23 @@ static void mana_destroy_rxq(struct mana_port_context *apc,
 	kvfree(rxq);
 }
 
+static void mana_destroy_rxq(struct mana_port_context *apc,
+			     struct mana_rxq *rxq, bool napi_initialized)
+
+{
+	if (!rxq)
+		return;
+
+	mana_quiesce_rxq(apc, rxq, napi_initialized);
+
+	/* Wait for in-flight EQ handlers that may have loaded the old CQ
+	 * pointer via rcu_dereference() before freeing.
+	 */
+	synchronize_rcu();
+
+	mana_free_rxq(apc, rxq);
+}
+
 static int mana_fill_rx_oob(struct mana_recv_buf_oob *rx_oob, u32 mem_key,
 			    struct mana_rxq *rxq, struct device *dev)
 {
@@ -2821,6 +2881,7 @@ static struct mana_rxq *mana_create_rxq(struct mana_port_context *apc,
 	struct gdma_queue_spec spec;
 	struct mana_cq *cq = NULL;
 	struct gdma_context *gc;
+	struct gdma_queue __rcu **cq_table;
 	u32 cq_size, rq_size;
 	struct mana_rxq *rxq;
 	int err;
@@ -2905,12 +2966,18 @@ static struct mana_rxq *mana_create_rxq(struct mana_port_context *apc,
 	if (err)
 		goto out;
 
-	if (WARN_ON(cq->gdma_id >= gc->max_num_cqs)) {
+	/* No rcu_read_lock(): mana_create_rxq runs under RTNL during netdev
+	 * bring-up, inside the netdev lifetime that mana_remove() drains
+	 * before the base cq_table can be freed.  See gdma_context::cq_table
+	 * in gdma.h for why "true" is sound.
+	 */
+	cq_table = rcu_dereference_protected(gc->cq_table, true);
+	if (WARN_ON(!cq_table || cq->gdma_id >= gc->max_num_cqs)) {
 		err = -EINVAL;
 		goto out;
 	}
 
-	gc->cq_table[cq->gdma_id] = cq->gdma_cq;
+	rcu_assign_pointer(cq_table[cq->gdma_id], cq->gdma_cq);
 
 	netif_napi_add_weight_locked(ndev, &cq->napi, mana_poll, 1);
 
@@ -2987,16 +3054,31 @@ static void mana_destroy_rxqs(struct mana_port_context *apc)
 	struct mana_rxq *rxq;
 	u32 rxq_idx;
 
-	if (apc->rxqs) {
+	if (!apc->rxqs)
+		return;
 
-		for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
-			rxq = apc->rxqs[rxq_idx];
-			if (!rxq)
-				continue;
+	/* Pass 1: quiesce every RXQ's CQ and clear its cq_table slot. */
+	for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
+		rxq = apc->rxqs[rxq_idx];
+		if (!rxq)
+			continue;
 
-			mana_destroy_rxq(apc, rxq, true);
-			apc->rxqs[rxq_idx] = NULL;
-		}
+		mana_quiesce_rxq(apc, rxq, true);
+	}
+
+	/* One grace period for the whole port instead of one per RXQ. */
+	synchronize_rcu();
+
+	/* Pass 2: the slots are clear, so mana_gd_destroy_cq() skips its own
+	 * grace period; free each RXQ.
+	 */
+	for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
+		rxq = apc->rxqs[rxq_idx];
+		if (!rxq)
+			continue;
+
+		mana_free_rxq(apc, rxq);
+		apc->rxqs[rxq_idx] = NULL;
 	}
 }
 
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 0c395917b214..b8b1b23f3c36 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -418,7 +418,31 @@ struct gdma_context {
 
 	/* This maps a CQ index to the queue structure. */
 	unsigned int		max_num_cqs;
-	struct gdma_queue	**cq_table;
+	/* Both the base pointer and each entry are RCU-managed.  The fast
+	 * path (mana_gd_process_eqe) reads the base via rcu_dereference()
+	 * under rcu_read_lock(), so the table is freed with
+	 * rcu_assign_pointer(NULL) + synchronize_rcu() and an in-flight
+	 * reader can never observe freed memory.
+	 *
+	 * The slow paths -- mana_gd_destroy_cq() and the CQ install/remove
+	 * callers (mana_create_txq/_rxq, mana_ib_install/remove_cq_cb) --
+	 * instead read the base with rcu_dereference_protected(cq_table,
+	 * true).  The bare "true" asserts teardown/bring-up ordering, not a
+	 * lock: the base table is allocated in mana_hwc_establish_channel()
+	 * and replaced+freed only by mana_hwc_destroy_channel() (via
+	 * mana_gd_cleanup_device()) and the create-time reinit.  The reinit
+	 * runs before either consumer is probed, and cleanup_device() runs
+	 * after mana_remove() / mana_rdma_remove() have detached the ports
+	 * under RTNL and drained the IB device, so no install/remove caller
+	 * is running when the base is freed.  This is an ordering argument
+	 * about when cleanup_device() runs: suspend and shutdown keep the
+	 * netdev registered, so it does not rely on unregister_netdevice()
+	 * having run on every path.  mana_hwc_destroy_channel() itself reads
+	 * cq_table (mana_hwc_destroy_cq()) before it replaces and vfree()s
+	 * the base, so that access is ordered ahead of the free by program
+	 * order.
+	 */
+	struct gdma_queue	__rcu * __rcu *cq_table;
 
 	/* Protect eq_test_event and test_event_eq_id  */
 	struct mutex		eq_test_event_mutex;
@@ -496,6 +520,12 @@ int mana_gd_create_mana_wq_cq(struct gdma_dev *gd,
 
 void mana_gd_destroy_queue(struct gdma_context *gc, struct gdma_queue *queue);
 
+/* Clear a CQ's cq_table slot without waiting for a grace period.  Batched
+ * teardown paths clear several slots and then take a single synchronize_rcu();
+ * single-CQ callers use mana_gd_destroy_cq() instead, which also waits.
+ */
+bool mana_gd_unpublish_cq(struct gdma_context *gc, struct gdma_queue *queue);
+
 int mana_gd_poll_cq(struct gdma_queue *cq, struct gdma_comp *comp, int num_cqe);
 
 void mana_gd_ring_cq(struct gdma_queue *cq, u8 arm_bit);
-- 
2.43.0

[PATCH net v4 2/7] net: mana: fix HWC RQ/SQ buffer size swap

[Long Li]

The HWC RQ receives responses and the SQ sends requests, but
mana_hwc_init_queues() sized the RQ with max_req_msg_size and the SQ with
max_resp_msg_size -- backwards.  mana_hwc_rx_event_handler() also
recovered the RX slot index by dividing by max_req_msg_size instead of
the response size that strides the RQ buffer.

Both mistakes are latent today: the only caller passes
HW_CHANNEL_MAX_REQUEST_SIZE and HW_CHANNEL_MAX_RESPONSE_SIZE, which are
both 0x1000, and the queues are never re-created with the
hardware-reported sizes (those are only used by mana_hwc_test_channel()),
so max_req_msg_size == max_resp_msg_size on every path and there is no
observable overflow or mis-indexing.  The bug would only surface if the
two sizes ever diverged.

Size the RQ by max_resp_msg_size and the SQ by max_req_msg_size, store
max_resp_msg_size in hw_channel_context, and use it as the RX slot stride
so the code is correct regardless of the two sizes.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v4:
 - Reworded the changelog as a latent-correctness fix (both message
   sizes are 0x1000, so the swap has no observable overflow) and
   dropped the note about hoisting the queue dimensions above
   mana_hwc_create_cq().

 drivers/net/ethernet/microsoft/mana/hw_channel.c | 7 ++++---
 include/net/mana/hw_channel.h                    | 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 409e20caeccd..3f011ebbe7b3 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -263,7 +263,7 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
 
 	/* Select the RX work request for virtual address and for reposting. */
 	rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
-	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_req_msg_size;
+	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size;
 
 	if (rx_req_idx >= hwc_rxq->msg_buf->num_reqs) {
 		dev_err(hwc->dev, "HWC RX: wrong rx_req_idx=%llu, num_reqs=%u\n",
@@ -733,14 +733,14 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
 		goto out;
 	}
 
-	err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_req_msg_size,
+	err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_resp_msg_size,
 				 hwc->cq, &hwc->rxq);
 	if (err) {
 		dev_err(hwc->dev, "Failed to create HWC RQ: %d\n", err);
 		goto out;
 	}
 
-	err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_resp_msg_size,
+	err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_req_msg_size,
 				 hwc->cq, &hwc->txq);
 	if (err) {
 		dev_err(hwc->dev, "Failed to create HWC SQ: %d\n", err);
@@ -749,6 +749,7 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
 
 	hwc->num_inflight_msg = q_depth;
 	hwc->max_req_msg_size = max_req_msg_size;
+	hwc->max_resp_msg_size = max_resp_msg_size;
 
 	return 0;
 out:
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 16feb39616c1..73671f479399 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -181,6 +181,7 @@ struct hw_channel_context {
 
 	u16 num_inflight_msg;
 	u32 max_req_msg_size;
+	u32 max_resp_msg_size;
 
 	u16 hwc_init_q_depth_max;
 	u32 hwc_init_max_req_msg_size;
-- 
2.43.0

[PATCH net v4 3/7] net: mana: free HWC comp_buf after destroying the EQ

[Long Li]

mana_hwc_destroy_cq() freed hwc_cq->comp_buf and destroyed the CQ before
the EQ.  That was unsafe while the EQ was still registered: the EQ
interrupt handler reaches comp_buf via mana_hwc_comp_event() and the CQ
object (hwc->cq->gdma_cq) via mana_hwc_init_event_handler(), so a late
EQE dispatched after the free could touch freed memory.

Destroy the EQ first.  mana_gd_destroy_queue() on the EQ deregisters its
IRQ and waits out in-flight handlers, fencing all EQE dispatch; only then
free the CQ and comp_buf.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v4:
 - No functional change since v3; the teardown-ordering guarantees this
   patch relies on are made explicit in patch 5.

 .../net/ethernet/microsoft/mana/hw_channel.c   | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 3f011ebbe7b3..19896bb5ce1a 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -384,14 +384,24 @@ static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)
 
 static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
 {
-	kfree(hwc_cq->comp_buf);
+	/* Destroy the EQ before the CQ.  mana_gd_destroy_queue() on the EQ
+	 * deregisters its IRQ and waits out in-flight handlers, fencing all
+	 * EQE dispatch — both the completion path and HWC init/reconfig
+	 * events.  Freeing the CQ first would leave the EQ live and able to
+	 * dispatch an event that dereferences hwc->cq->gdma_cq (e.g.
+	 * mana_hwc_init_event_handler()) after it has been freed.
+	 */
+	if (hwc_cq->gdma_eq)
+		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
 
+	/* comp_buf is reached only by mana_hwc_comp_event(), invoked from
+	 * the now-fenced EQ handler, so it is safe to free once the EQ and
+	 * CQ are gone.
+	 */
 	if (hwc_cq->gdma_cq)
 		mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
 
-	if (hwc_cq->gdma_eq)
-		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
-
+	kfree(hwc_cq->comp_buf);
 	kfree(hwc_cq);
 }
 
-- 
2.43.0

[PATCH net v4 4/7] net: mana: validate hardware-supplied values in the HWC RX path

[Long Li]

mana_hwc_rx_event_handler() consumed lengths and indices taken straight
from device DMA without validation.  A buggy firmware or a malicious host
(in a confidential VM, where the DMA buffer is shared) could drive a
wrong or reused in-flight request to completion or index out of bounds.
Validate before use:

  - snapshot the device-supplied inline_oob_size_div4 (read once through
    its u32 flags word with READ_ONCE(), as it is a bit-field) and reject
    any value other than the one the driver programs
    (INLINE_OOB_SMALL_SIZE / 4), so a corrupted OOB size cannot move the
    SGE out of the WQE before it is dereferenced;
  - snapshot sge->address with READ_ONCE() and validate and use only the
    snapshot, so the value that is bounds-checked is the value that is
    used (the DMA buffer is host-writable in a confidential VM);
  - match the SGE address against the address the driver posted for that
    slot, not just an in-range index -- an in-range but wrong SGE would
    otherwise truncate onto a neighbouring slot and read a stale response;
  - reject a resp_len larger than the RX buffer.

As defence in depth, mana_hwc_handle_resp() also bounds-checks hwc_msg_id
before indexing the inflight bitmap and caller_ctx.  Its only caller
already rejects the same range with the value it passes by value, so this
is a guard at the indexing site, not a reachable out-of-bounds.

Repost the RX WQE on every validation early-return that can still
identify its slot.  The paths that cannot -- an unexpected OOB size, an
out-of-range index, or an SGE address matching no posted slot --
intentionally leak a single WQE rather than risk reposting the wrong one.
Because the HWC RQ depth is never replenished, count those leaks and,
once they exhaust the posted depth, log the terminal state and shorten
the command timeout so callers fail fast instead of draining silently.

A short response is no longer rejected in the handler: it reaches
mana_hwc_handle_resp(), whose mana_hwc_verify_resp_msg() fails it with
-EPROTO and completes the waiting sender, so a single malformed response
cannot convert into a channel-wide timeout.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v4:
 - Removed the short-response early return so a malformed response
   reaches verify_resp_msg() -> -EPROTO and completes the sender
   instead of hanging it.
 - Account leaked RX WQEs and trip hwc_timeout on RQ exhaustion.
 - Read inline_oob_size_div4 (through its u32 flags word, as it is a
   bit-field) and sge->address with READ_ONCE() and reject any value
   other than the one the driver programs.
 - Reframed the msg_id check as defense in depth in the changelog.

 .../net/ethernet/microsoft/mana/hw_channel.c  | 116 ++++++++++++++++--
 include/net/mana/hw_channel.h                 |   6 +
 2 files changed, 111 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 19896bb5ce1a..5db8cfe2d844 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -83,6 +83,19 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
 	struct hwc_caller_ctx *ctx;
 	int err;
 
+	/* Defence in depth: the sole caller, mana_hwc_rx_event_handler(),
+	 * already rejects msg_id >= hwc->num_inflight_msg with the value it
+	 * passes here by value, so this cannot be reached out of range.  Keep
+	 * the guard at the indexing site so the bitmap and caller_ctx array
+	 * are never indexed without a bound in view.
+	 */
+	if (msg_id >= hwc->num_inflight_msg) {
+		dev_err(hwc->dev, "hwc_rx: msg_id %u >= max %u\n",
+			msg_id, hwc->num_inflight_msg);
+		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
+		return;
+	}
+
 	if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
 		dev_err(hwc->dev, "hwc_rx: invalid msg_id = %u\n", msg_id);
 		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
@@ -90,6 +103,18 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
 	}
 
 	ctx = hwc->caller_ctx + msg_id;
+
+	/* Reject responses larger than the RX DMA buffer — the SGE
+	 * limits what hardware can DMA, so an oversized resp_len
+	 * indicates a firmware bug.  Fail rather than silently
+	 * truncating.
+	 */
+	if (resp_len > rx_req->buf_len) {
+		dev_err(hwc->dev, "HWC RX: resp_len %u > buf_len %u\n",
+			resp_len, rx_req->buf_len);
+		resp_len = 0;
+	}
+
 	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
 	if (err)
 		goto out;
@@ -237,18 +262,39 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
 	}
 }
 
+/* An RX WQE whose SGE the handler cannot trust is deliberately not
+ * reposted: reposting a slot we may have mis-identified could double-post
+ * a buffer the device still owns.  Each such leak permanently lowers the
+ * RQ's posted depth, so once the whole depth is gone the channel can no
+ * longer receive responses.  Make that terminal state explicit -- log it
+ * once and shorten the command timeout so callers fail fast -- rather than
+ * letting every later command drain its full timeout against a dead RQ.
+ */
+static void mana_hwc_rx_leak_wqe(struct hw_channel_context *hwc)
+{
+	if (++hwc->rx_leaked_wqe == hwc->rxq->queue_depth) {
+		dev_err(hwc->dev,
+			"HWC RX: RQ exhausted after %u leaked WQEs; channel unusable\n",
+			hwc->rx_leaked_wqe);
+		hwc->hwc_timeout = 1;
+	}
+}
+
 static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
 				      const struct hwc_rx_oob *rx_oob)
 {
 	struct hw_channel_context *hwc = ctx;
 	struct hwc_wq *hwc_rxq = hwc->rxq;
 	struct hwc_work_request *rx_req;
+	struct gdma_wqe oob_snapshot;
 	struct gdma_resp_hdr *resp;
 	struct gdma_wqe *dma_oob;
 	struct gdma_queue *rq;
 	struct gdma_sge *sge;
 	u64 rq_base_addr;
 	u64 rx_req_idx;
+	u64 sge_addr;
+	u32 oob_div4;
 	u16 msg_id;
 	u8 *wqe;
 
@@ -259,28 +305,76 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
 	wqe = mana_gd_get_wqe_ptr(rq, rx_oob->wqe_offset / GDMA_WQE_BU_SIZE);
 	dma_oob = (struct gdma_wqe *)wqe;
 
-	sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 * 4);
-
-	/* Select the RX work request for virtual address and for reposting. */
+	/* inline_oob_size_div4 lives in device-accessible RQ memory (shared
+	 * and host-writable in a confidential VM), so snapshot it once and
+	 * validate and use only the snapshot.  It is a bit-field, which
+	 * READ_ONCE() cannot take the size of, so read the u32 flags word it
+	 * shares through the union and extract the field from the local copy.
+	 * The driver programs INLINE_OOB_SMALL_SIZE for every HWC RQ WQE via
+	 * mana_gd_post_work_request(), so the only valid value is
+	 * INLINE_OOB_SMALL_SIZE / 4, which puts the SGE at wqe + 16 inside
+	 * this WQE's own BU.  Reject anything else -- the slot cannot be
+	 * trusted, so leak this RX WQE rather than repost the wrong one.
+	 */
+	oob_snapshot.flags = READ_ONCE(dma_oob->flags);
+	oob_div4 = oob_snapshot.inline_oob_size_div4;
+	if (oob_div4 != INLINE_OOB_SMALL_SIZE / 4) {
+		dev_err(hwc->dev, "HWC RX: unexpected inline_oob_size_div4=%u\n",
+			oob_div4);
+		mana_hwc_rx_leak_wqe(hwc);
+		return;
+	}
+	sge = (struct gdma_sge *)(wqe + 8 + oob_div4 * 4);
+
+	/* Recover the originating RX slot from the SGE address.  Snapshot it
+	 * once, for the same shared-memory reason: of the three terms only
+	 * sge_addr comes from device memory; rq_base_addr and
+	 * max_resp_msg_size are driver-private.  An in-range but wrong SGE
+	 * would otherwise truncate onto a neighbouring slot, letting us read
+	 * a stale response that could complete the wrong, reused in-flight
+	 * request.  Require the index in range AND the address to exactly
+	 * match the value the driver posted for that slot.
+	 */
+	sge_addr = READ_ONCE(sge->address);
 	rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
-	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size;
-
-	if (rx_req_idx >= hwc_rxq->msg_buf->num_reqs) {
-		dev_err(hwc->dev, "HWC RX: wrong rx_req_idx=%llu, num_reqs=%u\n",
-			rx_req_idx, hwc_rxq->msg_buf->num_reqs);
+	rx_req_idx = (sge_addr - rq_base_addr) / hwc->max_resp_msg_size;
+
+	if (rx_req_idx >= hwc_rxq->queue_depth) {
+		/* Cannot identify the slot, so we cannot safely repost this
+		 * WQE; leak it.  An out-of-range index means a corrupted SGE
+		 * from hardware or host tampering.
+		 */
+		dev_err(hwc->dev, "HWC RX: SGE idx %llu out of range\n",
+			rx_req_idx);
+		mana_hwc_rx_leak_wqe(hwc);
 		return;
 	}
 
 	rx_req = &hwc_rxq->msg_buf->reqs[rx_req_idx];
+	if (sge_addr != (u64)rx_req->buf_sge_addr) {
+		/* In-range index but the address does not match what the
+		 * driver posted for that slot; the same unrecoverable case,
+		 * so leak this WQE rather than repost the wrong one.
+		 */
+		dev_err(hwc->dev, "HWC RX: invalid SGE address %llx (idx=%llu)\n",
+			sge_addr, rx_req_idx);
+		mana_hwc_rx_leak_wqe(hwc);
+		return;
+	}
+
 	resp = (struct gdma_resp_hdr *)rx_req->buf_va;
 
-	/* Read msg_id once from DMA buffer to prevent TOCTOU:
-	 * DMA memory is shared/unencrypted in CVMs - host can
-	 * modify it between reads.
+	/* Read msg_id once from the DMA buffer to prevent TOCTOU: DMA memory
+	 * is shared/unencrypted in CVMs, so the host can modify it between
+	 * reads.  A short response is not rejected here; it is handed to
+	 * mana_hwc_handle_resp() below, whose mana_hwc_verify_resp_msg()
+	 * fails it with -EPROTO and completes the waiting sender, so one
+	 * malformed response cannot stall the whole channel.
 	 */
 	msg_id = READ_ONCE(resp->response.hwc_msg_id);
 	if (msg_id >= hwc->num_inflight_msg) {
 		dev_err(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
+		mana_hwc_post_rx_wqe(hwc_rxq, rx_req);
 		return;
 	}
 
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 73671f479399..787c6f96d5b5 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -200,6 +200,12 @@ struct hw_channel_context {
 	u32 pf_dest_vrcq_id;
 	u32 hwc_timeout;
 
+	/* Count of RX WQEs deliberately not reposted after an untrusted SGE
+	 * (see mana_hwc_rx_leak_wqe()); once it reaches the RQ depth the
+	 * channel can no longer receive responses.
+	 */
+	u32 rx_leaked_wqe;
+
 	struct hwc_caller_ctx *caller_ctx;
 };
 
-- 
2.43.0

[PATCH net v4 5/7] net: mana: fix HWC teardown safety with setup_active flag and destroy ordering

[Long Li]

Three teardown hazards let the hardware touch memory the driver freed.

First, once mana_smc_setup_hwc() succeeds the device has active MST
entries and can DMA into the HWC queue buffers.  If a later step in
mana_hwc_establish_channel() fails, the caller had no reliable way to
know teardown was required and could free those buffers while the
mappings were still live -- a DMA-after-free.  max_num_cqs was used as a
"HWC is up" proxy, but it is only set when the init EQE arrives.

Add a setup_active flag, set the moment setup_hwc activates MST entries.
On a later failure establish_channel() just returns the error; the
caller's error path (mana_hwc_create_channel() -> destroy_channel())
performs the single teardown, gated on setup_active.  Tearing down inline
as well would run teardown twice -- doubling the 60s hardware timeout on
failure and masking the original error code.  max_num_cqs is no longer
reset: it is an immutable bound (see gdma.h) and cq_table == NULL is the
sole teardown signal.

Second, destroy_channel() freed the TXQ/RXQ buffers while the HWC EQ was
still on the interrupt dispatch list, so an in-flight interrupt could run
the handler against freed buffers:

  CPU A (mana_gd_intr, hard IRQ)        CPU B (destroy_channel)
  ----------------------------------    ------------------------------
                                        free TXQ/RXQ DMA buffers
  handler accesses RQ/TXQ buffers       (EQ still registered)

Destroy the CQ first: mana_hwc_destroy_cq() -> mana_gd_deregister_irq()
removes the EQ via list_del_rcu() + synchronize_rcu(), after which no
handler can reach the queues; only then free the TXQ and RXQ.

Third, if mana_smc_teardown_hwc() itself fails the MST entries stay
live, yet destroy_channel() went on to free the CQ/RQ/TXQ buffers the
device can still DMA into -- a DMA-after-free on systems without an
IOMMU to fault the stale access.  Leak the HWC resources on teardown
failure instead of freeing memory the hardware can still reach, and
keep setup_active set so the failure is not mistaken for a clean
teardown.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v4:
 - Arm setup_active immediately after mana_smc_setup_hwc() succeeds.
 - Destroy the EQ (IRQ deregister + drain) before the CQ.
 - Dropped the redundant teardown in mana_hwc_establish_channel() that
   caused a double hardware timeout and masked the original error code.

 .../net/ethernet/microsoft/mana/hw_channel.c  | 56 ++++++++++++++-----
 include/net/mana/gdma.h                       |  8 ++-
 include/net/mana/hw_channel.h                 |  9 +++
 3 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 5db8cfe2d844..adc7ad98ca8d 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -4,6 +4,7 @@
 #include <net/mana/gdma.h>
 #include <net/mana/mana.h>
 #include <net/mana/hw_channel.h>
+#include <linux/pci.h>
 #include <linux/vmalloc.h>
 
 static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
@@ -792,6 +793,15 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 	if (err)
 		return err;
 
+	/* setup_hwc activated MST entries — hardware can now DMA into our
+	 * queue buffers.  Record that in setup_active so the caller's error
+	 * path (mana_hwc_create_channel() -> mana_hwc_destroy_channel())
+	 * tears the HWC down exactly once.  Do not also tear down here: a
+	 * second teardown would double the hardware timeout on failure and
+	 * mask the original error code.
+	 */
+	hwc->setup_active = true;
+
 	if (!wait_for_completion_timeout(&hwc->hwc_init_eqe_comp, 60 * HZ))
 		return -ETIMEDOUT;
 
@@ -926,11 +936,38 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	if (!hwc)
 		return;
 
-	/* gc->max_num_cqs is set in mana_hwc_init_event_handler(). If it's
-	 * non-zero, the HWC worked and we should tear down the HWC here.
+	/* Tear down the HWC if setup_hwc previously activated MST entries.
+	 * This is the definitive flag — unlike max_num_cqs which depends
+	 * on the init EQE arriving.
+	 *
+	 * If teardown fails the device may still have active MST entries
+	 * and can DMA into the HWC queue buffers.  Freeing them would risk
+	 * memory corruption on systems without an IOMMU to fault the stale
+	 * DMA, so leak the HWC resources instead of handing the pages back
+	 * to the allocator.  Keep setup_active set so the failure is not
+	 * mistaken for a clean teardown.
+	 */
+	if (hwc->setup_active) {
+		int td_err = mana_smc_teardown_hwc(&gc->shm_channel, false);
+
+		if (td_err) {
+			dev_err(gc->dev,
+				"HWC teardown failed: %d, leaking resources\n",
+				td_err);
+			return;
+		}
+
+		hwc->setup_active = false;
+	}
+
+	/* Tear down the HWC CQ object first — mana_hwc_destroy_cq()
+	 * both unpublishes the CQ from cq_table (+synchronize_rcu) and
+	 * deregisters the HWC EQ from the interrupt handler list (via
+	 * mana_gd_deregister_irq + synchronize_rcu), guaranteeing no
+	 * interrupt handler can access RQ/TXQ buffers after this point.
 	 */
-	if (gc->max_num_cqs > 0)
-		mana_smc_teardown_hwc(&gc->shm_channel, false);
+	if (hwc->cq)
+		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
 
 	if (hwc->txq)
 		mana_hwc_destroy_wq(hwc, hwc->txq);
@@ -938,17 +975,6 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	if (hwc->rxq)
 		mana_hwc_destroy_wq(hwc, hwc->rxq);
 
-	if (hwc->cq)
-		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
-
-	/* Reset only after mana_hwc_destroy_cq() above has run with a valid
-	 * max_num_cqs so mana_gd_destroy_cq() clears the CQ table slot and
-	 * waits out in-flight EQ handlers (synchronize_rcu) before the CQ is
-	 * freed.  Clearing it earlier would make that path early-return and
-	 * skip the slot clear, leaving a dangling cq_table entry.
-	 */
-	gc->max_num_cqs = 0;
-
 	kfree(hwc->caller_ctx);
 	hwc->caller_ctx = NULL;
 
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index b8b1b23f3c36..e59b8b31e834 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -416,7 +416,13 @@ struct gdma_context {
 	/* L2 MTU */
 	u16 adapter_mtu;
 
-	/* This maps a CQ index to the queue structure. */
+	/* Size of cq_table, i.e. the largest valid CQ index + 1.  Set once
+	 * when cq_table is allocated and treated as immutable for the
+	 * table's lifetime (a bound only) -- it is never reset on teardown.
+	 * cq_table == NULL is the sole "table torn down" signal, so every
+	 * cq_table[id] access must guard with both !cq_table (gone) and
+	 * id >= max_num_cqs (out of bounds).
+	 */
 	unsigned int		max_num_cqs;
 	/* Both the base pointer and each entry are RCU-managed.  The fast
 	 * path (mana_gd_process_eqe) reads the base via rcu_dereference()
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 787c6f96d5b5..8340abd36af6 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -206,6 +206,15 @@ struct hw_channel_context {
 	 */
 	u32 rx_leaked_wqe;
 
+	/* Set after mana_smc_setup_hwc() succeeds (hardware has active
+	 * MST entries).  Cleared only after mana_smc_teardown_hwc()
+	 * succeeds, on both the recoverable establish_channel path and the
+	 * terminal destroy_channel path.  If teardown fails it stays set:
+	 * establish_channel() skips its retry and destroy_channel() leaks
+	 * the HWC rather than free buffers the device may still DMA into.
+	 */
+	bool setup_active;
+
 	struct hwc_caller_ctx *caller_ctx;
 };
 
-- 
2.43.0

[PATCH net v4 6/7] net: mana: fix stale HWC response after command timeout

[Long Li]

The HWC freed a message slot (mana_hwc_put_msg_index) the instant
mana_hwc_send_request() timed out, while the hardware command was still
pending and caller_ctx.output_buf still pointed at the caller's response
buffer.  A late response then raced two ways:

  - handle_resp() runs in CQ interrupt context and memcpy()'d into
    output_buf after the sender had returned and its buffer was gone.

  - the freed slot was reused by the next request, so the stale
    response completed the wrong command with another request's data.

Give each caller_ctx a spinlock, a refcount and an -EINPROGRESS
sentinel (and change caller_ctx::error from u32 to int so it holds
the negative errno values, including the sentinel, without relying on
unsigned wraparound):

  - The sender publishes output_buf under the slot lock and NULLs it
    under the same lock on timeout/exit, so handle_resp() (also under
    the lock) skips the copy once the sender is gone.

  - The slot is released only when both the sender and handle_resp()
    have dropped their reference, so a msg_id whose response is still
    outstanding is never handed to a new request.

  - Both references are taken up front in mana_hwc_get_msg_index(),
    under the same lock that publishes the slot, so a stale, duplicate
    or early response that arrives before the sender posts drops only
    the response-side reference and cannot release the slot out from
    under the sender.  A per-slot "responded" flag drops the payload of
    any such extra response.

  - On a genuine timeout the channel is marked hwc_timed_out and further
    mana_hwc_get_msg_index() callers fail with -ETIMEDOUT instead of
    reusing a slot whose response may still arrive.  The flag is read
    with READ_ONCE() outside the bitmap lock and written with
    WRITE_ONCE() under it.

Replace the counting semaphore with a waitqueue + bitmap so a slot held
past a timeout does not deadlock admission and timed-out waiters can be
released.

Because the timeout latch keys off wait_for_completion_timeout()
returning immediately, a zero hwc_timeout would time out every command
at once and latch the whole channel.  Ignore a device-reported zero from
both sources that feed hwc_timeout -- the HWC_DATA_CFG_HWC_TIMEOUT
reconfig event and the GDMA_QUERY_HWC_TIMEOUT response -- and keep the
positive default instead.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v4:
 - Take both the sender and response-side references up front in
   mana_hwc_get_msg_index() (refcount initialised to 2, under the lock
   that publishes the slot) so an early/stale/forged response cannot
   free the slot before the sender posts; the pre-post error path
   latches ->responded to avoid a double drop.
 - Changed caller_ctx::error from u32 to int so it holds the negative
   -EINPROGRESS sentinel and errno values directly.
 - Reject a zero firmware-supplied HWC timeout in the query path as
   well as the reconfig path.
 - Access hwc_timed_out with READ_ONCE()/WRITE_ONCE(); comment and
   changelog fixes.

 .../net/ethernet/microsoft/mana/gdma_main.c   |   7 +-
 .../net/ethernet/microsoft/mana/hw_channel.c  | 218 +++++++++++++++---
 include/net/mana/hw_channel.h                 |  27 ++-
 3 files changed, 214 insertions(+), 38 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index 7714040d1df4..9c2f5e0cfcbc 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -310,7 +310,12 @@ static int mana_gd_query_hwc_timeout(struct pci_dev *pdev, u32 *timeout_val)
 	if (err || resp.hdr.status)
 		return err ? err : -EPROTO;
 
-	*timeout_val = resp.timeout_ms;
+	/* A zero timeout would make every HWC command time out immediately
+	 * and latch the channel (see the HWC_DATA_CFG_HWC_TIMEOUT handler).
+	 * Ignore a zero from the device and keep the caller's positive value.
+	 */
+	if (resp.timeout_ms)
+		*timeout_val = resp.timeout_ms;
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index adc7ad98ca8d..2f0dae353955 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -7,25 +7,58 @@
 #include <linux/pci.h>
 #include <linux/vmalloc.h>
 
+/* Acquire a free message slot from the inflight bitmap.  Returns
+ * -ETIMEDOUT if a prior HWC command has timed out (preserving the
+ * error code callers expect).
+ */
 static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
 {
 	struct gdma_resource *r = &hwc->inflight_msg_res;
 	unsigned long flags;
 	u32 index;
 
-	down(&hwc->sema);
+	for (;;) {
+		spin_lock_irqsave(&r->lock, flags);
 
-	spin_lock_irqsave(&r->lock, flags);
+		if (hwc->hwc_timed_out) {
+			spin_unlock_irqrestore(&r->lock, flags);
+			return -ETIMEDOUT;
+		}
 
-	index = find_first_zero_bit(hwc->inflight_msg_res.map,
-				    hwc->inflight_msg_res.size);
+		index = find_first_zero_bit(r->map, r->size);
+		if (index < r->size) {
+			struct hwc_caller_ctx *ctx;
+
+			bitmap_set(r->map, index, 1);
+			ctx = &hwc->caller_ctx[index];
+			reinit_completion(&ctx->comp_event);
+			/* Take the response-side reference here, under
+			 * r->lock and together with the slot bitmap bit,
+			 * so a stale or duplicate response that lands
+			 * before mana_hwc_send_request() posts the request
+			 * cannot drop the refcount to zero and free the
+			 * slot under the sender.  One reference is the
+			 * sender's; the other is released by
+			 * mana_hwc_handle_resp().
+			 */
+			refcount_set(&ctx->refcnt, 2);
+			ctx->responded = false;
+			ctx->msg_id = index;
+			ctx->error = -EINPROGRESS;
+			spin_unlock_irqrestore(&r->lock, flags);
+			break;
+		}
+		spin_unlock_irqrestore(&r->lock, flags);
 
-	bitmap_set(hwc->inflight_msg_res.map, index, 1);
+		wait_event(hwc->msg_waitq,
+			   READ_ONCE(hwc->hwc_timed_out) ||
+			   !bitmap_full(r->map, r->size));
 
-	spin_unlock_irqrestore(&r->lock, flags);
+		if (READ_ONCE(hwc->hwc_timed_out))
+			return -ETIMEDOUT;
+	}
 
 	*msg_id = index;
-
 	return 0;
 }
 
@@ -35,10 +68,17 @@ static void mana_hwc_put_msg_index(struct hw_channel_context *hwc, u16 msg_id)
 	unsigned long flags;
 
 	spin_lock_irqsave(&r->lock, flags);
-	bitmap_clear(hwc->inflight_msg_res.map, msg_id, 1);
+	bitmap_clear(r->map, msg_id, 1);
 	spin_unlock_irqrestore(&r->lock, flags);
 
-	up(&hwc->sema);
+	wake_up(&hwc->msg_waitq);
+}
+
+static void hwc_ctx_put(struct hw_channel_context *hwc,
+			struct hwc_caller_ctx *ctx)
+{
+	if (refcount_dec_and_test(&ctx->refcnt))
+		mana_hwc_put_msg_index(hwc, ctx->msg_id);
 }
 
 static int mana_hwc_verify_resp_msg(const struct hwc_caller_ctx *caller_ctx,
@@ -116,22 +156,44 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
 		resp_len = 0;
 	}
 
-	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
-	if (err)
-		goto out;
+	spin_lock(&ctx->lock);
 
-	ctx->status_code = resp_msg->status;
+	if (ctx->responded) {
+		/* A response for this slot was already delivered; this is a
+		 * duplicate or replayed one.  Drop it so the hwc_ctx_put()
+		 * a first response performs is not done twice, which would
+		 * over-release the slot while the sender still owns it.
+		 */
+		spin_unlock(&ctx->lock);
+		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
+		return;
+	}
+	ctx->responded = true;
 
-	memcpy(ctx->output_buf, resp_msg, resp_len);
-out:
-	ctx->error = err;
+	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
+
+	if (!err && ctx->output_buf) {
+		ctx->status_code = resp_msg->status;
+		memcpy(ctx->output_buf, resp_msg, resp_len);
+		ctx->error = 0;
+	} else if (ctx->output_buf) {
+		/* Record the error only while the sender still owns the
+		 * request: a non-NULL output_buf means it is still waiting.
+		 * Once it has timed out (or been force-completed by destroy)
+		 * it clears output_buf and takes its own error, so a late
+		 * response must not write ctx->error or the buffer here.
+		 */
+		ctx->error = err;
+	}
 
-	/* Must post rx wqe before complete(), otherwise the next rx may
-	 * hit no_wqe error.
+	/* Post RX WQE before completing — the next response may arrive
+	 * immediately and needs a posted buffer.
 	 */
 	mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
-
 	complete(&ctx->comp_event);
+	spin_unlock(&ctx->lock);
+
+	hwc_ctx_put(hwc, ctx);
 }
 
 static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
@@ -218,7 +280,12 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
 
 		switch (type) {
 		case HWC_DATA_CFG_HWC_TIMEOUT:
-			hwc->hwc_timeout = val;
+			/* A zero timeout would make every command time out
+			 * immediately and latch hwc_timed_out, disabling the
+			 * channel.  Ignore it and keep the positive default.
+			 */
+			if (val)
+				hwc->hwc_timeout = val;
 			break;
 
 		case HWC_DATA_HW_LINK_CONNECT:
@@ -732,7 +799,7 @@ static int mana_hwc_init_inflight_msg(struct hw_channel_context *hwc,
 {
 	int err;
 
-	sema_init(&hwc->sema, num_msg);
+	init_waitqueue_head(&hwc->msg_waitq);
 
 	err = mana_gd_alloc_res_map(num_msg, &hwc->inflight_msg_res);
 	if (err)
@@ -762,8 +829,10 @@ static int mana_hwc_test_channel(struct hw_channel_context *hwc, u16 q_depth,
 	if (!ctx)
 		return -ENOMEM;
 
-	for (i = 0; i < q_depth; ++i)
+	for (i = 0; i < q_depth; ++i) {
+		spin_lock_init(&ctx[i].lock);
 		init_completion(&ctx[i].comp_event);
+	}
 
 	hwc->caller_ctx = ctx;
 
@@ -774,6 +843,9 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 				      u32 *max_req_msg_size,
 				      u32 *max_resp_msg_size)
 {
+	/* No RCU needed: called only from mana_hwc_create_channel
+	 * during init, before the channel is published to senders.
+	 */
 	struct hw_channel_context *hwc = gc->hwc.driver_data;
 	struct gdma_queue *rq = hwc->rxq->gdma_wq;
 	struct gdma_queue *sq = hwc->txq->gdma_wq;
@@ -1004,13 +1076,18 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 	struct hwc_wq *txq = hwc->txq;
 	struct gdma_req_hdr *req_msg;
 	struct hwc_caller_ctx *ctx;
+	unsigned long flags;
+	bool drop_resp_ref;
 	u32 dest_vrcq = 0;
 	u32 dest_vrq = 0;
 	u32 command;
+	u32 status;
 	u16 msg_id;
 	int err;
 
-	mana_hwc_get_msg_index(hwc, &msg_id);
+	err = mana_hwc_get_msg_index(hwc, &msg_id);
+	if (err)
+		return err;
 
 	tx_wr = &txq->msg_buf->reqs[msg_id];
 
@@ -1022,8 +1099,11 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 	}
 
 	ctx = hwc->caller_ctx + msg_id;
+
+	spin_lock_irqsave(&ctx->lock, flags);
 	ctx->output_buf = resp;
 	ctx->output_buflen = resp_len;
+	spin_unlock_irqrestore(&ctx->lock, flags);
 
 	req_msg = (struct gdma_req_hdr *)tx_wr->buf_va;
 	if (req)
@@ -1039,6 +1119,10 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 		dest_vrcq = hwc->pf_dest_vrcq_id;
 	}
 
+	/* handle_resp()'s reference was taken in mana_hwc_get_msg_index(),
+	 * so hardware responding immediately after the doorbell ring cannot
+	 * release the slot before this sender is done with it.
+	 */
 	err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
 	if (err) {
 		dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n", err);
@@ -1051,31 +1135,95 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 			dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
 				command, hwc->hwc_timeout);
 
-		/* Reduce further waiting if HWC no response */
+		/* NULL out output_buf so a late handle_resp() won't write
+		 * into the caller's buffer after the sender returns, then
+		 * check whether handle_resp() already delivered a valid
+		 * response between the timeout firing and this lock
+		 * acquisition — ctx->error != -EINPROGRESS means it ran.
+		 */
+		spin_lock_irqsave(&ctx->lock, flags);
+		ctx->output_buf = NULL;
+		err = ctx->error;
+		status = ctx->status_code;
+		spin_unlock_irqrestore(&ctx->lock, flags);
+
+		if (err != -EINPROGRESS) {
+			/* handle_resp() delivered a valid response just after
+			 * the timeout fired.  The hardware is alive, so use
+			 * the response and leave the channel usable; do not
+			 * latch hwc_timed_out or degrade hwc_timeout for what
+			 * turned out to be a transient race.
+			 */
+			hwc_ctx_put(hwc, ctx);
+			goto check_status;
+		}
+
+		/* Genuine timeout: no response arrived.  Reduce further
+		 * waiting, and mark the channel timed out under the bitmap
+		 * lock so get_msg_index() cannot acquire new slots after this.
+		 */
 		if (hwc->hwc_timeout > 1)
 			hwc->hwc_timeout = 1;
 
+		spin_lock_irqsave(&hwc->inflight_msg_res.lock, flags);
+		WRITE_ONCE(hwc->hwc_timed_out, true);
+		spin_unlock_irqrestore(&hwc->inflight_msg_res.lock, flags);
+		wake_up_all(&hwc->msg_waitq);
+
 		err = -ETIMEDOUT;
-		goto out;
+		hwc_ctx_put(hwc, ctx);
+		goto done;
 	}
 
-	if (ctx->error) {
-		err = ctx->error;
-		goto out;
-	}
+	/* NULL output_buf so a late handle_resp() won't memcpy into
+	 * the caller's buffer after the sender exits.  Read error and
+	 * status_code under the same lock — after hwc_ctx_put the slot
+	 * may be reused and these fields overwritten.
+	 */
+	spin_lock_irqsave(&ctx->lock, flags);
+	ctx->output_buf = NULL;
+	err = ctx->error;
+	status = ctx->status_code;
+	spin_unlock_irqrestore(&ctx->lock, flags);
+	hwc_ctx_put(hwc, ctx);
+
+check_status:
+	if (err)
+		goto done;
 
-	if (ctx->status_code && ctx->status_code != GDMA_STATUS_MORE_ENTRIES) {
-		if (ctx->status_code == GDMA_STATUS_CMD_UNSUPPORTED) {
+	if (status && status != GDMA_STATUS_MORE_ENTRIES) {
+		if (status == GDMA_STATUS_CMD_UNSUPPORTED) {
 			err = -EOPNOTSUPP;
-			goto out;
+			goto done;
 		}
+
 		if (command != MANA_QUERY_PHY_STAT)
 			dev_err(hwc->dev, "Command 0x%x failed with status: 0x%x\n",
-				command, ctx->status_code);
+				command, status);
 		err = -EPROTO;
-		goto out;
+		goto done;
 	}
+
+	err = 0;
+	goto done;
 out:
-	mana_hwc_put_msg_index(hwc, msg_id);
+	/* Pre-post error paths: the request was never submitted, so in the
+	 * common case mana_hwc_handle_resp() will not run for this slot and
+	 * the sender must drop both the response-side reference taken in
+	 * mana_hwc_get_msg_index() and its own.  Guard against a stale or
+	 * forged response that raced in first: latch ->responded under the
+	 * lock so any later handle_resp() is a no-op, and drop the response-
+	 * side reference here only if handle_resp() has not already done so.
+	 */
+	ctx = hwc->caller_ctx + msg_id;
+	spin_lock_irqsave(&ctx->lock, flags);
+	ctx->output_buf = NULL;
+	drop_resp_ref = !ctx->responded;
+	ctx->responded = true;
+	spin_unlock_irqrestore(&ctx->lock, flags);
+	if (drop_resp_ref)
+		refcount_dec(&ctx->refcnt);
+	hwc_ctx_put(hwc, ctx);
+done:
 	return err;
 }
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 8340abd36af6..23bf83e2a3ec 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -171,8 +171,25 @@ struct hwc_caller_ctx {
 	void *output_buf;
 	u32 output_buflen;
 
-	u32 error; /* Linux error code */
+	int error; /* Linux error code (negative errno or 0) */
 	u32 status_code;
+
+	/* Protects output_buf against concurrent access from
+	 * handle_resp() (CQ interrupt) and the sender timeout path.
+	 */
+	spinlock_t lock;
+
+	/* Tracks sender + handle_resp ownership.  The last put
+	 * (refcount reaches 0) releases the bitmap slot.
+	 */
+	refcount_t refcnt;
+	u16 msg_id;
+
+	/* Set under lock by the first handle_resp() for this slot so a
+	 * duplicate or replayed response is dropped instead of consuming
+	 * the response-side reference a second time.
+	 */
+	bool responded;
 };
 
 struct hw_channel_context {
@@ -193,8 +210,9 @@ struct hw_channel_context {
 	struct hwc_wq *txq;
 	struct hwc_cq *cq;
 
-	struct semaphore sema;
 	struct gdma_resource inflight_msg_res;
+	/* Waitqueue for senders blocked on a full inflight bitmap. */
+	wait_queue_head_t msg_waitq;
 
 	u32 pf_dest_vrq_id;
 	u32 pf_dest_vrcq_id;
@@ -206,6 +224,11 @@ struct hw_channel_context {
 	 */
 	u32 rx_leaked_wqe;
 
+	/* Set on first HWC timeout.  Causes get_msg_index() to return
+	 * -ETIMEDOUT instead of waiting, draining all queued senders.
+	 */
+	bool hwc_timed_out;
+
 	/* Set after mana_smc_setup_hwc() succeeds (hardware has active
 	 * MST entries).  Cleared only after mana_smc_teardown_hwc()
 	 * succeeds, on both the recoverable establish_channel path and the
-- 
2.43.0

[PATCH net v4 7/7] net: mana: keep max_num_cqs immutable once cq_table is allocated

[Long Li]

mana_hwc_init_event_handler() applied every HWC_INIT_DATA_MAX_NUM_CQS
event straight to gc->max_num_cqs.  That handler stays live for the whole
channel lifetime -- it also services runtime reconfig and link events --
so it is not confined to the initial bootstrap.

gc->cq_table is allocated once, sized to the max_num_cqs seen at bootstrap,
and every reader (mana_gd_process_eqe(), mana_create_rxq() and
mana_create_txq()) bounds-checks a CQ index against gc->max_num_cqs before
indexing gc->cq_table.  A device -- or a malicious host in a confidential
VM -- that sends a later HWC_INIT_DATA_MAX_NUM_CQS with a larger value
inflates the bound past the allocation.  This includes an event timed to
land while mana_hwc_establish_channel() is between reading the count and
publishing cq_table.  A subsequent out-of-range CQ id then passes the
bounds check and indexes cq_table out of bounds: an out-of-bounds read in
the EQ fast path, or an out-of-bounds pointer write in
mana_create_rxq()/mana_create_txq(), corrupting guest kernel memory.

Stop writing gc->max_num_cqs from the event handler.  Store the reported
value in hwc_init_max_num_cqs, and let mana_hwc_establish_channel() commit
it to gc->max_num_cqs once, from the same snapshot that sizes cq_table.
The handler store uses WRITE_ONCE() and the establish-time read uses
READ_ONCE(), since the two run concurrently (EQ interrupt vs process
context); the single, non-reloadable read is what guarantees the value
that sizes cq_table is the same one published as the bound, even across
the sleeping vcalloc().  gc->max_num_cqs then always matches the
allocation and no later event can change the bound after the table is
published, so the existing bounds checks are sufficient.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v4:
 - New patch in v4, split out of the v3 teardown-safety work in
   response to review: gc->max_num_cqs is set once when cq_table is
   allocated and never reset, so a spoofed post-init HWC event cannot
   inflate the bound past the allocation.

 .../net/ethernet/microsoft/mana/hw_channel.c  | 34 ++++++++++++++++---
 include/net/mana/hw_channel.h                 |  1 +
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 2f0dae353955..03b5e2f02e35 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -246,7 +246,15 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
 			break;
 
 		case HWC_INIT_DATA_MAX_NUM_CQS:
-			gd->gdma_context->max_num_cqs = val;
+			/* Store, don't apply: mana_hwc_establish_channel()
+			 * commits this to gc->max_num_cqs once, together
+			 * with sizing cq_table, so a spoofed post-init event
+			 * cannot inflate the bound past the allocation.
+			 * WRITE_ONCE() pairs with the READ_ONCE() there:
+			 * this store runs in EQ interrupt context,
+			 * concurrently with that process-context read.
+			 */
+			WRITE_ONCE(hwc->hwc_init_max_num_cqs, val);
 			break;
 
 		case HWC_INIT_DATA_PDID:
@@ -852,6 +860,8 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 	struct gdma_queue *eq = hwc->cq->gdma_eq;
 	struct gdma_queue *cq = hwc->cq->gdma_cq;
 	struct gdma_queue __rcu **cq_table;
+	u32 num_cqs;
+	u32 cq_id;
 	int err;
 
 	init_completion(&hwc->hwc_init_eqe_comp);
@@ -881,15 +891,29 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 	*max_req_msg_size = hwc->hwc_init_max_req_msg_size;
 	*max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;
 
-	/* Both were set in mana_hwc_init_event_handler(). */
-	if (WARN_ON(cq->id >= gc->max_num_cqs))
+	/* Snapshot the device-reported CQ count and CQ id into locals and
+	 * use only the locals below, so the same value that sizes cq_table
+	 * also bounds and indexes it -- even across the sleeping vcalloc().
+	 * Both fields are written by mana_hwc_init_event_handler() from EQ
+	 * interrupt context: hwc_init_max_num_cqs under WRITE_ONCE() (paired
+	 * here), and cq->id as an ordinary store.  READ_ONCE() keeps each
+	 * read tear-free and, crucially, non-reloadable, so a spoofed
+	 * post-init event cannot make the WARN_ON() pass against one value
+	 * while the allocation or the index uses another.
+	 */
+	num_cqs = READ_ONCE(hwc->hwc_init_max_num_cqs);
+	cq_id = READ_ONCE(cq->id);
+
+	if (WARN_ON(cq_id >= num_cqs))
 		return -EPROTO;
 
-	cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
+	cq_table = vcalloc(num_cqs, sizeof(*cq_table));
 	if (!cq_table)
 		return -ENOMEM;
 
-	rcu_assign_pointer(cq_table[cq->id], cq);
+	gc->max_num_cqs = num_cqs;
+
+	rcu_assign_pointer(cq_table[cq_id], cq);
 	/* Publish the fully-initialised table last; pairs with the
 	 * rcu_dereference(gc->cq_table) in mana_gd_process_eqe().
 	 */
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 23bf83e2a3ec..c275150baf49 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -203,6 +203,7 @@ struct hw_channel_context {
 	u16 hwc_init_q_depth_max;
 	u32 hwc_init_max_req_msg_size;
 	u32 hwc_init_max_resp_msg_size;
+	u32 hwc_init_max_num_cqs;
 
 	struct completion hwc_init_eqe_comp;
 
-- 
2.43.0