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

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

[Long Li]

This series fixes a set of reliability and safety bugs in the MANA HW
communication channel (HWC) and hardens the paths that consume
device-supplied values.  The HWC bootstrap and RX data come from the PF,
which is untrusted from a confidential-VM guest, so several of these are
also hardening fixes against a malicious or buggy host.

Patch 1 reference-counts the CQs looked up from the EQ interrupt handler so
a completion cannot race a concurrent CQ destroy.  Patches 2-3 fix an
RQ/SQ size swap and a use-after-free of the HWC comp_buf during teardown.
Patch 4 validates the lengths and indices taken from device DMA in the HWC
RX path.  Patch 5 makes HWC teardown safe when a device stops responding.
Patch 6 stops a stale/late HWC response from completing the wrong command
after a timeout.  Patch 7 keeps max_num_cqs immutable once cq_table is
allocated, so a later device event cannot inflate the bound past the
allocation.

The series has been build- and sparse-tested (C=2), and checkpatch is
clean on every patch.

Changes since v6:
- Patch 1: reworked from the lock-based scheme to lockless reference
  counting per review feedback.  Dropped gc->cq_table_lock entirely;
  lookups now take a reference under RCU (refcount_inc_not_zero) and the
  CQ is freed with kfree_rcu(), publish/unpublish are lockless.  Reordered
  mana_ib_destroy_cq() to detach the software callback before destroying
  the hardware CQ, closing a CQ-id recycle window.  Retitled accordingly
  ("reference-count CQs looked up from the EQ handler").
- Patch 4: bound the RX slot index by msg_buf->num_reqs (the __counted_by
  array bound) rather than the queue depth, and rate-limit the
  device-triggered RX error messages.
- Patch 5: the leak-on-teardown-failure branch now deregisters the HWC EQ
  IRQ and unpublishes the CQ before returning, so no late EQE can reach the
  leaked buffers.  Corrected the setup_active kerneldoc comment.
- Patch 6: dropped the terminal "timed out" latch, which could stop
  teardown commands from ever being posted to a slow device; the core
  stale-response fix (per-slot lock/refcount/responded flag) and the
  wait-queue admission change are retained, and the timeout still shortens
  later waits so teardown is posted.
- Patch 7: reject an out-of-range CQ id with a rate-limited error and
  -EPROTO instead of WARN_ON(), since both operands are device-controlled
  and WARN_ON() could panic a panic_on_warn guest.
- Patches 2, 3: commit-message wording only; no code change.

The v6 posting is at:
https://lore.kernel.org/netdev/20260811023823.2391255-1-longli@microsoft.com/

Long Li (7):
  net: mana: reference-count CQs looked up from the EQ handler
  net: mana: fix HWC RQ/SQ buffer size swap
  net: mana: free HWC comp_buf after destroying the EQ
  net: mana: validate hardware-supplied values in the HWC RX path
  net: mana: fix HWC teardown safety with setup_active flag and destroy
    ordering
  net: mana: fix stale HWC response after command timeout
  net: mana: keep max_num_cqs immutable once cq_table is allocated

 drivers/infiniband/hw/mana/cq.c               |  41 +-
 .../net/ethernet/microsoft/mana/gdma_main.c   | 111 ++++-
 .../net/ethernet/microsoft/mana/hw_channel.c  | 402 ++++++++++++++----
 drivers/net/ethernet/microsoft/mana/mana_en.c |   8 +-
 include/net/mana/gdma.h                       |  29 +-
 include/net/mana/hw_channel.h                 |  34 +-
 6 files changed, 515 insertions(+), 110 deletions(-)


base-commit: f1b3416ceaf7ca4cb5cbd986ee8fe3ffaeda2d48
-- 
2.43.0

[PATCH net v7 1/7] net: mana: reference-count CQs looked up from the EQ handler

[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.  cq_table was a plain array,
read without RCU and freed without a grace period, so a concurrent CQ
teardown races the lookup 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

Reference-count the CQ, like the driver's existing QP get/put.  Mark
cq_table __rcu and look it up under the handler's rcu_read_lock():
mana_gd_get_cq() takes a reference with refcount_inc_not_zero() and
mana_gd_put_cq() drops it after the callback.  Teardown clears the slot,
drops the publish reference, waits for any in-flight handler, then frees
the CQ with kfree_rcu().

On the RDMA destroy path (mana_ib_destroy_cq) clear the dispatch entry
before destroying the HW CQ.  A late completion then finds an empty slot
and is dropped, and a cq_id the device recycles cannot alias the
outgoing entry.

The cq_id bound is hardened in a later patch.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes since v6:
Reworked from the v6 lock-based RCU scheme to lockless reference
counting per review feedback (Leon Romanovsky):
- Dropped gc->cq_table_lock; lookups take a reference under RCU with
  refcount_inc_not_zero() and the CQ is freed via kfree_rcu().
- publish/unpublish are lockless; the EQ-handler lookup uses
  smp_load_acquire() paired with the release-store that publishes the
  table.
- Reordered mana_ib_destroy_cq() to detach the software callback before
  destroying the hardware CQ, closing a CQ-id recycle window.
- Retitled (was "RCU-protect gc->cq_table lookups against concurrent CQ
  destroy").
 drivers/infiniband/hw/mana/cq.c               |  41 ++++---
 .../net/ethernet/microsoft/mana/gdma_main.c   | 100 +++++++++++++++---
 .../net/ethernet/microsoft/mana/hw_channel.c  |  29 +++--
 drivers/net/ethernet/microsoft/mana/mana_en.c |   8 +-
 include/net/mana/gdma.h                       |  23 +++-
 5 files changed, 160 insertions(+), 41 deletions(-)

diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
index f2547989f422901075fa19a1ba48daf3e9a1ec96..022c82479ef6c47d78bc64b638f18aa25bbd3308 100644
--- a/drivers/infiniband/hw/mana/cq.c
+++ b/drivers/infiniband/hw/mana/cq.c
@@ -108,11 +108,12 @@ int mana_ib_destroy_cq(struct ib_cq *ibcq, struct ib_udata *udata)
 
 	mdev = container_of(ibdev, struct mana_ib_dev, ib_dev);
 
+	/* Detach the dispatch entry first, then stop the HW CQ and free the
+	 * queue.  A completion racing teardown then finds an empty slot, and
+	 * a recycled cq_id cannot alias this CQ.  Errors are logged inside.
+	 */
 	mana_ib_remove_cq_cb(mdev, cq);
 
-	/* Ignore return code as there is not much we can do about it.
-	 * The error message is printed inside.
-	 */
 	mana_ib_gd_destroy_cq(mdev, cq);
 
 	mana_ib_destroy_queue(mdev, &cq->queue);
@@ -132,12 +133,8 @@ 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 *gdma_cq;
+	int err;
 
-	if (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])
-		return -EINVAL;
 	if (cq->queue.kmem)
 		gdma_cq = cq->queue.kmem;
 	else
@@ -149,23 +146,41 @@ 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;
-	return 0;
+
+	err = mana_gd_publish_cq(gc, gdma_cq);
+	if (err && !cq->queue.kmem)
+		kfree(gdma_cq);
+
+	return err;
 }
 
 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 || cq->queue.id >= gc->max_num_cqs)
 		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;
+	rcu_read_lock();
+	cq_table = READ_ONCE(gc->cq_table);
+	gdma_cq = cq_table ? rcu_dereference(cq_table[cq->queue.id]) : NULL;
+	/* Match the CQ under RCU so the slot cannot be freed mid-check. */
+	if (gdma_cq && gdma_cq->cq.context != cq)
+		gdma_cq = NULL;
+	rcu_read_unlock();
+
+	if (!gdma_cq)
+		return;
+
+	/* Remove from the table, then free after a grace period. */
+	mana_gd_unpublish_cq(gc, gdma_cq);
+	kfree_rcu(gdma_cq, rcu);
 }
 
 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 e8b7ffb47eb982d139b80bc4fb4bbb0ad5307962..b29e078b419b3c16326ad890c8e97401e1d3f3a9 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -724,6 +724,9 @@ int mana_schedule_serv_work(struct gdma_context *gc, enum gdma_eqe_type type)
 	return 0;
 }
 
+static struct gdma_queue *mana_gd_get_cq(struct gdma_context *gc, u32 cq_id);
+static void mana_gd_put_cq(struct gdma_queue *cq);
+
 static void mana_gd_process_eqe(struct gdma_queue *eq)
 {
 	u32 head = eq->head % (eq->queue_size / GDMA_EQE_SIZE);
@@ -743,16 +746,16 @@ 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))
-			break;
-
-		cq = gc->cq_table[cq_id];
-		if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id))
+		cq = mana_gd_get_cq(gc, cq_id);
+		/* CQ already torn down: stale completion, drop it. */
+		if (!cq)
 			break;
 
-		if (cq->cq.callback)
+		if (!WARN_ON_ONCE(cq->type != GDMA_CQ || cq->id != cq_id) &&
+		    cq->cq.callback)
 			cq->cq.callback(cq->cq.context, cq);
 
+		mana_gd_put_cq(cq);
 		break;
 
 	case GDMA_EQE_TEST_EVENT:
@@ -1050,18 +1053,81 @@ static void mana_gd_create_cq(const struct gdma_queue_spec *spec,
 	queue->cq.callback = spec->cq.callback;
 }
 
-static void mana_gd_destroy_cq(struct gdma_context *gc,
-			       struct gdma_queue *queue)
+static struct gdma_queue *mana_gd_get_cq(struct gdma_context *gc, u32 cq_id)
 {
-	u32 id = queue->id;
+	struct gdma_queue __rcu **cq_table;
+	struct gdma_queue *cq = NULL;
 
-	if (id >= gc->max_num_cqs)
-		return;
+	/* IRQ reader: a stray completion can race the table publish in
+	 * mana_hwc_establish_channel(), so the acquire pairs with its
+	 * smp_store_release() to see a consistent table and bound.
+	 */
+	cq_table = smp_load_acquire(&gc->cq_table);
+	if (cq_table && cq_id < gc->max_num_cqs) {
+		cq = rcu_dereference(cq_table[cq_id]);
+		/* Fails if the CQ is being torn down. */
+		if (cq && !refcount_inc_not_zero(&cq->cq.refcount))
+			cq = NULL;
+	}
+
+	return cq;
+}
 
-	if (!gc->cq_table[id])
+static void mana_gd_put_cq(struct gdma_queue *cq)
+{
+	if (cq && refcount_dec_and_test(&cq->cq.refcount))
+		complete(&cq->cq.free);
+}
+
+int mana_gd_publish_cq(struct gdma_context *gc, struct gdma_queue *queue)
+{
+	struct gdma_queue __rcu **cq_table;
+
+	/* Only mana_gd_get_cq() (IRQ) races the table publish and needs the
+	 * acquire; this control path does not.
+	 */
+	cq_table = READ_ONCE(gc->cq_table);
+	if (!cq_table || queue->id >= gc->max_num_cqs)
+		return -EINVAL;
+
+	/* Sharing a CQ between WQs is not supported. */
+	if (rcu_access_pointer(cq_table[queue->id]))
+		return -EINVAL;
+
+	refcount_set(&queue->cq.refcount, 1);
+	init_completion(&queue->cq.free);
+	rcu_assign_pointer(cq_table[queue->id], queue);
+
+	return 0;
+}
+EXPORT_SYMBOL_NS(mana_gd_publish_cq, "NET_MANA");
+
+void mana_gd_unpublish_cq(struct gdma_context *gc, struct gdma_queue *queue)
+{
+	struct gdma_queue __rcu **cq_table;
+
+	/* Only mana_gd_get_cq() (IRQ) races the table publish and needs the
+	 * acquire; this control path does not.
+	 */
+	cq_table = READ_ONCE(gc->cq_table);
+	if (!cq_table || queue->id >= gc->max_num_cqs ||
+	    rcu_access_pointer(cq_table[queue->id]) != queue)
 		return;
 
-	gc->cq_table[id] = NULL;
+	rcu_assign_pointer(cq_table[queue->id], NULL);
+
+	/* Drop the publish reference and wait for any handler that already
+	 * took one, so the caller can free the CQ.
+	 */
+	mana_gd_put_cq(queue);
+	wait_for_completion(&queue->cq.free);
+}
+EXPORT_SYMBOL_NS(mana_gd_unpublish_cq, "NET_MANA");
+
+static void mana_gd_destroy_cq(struct gdma_context *gc,
+			       struct gdma_queue *queue)
+{
+	mana_gd_unpublish_cq(gc, queue);
 }
 
 int mana_gd_create_hwc_queue(struct gdma_dev *gd,
@@ -1333,7 +1399,13 @@ void mana_gd_destroy_queue(struct gdma_context *gc, struct gdma_queue *queue)
 
 	mana_gd_destroy_dma_region(gc, gmi->dma_region_handle);
 	mana_gd_free_memory(gmi);
-	kfree(queue);
+	/* The EQ handler may still be looking this CQ up; free it after a
+	 * grace period.
+	 */
+	if (queue->type == GDMA_CQ)
+		kfree_rcu(queue, rcu);
+	else
+		kfree(queue);
 }
 EXPORT_SYMBOL_NS(mana_gd_destroy_queue, "NET_MANA");
 
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index e3c24d50dad07c65be9e94129dc09af9264f9f8d..b5ed2dbce6ceb7f7a5196dfe5ba3534eb4c5d330 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,19 @@ 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;
+	/* Publish the initialised table; pairs with smp_load_acquire()
+	 * in mana_gd_get_cq().
+	 */
+	smp_store_release(&gc->cq_table, cq_table);
+
+	/* Publish the HWC CQ now that the table is in place. */
+	err = mana_gd_publish_cq(gc, cq);
+	if (WARN_ON(err))
+		return err;
 
 	return 0;
 }
@@ -811,6 +820,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 +828,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 +840,11 @@ 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() has cleared the CQ table
+	 * slot, so it is not left dangling.
+	 */
+	gc->max_num_cqs = 0;
+
 	kfree(hwc->caller_ctx);
 	hwc->caller_ctx = NULL;
 
@@ -848,8 +861,10 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	gc->hwc.driver_data = NULL;
 	gc->hwc.gdma_context = NULL;
 
-	vfree(gc->cq_table);
+	old_cq_table = gc->cq_table;
 	gc->cq_table = NULL;
+	/* All EQs are gone, so no EQ handler can be using the table. */
+	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 92bb55935c1c4e76e3912794eb3c4483fb331821..515b39f085c0d6519cc90a53b8d879c07e33264e 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -2596,13 +2596,11 @@ 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)) {
+		if (WARN_ON(mana_gd_publish_cq(gc, cq->gdma_cq))) {
 			err = -EINVAL;
 			goto out;
 		}
 
-		gc->cq_table[cq->gdma_id] = cq->gdma_cq;
-
 		mana_create_txq_debugfs(apc, i);
 
 		set_bit(NAPI_STATE_NO_BUSY_POLL, &cq->napi.state);
@@ -2905,13 +2903,11 @@ 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)) {
+	if (WARN_ON(mana_gd_publish_cq(gc, cq->gdma_cq))) {
 		err = -EINVAL;
 		goto out;
 	}
 
-	gc->cq_table[cq->gdma_id] = cq->gdma_cq;
-
 	netif_napi_add_weight_locked(ndev, &cq->napi, mana_poll, 1);
 
 	WARN_ON(xdp_rxq_info_reg(&rxq->xdp_rxq, ndev, rxq_idx,
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 0c395917b2144ec4c2faafa5d6c7de7a452f1ebf..abf243358bf82e2428478cb3cf2f387d9cd9ea28 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -333,6 +333,9 @@ struct gdma_queue {
 	u32 tail;
 	struct list_head entry;
 
+	/* For kfree_rcu(): CQs are looked up locklessly from the EQ handler. */
+	struct rcu_head rcu;
+
 	/* Extra fields specific to EQ/CQ. */
 	union {
 		struct {
@@ -352,6 +355,12 @@ struct gdma_queue {
 			void *context;
 
 			struct gdma_queue *parent; /* For CQ/EQ relationship */
+
+			/* Keep the CQ alive while the EQ handler runs its
+			 * callback; teardown waits on @free.
+			 */
+			refcount_t refcount;
+			struct completion free;
 		} cq;
 	};
 };
@@ -418,7 +427,11 @@ struct gdma_context {
 
 	/* This maps a CQ index to the queue structure. */
 	unsigned int		max_num_cqs;
-	struct gdma_queue	**cq_table;
+	/* Entries are published/cleared by CQ create/destroy and read
+	 * locklessly by the EQ handler under RCU.  max_num_cqs is the table
+	 * size; NULL means the table is torn down.
+	 */
+	struct gdma_queue __rcu	**cq_table;
 
 	/* Protect eq_test_event and test_event_eq_id  */
 	struct mutex		eq_test_event_mutex;
@@ -496,6 +509,14 @@ int mana_gd_create_mana_wq_cq(struct gdma_dev *gd,
 
 void mana_gd_destroy_queue(struct gdma_context *gc, struct gdma_queue *queue);
 
+/* Add a CQ to cq_table so the EQ handler can dispatch to it.  Returns
+ * -EINVAL if the id is out of range or already in use.
+ */
+int mana_gd_publish_cq(struct gdma_context *gc, struct gdma_queue *queue);
+
+/* Remove a CQ from cq_table and wait for the EQ handler to stop using it. */
+void mana_gd_unpublish_cq(struct gdma_context *gc, struct gdma_queue *queue);
+
 int mana_gd_poll_cq(struct gdma_queue *cq, struct gdma_comp *comp, int num_cqe);
 
 void mana_gd_ring_cq(struct gdma_queue *cq, u8 arm_bit);
-- 
2.43.0

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

[Long Li]

mana_hwc_init_queues() sized the RQ (which receives responses) with
max_req_msg_size and the SQ (which sends requests) with max_resp_msg_size
-- backwards -- and mana_hwc_rx_event_handler() strided the RQ by
max_req_msg_size when recovering the RX slot index.

This is latent today: the only caller passes equal sizes (both 0x1000)
and the queues are never re-created with the hardware-reported sizes, so
nothing overflows.  It would only surface if the two sizes diverged.

Size the RQ by max_resp_msg_size and the SQ by max_req_msg_size, and use
max_resp_msg_size as the RX slot stride.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes since v6:
Commit-message wording only; no code change.
 drivers/net/ethernet/microsoft/mana/hw_channel.c | 7 ++++---
 include/net/mana/hw_channel.h                    | 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index b5ed2dbce6ceb7f7a5196dfe5ba3534eb4c5d330..ccef9bf9c6bfde754c28f86103f0b05489091f02 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",
@@ -737,14 +737,14 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
 		goto out;
 	}
 
-	err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_req_msg_size,
+	err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_resp_msg_size,
 				 hwc->cq, &hwc->rxq);
 	if (err) {
 		dev_err(hwc->dev, "Failed to create HWC RQ: %d\n", err);
 		goto out;
 	}
 
-	err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_resp_msg_size,
+	err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_req_msg_size,
 				 hwc->cq, &hwc->txq);
 	if (err) {
 		dev_err(hwc->dev, "Failed to create HWC SQ: %d\n", err);
@@ -753,6 +753,7 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
 
 	hwc->num_inflight_msg = q_depth;
 	hwc->max_req_msg_size = max_req_msg_size;
+	hwc->max_resp_msg_size = max_resp_msg_size;
 
 	return 0;
 out:
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 16feb39616c1bead1a043b3fadc2e18a90651516..73671f479399ac296cf472ec6449e5e6b00a8515 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 v7 3/7] net: mana: free HWC comp_buf after destroying the EQ

[Long Li]

mana_hwc_destroy_cq() freed comp_buf and the CQ before the EQ.  While the
EQ was still registered its handler could reach comp_buf (via
mana_hwc_comp_event()) and the CQ (via mana_hwc_init_event_handler()), so
a late EQE could touch freed memory.

Destroy the EQ first: mana_gd_destroy_queue() deregisters its IRQ and
waits out in-flight handlers, so no EQE can dispatch; only then free the
CQ and comp_buf.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes since v6:
Commit-message and comment wording only; no code change.
 drivers/net/ethernet/microsoft/mana/hw_channel.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index ccef9bf9c6bfde754c28f86103f0b05489091f02..7e01596df11b639b1801bef7bdb09c91dfeb0543 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -384,14 +384,17 @@ static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)
 
 static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
 {
-	kfree(hwc_cq->comp_buf);
+	/* Destroy the EQ first: it deregisters the IRQ and drains in-flight
+	 * handlers, so none can touch the CQ after it is freed.
+	 */
+	if (hwc_cq->gdma_eq)
+		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
 
+	/* Safe to free now that the EQ handler is fenced. */
 	if (hwc_cq->gdma_cq)
 		mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
 
-	if (hwc_cq->gdma_eq)
-		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
-
+	kfree(hwc_cq->comp_buf);
 	kfree(hwc_cq);
 }
 
-- 
2.43.0

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

[Long Li]

mana_hwc_rx_event_handler() used 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 complete a wrong
or reused request or index out of bounds.  Validate before use:

  - read inline_oob_size_div4 once (through its u32 flags word) and reject
    any value but the one the driver programs, so a corrupted OOB size
    cannot move the SGE out of the WQE;
  - read sge->address once and require it to match the address the driver
    posted for that slot, not just an in-range index, so a wrong SGE
    cannot complete a neighbouring slot's request;
  - reject a resp_len larger than the RX buffer;
  - bounds-check hwc_msg_id before indexing the inflight bitmap and
    caller_ctx.

Repost the RX WQE on every early-return that can still identify its slot.
The cases that cannot -- a bad OOB size, an out-of-range index, or an
address matching no slot -- leak one WQE rather than repost the wrong one.
The RQ depth is never replenished, so count the leaks and, once they
exhaust it, log it and shorten the command timeout so callers fail fast.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes since v6:
- Bound the RX slot index by msg_buf->num_reqs (the __counted_by array
  bound) instead of the queue depth.
- Rate-limit the device-triggered RX rejection messages.
- Tightened the commit message.
 .../net/ethernet/microsoft/mana/hw_channel.c  | 85 ++++++++++++++++---
 include/net/mana/hw_channel.h                 |  5 ++
 2 files changed, 80 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 7e01596df11b639b1801bef7bdb09c91dfeb0543..2691d609459122bdbafb144a8b77ac3785c0eddb 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -83,13 +83,29 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
 	struct hwc_caller_ctx *ctx;
 	int err;
 
+	/* The caller already bounds msg_id; re-check at the indexing site. */
+	if (msg_id >= hwc->num_inflight_msg) {
+		dev_err_ratelimited(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);
+		dev_err_ratelimited(hwc->dev, "hwc_rx: invalid msg_id = %u\n", msg_id);
 		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
 		return;
 	}
 
 	ctx = hwc->caller_ctx + msg_id;
+
+	/* An oversized resp_len cannot fit the RX buffer: reject it. */
+	if (resp_len > rx_req->buf_len) {
+		dev_err_ratelimited(hwc->dev, "HWC RX: resp_len %u > buf_len %u\n",
+				    resp_len, rx_req->buf_len);
+		resp_len = 0;
+	}
+
 	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
 	if (err)
 		goto out;
@@ -237,18 +253,37 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
 	}
 }
 
+/* Drop an RX WQE with an untrusted SGE rather than repost it, which could
+ * double-post a buffer the device still owns.  This lowers the RQ depth;
+ * once it is exhausted the channel can no longer receive, so log it and
+ * shorten the timeout to fail callers fast.
+ */
+static void mana_hwc_rx_leak_wqe(struct hw_channel_context *hwc)
+{
+	if (++hwc->rx_leaked_wqe == hwc->rxq->queue_depth) {
+		dev_err(hwc->dev,
+			"HWC RX: RQ exhausted after %u leaked WQEs; channel unusable\n",
+			hwc->rx_leaked_wqe);
+		if (hwc->hwc_timeout > 1)
+			hwc->hwc_timeout = 1;
+	}
+}
+
 static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
 				      const struct hwc_rx_oob *rx_oob)
 {
 	struct hw_channel_context *hwc = ctx;
 	struct hwc_wq *hwc_rxq = hwc->rxq;
 	struct hwc_work_request *rx_req;
+	struct gdma_wqe oob_snapshot;
 	struct gdma_resp_hdr *resp;
 	struct gdma_wqe *dma_oob;
 	struct gdma_queue *rq;
 	struct gdma_sge *sge;
 	u64 rq_base_addr;
 	u64 rx_req_idx;
+	u64 sge_addr;
+	u32 oob_div4;
 	u16 msg_id;
 	u8 *wqe;
 
@@ -259,28 +294,58 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
 	wqe = mana_gd_get_wqe_ptr(rq, rx_oob->wqe_offset / GDMA_WQE_BU_SIZE);
 	dma_oob = (struct gdma_wqe *)wqe;
 
-	sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 * 4);
+	/* inline_oob_size_div4 comes from device memory (host-writable in a
+	 * CVM), so read it once from the shared flags word.  The driver only
+	 * ever programs INLINE_OOB_SMALL_SIZE, so reject any other value.
+	 */
+	oob_snapshot.flags = READ_ONCE(dma_oob->flags);
+	oob_div4 = oob_snapshot.inline_oob_size_div4;
+	if (oob_div4 != INLINE_OOB_SMALL_SIZE / 4) {
+		dev_err_ratelimited(hwc->dev,
+				    "HWC RX: unexpected inline_oob_size_div4=%u\n",
+				    oob_div4);
+		mana_hwc_rx_leak_wqe(hwc);
+		return;
+	}
+	sge = (struct gdma_sge *)(wqe + 8 + oob_div4 * 4);
 
-	/* Select the RX work request for virtual address and for reposting. */
+	/* Recover the RX slot from the SGE address (read once, it is device
+	 * memory).  Require both an in-range index and an exact address
+	 * match, so a wrong SGE cannot complete an unrelated request.
+	 */
+	sge_addr = READ_ONCE(sge->address);
 	rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
-	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size;
+	rx_req_idx = (sge_addr - 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);
+		/* Out-of-range index: corrupted SGE, leak the WQE. */
+		dev_err_ratelimited(hwc->dev,
+				    "HWC RX: SGE idx %llu >= num_reqs %u\n",
+				    rx_req_idx, hwc_rxq->msg_buf->num_reqs);
+		mana_hwc_rx_leak_wqe(hwc);
 		return;
 	}
 
 	rx_req = &hwc_rxq->msg_buf->reqs[rx_req_idx];
+	if (sge_addr != (u64)rx_req->buf_sge_addr) {
+		/* Address does not match the posted slot: leak the WQE. */
+		dev_err_ratelimited(hwc->dev,
+				    "HWC RX: invalid SGE address %llx (idx=%llu)\n",
+				    sge_addr, rx_req_idx);
+		mana_hwc_rx_leak_wqe(hwc);
+		return;
+	}
+
 	resp = (struct gdma_resp_hdr *)rx_req->buf_va;
 
-	/* Read msg_id once from DMA buffer to prevent TOCTOU:
-	 * DMA memory is shared/unencrypted in CVMs - host can
-	 * modify it between reads.
+	/* Read msg_id once: it is host-writable DMA memory.  A short response
+	 * is left for mana_hwc_handle_resp() to reject, so it cannot stall
+	 * the channel.
 	 */
 	msg_id = READ_ONCE(resp->response.hwc_msg_id);
 	if (msg_id >= hwc->num_inflight_msg) {
-		dev_err(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
+		dev_err_ratelimited(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
+		mana_hwc_post_rx_wqe(hwc_rxq, rx_req);
 		return;
 	}
 
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 73671f479399ac296cf472ec6449e5e6b00a8515..58ea72f32135674d41fb1469e4cce4b0d8e87560 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -200,6 +200,11 @@ struct hw_channel_context {
 	u32 pf_dest_vrcq_id;
 	u32 hwc_timeout;
 
+	/* RX WQEs dropped after an untrusted SGE; at RQ depth the channel
+	 * can no longer receive.
+	 */
+	u32 rx_leaked_wqe;
+
 	struct hwc_caller_ctx *caller_ctx;
 };
 
-- 
2.43.0

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

[Long Li]

Three teardown hazards let the hardware touch freed memory.

First, once mana_smc_setup_hwc() succeeds the device can DMA into the HWC
queue buffers.  If a later step in mana_hwc_establish_channel() failed,
the caller had no reliable signal that teardown was needed and could free
those buffers while the mappings were live.  Add a setup_active flag, set
the moment setup_hwc() activates the device; on failure establish_channel()
just returns and the caller's error path does the single teardown, gated
on setup_active.  (max_num_cqs was the old proxy, but it is only set when
the init EQE arrives.)

Second, destroy_channel() freed the TXQ/RXQ while the HWC EQ was still on
the interrupt dispatch list, so an in-flight interrupt could run the
handler against freed buffers.  Destroy the CQ first --
mana_hwc_destroy_cq() deregisters the EQ (list_del_rcu() +
synchronize_rcu()) -- then free the TXQ/RXQ.

Third, if mana_smc_teardown_hwc() itself fails the MST entries stay live,
yet destroy_channel() went on to free buffers the device can still DMA
into.  Leak the HWC resources on teardown failure instead, but still
deregister the EQ IRQ and unpublish the CQ so the leaked buffers are
unreachable from the interrupt handler, and keep setup_active set so it is
not mistaken for a clean teardown.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes since v6:
- The leak-on-teardown-failure branch now fences the interrupt path
  before returning: it deregisters the HWC EQ IRQ (calling the existing
  mana_gd_destroy_eq(), now made non-static) and unpublishes the CQ, so
  no late EQE can reach the leaked buffers.
- Corrected the setup_active kerneldoc comment to match the code.
 .../net/ethernet/microsoft/mana/gdma_main.c   |  4 +-
 .../net/ethernet/microsoft/mana/hw_channel.c  | 63 ++++++++++++++++---
 include/net/mana/gdma.h                       |  6 ++
 include/net/mana/hw_channel.h                 |  6 ++
 4 files changed, 68 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index b29e078b419b3c16326ad890c8e97401e1d3f3a9..02f901b2cb1faf4a34e6e3e53deebeeb056f528f 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -976,8 +976,8 @@ int mana_gd_test_eq(struct gdma_context *gc, struct gdma_queue *eq)
 	return err;
 }
 
-static void mana_gd_destroy_eq(struct gdma_context *gc, bool flush_evenets,
-			       struct gdma_queue *queue)
+void mana_gd_destroy_eq(struct gdma_context *gc, bool flush_evenets,
+			struct gdma_queue *queue)
 {
 	int err;
 
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 2691d609459122bdbafb144a8b77ac3785c0eddb..88188523dcd4863b254451e77cb569c12c150033 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -747,6 +747,13 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 
 	init_completion(&hwc->hwc_init_eqe_comp);
 
+	/* Set before setup_hwc() activates the device's DMA into our buffers,
+	 * so a later failure still tears the HWC down instead of freeing
+	 * buffers the device may write to.  Do not tear down here: that would
+	 * double the timeout and mask the error.
+	 */
+	hwc->setup_active = true;
+
 	err = mana_smc_setup_hwc(&gc->shm_channel, false,
 				 eq->mem_info.dma_handle,
 				 cq->mem_info.dma_handle,
@@ -837,6 +844,16 @@ int mana_hwc_create_channel(struct gdma_context *gc)
 	u16 q_depth_max;
 	int err;
 
+	/* A previous teardown may have failed and left the old context
+	 * reachable.  Retry it before building a new channel; if it still
+	 * fails, return an error so mana_serv_reset() does a full PCI rescan.
+	 */
+	if (gd->driver_data) {
+		mana_hwc_destroy_channel(gc);
+		if (gd->driver_data)
+			return -ETIMEDOUT;
+	}
+
 	hwc = kzalloc_obj(*hwc);
 	if (!hwc)
 		return -ENOMEM;
@@ -894,18 +911,40 @@ 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.
+	/* Only tear down if setup_hwc() activated the device.  If teardown
+	 * fails the device may still DMA into these buffers, so leak them
+	 * rather than free, and keep setup_active set.
 	 */
-	if (gc->max_num_cqs > 0)
-		mana_smc_teardown_hwc(&gc->shm_channel, false);
-
-	if (hwc->txq)
-		mana_hwc_destroy_wq(hwc, hwc->txq);
+	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);
+			/* The device may still DMA into these buffers, so
+			 * leak them.  Still fence the interrupt path: drop
+			 * the EQ from the handler list and unpublish the CQ,
+			 * and NULL them so a later retry does not touch the
+			 * leaked queues again.
+			 */
+			if (hwc->cq && hwc->cq->gdma_eq) {
+				mana_gd_destroy_eq(gc, false, hwc->cq->gdma_eq);
+				hwc->cq->gdma_eq = NULL;
+			}
+			if (hwc->cq && hwc->cq->gdma_cq) {
+				mana_gd_unpublish_cq(gc, hwc->cq->gdma_cq);
+				hwc->cq->gdma_cq = NULL;
+			}
+			return;
+		}
 
-	if (hwc->rxq)
-		mana_hwc_destroy_wq(hwc, hwc->rxq);
+		hwc->setup_active = false;
+	}
 
+	/* Tear down the CQ/EQ first so no interrupt handler can touch the
+	 * RQ/TXQ buffers after this point.
+	 */
 	if (hwc->cq)
 		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
 
@@ -914,6 +953,12 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 	 */
 	gc->max_num_cqs = 0;
 
+	if (hwc->txq)
+		mana_hwc_destroy_wq(hwc, hwc->txq);
+
+	if (hwc->rxq)
+		mana_hwc_destroy_wq(hwc, hwc->rxq);
+
 	kfree(hwc->caller_ctx);
 	hwc->caller_ctx = NULL;
 
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index abf243358bf82e2428478cb3cf2f387d9cd9ea28..1642d5413897c1dea49d1d4c5d80684163234e76 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -509,6 +509,12 @@ int mana_gd_create_mana_wq_cq(struct gdma_dev *gd,
 
 void mana_gd_destroy_queue(struct gdma_context *gc, struct gdma_queue *queue);
 
+/* Flush (optional), deregister the IRQ for, and disable an EQ, without
+ * freeing its queue memory.
+ */
+void mana_gd_destroy_eq(struct gdma_context *gc, bool flush_events,
+			struct gdma_queue *queue);
+
 /* Add a CQ to cq_table so the EQ handler can dispatch to it.  Returns
  * -EINVAL if the id is out of range or already in use.
  */
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 58ea72f32135674d41fb1469e4cce4b0d8e87560..6e77163a06d8f2622430281647268c54939c4dc0 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -205,6 +205,12 @@ struct hw_channel_context {
 	 */
 	u32 rx_leaked_wqe;
 
+	/* True once setup_hwc() may have activated the device's DMA into the
+	 * HWC buffers: set before the setup command, cleared after teardown
+	 * succeeds.
+	 */
+	bool setup_active;
+
 	struct hwc_caller_ctx *caller_ctx;
 };
 
-- 
2.43.0

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

[Long Li]

mana_hwc_send_request() freed the message slot (mana_hwc_put_msg_index)
as soon as it timed out, while the command was still pending and
caller_ctx.output_buf still pointed at the caller's buffer.  A late
response then either memcpy()'d into that buffer from CQ interrupt context
after the sender had returned, or completed the next request that reused
the slot with stale data.

Give each caller_ctx a spinlock, a refcount and an -EINPROGRESS sentinel
(caller_ctx::error becomes int to hold the negative value):

  - The sender publishes and clears output_buf under the slot lock;
    handle_resp() takes the same lock and skips the copy once it is NULL,
    so a late response can no longer write into a sender's buffer after
    it has returned.
  - Both references (sender and handle_resp) are taken up front in
    mana_hwc_get_msg_index(), so an early response cannot drop the slot
    under the sender, and a per-slot "responded" flag drops a duplicate
    response.

Responses are still correlated only by the reusable hwc_msg_id, so a
response that arrives after its slot has been released and reused can
still be delivered to the new owner; closing that fully needs a
per-request identity and is left to a separate change.

Replace the counting semaphore with a waitqueue + bitmap so a slot held
past a timeout cannot deadlock admission.

Ignore a device-reported zero hwc_timeout from both sources that feed it
and keep the positive default.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes since v6:
- Dropped the terminal "timed out" latch: it could stop teardown
  commands from ever being posted to a slow device.  The core
  stale-response fix (per-slot lock, refcount and "responded" flag) and
  the wait-queue admission change are retained, and a timeout still
  shortens later waits so teardown is still posted.
- Softened the changelog; the msg_id reuse correlation gap is pre-existing
  and noted as out of scope.
 .../net/ethernet/microsoft/mana/gdma_main.c   |   7 +-
 .../net/ethernet/microsoft/mana/hw_channel.c  | 186 ++++++++++++++----
 include/net/mana/hw_channel.h                 |  21 +-
 3 files changed, 173 insertions(+), 41 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index 02f901b2cb1faf4a34e6e3e53deebeeb056f528f..418e55aa033c3141f922fc7b38c9a44696dc9f0f 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -310,7 +310,12 @@ static int mana_gd_query_hwc_timeout(struct pci_dev *pdev, u32 *timeout_val)
 	if (err || resp.hdr.status)
 		return err ? err : -EPROTO;
 
-	*timeout_val = resp.timeout_ms;
+	/* A zero timeout would make every HWC command time out immediately
+	 * and latch the channel (see the HWC_DATA_CFG_HWC_TIMEOUT handler).
+	 * Ignore a zero from the device and keep the caller's positive value.
+	 */
+	if (resp.timeout_ms)
+		*timeout_val = resp.timeout_ms;
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 88188523dcd4863b254451e77cb569c12c150033..b1269f7da0563a22c3cbe599df55572e36908139 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -6,25 +6,41 @@
 #include <net/mana/hw_channel.h>
 #include <linux/vmalloc.h>
 
+/* Acquire a free inflight message slot, waiting for one if all are in use. */
 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);
-
-	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(hwc->inflight_msg_res.map, index, 1);
+			ctx = &hwc->caller_ctx[index];
+			reinit_completion(&ctx->comp_event);
+			/* Take both references (sender + handle_resp) before
+			 * publishing the slot, so an early response cannot free
+			 * it under the sender.
+			 */
+			refcount_set(&ctx->refcnt, 2);
+			ctx->responded = false;
+			ctx->msg_id = index;
+			ctx->error = -EINPROGRESS;
+			/* Publish the slot last, after it is fully initialised. */
+			bitmap_set(r->map, index, 1);
+			spin_unlock_irqrestore(&r->lock, flags);
+			break;
+		}
+		spin_unlock_irqrestore(&r->lock, flags);
 
-	spin_unlock_irqrestore(&r->lock, flags);
+		wait_event(hwc->msg_waitq, !bitmap_full(r->map, r->size));
+	}
 
 	*msg_id = index;
-
 	return 0;
 }
 
@@ -34,10 +50,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,
@@ -106,22 +129,34 @@ 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;
+	/* Honour a response only while the sender owns the slot (output_buf
+	 * published) and has not already been answered; otherwise drop it as
+	 * premature, stale or duplicate without touching the refcount.
+	 */
+	if (!ctx->output_buf || ctx->responded) {
+		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:
+	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
+	if (!err) {
+		ctx->status_code = resp_msg->status;
+		memcpy(ctx->output_buf, resp_msg, resp_len);
+	}
 	ctx->error = err;
 
-	/* Must post rx wqe before complete(), otherwise the next rx may
-	 * hit no_wqe error.
+	/* Post RX WQE before completing — the next response may arrive
+	 * immediately and needs a posted buffer.
 	 */
 	mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
-
 	complete(&ctx->comp_event);
+	spin_unlock(&ctx->lock);
+
+	hwc_ctx_put(hwc, ctx);
 }
 
 static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
@@ -208,7 +243,9 @@ 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;
+			/* Ignore a zero timeout; keep the positive default. */
+			if (val)
+				hwc->hwc_timeout = val;
 			break;
 
 		case HWC_DATA_HW_LINK_CONNECT:
@@ -695,7 +732,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)
@@ -725,8 +762,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;
 
@@ -737,6 +776,7 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 				      u32 *max_req_msg_size,
 				      u32 *max_resp_msg_size)
 {
+	/* Runs at init before the channel is used, so no locking is needed. */
 	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;
@@ -989,13 +1029,19 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 	struct hwc_wq *txq = hwc->txq;
 	struct gdma_req_hdr *req_msg;
 	struct hwc_caller_ctx *ctx;
+	unsigned long flags;
+	bool drop_resp_ref;
 	u32 dest_vrcq = 0;
 	u32 dest_vrq = 0;
 	u32 command;
+	u32 status;
+	u32 wait_ms;
 	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];
 
@@ -1007,8 +1053,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)
@@ -1024,43 +1073,104 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 		dest_vrcq = hwc->pf_dest_vrcq_id;
 	}
 
+	/* The response-side reference (from get_msg_index) keeps the slot
+	 * alive if hardware responds right after the doorbell.
+	 */
 	err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
 	if (err) {
 		dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n", err);
 		goto out;
 	}
 
+	wait_ms = hwc->hwc_timeout;
 	if (!wait_for_completion_timeout(&ctx->comp_event,
-					 (msecs_to_jiffies(hwc->hwc_timeout)))) {
-		if (hwc->hwc_timeout != 0)
+					 msecs_to_jiffies(wait_ms))) {
+		if (wait_ms != 0)
 			dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
-				command, hwc->hwc_timeout);
+				command, wait_ms);
+
+		/* Clear output_buf so a late response cannot write the caller's
+		 * buffer, then check whether one already arrived
+		 * (error != -EINPROGRESS).
+		 */
+		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) {
+			/* A valid response raced in just after the timeout;
+			 * the hardware is alive, so use it and keep the channel.
+			 */
+			hwc_ctx_put(hwc, ctx);
+			goto check_status;
+		}
+
+		err = -ETIMEDOUT;
+
+		/* No-wait teardown (hwc_timeout == 0) is expected to expire;
+		 * just release the slot so the next teardown command can reuse
+		 * it.
+		 */
+		if (wait_ms == 0)
+			goto out;
 
-		/* Reduce further waiting if HWC no response */
+		/* Genuine timeout: shorten later waits so subsequent commands
+		 * fail fast instead of each draining the full timeout.
+		 */
 		if (hwc->hwc_timeout > 1)
 			hwc->hwc_timeout = 1;
 
-		err = -ETIMEDOUT;
+		/* Release the slot via out:; a late response no longer touches
+		 * it, so the sender must drop the reference here.
+		 */
 		goto out;
 	}
 
-	if (ctx->error) {
-		err = ctx->error;
-		goto out;
-	}
+	/* Clear output_buf and read the result under the lock; the slot may
+	 * be reused after hwc_ctx_put().
+	 */
+	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);
+	/* Error, no-wait teardown, or timeout: drop the sender's and the
+	 * response-side references.  Latch ->responded so a racing response
+	 * is a no-op, and only drop the response-side ref if it has not.
+	 */
+	ctx = hwc->caller_ctx + msg_id;
+	spin_lock_irqsave(&ctx->lock, flags);
+	ctx->output_buf = NULL;
+	drop_resp_ref = !ctx->responded;
+	ctx->responded = true;
+	spin_unlock_irqrestore(&ctx->lock, flags);
+	if (drop_resp_ref)
+		refcount_dec(&ctx->refcnt);
+	hwc_ctx_put(hwc, ctx);
+done:
 	return err;
 }
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 6e77163a06d8f2622430281647268c54939c4dc0..ceabdc6242573f13d1284a967ea73bc01698e37d 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -171,8 +171,24 @@ struct hwc_caller_ctx {
 	void *output_buf;
 	u32 output_buflen;
 
-	u32 error; /* Linux error code */
+	int error; /* Linux error code (negative errno or 0) */
 	u32 status_code;
+
+	/* Protects output_buf against concurrent access from
+	 * handle_resp() (CQ interrupt) and the sender timeout path.
+	 */
+	spinlock_t lock;
+
+	/* Tracks sender + handle_resp ownership.  The last put
+	 * (refcount reaches 0) releases the bitmap slot.
+	 */
+	refcount_t refcnt;
+	u16 msg_id;
+
+	/* Set by the first handle_resp(), or by the sender's timeout path,
+	 * so a later or duplicate response is dropped.
+	 */
+	bool responded;
 };
 
 struct hw_channel_context {
@@ -193,8 +209,9 @@ struct hw_channel_context {
 	struct hwc_wq *txq;
 	struct hwc_cq *cq;
 
-	struct semaphore sema;
 	struct gdma_resource inflight_msg_res;
+	/* Waitqueue for senders blocked on a full inflight bitmap. */
+	wait_queue_head_t msg_waitq;
 
 	u32 pf_dest_vrq_id;
 	u32 pf_dest_vrcq_id;
-- 
2.43.0

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

[Long Li]

mana_hwc_init_event_handler() applied every HWC_INIT_DATA_MAX_NUM_CQS
event straight to gc->max_num_cqs, but that handler stays live for the
whole channel lifetime, not just bootstrap.

cq_table is allocated once, sized to the bootstrap max_num_cqs, and every
reader bounds-checks a CQ index against gc->max_num_cqs before indexing
it.  A later event with a larger value -- from the device or a malicious
host -- inflates the bound past the allocation, so an out-of-range CQ id
then passes the check and indexes cq_table out of bounds (an OOB read in
the EQ path, or an OOB pointer write in mana_create_rxq()/txq()).

Stop writing gc->max_num_cqs from the handler.  Store the reported value
in hwc_init_max_num_cqs (WRITE_ONCE()) and let
mana_hwc_establish_channel() commit it to gc->max_num_cqs once
(READ_ONCE()), from the same snapshot that sizes cq_table.  The bound then
always matches the allocation and no later event can change it.

Reject an out-of-range CQ id with a rate-limited error and -EPROTO rather
than WARN_ON(): both operands are device-controlled -- a host that omits
MAX_NUM_CQS leaves the bound at 0 -- so WARN_ON() would let a malformed
response panic a panic_on_warn guest.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes since v6:
- Reject an out-of-range CQ id with a rate-limited dev_err() and -EPROTO
  instead of WARN_ON(): both operands are device-controlled, so WARN_ON()
  could panic a panic_on_warn guest.
- Tightened the commit message.
 .../net/ethernet/microsoft/mana/hw_channel.c  | 33 +++++++++++++++----
 include/net/mana/hw_channel.h                 |  1 +
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index b1269f7da0563a22c3cbe599df55572e36908139..d9bff4634dc35be772eaeea2c56bfe2562c9d6aa 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -209,7 +209,11 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
 			break;
 
 		case HWC_INIT_DATA_MAX_NUM_CQS:
-			gd->gdma_context->max_num_cqs = val;
+			/* Store only; establish_channel() commits it to
+			 * max_num_cqs once, so a later event cannot grow the
+			 * bound past the allocation.  Pairs with its READ_ONCE().
+			 */
+			WRITE_ONCE(hwc->hwc_init_max_num_cqs, val);
 			break;
 
 		case HWC_INIT_DATA_PDID:
@@ -783,6 +787,8 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 	struct gdma_queue *eq = hwc->cq->gdma_eq;
 	struct gdma_queue *cq = hwc->cq->gdma_cq;
 	struct gdma_queue __rcu **cq_table;
+	u32 num_cqs;
+	u32 cq_id;
 	int err;
 
 	init_completion(&hwc->hwc_init_eqe_comp);
@@ -810,17 +816,32 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 	*max_req_msg_size = hwc->hwc_init_max_req_msg_size;
 	*max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;
 
-	/* Both were set in mana_hwc_init_event_handler(). */
-	if (WARN_ON(cq->id >= gc->max_num_cqs))
+	/* Snapshot the device-reported count and id once, so the same value
+	 * sizes, bounds and indexes cq_table even across the sleeping
+	 * vcalloc() and a concurrent init event.
+	 */
+	num_cqs = READ_ONCE(hwc->hwc_init_max_num_cqs);
+	cq_id = READ_ONCE(cq->id);
+
+	/* Both operands come from untrusted HWC bootstrap events; a missing
+	 * MAX_NUM_CQS leaves num_cqs at 0.  Reject rather than WARN_ON() so a
+	 * malformed device response cannot panic a panic_on_warn guest.
+	 */
+	if (cq_id >= num_cqs) {
+		dev_err_ratelimited(hwc->dev,
+				    "HWC: bad CQ id %u >= max %u\n",
+				    cq_id, num_cqs);
 		return -EPROTO;
+	}
 
-	cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
+	cq_table = vcalloc(num_cqs, sizeof(*cq_table));
 	if (!cq_table)
 		return -ENOMEM;
 
-	/* Publish the initialised table; pairs with smp_load_acquire()
-	 * in mana_gd_get_cq().
+	/* Publish the bound and the initialised table together; the release
+	 * pairs with smp_load_acquire() in mana_gd_get_cq().
 	 */
+	gc->max_num_cqs = num_cqs;
 	smp_store_release(&gc->cq_table, cq_table);
 
 	/* Publish the HWC CQ now that the table is in place. */
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index ceabdc6242573f13d1284a967ea73bc01698e37d..f6cac0b0e44c5abd72c308ddc9278e4d85ed41ab 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -202,6 +202,7 @@ struct hw_channel_context {
 	u16 hwc_init_q_depth_max;
 	u32 hwc_init_max_req_msg_size;
 	u32 hwc_init_max_resp_msg_size;
+	u32 hwc_init_max_num_cqs;
 
 	struct completion hwc_init_eqe_comp;
 
-- 
2.43.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] A timed-out HWC request recycles its msg_id immediately, which can cause a delayed hardware response to corrupt the memory of a subsequent request that reused the same msg_id.
- [High] The dynamically allocated `mana_serv_work` item is not cancelled during device removal, leading to a race condition where the work item can access a freed `gdma_context` (`gc`).
--

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

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

This patch addresses teardown hazards that could allow the hardware to access
freed memory. It introduces a setup_active flag to ensure teardowns only occur
when necessary, reorders queue destruction to prevent interrupts from accessing
freed buffers, and intentionally leaks resources if the teardown itself fails.

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

[ ... ]

> @@ -837,6 +844,16 @@ int mana_hwc_create_channel(struct gdma_context *gc)
>  	u16 q_depth_max;
>  	int err;
>  
> +	/* A previous teardown may have failed and left the old context
> +	 * reachable.  Retry it before building a new channel; if it still
> +	 * fails, return an error so mana_serv_reset() does a full PCI rescan.
> +	 */

[Severity: High]
This isn't a bug introduced by this patch, but since mana_serv_reset() is
mentioned here, is there a use-after-free risk during device removal?

If an error triggers mana_schedule_serv_work(), it schedules a dynamically
allocated work item on the system workqueue:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_schedule_serv_work() {
    ...
    INIT_WORK(&mns_wk->serv_work, mana_serv_func);
    schedule_work(&mns_wk->serv_work);
    ...
}

If the driver is unbound concurrently, mana_gd_remove() frees the gdma_context
without waiting for or cancelling this work item:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_remove() {
    ...
    vfree(gc);
    ...
}

Could the pending work item then execute mana_serv_reset(), read the context
via pci_get_drvdata(), and dereference the freed gc pointer?

[ ... ]

> @@ -914,6 +953,12 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
>  	 */
>  	gc->max_num_cqs = 0;
>  
> +	if (hwc->txq)
> +		mana_hwc_destroy_wq(hwc, hwc->txq);
> +
> +	if (hwc->rxq)
> +		mana_hwc_destroy_wq(hwc, hwc->rxq);
> +
>  	kfree(hwc->caller_ctx);
>  	hwc->caller_ctx = NULL;

[Severity: High]
This is a pre-existing issue, but while reviewing the caller context cleanup,
is there a memory corruption risk if a hardware channel request times out?

If a request times out in mana_hwc_send_request(), the msg_id is immediately
recycled:

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_send_request() {
    ...
out:
    mana_hwc_put_msg_index(hwc, msg_id);
    return err;
}

If another thread allocates the same msg_id, and the hardware later sends a
delayed response for the original timed-out request, could
mana_hwc_handle_resp() overwrite the new request's memory?

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_handle_resp() {
    ...
    memcpy(ctx->output_buf, resp_msg, resp_len);
    ...
    complete(&ctx->comp_event);
}

Could this delayed response blindly overwrite the newly allocated caller's
output_buf and prematurely wake it up?

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

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

[sashiko-bot]

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

New issues:
- [Medium] Unprotected concurrent modification of `hwc->hwc_timeout` across hardirq and process contexts.

Pre-existing issues:
- [Critical] A race condition exists between `mana_hwc_send_request` timeouts and delayed hardware completions, leading to Use-After-Free and stack corruption of reused message slots.
- [High] Short or malformed hardware responses abort unrelated in-flight requests due to reading stale `msg_id` data from the DMA buffer.
--

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

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

This commit adds validation for lengths and indices received from device DMA
in the HWC RX path. It ensures OOB sizes, SGE addresses, response lengths,
and message IDs are within expected bounds before processing to prevent
out-of-bounds access or processing wrong requests.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 7e01596df11b6..2691d60945912 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -83,13 +83,29 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
>  	struct hwc_caller_ctx *ctx;
>  	int err;
>  
[ ... ]
>  	ctx = hwc->caller_ctx + msg_id;
> +
> +	/* An oversized resp_len cannot fit the RX buffer: reject it. */
> +	if (resp_len > rx_req->buf_len) {
> +		dev_err_ratelimited(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);

[Severity: Critical]
This isn't a bug introduced by this patch, but does a race condition exist
between mana_hwc_send_request timeouts and delayed hardware completions?

If mana_hwc_send_request times out, it releases the msg_id back to the pool
without synchronizing with the asynchronous response handler, abandoning the
request and destroying its stack frame.

If a delayed hardware completion triggers mana_hwc_handle_resp() later, could
this lead to a use-after-free and stack memory corruption when it copies data
into the newly reused ctx->output_buf?

[ ... ]
> @@ -237,18 +253,37 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
>  	}
>  }
>  
> +/* Drop an RX WQE with an untrusted SGE rather than repost it, which could
> + * double-post a buffer the device still owns.  This lowers the RQ depth;
> + * once it is exhausted the channel can no longer receive, so log it and
> + * shorten the timeout to fail callers fast.
> + */
> +static void mana_hwc_rx_leak_wqe(struct hw_channel_context *hwc)
> +{
> +	if (++hwc->rx_leaked_wqe == hwc->rxq->queue_depth) {
> +		dev_err(hwc->dev,
> +			"HWC RX: RQ exhausted after %u leaked WQEs; channel unusable\n",
> +			hwc->rx_leaked_wqe);
> +		if (hwc->hwc_timeout > 1)
> +			hwc->hwc_timeout = 1;

[Severity: Medium]
Is this modification of hwc->hwc_timeout safe from concurrent access?

This function introduces an unprotected modification to the shared timeout
state. Since it modifies hwc->hwc_timeout without locks or WRITE_ONCE() from
hardirq context, could this cause unpredictable timeout evaluations if it
races with process context readers or writers?

> +	}
> +}
> +
[ ... ]
> @@ -259,28 +294,58 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
[ ... ]
> -	/* Read msg_id once from DMA buffer to prevent TOCTOU:
> -	 * DMA memory is shared/unencrypted in CVMs - host can
> -	 * modify it between reads.
> +	/* Read msg_id once: it is host-writable DMA memory.  A short response
> +	 * is left for mana_hwc_handle_resp() to reject, so it cannot stall
> +	 * the channel.
>  	 */
>  	msg_id = READ_ONCE(resp->response.hwc_msg_id);

[Severity: High]
This is a pre-existing issue, but can reading msg_id from DMA memory here
cause unrelated in-flight requests to be aborted?

If a short or malformed hardware response is received, the code reads a
potentially stale msg_id directly from the DMA buffer and delegates rejection
to mana_hwc_handle_resp().

Could a malicious host or buggy hardware force the completion and abortion of
an unrelated in-flight request using this unverified msg_id?

>  	if (msg_id >= hwc->num_inflight_msg) {
> -		dev_err(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
> +		dev_err_ratelimited(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
> +		mana_hwc_post_rx_wqe(hwc_rxq, rx_req);
>  		return;
>  	}

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Incomplete Fix / Use-After-Free of WQ structures due to incorrect channel teardown order.
- [High] Stack Memory Corruption and Logic Error due to missing reinitialization after request timeout.
- [High] Hardware DMA Use-After-Free when channel establishment times out.
--

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

net: mana: free HWC comp_buf after destroying the EQ

mana_hwc_destroy_cq() freed comp_buf and the CQ before the EQ. While the
EQ was still registered its handler could reach comp_buf and the CQ, so
a late EQE could touch freed memory. This commit fixes the teardown order.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index ccef9bf9c6bfd..7e01596df11b6 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -384,14 +384,17 @@ static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)
>  
>  static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
>  {
> -	kfree(hwc_cq->comp_buf);
> +	/* Destroy the EQ first: it deregisters the IRQ and drains in-flight
> +	 * handlers, so none can touch the CQ after it is freed.
> +	 */
> +	if (hwc_cq->gdma_eq)
> +		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
>  
> +	/* Safe to free now that the EQ handler is fenced. */
>  	if (hwc_cq->gdma_cq)
>  		mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
>  
> -	if (hwc_cq->gdma_eq)
> -		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
> -
> +	kfree(hwc_cq->comp_buf);
>  	kfree(hwc_cq);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the teardown order in
mana_hwc_destroy_channel() leave a use-after-free for the WQ structures?

In mana_hwc_destroy_channel(), the txq and rxq wrappers are destroyed
before mana_hwc_destroy_cq() fences the EQ:

mana_hwc_destroy_channel() {
    ...
    if (hwc->txq)
        mana_hwc_destroy_wq(hwc, hwc->txq);
        
    if (hwc->rxq)
        mana_hwc_destroy_wq(hwc, hwc->rxq);
        
    if (hwc->cq)
        mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
    ...
}

Since mana_hwc_destroy_wq() frees the wq structure, can an unfenced EQ 
handler dereference the freed hwc->rxq pointer in 
mana_hwc_rx_event_handler() while the EQ is still active?

[Severity: High]
This is a pre-existing issue, but does mana_hwc_send_request() have a stack
corruption bug if a request times out?

If wait_for_completion_timeout() times out, the function jumps to out and 
releases the msg_id:

mana_hwc_send_request() {
    ...
    if (!wait_for_completion_timeout(&ctx->comp_event, ...)) {
        err = -ETIMEDOUT;
        goto out;
    }
    ...
}

A new request could then allocate the same msg_id without reinitializing
ctx->comp_event. If a late response arrives for the original request, 
mana_hwc_handle_resp() will copy the stale payload into the new caller's 
stack buffer and complete the event:

mana_hwc_handle_resp() {
    ...
    memcpy(ctx->output_buf, resp_msg, resp_len);
out:
    ctx->error = err;
    ...
    complete(&ctx->comp_event);
}

Could this cause the new request to receive stale data and continue 
prematurely?

[Severity: High]
This is a pre-existing issue, but does a timeout during channel 
establishment result in a DMA use-after-free?

In mana_hwc_establish_channel(), if the hardware takes too long to reply:

mana_hwc_establish_channel() {
    ...
    if (!wait_for_completion_timeout(&hwc->hwc_init_eqe_comp, 60 * HZ))
        return -ETIMEDOUT;
    ...
}

The error path calls mana_hwc_destroy_channel(), which checks max_num_cqs
before tearing down the hardware channel:

mana_hwc_destroy_channel() {
    ...
    if (gc->max_num_cqs > 0)
        mana_smc_teardown_hwc(&gc->shm_channel, false);
    ...
}

Since max_num_cqs remains 0 during an init timeout, mana_smc_teardown_hwc() 
is skipped. Are the DMA buffers then freed while the hardware channel is 
still active and capable of writing to them?

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

Re: [PATCH net v7 1/7] net: mana: reference-count CQs looked up from the EQ handler

[sashiko-bot]

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

Pre-existing issues:
- [High] Incomplete fix for CQ ID recycle window on Ethernet and RDMA kernel CQs
--

--- Patch [1]: [PATCH net v7 1/7] net: mana: reference-count CQs looked up from the EQ handler ---
commit 26834a69bfec0a9b46df2c9aa1aa0f248272df13
Author: Long Li <longli@microsoft.com>

net: mana: reference-count CQs looked up from the EQ handler

This patch introduces reference counting for CQs looked up from the EQ
handler in the MANA driver to prevent a use-after-free. It also reorders
the RDMA teardown path to clear the dispatch entry before destroying the
hardware CQ, aiming to close a window where a recycled cq_id could alias
the outgoing entry.

> diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
> index f2547989f4229..022c82479ef6c 100644
> --- a/drivers/infiniband/hw/mana/cq.c
> +++ b/drivers/infiniband/hw/mana/cq.c
> @@ -108,11 +108,12 @@ int mana_ib_destroy_cq(struct ib_cq *ibcq, struct ib_udata *udata)
>  
>  	mdev = container_of(ibdev, struct mana_ib_dev, ib_dev);
>  
> +	/* Detach the dispatch entry first, then stop the HW CQ and free the
> +	 * queue.  A completion racing teardown then finds an empty slot, and
> +	 * a recycled cq_id cannot alias this CQ.  Errors are logged inside.
> +	 */
>  	mana_ib_remove_cq_cb(mdev, cq);
>  
> -	/* Ignore return code as there is not much we can do about it.
> -	 * The error message is printed inside.
> -	 */
>  	mana_ib_gd_destroy_cq(mdev, cq);
>  
>  	mana_ib_destroy_queue(mdev, &cq->queue);

[ ... ]

> @@ -149,23 +146,41 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
>  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 || cq->queue.id >= gc->max_num_cqs)
>  		return;
>  
>  	if (cq->queue.kmem)
>  	/* Then it will be cleaned and removed by the mana */
>  		return;

[Severity: High]
This is a pre-existing issue, but does this early return leave the CQ ID
recycle window open for RDMA kernel CQs?

For kernel CQs, mana_ib_remove_cq_cb() returns early, bypassing the software
CQ unpublish step that was explicitly added to close the recycle window.
Because it skips this, the hardware CQ is destroyed via
mana_ib_gd_destroy_cq() before the queue is torn down.

If the hardware immediately frees the cq_id, a concurrent thread could
create a new CQ, be assigned the same cq_id, and fail with -EINVAL in
mana_gd_publish_cq() because the old CQ is still in gc->cq_table.

Also, does this same race exist in the Ethernet paths? Looking at
mana_destroy_rxq() in drivers/net/ethernet/microsoft/mana/mana_en.c,
the hardware object is destroyed before mana_deinit_cq() removes the
old software CQ from the dispatch table:

mana_destroy_rxq() {
	...
	if (rxq->rxobj != INVALID_MANA_HANDLE)
		mana_destroy_wq_obj(apc, GDMA_RQ, rxq->rxobj);

	mana_deinit_cq(apc, &rxq->rx_cq);
	...
}

A similar inverted destruction sequence seems to exist in mana_destroy_txq().
Could this lead to spurious queue creation failures and permanently detached
network interfaces after operations like MTU changes?

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in `mana_gd_publish_cq()` and `mana_gd_unpublish_cq()` allows a malicious device to cause out-of-bounds array reads and writes, bypassing the patch's protection.
- [High] The untouched `WARN_ON(err)` in `mana_hwc_establish_channel()` still allows a maliciously crafted concurrent device response to trigger a kernel panic, defeating the purpose of the patch.
- [High] Multiple other `WARN_ON_ONCE` statements evaluating device-controlled inputs remain in the code, leaving the guest vulnerable to a device-induced panic.
--

--- Patch [7]: [PATCH net v7 7/7] net: mana: keep max_num_cqs immutable once cq_table is allocated ---
commit e063157399f0ceb210a06b4543697e98b1cac7e9
Author: Long Li <longli@microsoft.com>

net: mana: keep max_num_cqs immutable once cq_table is allocated

This commit addresses an issue where an untrusted device event can update the
max_num_cqs bound after cq_table allocation, leading to out-of-bounds
accesses. It fixes this by snapshotting the bound in hwc_init_max_num_cqs and
reading it once in mana_hwc_establish_channel(). It also replaces a WARN_ON
with an error return so that a malicious device cannot panic a panic_on_warn
guest.

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

[ ... ]

> @@ -209,7 +209,11 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
>  			break;
>  
>  		case HWC_INIT_DATA_MAX_NUM_CQS:
> -			gd->gdma_context->max_num_cqs = val;
> +			/* Store only; establish_channel() commits it to
> +			 * max_num_cqs once, so a later event cannot grow the
> +			 * bound past the allocation.  Pairs with its READ_ONCE().
> +			 */
> +			WRITE_ONCE(hwc->hwc_init_max_num_cqs, val);
>  			break;
>  

[Severity: High]
This is a pre-existing issue, but does this vulnerability still exist for the
CQ ID?

In mana_hwc_init_event_handler(), the untrusted device event can concurrently
overwrite the queue ID without synchronization:

		case HWC_INIT_DATA_CQID:
			hwc->cq->gdma_cq->id = val;
			break;

Meanwhile, mana_gd_publish_cq() reads queue->id multiple times, first to
validate the bound and then to index the array:

	if (!cq_table || queue->id >= gc->max_num_cqs)
		return -EINVAL;

	/* Sharing a CQ between WQs is not supported. */
	if (rcu_access_pointer(cq_table[queue->id]))
		return -EINVAL;
	...
	rcu_assign_pointer(cq_table[queue->id], queue);

If the device sends a malformed CQID event that arrives after the bounds
check but before the array indexing, couldn't this force an out-of-bounds
array write of the queue pointer? The same Time-of-Check to Time-of-Use race
condition appears to exist in mana_gd_unpublish_cq().

[ ... ]

> @@ -810,17 +816,32 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
>  	*max_req_msg_size = hwc->hwc_init_max_req_msg_size;
>  	*max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;
>  
> -	/* Both were set in mana_hwc_init_event_handler(). */
> -	if (WARN_ON(cq->id >= gc->max_num_cqs))
> +	/* Snapshot the device-reported count and id once, so the same value
> +	 * sizes, bounds and indexes cq_table even across the sleeping
> +	 * vcalloc() and a concurrent init event.
> +	 */
> +	num_cqs = READ_ONCE(hwc->hwc_init_max_num_cqs);
> +	cq_id = READ_ONCE(cq->id);
> +
> +	/* Both operands come from untrusted HWC bootstrap events; a missing
> +	 * MAX_NUM_CQS leaves num_cqs at 0.  Reject rather than WARN_ON() so a
> +	 * malformed device response cannot panic a panic_on_warn guest.
> +	 */
> +	if (cq_id >= num_cqs) {
> +		dev_err_ratelimited(hwc->dev,
> +				    "HWC: bad CQ id %u >= max %u\n",
> +				    cq_id, num_cqs);
>  		return -EPROTO;
> +	}
>  
> -	cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
> +	cq_table = vcalloc(num_cqs, sizeof(*cq_table));
>  	if (!cq_table)
>  		return -ENOMEM;
>  
> -	/* Publish the initialised table; pairs with smp_load_acquire()
> -	 * in mana_gd_get_cq().
> +	/* Publish the bound and the initialised table together; the release
> +	 * pairs with smp_load_acquire() in mana_gd_get_cq().
>  	 */
> +	gc->max_num_cqs = num_cqs;
>  	smp_store_release(&gc->cq_table, cq_table);
>  
>  	/* Publish the HWC CQ now that the table is in place. */

[Severity: High]
This is a pre-existing issue, but does a maliciously crafted concurrent device
response still have the ability to trigger a kernel panic here?

Just below this context in mana_hwc_establish_channel(), there is an
untouched WARN_ON():

	err = mana_gd_publish_cq(gc, cq);
	if (WARN_ON(err))
		return err;

If a HWC_INIT_DATA_CQID event is processed after the READ_ONCE(cq->id)
snapshot earlier in this function, mana_gd_publish_cq() will correctly reject
the modified ID and return -EINVAL. However, won't that returned error
trigger this WARN_ON(), crashing a panic_on_warn guest anyway?

[Severity: High]
This is a pre-existing issue, but are there other WARN_ON_ONCE() statements
evaluating device-controlled inputs that could lead to a device-induced panic?

For example, in mana_hwc_rx_event_handler():

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

In mana_hwc_tx_event_handler():

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

And in mana_gd_read_cqe():

	new_bits = (cq->head / num_cqe) & GDMA_CQE_OWNER_MASK;
	/* Return -1 if overflow detected. */
	if (WARN_ON_ONCE(owner_bits != new_bits))
		return -1;

Since these validate untrusted values from the DMA completion ring, could a
malformed response trigger these warnings and panic a guest?

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

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] A timed-out HWC request prematurely releases its msg_id slot without hardware cancellation, allowing a delayed hardware response to corrupt the memory of a subsequent request.
- [High] mana_hwc_send_request() does not reinitialize the completion event before waiting, making it vulnerable to duplicate hardware responses.
- [High] The asynchronous work item `mana_serv_func` is scheduled on the global system workqueue but never canceled during device removal, leading to a race condition and UAF on `gc`.
- [High] Untrusted inline_oob_size_div4 from DMA memory causes an out-of-bounds read when parsing the WQE.
- [High] The HWC RX event handler drops RX WQEs and permanently drains the receive queue if the device returns a bogus msg_id or rx_req_idx.
--

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

This patch corrects the buffer sizes used for the hardware channel (HWC)
queues. It sizes the receive queue (RQ) with max_resp_msg_size and the send
queue (SQ) with max_req_msg_size, fixing a latent bug where these sizes
were incorrectly swapped during queue initialization.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index b5ed2dbce6ceb..ccef9bf9c6bfd 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",

[Severity: High]
This isn't a bug introduced by this patch, but does this code properly
bound-check inline_oob_size_div4?

In mana_hwc_rx_event_handler():

	wqe = mana_gd_get_wqe_ptr(rq, rx_oob->wqe_offset / GDMA_WQE_BU_SIZE);
	dma_oob = (struct gdma_wqe *)wqe;

	sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 * 4);

If the host returns a maliciously large inline_oob_size_div4 in the DMA buffer,
could this cause an out-of-bounds memory read when dereferencing sge->address
on the line modified above?

[Severity: High]
This is also a pre-existing issue, but does this error path permanently
leak the RX WQE?

By returning early on an invalid rx_req_idx (or msg_id further down), this
skips reposting the RX WQE back to the hardware via mana_hwc_post_rx_wqe().

Could a malicious host repeatedly send invalid responses to completely drain
the receive queue and halt control message processing?

> @@ -737,14 +737,14 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
>  		goto out;
>  	}
>  
> -	err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_req_msg_size,
> +	err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_resp_msg_size,
>  				 hwc->cq, &hwc->rxq);
>  	if (err) {
>  		dev_err(hwc->dev, "Failed to create HWC RQ: %d\n", err);
>  		goto out;
>  	}
>  
> -	err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_resp_msg_size,
> +	err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_req_msg_size,
>  				 hwc->cq, &hwc->txq);
>  	if (err) {
>  		dev_err(hwc->dev, "Failed to create HWC SQ: %d\n", err);
> @@ -753,6 +753,7 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
>  
>  	hwc->num_inflight_msg = q_depth;
>  	hwc->max_req_msg_size = max_req_msg_size;
> +	hwc->max_resp_msg_size = max_resp_msg_size;
>  
>  	return 0;
>  out:

[ ... ]

[Severity: Critical]
This isn't a bug introduced by this patch, but does the timeout handling in
mana_hwc_send_request() allow a delayed hardware response to corrupt memory?

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

The timeout path jumps to out: and releases the msg_id back to the bitmap
without waiting for or canceling the hardware operation.

If a new request reuses this msg_id, and the hardware subsequently delivers
a delayed response for the original request, could mana_hwc_handle_resp()
blindly memcpy() the delayed response into the new request's output_buf?

[Severity: High]
This is a pre-existing issue, but is ctx->comp_event properly reinitialized
when a msg_id is reused?

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_send_request() {
	ctx = hwc->caller_ctx + msg_id;
	ctx->output_buf = resp;
	ctx->output_buflen = resp_len;
	...
	if (!wait_for_completion_timeout(&ctx->comp_event, ...)) {
...
}

Because reinit_completion() is never called before wait_for_completion_timeout(),
a previous delayed response could cause mana_hwc_handle_resp() to call complete().

If that happens, could wait_for_completion_timeout() return immediately for
a new request?

This would cause the caller to process uninitialized data from ctx->output_buf.

[Severity: High]
This is a pre-existing issue, but can the global serv_work cause a
use-after-free during device removal?

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_schedule_serv_work() {
	INIT_WORK(&mns_wk->serv_work, mana_serv_func);
	schedule_work(&mns_wk->serv_work);
}

The work item is scheduled on the global system workqueue. During device
removal, mana_gd_remove() frees the gdma context (gc):

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_remove() {
	...
	vfree(gc);
}

Since the driver does not wait for or cancel this asynchronous work item,
could mana_serv_func() execute after mana_gd_remove() and dereference the
freed gc pointer in mana_serv_reset()?

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a remaining protocol correlation gap. Responses from the hardware are matched to requests solely by the reusable `hwc_msg_id` slot index. If a command times out (or is a zero-wait teardown), its slot is released and can be immediately reused by a new request. If the hardware later responds to the original timed-out command, the response will carry the same `msg_id`. Because the slot now belongs to the new request (and `ctx->output_buf` is published), `mana_hwc_handle_resp()` will incorrectly accept the stale response and copy its payload into the new request's buffer, leading to silent data corruption.
--

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

This commit introduces a per-slot spinlock, refcount, and a responded flag
to prevent a late response from writing into a sender's buffer after it has
returned due to a timeout. It also replaces the counting semaphore with a
waitqueue for admission control.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 88188523dcd48..b1269f7da0563 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -106,22 +129,34 @@ 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;
> +	/* Honour a response only while the sender owns the slot (output_buf
> +	 * published) and has not already been answered; otherwise drop it as
> +	 * premature, stale or duplicate without touching the refcount.
> +	 */
> +	if (!ctx->output_buf || ctx->responded) {
> +		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:
> +	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
> +	if (!err) {
> +		ctx->status_code = resp_msg->status;
> +		memcpy(ctx->output_buf, resp_msg, resp_len);
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but is there a remaining protocol
correlation gap here?

Responses from the hardware appear to be matched to requests solely by the
reusable hwc_msg_id slot index, with no generation counter or unique
transaction ID.

If a command times out (or is a zero-wait teardown), its slot is released
and can be immediately reused by a new request. If the hardware later
responds to the original timed-out command, won't the response carry the
same msg_id?

Because the slot now belongs to the new request (and ctx->output_buf is
published), could mana_hwc_handle_resp() incorrectly accept this stale
response and copy its payload into the new request's buffer, leading to
silent data corruption?

>  	ctx->error = err;
>  
> -	/* Must post rx wqe before complete(), otherwise the next rx may
> -	 * hit no_wqe error.
> +	/* Post RX WQE before completing — the next response may arrive
> +	 * immediately and needs a posted buffer.
>  	 */
>  	mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> -
>  	complete(&ctx->comp_event);
> +	spin_unlock(&ctx->lock);
> +
> +	hwc_ctx_put(hwc, ctx);
>  }

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