[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

[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?