* [PATCH net v6 1/7] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy
2026-08-11 2:38 [PATCH net v6 0/7] net: mana: HW channel reliability and hardening fixes Long Li
@ 2026-08-11 2:38 ` Long Li
2026-08-11 8:18 ` Leon Romanovsky
2026-08-12 23:46 ` Jakub Kicinski
2026-08-11 2:38 ` [PATCH net v6 2/7] net: mana: fix HWC RQ/SQ buffer size swap Long Li
` (5 subsequent siblings)
6 siblings, 2 replies; 21+ messages in thread
From: Long Li @ 2026-08-11 2:38 UTC (permalink / raw)
To: Long Li, Konstantin Taranov, Jakub Kicinski, David S . Miller,
Paolo Abeni, Eric Dumazet, Andrew Lunn, Jason Gunthorpe,
Leon Romanovsky, Haiyang Zhang, K . Y . Srinivasan, Wei Liu,
Dexuan Cui, shradhagupta, Simon Horman, ernis, stephen
Cc: netdev, linux-rdma, linux-hyperv, linux-kernel
The EQ interrupt handler (mana_gd_process_eqe) looks up the completing CQ
in gc->cq_table[cq_id] and runs its callback, concurrently with CQ
teardown on another CPU that clears the slot and frees the CQ. cq_table
was a plain pointer array freed with no grace period, so the two race
into a use-after-free:
CPU A (mana_gd_intr, hard IRQ) CPU B (CQ destroy)
---------------------------------- ------------------------------
cq = gc->cq_table[cq_id]; // valid
gc->cq_table[id] = NULL;
kfree(cq); // freed
cq->cq.callback(ctx, cq); // use-after-free
The handler's existing rcu_read_lock() only guards the per-IRQ EQ list
traversal; cq_table was never under any RCU contract, and a read-side
lock is inert unless the freer also defers the free past a grace period.
Put cq_table under RCU: annotate the base pointer and entries __rcu, read
with rcu_dereference() in the handler, publish with rcu_assign_pointer(),
and on teardown clear the slot then synchronize_rcu() before freeing the
CQ. The grace period blocks until every in-flight handler has dropped
the old pointer, so the kfree() can no longer race the callback.
This fixes only the CQ lifetime (the use-after-free); it does not make
the cq_id bound trustworthy. gc->max_num_cqs is still range-checked
outside the published table, and hardening that field against a spoofed
device value is a separate change.
netdev teardown destroys a CQ per TX and per RX queue, so one grace
period each in mana_gd_destroy_cq() would serialize up to
2 * MANA_MAX_NUM_QUEUES synchronize_rcu() calls under RTNL on every
ifdown, MTU change or ring/channel reconfigure. Clear all of a port's
CQ slots first and take a single grace period per teardown instead:
mana_gd_unpublish_cq() clears a slot without waiting, and
mana_gd_destroy_cq() -- which still serves the single-CQ callers --
finds the slot already cleared and skips its own synchronize_rcu().
Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v6:
- mana_gd_unpublish_cq() and mana_ib_remove_cq_cb() clear a cq_table
slot only when it still points at the CQ being torn down, so the
two-pass teardown cannot wipe an entry a concurrent RDMA CQ create
recycled during the grace period.
- mana_gd_process_eqe() drops an already-unpublished (NULL) slot quietly
instead of a WARN_ON_ONCE() splat during a normal ifdown/MTU change,
and reads gc->cq_table before gc->max_num_cqs with an smp_rmb()
between them so a shrinking re-establish cannot pair a stale bound
with a newly published, smaller table.
- Documented the cq_table/max_num_cqs contract on the cq_table field
instead of rewording the comment above max_num_cqs.
Changes in v5:
- No code changes since v4 (resend as a standalone thread).
Changes in v4:
- Replaced the per-CQ synchronize_rcu() in the netdev teardown paths
with a two-pass quiesce/free that takes one grace period per
teardown; mana_gd_unpublish_cq() splits the slot-clear from the grace
period.
- Snapshot cq->id and max_num_cqs with READ_ONCE() in
mana_hwc_establish_channel() so one value sizes, bounds and indexes
cq_table.
- Corrected the gc->cq_table lifetime comment in gdma.h; rescoped the
changelog to the use-after-free fix (the bound is patch 7).
drivers/infiniband/hw/mana/cq.c | 51 ++++++-
.../net/ethernet/microsoft/mana/gdma_main.c | 65 +++++++--
.../net/ethernet/microsoft/mana/hw_channel.c | 29 ++--
drivers/net/ethernet/microsoft/mana/mana_en.c | 136 ++++++++++++++----
include/net/mana/gdma.h | 37 ++++-
5 files changed, 268 insertions(+), 50 deletions(-)
diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
index f2547989f422901075fa19a1ba48daf3e9a1ec96..73d97b2f5cf9bba2c4da8c1b14dd1a0d76397be0 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,54 @@ 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);
+ /* Clear the slot only if it still holds the entry this CQ installed
+ * (gdma_cq->cq.context == cq). If the id was already removed, or was
+ * recycled and republished for another CQ, leave the current entry
+ * intact instead of wiping a live one.
+ */
+ if (!gdma_cq || gdma_cq->cq.context != cq)
+ return;
+
+ 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 e8b7ffb47eb982d139b80bc4fb4bbb0ad5307962..d40f25a1a74a739315716a4066987f1137de88d9 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,11 +744,30 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
switch (type) {
case GDMA_EQE_COMPLETION:
cq_id = eqe->details[0] & 0xFFFFFF;
+ cq_table = rcu_dereference(gc->cq_table);
+ if (WARN_ON_ONCE(!cq_table))
+ break;
+
+ /* Pair with the rcu_assign_pointer(gc->cq_table) release in
+ * mana_hwc_establish_channel(), which publishes the table
+ * after storing gc->max_num_cqs. The rmb keeps this bound
+ * read ordered after the table load, so a shrinking
+ * re-establish cannot pair a stale, larger max_num_cqs with a
+ * newly published, smaller table and index out of bounds.
+ */
+ smp_rmb();
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 = rcu_dereference(cq_table[cq_id]);
+ /* A NULL entry is expected while a concurrent teardown
+ * (e.g. ifdown or an MTU change) has unpublished this CQ but
+ * not yet freed it; the completion is stale, so drop it
+ * quietly rather than warning.
+ */
+ if (!cq)
+ break;
+ if (WARN_ON_ONCE(cq->type != GDMA_CQ || cq->id != cq_id))
break;
if (cq->cq.callback)
@@ -1050,18 +1070,47 @@ static void mana_gd_create_cq(const struct gdma_queue_spec *spec,
queue->cq.callback = spec->cq.callback;
}
-static void mana_gd_destroy_cq(struct gdma_context *gc,
- struct gdma_queue *queue)
+bool mana_gd_unpublish_cq(struct gdma_context *gc, struct gdma_queue *queue)
{
+ struct gdma_queue __rcu **cq_table;
u32 id = queue->id;
- if (id >= gc->max_num_cqs)
- return;
+ /* No rcu_read_lock() here: unpublish runs only on the
+ * CQ-destroy/teardown path, where the base cq_table is stable. See
+ * the lifecycle note on gdma_context::cq_table in gdma.h for why the
+ * "true" predicate is sound.
+ */
+ cq_table = rcu_dereference_protected(gc->cq_table, true);
+ if (!cq_table || id >= gc->max_num_cqs)
+ return false;
+
+ /* Clear the slot only if it still refers to this queue. The
+ * Ethernet two-pass teardown unpublishes the same index twice, a
+ * grace period apart, and a CQ that legitimately recycled this id in
+ * between (e.g. a new RDMA CQ via mana_ib_install_cq_cb()) must not
+ * have its fresh entry wiped by the second pass.
+ */
+ if (rcu_access_pointer(cq_table[id]) != queue)
+ return false;
+
+ rcu_assign_pointer(cq_table[id], NULL);
+ return true;
+}
- if (!gc->cq_table[id])
+static void mana_gd_destroy_cq(struct gdma_context *gc,
+ struct gdma_queue *queue)
+{
+ /* A batched teardown may already have cleared the slot and taken the
+ * grace period; then there is nothing left to wait for.
+ */
+ if (!mana_gd_unpublish_cq(gc, queue))
return;
- gc->cq_table[id] = NULL;
+ /* Wait for in-flight EQ handlers that may have loaded the old
+ * pointer via rcu_dereference() to finish before the caller
+ * frees the CQ memory.
+ */
+ synchronize_rcu();
}
int mana_gd_create_hwc_queue(struct gdma_dev *gd,
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index e3c24d50dad07c65be9e94129dc09af9264f9f8d..409e20caeccdcccec0f8972c95db69ebff7ce30c 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 92bb55935c1c4e76e3912794eb3c4483fb331821..5d215981bba83788697d33fa4be047edf8506e42 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -2427,12 +2427,18 @@ static void mana_deinit_txq(struct mana_port_context *apc, struct mana_txq *txq)
static void mana_destroy_txq(struct mana_port_context *apc)
{
+ struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
struct napi_struct *napi;
int i;
if (!apc->tx_qp)
return;
+ /* Pass 1: quiesce each CQ on the device and clear its cq_table slot.
+ * Taking one grace period below for the whole port avoids up to
+ * apc->num_queues serialized synchronize_rcu() calls (one per CQ in
+ * mana_gd_destroy_cq()) under RTNL on every teardown.
+ */
for (i = 0; i < apc->num_queues; i++) {
if (!apc->tx_qp[i])
continue;
@@ -2448,8 +2454,24 @@ static void mana_destroy_txq(struct mana_port_context *apc)
apc->tx_qp[i]->txq.napi_initialized = false;
}
- if (apc->tx_qp[i]->tx_object != INVALID_MANA_HANDLE)
- mana_destroy_wq_obj(apc, GDMA_SQ, apc->tx_qp[i]->tx_object);
+ if (apc->tx_qp[i]->tx_object != INVALID_MANA_HANDLE) {
+ mana_destroy_wq_obj(apc, GDMA_SQ,
+ apc->tx_qp[i]->tx_object);
+ apc->tx_qp[i]->tx_object = INVALID_MANA_HANDLE;
+ }
+
+ if (apc->tx_qp[i]->tx_cq.gdma_cq)
+ mana_gd_unpublish_cq(gc, apc->tx_qp[i]->tx_cq.gdma_cq);
+ }
+
+ synchronize_rcu();
+
+ /* Pass 2: the slots are clear, so mana_gd_destroy_cq() skips its own
+ * grace period; free the CQ, the TXQ and the queue pair.
+ */
+ for (i = 0; i < apc->num_queues; i++) {
+ if (!apc->tx_qp[i])
+ continue;
mana_deinit_cq(apc, &apc->tx_qp[i]->tx_cq);
@@ -2496,6 +2518,7 @@ static int mana_create_txq(struct mana_port_context *apc,
struct mana_obj_spec cq_spec;
struct gdma_queue_spec spec;
struct gdma_context *gc;
+ struct gdma_queue __rcu **cq_table;
struct mana_txq *txq;
struct mana_cq *cq;
u32 txq_size;
@@ -2596,12 +2619,18 @@ static int mana_create_txq(struct mana_port_context *apc,
cq->gdma_id = cq->gdma_cq->id;
- if (WARN_ON(cq->gdma_id >= gc->max_num_cqs)) {
+ /* No rcu_read_lock(): mana_create_txq runs under RTNL during
+ * netdev bring-up, inside the netdev lifetime that
+ * mana_remove() drains before the base cq_table can be freed.
+ * See gdma_context::cq_table in gdma.h for why "true" is sound.
+ */
+ cq_table = rcu_dereference_protected(gc->cq_table, true);
+ if (WARN_ON(!cq_table || cq->gdma_id >= gc->max_num_cqs)) {
err = -EINVAL;
goto out;
}
- gc->cq_table[cq->gdma_id] = cq->gdma_cq;
+ rcu_assign_pointer(cq_table[cq->gdma_id], cq->gdma_cq);
mana_create_txq_debugfs(apc, i);
@@ -2621,25 +2650,20 @@ static int mana_create_txq(struct mana_port_context *apc,
return err;
}
-static void mana_destroy_rxq(struct mana_port_context *apc,
+/* Quiesce an RXQ's CQ on the device and clear its cq_table slot, without
+ * waiting for a grace period. Split out of mana_destroy_rxq() so a batch
+ * teardown (mana_destroy_rxqs()) can quiesce every RXQ and then take a
+ * single synchronize_rcu() instead of one per RXQ.
+ */
+static void mana_quiesce_rxq(struct mana_port_context *apc,
struct mana_rxq *rxq, bool napi_initialized)
-
{
struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
- struct mana_recv_buf_oob *rx_oob;
- struct device *dev = gc->dev;
- struct napi_struct *napi;
- struct page *page;
- int i;
-
- if (!rxq)
- return;
+ struct napi_struct *napi = &rxq->rx_cq.napi;
debugfs_remove_recursive(rxq->mana_rx_debugfs);
rxq->mana_rx_debugfs = NULL;
- napi = &rxq->rx_cq.napi;
-
if (napi_initialized) {
napi_synchronize(napi);
@@ -2650,8 +2674,27 @@ static void mana_destroy_rxq(struct mana_port_context *apc,
if (xdp_rxq_info_is_reg(&rxq->xdp_rxq))
xdp_rxq_info_unreg(&rxq->xdp_rxq);
- if (rxq->rxobj != INVALID_MANA_HANDLE)
+ if (rxq->rxobj != INVALID_MANA_HANDLE) {
mana_destroy_wq_obj(apc, GDMA_RQ, rxq->rxobj);
+ rxq->rxobj = INVALID_MANA_HANDLE;
+ }
+
+ if (rxq->rx_cq.gdma_cq)
+ mana_gd_unpublish_cq(gc, rxq->rx_cq.gdma_cq);
+}
+
+/* Free an RXQ once its cq_table slot has been cleared and a grace period
+ * has elapsed (see mana_quiesce_rxq()). mana_deinit_cq() ->
+ * mana_gd_destroy_cq() finds the slot already NULL and skips its own
+ * synchronize_rcu().
+ */
+static void mana_free_rxq(struct mana_port_context *apc, struct mana_rxq *rxq)
+{
+ struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
+ struct mana_recv_buf_oob *rx_oob;
+ struct device *dev = gc->dev;
+ struct page *page;
+ int i;
mana_deinit_cq(apc, &rxq->rx_cq);
@@ -2685,6 +2728,23 @@ static void mana_destroy_rxq(struct mana_port_context *apc,
kvfree(rxq);
}
+static void mana_destroy_rxq(struct mana_port_context *apc,
+ struct mana_rxq *rxq, bool napi_initialized)
+
+{
+ if (!rxq)
+ return;
+
+ mana_quiesce_rxq(apc, rxq, napi_initialized);
+
+ /* Wait for in-flight EQ handlers that may have loaded the old CQ
+ * pointer via rcu_dereference() before freeing.
+ */
+ synchronize_rcu();
+
+ mana_free_rxq(apc, rxq);
+}
+
static int mana_fill_rx_oob(struct mana_recv_buf_oob *rx_oob, u32 mem_key,
struct mana_rxq *rxq, struct device *dev)
{
@@ -2821,6 +2881,7 @@ static struct mana_rxq *mana_create_rxq(struct mana_port_context *apc,
struct gdma_queue_spec spec;
struct mana_cq *cq = NULL;
struct gdma_context *gc;
+ struct gdma_queue __rcu **cq_table;
u32 cq_size, rq_size;
struct mana_rxq *rxq;
int err;
@@ -2905,12 +2966,18 @@ static struct mana_rxq *mana_create_rxq(struct mana_port_context *apc,
if (err)
goto out;
- if (WARN_ON(cq->gdma_id >= gc->max_num_cqs)) {
+ /* No rcu_read_lock(): mana_create_rxq runs under RTNL during netdev
+ * bring-up, inside the netdev lifetime that mana_remove() drains
+ * before the base cq_table can be freed. See gdma_context::cq_table
+ * in gdma.h for why "true" is sound.
+ */
+ cq_table = rcu_dereference_protected(gc->cq_table, true);
+ if (WARN_ON(!cq_table || cq->gdma_id >= gc->max_num_cqs)) {
err = -EINVAL;
goto out;
}
- gc->cq_table[cq->gdma_id] = cq->gdma_cq;
+ rcu_assign_pointer(cq_table[cq->gdma_id], cq->gdma_cq);
netif_napi_add_weight_locked(ndev, &cq->napi, mana_poll, 1);
@@ -2987,16 +3054,31 @@ static void mana_destroy_rxqs(struct mana_port_context *apc)
struct mana_rxq *rxq;
u32 rxq_idx;
- if (apc->rxqs) {
+ if (!apc->rxqs)
+ return;
- for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
- rxq = apc->rxqs[rxq_idx];
- if (!rxq)
- continue;
+ /* Pass 1: quiesce every RXQ's CQ and clear its cq_table slot. */
+ for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
+ rxq = apc->rxqs[rxq_idx];
+ if (!rxq)
+ continue;
- mana_destroy_rxq(apc, rxq, true);
- apc->rxqs[rxq_idx] = NULL;
- }
+ mana_quiesce_rxq(apc, rxq, true);
+ }
+
+ /* One grace period for the whole port instead of one per RXQ. */
+ synchronize_rcu();
+
+ /* Pass 2: the slots are clear, so mana_gd_destroy_cq() skips its own
+ * grace period; free each RXQ.
+ */
+ for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
+ rxq = apc->rxqs[rxq_idx];
+ if (!rxq)
+ continue;
+
+ mana_free_rxq(apc, rxq);
+ apc->rxqs[rxq_idx] = NULL;
}
}
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 0c395917b2144ec4c2faafa5d6c7de7a452f1ebf..0f591029d363b5a1b5c12f4bb1416f5da0414a28 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -418,7 +418,36 @@ struct gdma_context {
/* This maps a CQ index to the queue structure. */
unsigned int max_num_cqs;
- struct gdma_queue **cq_table;
+ /* max_num_cqs above is the size of cq_table and an upper bound on
+ * valid CQ indices for the table's lifetime. cq_table == NULL is the
+ * "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).
+ *
+ * Both the base pointer and each entry are RCU-managed. The fast
+ * path (mana_gd_process_eqe) reads the base via rcu_dereference()
+ * under rcu_read_lock(), so the table is freed with
+ * rcu_assign_pointer(NULL) + synchronize_rcu() and an in-flight
+ * reader can never observe freed memory.
+ *
+ * The slow paths -- mana_gd_destroy_cq() and the CQ install/remove
+ * callers (mana_create_txq/_rxq, mana_ib_install/remove_cq_cb) --
+ * instead read the base with rcu_dereference_protected(cq_table,
+ * true). The bare "true" asserts teardown/bring-up ordering, not a
+ * lock: the base table is allocated in mana_hwc_establish_channel()
+ * and replaced+freed only by mana_hwc_destroy_channel() (via
+ * mana_gd_cleanup_device()) and the create-time reinit. The reinit
+ * runs before either consumer is probed, and cleanup_device() runs
+ * after mana_remove() / mana_rdma_remove() have detached the ports
+ * under RTNL and drained the IB device, so no install/remove caller
+ * is running when the base is freed. This is an ordering argument
+ * about when cleanup_device() runs: suspend and shutdown keep the
+ * netdev registered, so it does not rely on unregister_netdevice()
+ * having run on every path. mana_hwc_destroy_channel() itself reads
+ * cq_table (mana_hwc_destroy_cq()) before it replaces and vfree()s
+ * the base, so that access is ordered ahead of the free by program
+ * order.
+ */
+ struct gdma_queue __rcu * __rcu *cq_table;
/* Protect eq_test_event and test_event_eq_id */
struct mutex eq_test_event_mutex;
@@ -496,6 +525,12 @@ int mana_gd_create_mana_wq_cq(struct gdma_dev *gd,
void mana_gd_destroy_queue(struct gdma_context *gc, struct gdma_queue *queue);
+/* Clear a CQ's cq_table slot without waiting for a grace period. Batched
+ * teardown paths clear several slots and then take a single synchronize_rcu();
+ * single-CQ callers use mana_gd_destroy_cq() instead, which also waits.
+ */
+bool mana_gd_unpublish_cq(struct gdma_context *gc, struct gdma_queue *queue);
+
int mana_gd_poll_cq(struct gdma_queue *cq, struct gdma_comp *comp, int num_cqe);
void mana_gd_ring_cq(struct gdma_queue *cq, u8 arm_bit);
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net v6 1/7] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy
2026-08-11 2:38 ` [PATCH net v6 1/7] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy Long Li
@ 2026-08-11 8:18 ` Leon Romanovsky
2026-08-11 21:25 ` [EXTERNAL] " Long Li
2026-08-12 23:46 ` Jakub Kicinski
1 sibling, 1 reply; 21+ messages in thread
From: Leon Romanovsky @ 2026-08-11 8:18 UTC (permalink / raw)
To: Long Li
Cc: Konstantin Taranov, Jakub Kicinski, David S . Miller, Paolo Abeni,
Eric Dumazet, Andrew Lunn, Jason Gunthorpe, Haiyang Zhang,
K . Y . Srinivasan, Wei Liu, Dexuan Cui, shradhagupta,
Simon Horman, ernis, stephen, netdev, linux-rdma, linux-hyperv,
linux-kernel
On Mon, Aug 10, 2026 at 07:38:15PM -0700, Long Li wrote:
> The EQ interrupt handler (mana_gd_process_eqe) looks up the completing CQ
> in gc->cq_table[cq_id] and runs its callback, concurrently with CQ
> teardown on another CPU that clears the slot and frees the CQ. cq_table
> was a plain pointer array freed with no grace period, so the two race
> into a use-after-free:
>
> CPU A (mana_gd_intr, hard IRQ) CPU B (CQ destroy)
> ---------------------------------- ------------------------------
> cq = gc->cq_table[cq_id]; // valid
> gc->cq_table[id] = NULL;
> kfree(cq); // freed
> cq->cq.callback(ctx, cq); // use-after-free
>
> The handler's existing rcu_read_lock() only guards the per-IRQ EQ list
> traversal; cq_table was never under any RCU contract, and a read-side
> lock is inert unless the freer also defers the free past a grace period.
>
> Put cq_table under RCU: annotate the base pointer and entries __rcu, read
> with rcu_dereference() in the handler, publish with rcu_assign_pointer(),
> and on teardown clear the slot then synchronize_rcu() before freeing the
> CQ. The grace period blocks until every in-flight handler has dropped
> the old pointer, so the kfree() can no longer race the callback.
>
> This fixes only the CQ lifetime (the use-after-free); it does not make
> the cq_id bound trustworthy. gc->max_num_cqs is still range-checked
> outside the published table, and hardening that field against a spoofed
> device value is a separate change.
>
> netdev teardown destroys a CQ per TX and per RX queue, so one grace
> period each in mana_gd_destroy_cq() would serialize up to
> 2 * MANA_MAX_NUM_QUEUES synchronize_rcu() calls under RTNL on every
> ifdown, MTU change or ring/channel reconfigure. Clear all of a port's
> CQ slots first and take a single grace period per teardown instead:
> mana_gd_unpublish_cq() clears a slot without waiting, and
> mana_gd_destroy_cq() -- which still serves the single-CQ callers --
> finds the slot already cleared and skips its own synchronize_rcu().
>
> Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
> Changes in v6:
> - mana_gd_unpublish_cq() and mana_ib_remove_cq_cb() clear a cq_table
> slot only when it still points at the CQ being torn down, so the
> two-pass teardown cannot wipe an entry a concurrent RDMA CQ create
> recycled during the grace period.
> - mana_gd_process_eqe() drops an already-unpublished (NULL) slot quietly
> instead of a WARN_ON_ONCE() splat during a normal ifdown/MTU change,
> and reads gc->cq_table before gc->max_num_cqs with an smp_rmb()
> between them so a shrinking re-establish cannot pair a stale bound
> with a newly published, smaller table.
> - Documented the cq_table/max_num_cqs contract on the cq_table field
> instead of rewording the comment above max_num_cqs.
>
> Changes in v5:
> - No code changes since v4 (resend as a standalone thread).
>
> Changes in v4:
> - Replaced the per-CQ synchronize_rcu() in the netdev teardown paths
> with a two-pass quiesce/free that takes one grace period per
> teardown; mana_gd_unpublish_cq() splits the slot-clear from the grace
> period.
> - Snapshot cq->id and max_num_cqs with READ_ONCE() in
> mana_hwc_establish_channel() so one value sizes, bounds and indexes
> cq_table.
> - Corrected the gc->cq_table lifetime comment in gdma.h; rescoped the
> changelog to the use-after-free fix (the bound is patch 7).
>
> drivers/infiniband/hw/mana/cq.c | 51 ++++++-
> .../net/ethernet/microsoft/mana/gdma_main.c | 65 +++++++--
> .../net/ethernet/microsoft/mana/hw_channel.c | 29 ++--
> drivers/net/ethernet/microsoft/mana/mana_en.c | 136 ++++++++++++++----
> include/net/mana/gdma.h | 37 ++++-
> 5 files changed, 268 insertions(+), 50 deletions(-)
This patch is so bloated with AI that it is hard to read and difficult to justify
such a large diff for a simple change, which all drivers experience that
flow.
As a bare minimum. you need to reorder mana_ib_gd_destroy_cq(), mana_ib_destroy_queue(),
and mana_ib_remove_cq_cb() so that HW objects are stopped before SW state is torn down.
And probably introduce get/put CQ primitives.
Thanks
^ permalink raw reply [flat|nested] 21+ messages in thread* RE: [EXTERNAL] Re: [PATCH net v6 1/7] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy
2026-08-11 8:18 ` Leon Romanovsky
@ 2026-08-11 21:25 ` Long Li
0 siblings, 0 replies; 21+ messages in thread
From: Long Li @ 2026-08-11 21:25 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Konstantin Taranov, Jakub Kicinski, David S . Miller, Paolo Abeni,
Eric Dumazet, Andrew Lunn, Jason Gunthorpe, Haiyang Zhang,
KY Srinivasan, Wei Liu, Dexuan Cui,
shradhagupta@linux.microsoft.com, Simon Horman,
ernis@linux.microsoft.com, stephen@networkplumber.org,
netdev@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
> On Mon, Aug 10, 2026 at 07:38:15PM -0700, Long Li wrote:
> > The EQ interrupt handler (mana_gd_process_eqe) looks up the completing
> > CQ in gc->cq_table[cq_id] and runs its callback, concurrently with CQ
> > teardown on another CPU that clears the slot and frees the CQ.
> > cq_table was a plain pointer array freed with no grace period, so the
> > two race into a use-after-free:
> >
> > CPU A (mana_gd_intr, hard IRQ) CPU B (CQ destroy)
> > ---------------------------------- ------------------------------
> > cq = gc->cq_table[cq_id]; // valid
> > gc->cq_table[id] = NULL;
> > kfree(cq); // freed
> > cq->cq.callback(ctx, cq); // use-after-free
> >
> > The handler's existing rcu_read_lock() only guards the per-IRQ EQ list
> > traversal; cq_table was never under any RCU contract, and a read-side
> > lock is inert unless the freer also defers the free past a grace period.
> >
> > Put cq_table under RCU: annotate the base pointer and entries __rcu,
> > read with rcu_dereference() in the handler, publish with
> > rcu_assign_pointer(), and on teardown clear the slot then
> > synchronize_rcu() before freeing the CQ. The grace period blocks
> > until every in-flight handler has dropped the old pointer, so the kfree() can
> no longer race the callback.
> >
> > This fixes only the CQ lifetime (the use-after-free); it does not make
> > the cq_id bound trustworthy. gc->max_num_cqs is still range-checked
> > outside the published table, and hardening that field against a
> > spoofed device value is a separate change.
> >
> > netdev teardown destroys a CQ per TX and per RX queue, so one grace
> > period each in mana_gd_destroy_cq() would serialize up to
> > 2 * MANA_MAX_NUM_QUEUES synchronize_rcu() calls under RTNL on
> every
> > ifdown, MTU change or ring/channel reconfigure. Clear all of a port's
> > CQ slots first and take a single grace period per teardown instead:
> > mana_gd_unpublish_cq() clears a slot without waiting, and
> > mana_gd_destroy_cq() -- which still serves the single-CQ callers --
> > finds the slot already cleared and skips its own synchronize_rcu().
> >
> > Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure
> > Network Adapter (MANA)")
> > Signed-off-by: Long Li <longli@microsoft.com>
> > ---
> > Changes in v6:
> > - mana_gd_unpublish_cq() and mana_ib_remove_cq_cb() clear a cq_table
> > slot only when it still points at the CQ being torn down, so the
> > two-pass teardown cannot wipe an entry a concurrent RDMA CQ create
> > recycled during the grace period.
> > - mana_gd_process_eqe() drops an already-unpublished (NULL) slot
> quietly
> > instead of a WARN_ON_ONCE() splat during a normal ifdown/MTU
> change,
> > and reads gc->cq_table before gc->max_num_cqs with an smp_rmb()
> > between them so a shrinking re-establish cannot pair a stale bound
> > with a newly published, smaller table.
> > - Documented the cq_table/max_num_cqs contract on the cq_table field
> > instead of rewording the comment above max_num_cqs.
> >
> > Changes in v5:
> > - No code changes since v4 (resend as a standalone thread).
> >
> > Changes in v4:
> > - Replaced the per-CQ synchronize_rcu() in the netdev teardown paths
> > with a two-pass quiesce/free that takes one grace period per
> > teardown; mana_gd_unpublish_cq() splits the slot-clear from the grace
> > period.
> > - Snapshot cq->id and max_num_cqs with READ_ONCE() in
> > mana_hwc_establish_channel() so one value sizes, bounds and indexes
> > cq_table.
> > - Corrected the gc->cq_table lifetime comment in gdma.h; rescoped the
> > changelog to the use-after-free fix (the bound is patch 7).
> >
> > drivers/infiniband/hw/mana/cq.c | 51 ++++++-
> > .../net/ethernet/microsoft/mana/gdma_main.c | 65 +++++++--
> > .../net/ethernet/microsoft/mana/hw_channel.c | 29 ++--
> > drivers/net/ethernet/microsoft/mana/mana_en.c | 136 ++++++++++++++----
> > include/net/mana/gdma.h | 37 ++++-
> > 5 files changed, 268 insertions(+), 50 deletions(-)
>
> This patch is so bloated with AI that it is hard to read and difficult to justify
> such a large diff for a simple change, which all drivers experience that flow.
>
> As a bare minimum. you need to reorder mana_ib_gd_destroy_cq(),
> mana_ib_destroy_queue(), and mana_ib_remove_cq_cb() so that HW objects
> are stopped before SW state is torn down.
>
> And probably introduce get/put CQ primitives.
>
> Thanks
I'll clean up and follow your suggestions on v7.
Thanks
Long
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net v6 1/7] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy
2026-08-11 2:38 ` [PATCH net v6 1/7] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy Long Li
2026-08-11 8:18 ` Leon Romanovsky
@ 2026-08-12 23:46 ` Jakub Kicinski
2026-08-13 0:25 ` [EXTERNAL] " Long Li
1 sibling, 1 reply; 21+ messages in thread
From: Jakub Kicinski @ 2026-08-12 23:46 UTC (permalink / raw)
To: longli
Cc: Jakub Kicinski, kotaranov, davem, pabeni, edumazet, andrew+netdev,
jgg, leon, haiyangz, kys, wei.liu, decui, shradhagupta, horms,
ernis, stephen, netdev, linux-rdma, linux-hyperv, linux-kernel
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 (base pointer and entries) under RCU so the EQ
interrupt handler can no longer dereference a CQ that a concurrent teardown
has freed. Teardown clears the slot and takes a grace period before the CQ
memory is released, and the Ethernet paths are converted to a two-pass
quiesce/free so the grace periods are batched instead of one per CQ.
A few questions below.
> diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
> index f2547989f4229..73d97b2f5cf9b 100644
> --- a/drivers/infiniband/hw/mana/cq.c
> +++ b/drivers/infiniband/hw/mana/cq.c
[ ... ]
> @@ -149,23 +157,54 @@ 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);
> + /* Clear the slot only if it still holds the entry this CQ installed
> + * (gdma_cq->cq.context == cq). If the id was already removed, or was
> + * recycled and republished for another CQ, leave the current entry
> + * intact instead of wiping a live one.
> + */
> + if (!gdma_cq || gdma_cq->cq.context != cq)
> + return;
> +
> + rcu_assign_pointer(cq_table[cq->queue.id], NULL);
[Severity: Low]
The comment says the identity test protects against an id that "was
recycled and republished for another CQ", but the compare and the clear
are two separate accesses with nothing making them indivisible. Both
mana_ib_remove_cq_cb() here and mana_gd_unpublish_cq() do:
/* read */
gdma_cq = rcu_dereference_protected(cq_table[cq->queue.id], true);
if (!gdma_cq || gdma_cq->cq.context != cq)
return;
/* separate store */
rcu_assign_pointer(cq_table[cq->queue.id], NULL);
The two sides of the table are also asymmetric: mana_ib_install_cq_cb()
refuses to publish into a non-NULL slot, but mana_create_txq() and
mana_create_rxq() publish with rcu_assign_pointer(cq_table[cq->gdma_id],
cq->gdma_cq) and no occupancy check at all.
So can this interleaving wipe a live entry?
remover publisher
reads slot, sees A
stores B' into the same slot
stores NULL (B' is now gone)
Since mana_gd_process_eqe() now drops a NULL slot silently, would every
completion for B' then be discarded with no diagnostic, and would B''s own
remover later see NULL and skip its kfree()? Would a cmpxchg on the slot,
or a small lock covering cq_table entry mutation, make the conditional
clear actually atomic?
This does need the device to hand the same CQ id to two live consumers, so
I could not demonstrate it in practice; the pre-patch code cleared the slot
unconditionally, so the guard is still an improvement.
> +
> + /* 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 e8b7ffb47eb98..d40f25a1a74a7 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
[ ... ]
> @@ -743,11 +744,30 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
> switch (type) {
> case GDMA_EQE_COMPLETION:
> cq_id = eqe->details[0] & 0xFFFFFF;
> + cq_table = rcu_dereference(gc->cq_table);
> + if (WARN_ON_ONCE(!cq_table))
> + break;
> +
> + /* Pair with the rcu_assign_pointer(gc->cq_table) release in
> + * mana_hwc_establish_channel(), which publishes the table
> + * after storing gc->max_num_cqs. The rmb keeps this bound
> + * read ordered after the table load, so a shrinking
> + * re-establish cannot pair a stale, larger max_num_cqs with a
> + * newly published, smaller table and index out of bounds.
> + */
> + smp_rmb();
> 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 = rcu_dereference(cq_table[cq_id]);
[Severity: High]
This isn't a bug introduced by this patch, but at this commit the bound
used here is still mutable while a table is published.
mana_hwc_init_event_handler() writes it straight from EQ interrupt context
from a device-supplied value:
case HWC_INIT_DATA_MAX_NUM_CQS:
gd->gdma_context->max_num_cqs = val;
while the table itself is sized once in mana_hwc_establish_channel() with
vcalloc(gc->max_num_cqs, sizeof(*cq_table)). A later, larger value would
inflate every bound test (cq_id >= gc->max_num_cqs here, cq->gdma_id >=
gc->max_num_cqs in mana_create_txq()/mana_create_rxq(), cq->queue.id >=
gc->max_num_cqs in mana_ib_install_cq_cb()) past the allocation, and a
smaller one would make mana_gd_unpublish_cq() return false so
mana_gd_destroy_cq() skips the grace period for a still-published CQ.
The gdma.h comment added by this patch states the invariant as already
holding ("max_num_cqs above is the size of cq_table and an upper bound on
valid CQ indices for the table's lifetime"), which only becomes true with
the last patch in this series, "net: mana: keep max_num_cqs immutable once
cq_table is allocated" -- that one moves the handler to
WRITE_ONCE(hwc->hwc_init_max_num_cqs, val) and has
mana_hwc_establish_channel() commit a single READ_ONCE() snapshot as both
the vcalloc() size and gc->max_num_cqs. Should the documented contract
land together with the change that establishes it, or reference it?
> + /* A NULL entry is expected while a concurrent teardown
> + * (e.g. ifdown or an MTU change) has unpublished this CQ but
> + * not yet freed it; the completion is stale, so drop it
> + * quietly rather than warning.
> + */
> + if (!cq)
> + break;
> + if (WARN_ON_ONCE(cq->type != GDMA_CQ || cq->id != cq_id))
> break;
>
> if (cq->cq.callback)
> @@ -1050,18 +1070,47 @@ static void mana_gd_create_cq(const struct gdma_queue_spec *spec,
> queue->cq.callback = spec->cq.callback;
> }
>
> -static void mana_gd_destroy_cq(struct gdma_context *gc,
> - struct gdma_queue *queue)
> +bool mana_gd_unpublish_cq(struct gdma_context *gc, struct gdma_queue *queue)
> {
> + struct gdma_queue __rcu **cq_table;
> u32 id = queue->id;
>
> - if (id >= gc->max_num_cqs)
> - return;
> + /* No rcu_read_lock() here: unpublish runs only on the
> + * CQ-destroy/teardown path, where the base cq_table is stable. See
> + * the lifecycle note on gdma_context::cq_table in gdma.h for why the
> + * "true" predicate is sound.
> + */
> + cq_table = rcu_dereference_protected(gc->cq_table, true);
> + if (!cq_table || id >= gc->max_num_cqs)
> + return false;
> +
> + /* Clear the slot only if it still refers to this queue. The
> + * Ethernet two-pass teardown unpublishes the same index twice, a
> + * grace period apart, and a CQ that legitimately recycled this id in
> + * between (e.g. a new RDMA CQ via mana_ib_install_cq_cb()) must not
> + * have its fresh entry wiped by the second pass.
> + */
> + if (rcu_access_pointer(cq_table[id]) != queue)
> + return false;
> +
> + rcu_assign_pointer(cq_table[id], NULL);
> + return true;
> +}
>
> - if (!gc->cq_table[id])
> +static void mana_gd_destroy_cq(struct gdma_context *gc,
> + struct gdma_queue *queue)
> +{
> + /* A batched teardown may already have cleared the slot and taken the
> + * grace period; then there is nothing left to wait for.
> + */
> + if (!mana_gd_unpublish_cq(gc, queue))
> return;
>
> - gc->cq_table[id] = NULL;
> + /* Wait for in-flight EQ handlers that may have loaded the old
> + * pointer via rcu_dereference() to finish before the caller
> + * frees the CQ memory.
> + */
> + synchronize_rcu();
> }
[Severity: Medium]
mana_gd_unpublish_cq() returns false for two different states, and
mana_gd_destroy_cq() treats both as "nothing left to wait for":
a) the slot is already NULL because a batched pass cleared it and
already took the grace period, and
b) rcu_access_pointer(cq_table[id]) != queue, i.e. this queue's entry
was overwritten by another publisher and no grace period has been
taken for this pointer.
In case (b), mana_gd_destroy_queue() then kfree()s the queue with no wait.
Can an in-flight mana_gd_process_eqe() that already did
cq = rcu_dereference(cq_table[cq_id]);
on exactly that pointer go on to read cq->type / cq->id and call
cq->cq.callback(cq->cq.context, cq) after the free?
One route into state (b): for an RDMA kernel (kmem) CQ,
mana_ib_install_cq_cb() publishes the entry, mana_ib_remove_cq_cb()
returns early for kmem ("Then it will be cleaned and removed by the mana"),
so the entry stays published across mana_ib_gd_destroy_cq() which releases
the CQ id on the device, and is only cleared later via
mana_ib_destroy_queue() -> mana_gd_destroy_queue() -> mana_gd_destroy_cq().
If the device hands that id to an Ethernet CQ being created in that window,
mana_create_txq()/mana_create_rxq() overwrite the slot unconditionally, and
the identity test above then fails.
Would it be safer to distinguish "we cleared our slot", "never published"
and "our entry was overwritten", and still synchronize in the last case?
I could not confirm the device actually recycles ids inside that window, so
this may not be reachable in practice.
> 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);
>
> return 0;
> }
[ ... ]
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 92bb55935c1c4..5d215981bba83 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -2427,12 +2427,18 @@ static void mana_deinit_txq(struct mana_port_context *apc, struct mana_txq *txq)
>
> static void mana_destroy_txq(struct mana_port_context *apc)
> {
> + struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
> struct napi_struct *napi;
> int i;
>
> if (!apc->tx_qp)
> return;
>
> + /* Pass 1: quiesce each CQ on the device and clear its cq_table slot.
> + * Taking one grace period below for the whole port avoids up to
> + * apc->num_queues serialized synchronize_rcu() calls (one per CQ in
> + * mana_gd_destroy_cq()) under RTNL on every teardown.
> + */
[Severity: Low]
This isn't a bug, but is "the whole port" accurate here? RX and TX are
batched independently: mana_destroy_rxqs() unpublishes only the RX CQs and
calls synchronize_rcu(), then mana_destroy_txq() unpublishes only the TX
CQs and calls synchronize_rcu() again, and mana_detach() runs both back to
back:
mana_destroy_rxqs(apc);
mana_destroy_txq(apc);
The same pairing happens on the attach error path. So each ifdown, MTU
change or ring reconfigure takes two grace periods per port rather than
one.
The commit message says the same thing:
Clear all of a port's CQ slots first and take a single grace period
per teardown instead
and the comment in mana_destroy_rxqs() reads "One grace period for the
whole port instead of one per RXQ". Would "one grace period per RX batch
and one per TX batch" describe the result more precisely?
> for (i = 0; i < apc->num_queues; i++) {
> if (!apc->tx_qp[i])
> continue;
> @@ -2448,8 +2454,24 @@ static void mana_destroy_txq(struct mana_port_context *apc)
> apc->tx_qp[i]->txq.napi_initialized = false;
> }
>
> - if (apc->tx_qp[i]->tx_object != INVALID_MANA_HANDLE)
> - mana_destroy_wq_obj(apc, GDMA_SQ, apc->tx_qp[i]->tx_object);
> + if (apc->tx_qp[i]->tx_object != INVALID_MANA_HANDLE) {
> + mana_destroy_wq_obj(apc, GDMA_SQ,
> + apc->tx_qp[i]->tx_object);
> + apc->tx_qp[i]->tx_object = INVALID_MANA_HANDLE;
> + }
> +
> + if (apc->tx_qp[i]->tx_cq.gdma_cq)
> + mana_gd_unpublish_cq(gc, apc->tx_qp[i]->tx_cq.gdma_cq);
> + }
> +
> + synchronize_rcu();
> +
> + /* Pass 2: the slots are clear, so mana_gd_destroy_cq() skips its own
> + * grace period; free the CQ, the TXQ and the queue pair.
> + */
> + for (i = 0; i < apc->num_queues; i++) {
> + if (!apc->tx_qp[i])
> + continue;
>
> mana_deinit_cq(apc, &apc->tx_qp[i]->tx_cq);
>
[ ... ]
> @@ -2987,16 +3054,31 @@ static void mana_destroy_rxqs(struct mana_port_context *apc)
> struct mana_rxq *rxq;
> u32 rxq_idx;
>
> - if (apc->rxqs) {
> + if (!apc->rxqs)
> + return;
>
> - for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
> - rxq = apc->rxqs[rxq_idx];
> - if (!rxq)
> - continue;
> + /* Pass 1: quiesce every RXQ's CQ and clear its cq_table slot. */
> + for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
> + rxq = apc->rxqs[rxq_idx];
> + if (!rxq)
> + continue;
>
> - mana_destroy_rxq(apc, rxq, true);
> - apc->rxqs[rxq_idx] = NULL;
> - }
> + mana_quiesce_rxq(apc, rxq, true);
> + }
> +
> + /* One grace period for the whole port instead of one per RXQ. */
> + synchronize_rcu();
[ ... ]
> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> index 0c395917b2144..0f591029d363b 100644
> --- a/include/net/mana/gdma.h
> +++ b/include/net/mana/gdma.h
[ ... ]
> @@ -496,6 +525,12 @@ int mana_gd_create_mana_wq_cq(struct gdma_dev *gd,
>
> void mana_gd_destroy_queue(struct gdma_context *gc, struct gdma_queue *queue);
>
> +/* Clear a CQ's cq_table slot without waiting for a grace period. Batched
> + * teardown paths clear several slots and then take a single synchronize_rcu();
> + * single-CQ callers use mana_gd_destroy_cq() instead, which also waits.
> + */
> +bool mana_gd_unpublish_cq(struct gdma_context *gc, struct gdma_queue *queue);
[Severity: Low]
This isn't a bug, but the comment directs includers of this header to an
API they cannot reach: mana_gd_destroy_cq() is static in gdma_main.c and is
not declared anywhere. The reachable entry point is
mana_gd_destroy_queue(), which is EXPORT_SYMBOL_NS("NET_MANA") and
dispatches GDMA_CQ into the private helper.
mana_gd_unpublish_cq() itself is non-static and declared here, but has no
EXPORT_SYMBOL_NS("NET_MANA") unlike its header-declared siblings, so a
mana_ib user of it would not link; its only users (gdma_main.c and
mana_en.c) are both in mana.ko.
Would keeping the declaration in a mana-private header, or exporting it and
pointing the comment at mana_gd_destroy_queue(), avoid the situation this
patch already runs into -- mana_ib_remove_cq_cb() open-coding the same
unpublish-then-wait sequence with a different identity predicate
(gdma_cq->cq.context != cq) from the one in mana_gd_unpublish_cq()
(rcu_access_pointer(cq_table[id]) != 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);
--
pw-bot: cr
^ permalink raw reply [flat|nested] 21+ messages in thread* RE: [EXTERNAL] Re: [PATCH net v6 1/7] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy
2026-08-12 23:46 ` Jakub Kicinski
@ 2026-08-13 0:25 ` Long Li
0 siblings, 0 replies; 21+ messages in thread
From: Long Li @ 2026-08-13 0:25 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Konstantin Taranov, davem@davemloft.net, pabeni@redhat.com,
edumazet@google.com, andrew+netdev@lunn.ch, jgg@ziepe.ca,
leon@kernel.org, Haiyang Zhang, KY Srinivasan, wei.liu@kernel.org,
Dexuan Cui, shradhagupta@linux.microsoft.com, horms@kernel.org,
ernis@linux.microsoft.com, stephen@networkplumber.org,
netdev@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
>
> 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 (base pointer and entries) under RCU so the EQ
> interrupt handler can no longer dereference a CQ that a concurrent teardown
> has freed. Teardown clears the slot and takes a grace period before the CQ
> memory is released, and the Ethernet paths are converted to a two-pass
> quiesce/free so the grace periods are batched instead of one per CQ.
>
> A few questions below.
I'm sending V7 addressing all the issues pointed out in this review, together with Leon's suggestion on rearrange CQ and adding refcounts.
Thanks,
Long
>
> > diff --git a/drivers/infiniband/hw/mana/cq.c
> > b/drivers/infiniband/hw/mana/cq.c index f2547989f4229..73d97b2f5cf9b
> > 100644
> > --- a/drivers/infiniband/hw/mana/cq.c
> > +++ b/drivers/infiniband/hw/mana/cq.c
>
> [ ... ]
>
> > @@ -149,23 +157,54 @@ 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);
> > + /* Clear the slot only if it still holds the entry this CQ installed
> > + * (gdma_cq->cq.context == cq). If the id was already removed, or was
> > + * recycled and republished for another CQ, leave the current entry
> > + * intact instead of wiping a live one.
> > + */
> > + if (!gdma_cq || gdma_cq->cq.context != cq)
> > + return;
> > +
> > + rcu_assign_pointer(cq_table[cq->queue.id], NULL);
>
> [Severity: Low]
> The comment says the identity test protects against an id that "was recycled
> and republished for another CQ", but the compare and the clear are two
> separate accesses with nothing making them indivisible. Both
> mana_ib_remove_cq_cb() here and mana_gd_unpublish_cq() do:
>
> /* read */
> gdma_cq = rcu_dereference_protected(cq_table[cq->queue.id], true);
> if (!gdma_cq || gdma_cq->cq.context != cq)
> return;
> /* separate store */
> rcu_assign_pointer(cq_table[cq->queue.id], NULL);
>
> The two sides of the table are also asymmetric: mana_ib_install_cq_cb()
> refuses to publish into a non-NULL slot, but mana_create_txq() and
> mana_create_rxq() publish with rcu_assign_pointer(cq_table[cq->gdma_id],
> cq->gdma_cq) and no occupancy check at all.
>
> So can this interleaving wipe a live entry?
>
> remover publisher
> reads slot, sees A
> stores B' into the same slot
> stores NULL (B' is now gone)
>
> Since mana_gd_process_eqe() now drops a NULL slot silently, would every
> completion for B' then be discarded with no diagnostic, and would B''s own
> remover later see NULL and skip its kfree()? Would a cmpxchg on the slot, or a
> small lock covering cq_table entry mutation, make the conditional clear actually
> atomic?
>
> This does need the device to hand the same CQ id to two live consumers, so I
> could not demonstrate it in practice; the pre-patch code cleared the slot
> unconditionally, so the guard is still an improvement.
>
> > +
> > + /* 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 e8b7ffb47eb98..d40f25a1a74a7 100644
> > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
>
> [ ... ]
>
> > @@ -743,11 +744,30 @@ static void mana_gd_process_eqe(struct
> gdma_queue *eq)
> > switch (type) {
> > case GDMA_EQE_COMPLETION:
> > cq_id = eqe->details[0] & 0xFFFFFF;
> > + cq_table = rcu_dereference(gc->cq_table);
> > + if (WARN_ON_ONCE(!cq_table))
> > + break;
> > +
> > + /* Pair with the rcu_assign_pointer(gc->cq_table) release in
> > + * mana_hwc_establish_channel(), which publishes the table
> > + * after storing gc->max_num_cqs. The rmb keeps this bound
> > + * read ordered after the table load, so a shrinking
> > + * re-establish cannot pair a stale, larger max_num_cqs with a
> > + * newly published, smaller table and index out of bounds.
> > + */
> > + smp_rmb();
> > 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 = rcu_dereference(cq_table[cq_id]);
>
> [Severity: High]
> This isn't a bug introduced by this patch, but at this commit the bound used
> here is still mutable while a table is published.
> mana_hwc_init_event_handler() writes it straight from EQ interrupt context
> from a device-supplied value:
>
> case HWC_INIT_DATA_MAX_NUM_CQS:
> gd->gdma_context->max_num_cqs = val;
>
> while the table itself is sized once in mana_hwc_establish_channel() with
> vcalloc(gc->max_num_cqs, sizeof(*cq_table)). A later, larger value would
> inflate every bound test (cq_id >= gc->max_num_cqs here, cq->gdma_id >=
> gc->max_num_cqs in mana_create_txq()/mana_create_rxq(), cq->queue.id >=
> gc->max_num_cqs in mana_ib_install_cq_cb()) past the allocation, and a
> smaller one would make mana_gd_unpublish_cq() return false so
> mana_gd_destroy_cq() skips the grace period for a still-published CQ.
>
> The gdma.h comment added by this patch states the invariant as already
> holding ("max_num_cqs above is the size of cq_table and an upper bound on
> valid CQ indices for the table's lifetime"), which only becomes true with the last
> patch in this series, "net: mana: keep max_num_cqs immutable once cq_table
> is allocated" -- that one moves the handler to WRITE_ONCE(hwc-
> >hwc_init_max_num_cqs, val) and has
> mana_hwc_establish_channel() commit a single READ_ONCE() snapshot as
> both the vcalloc() size and gc->max_num_cqs. Should the documented
> contract land together with the change that establishes it, or reference it?
>
> > + /* A NULL entry is expected while a concurrent teardown
> > + * (e.g. ifdown or an MTU change) has unpublished this CQ but
> > + * not yet freed it; the completion is stale, so drop it
> > + * quietly rather than warning.
> > + */
> > + if (!cq)
> > + break;
> > + if (WARN_ON_ONCE(cq->type != GDMA_CQ || cq->id !=
> cq_id))
> > break;
> >
> > if (cq->cq.callback)
> > @@ -1050,18 +1070,47 @@ static void mana_gd_create_cq(const struct
> gdma_queue_spec *spec,
> > queue->cq.callback = spec->cq.callback; }
> >
> > -static void mana_gd_destroy_cq(struct gdma_context *gc,
> > - struct gdma_queue *queue)
> > +bool mana_gd_unpublish_cq(struct gdma_context *gc, struct gdma_queue
> > +*queue)
> > {
> > + struct gdma_queue __rcu **cq_table;
> > u32 id = queue->id;
> >
> > - if (id >= gc->max_num_cqs)
> > - return;
> > + /* No rcu_read_lock() here: unpublish runs only on the
> > + * CQ-destroy/teardown path, where the base cq_table is stable. See
> > + * the lifecycle note on gdma_context::cq_table in gdma.h for why the
> > + * "true" predicate is sound.
> > + */
> > + cq_table = rcu_dereference_protected(gc->cq_table, true);
> > + if (!cq_table || id >= gc->max_num_cqs)
> > + return false;
> > +
> > + /* Clear the slot only if it still refers to this queue. The
> > + * Ethernet two-pass teardown unpublishes the same index twice, a
> > + * grace period apart, and a CQ that legitimately recycled this id in
> > + * between (e.g. a new RDMA CQ via mana_ib_install_cq_cb()) must
> not
> > + * have its fresh entry wiped by the second pass.
> > + */
> > + if (rcu_access_pointer(cq_table[id]) != queue)
> > + return false;
> > +
> > + rcu_assign_pointer(cq_table[id], NULL);
> > + return true;
> > +}
> >
> > - if (!gc->cq_table[id])
> > +static void mana_gd_destroy_cq(struct gdma_context *gc,
> > + struct gdma_queue *queue)
> > +{
> > + /* A batched teardown may already have cleared the slot and taken
> the
> > + * grace period; then there is nothing left to wait for.
> > + */
> > + if (!mana_gd_unpublish_cq(gc, queue))
> > return;
> >
> > - gc->cq_table[id] = NULL;
> > + /* Wait for in-flight EQ handlers that may have loaded the old
> > + * pointer via rcu_dereference() to finish before the caller
> > + * frees the CQ memory.
> > + */
> > + synchronize_rcu();
> > }
>
> [Severity: Medium]
> mana_gd_unpublish_cq() returns false for two different states, and
> mana_gd_destroy_cq() treats both as "nothing left to wait for":
>
> a) the slot is already NULL because a batched pass cleared it and
> already took the grace period, and
>
> b) rcu_access_pointer(cq_table[id]) != queue, i.e. this queue's entry
> was overwritten by another publisher and no grace period has been
> taken for this pointer.
>
> In case (b), mana_gd_destroy_queue() then kfree()s the queue with no wait.
> Can an in-flight mana_gd_process_eqe() that already did
>
> cq = rcu_dereference(cq_table[cq_id]);
>
> on exactly that pointer go on to read cq->type / cq->id and call
> cq->cq.callback(cq->cq.context, cq) after the free?
>
> One route into state (b): for an RDMA kernel (kmem) CQ,
> mana_ib_install_cq_cb() publishes the entry, mana_ib_remove_cq_cb()
> returns early for kmem ("Then it will be cleaned and removed by the mana"),
> so the entry stays published across mana_ib_gd_destroy_cq() which releases
> the CQ id on the device, and is only cleared later via
> mana_ib_destroy_queue() -> mana_gd_destroy_queue() ->
> mana_gd_destroy_cq().
> If the device hands that id to an Ethernet CQ being created in that window,
> mana_create_txq()/mana_create_rxq() overwrite the slot unconditionally, and
> the identity test above then fails.
>
> Would it be safer to distinguish "we cleared our slot", "never published"
> and "our entry was overwritten", and still synchronize in the last case?
>
> I could not confirm the device actually recycles ids inside that window, so this
> may not be reachable in practice.
>
> > 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);
> >
> > return 0;
> > }
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > index 92bb55935c1c4..5d215981bba83 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > @@ -2427,12 +2427,18 @@ static void mana_deinit_txq(struct
> > mana_port_context *apc, struct mana_txq *txq)
> >
> > static void mana_destroy_txq(struct mana_port_context *apc) {
> > + struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
> > struct napi_struct *napi;
> > int i;
> >
> > if (!apc->tx_qp)
> > return;
> >
> > + /* Pass 1: quiesce each CQ on the device and clear its cq_table slot.
> > + * Taking one grace period below for the whole port avoids up to
> > + * apc->num_queues serialized synchronize_rcu() calls (one per CQ in
> > + * mana_gd_destroy_cq()) under RTNL on every teardown.
> > + */
>
> [Severity: Low]
> This isn't a bug, but is "the whole port" accurate here? RX and TX are batched
> independently: mana_destroy_rxqs() unpublishes only the RX CQs and calls
> synchronize_rcu(), then mana_destroy_txq() unpublishes only the TX CQs and
> calls synchronize_rcu() again, and mana_detach() runs both back to
> back:
>
> mana_destroy_rxqs(apc);
> mana_destroy_txq(apc);
>
> The same pairing happens on the attach error path. So each ifdown, MTU
> change or ring reconfigure takes two grace periods per port rather than one.
>
> The commit message says the same thing:
>
> Clear all of a port's CQ slots first and take a single grace period
> per teardown instead
>
> and the comment in mana_destroy_rxqs() reads "One grace period for the
> whole port instead of one per RXQ". Would "one grace period per RX batch
> and one per TX batch" describe the result more precisely?
>
> > for (i = 0; i < apc->num_queues; i++) {
> > if (!apc->tx_qp[i])
> > continue;
> > @@ -2448,8 +2454,24 @@ static void mana_destroy_txq(struct
> mana_port_context *apc)
> > apc->tx_qp[i]->txq.napi_initialized = false;
> > }
> >
> > - if (apc->tx_qp[i]->tx_object != INVALID_MANA_HANDLE)
> > - mana_destroy_wq_obj(apc, GDMA_SQ, apc->tx_qp[i]-
> >tx_object);
> > + if (apc->tx_qp[i]->tx_object != INVALID_MANA_HANDLE) {
> > + mana_destroy_wq_obj(apc, GDMA_SQ,
> > + apc->tx_qp[i]->tx_object);
> > + apc->tx_qp[i]->tx_object = INVALID_MANA_HANDLE;
> > + }
> > +
> > + if (apc->tx_qp[i]->tx_cq.gdma_cq)
> > + mana_gd_unpublish_cq(gc, apc->tx_qp[i]-
> >tx_cq.gdma_cq);
> > + }
> > +
> > + synchronize_rcu();
> > +
> > + /* Pass 2: the slots are clear, so mana_gd_destroy_cq() skips its own
> > + * grace period; free the CQ, the TXQ and the queue pair.
> > + */
> > + for (i = 0; i < apc->num_queues; i++) {
> > + if (!apc->tx_qp[i])
> > + continue;
> >
> > mana_deinit_cq(apc, &apc->tx_qp[i]->tx_cq);
> >
>
> [ ... ]
>
> > @@ -2987,16 +3054,31 @@ static void mana_destroy_rxqs(struct
> mana_port_context *apc)
> > struct mana_rxq *rxq;
> > u32 rxq_idx;
> >
> > - if (apc->rxqs) {
> > + if (!apc->rxqs)
> > + return;
> >
> > - for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
> > - rxq = apc->rxqs[rxq_idx];
> > - if (!rxq)
> > - continue;
> > + /* Pass 1: quiesce every RXQ's CQ and clear its cq_table slot. */
> > + for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
> > + rxq = apc->rxqs[rxq_idx];
> > + if (!rxq)
> > + continue;
> >
> > - mana_destroy_rxq(apc, rxq, true);
> > - apc->rxqs[rxq_idx] = NULL;
> > - }
> > + mana_quiesce_rxq(apc, rxq, true);
> > + }
> > +
> > + /* One grace period for the whole port instead of one per RXQ. */
> > + synchronize_rcu();
>
> [ ... ]
>
> > diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h index
> > 0c395917b2144..0f591029d363b 100644
> > --- a/include/net/mana/gdma.h
> > +++ b/include/net/mana/gdma.h
>
> [ ... ]
>
> > @@ -496,6 +525,12 @@ int mana_gd_create_mana_wq_cq(struct
> gdma_dev
> > *gd,
> >
> > void mana_gd_destroy_queue(struct gdma_context *gc, struct gdma_queue
> > *queue);
> >
> > +/* Clear a CQ's cq_table slot without waiting for a grace period.
> > +Batched
> > + * teardown paths clear several slots and then take a single
> > +synchronize_rcu();
> > + * single-CQ callers use mana_gd_destroy_cq() instead, which also waits.
> > + */
> > +bool mana_gd_unpublish_cq(struct gdma_context *gc, struct gdma_queue
> > +*queue);
>
> [Severity: Low]
> This isn't a bug, but the comment directs includers of this header to an API
> they cannot reach: mana_gd_destroy_cq() is static in gdma_main.c and is not
> declared anywhere. The reachable entry point is mana_gd_destroy_queue(),
> which is EXPORT_SYMBOL_NS("NET_MANA") and dispatches GDMA_CQ into
> the private helper.
>
> mana_gd_unpublish_cq() itself is non-static and declared here, but has no
> EXPORT_SYMBOL_NS("NET_MANA") unlike its header-declared siblings, so a
> mana_ib user of it would not link; its only users (gdma_main.c and
> mana_en.c) are both in mana.ko.
>
> Would keeping the declaration in a mana-private header, or exporting it and
> pointing the comment at mana_gd_destroy_queue(), avoid the situation this
> patch already runs into -- mana_ib_remove_cq_cb() open-coding the same
> unpublish-then-wait sequence with a different identity predicate (gdma_cq-
> >cq.context != cq) from the one in mana_gd_unpublish_cq()
> (rcu_access_pointer(cq_table[id]) != 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);
> --
> pw-bot: cr
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net v6 2/7] net: mana: fix HWC RQ/SQ buffer size swap
2026-08-11 2:38 [PATCH net v6 0/7] net: mana: HW channel reliability and hardening fixes Long Li
2026-08-11 2:38 ` [PATCH net v6 1/7] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy Long Li
@ 2026-08-11 2:38 ` Long Li
2026-08-12 23:46 ` Jakub Kicinski
2026-08-11 2:38 ` [PATCH net v6 3/7] net: mana: free HWC comp_buf after destroying the EQ Long Li
` (4 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Long Li @ 2026-08-11 2:38 UTC (permalink / raw)
To: Long Li, Konstantin Taranov, Jakub Kicinski, David S . Miller,
Paolo Abeni, Eric Dumazet, Andrew Lunn, Jason Gunthorpe,
Leon Romanovsky, Haiyang Zhang, K . Y . Srinivasan, Wei Liu,
Dexuan Cui, shradhagupta, Simon Horman, ernis, stephen
Cc: netdev, linux-rdma, linux-hyperv, linux-kernel
The HWC RQ receives responses and the SQ sends requests, but
mana_hwc_init_queues() sized the RQ with max_req_msg_size and the SQ with
max_resp_msg_size -- backwards. mana_hwc_rx_event_handler() also
recovered the RX slot index by dividing by max_req_msg_size instead of
the response size that strides the RQ buffer.
Both mistakes are latent today: the only caller passes
HW_CHANNEL_MAX_REQUEST_SIZE and HW_CHANNEL_MAX_RESPONSE_SIZE, which are
both 0x1000, and the queues are never re-created with the
hardware-reported sizes (those are only used by mana_hwc_test_channel()),
so max_req_msg_size == max_resp_msg_size on every path and there is no
observable overflow or mis-indexing. The bug would only surface if the
two sizes ever diverged.
Size the RQ by max_resp_msg_size and the SQ by max_req_msg_size, store
max_resp_msg_size in hw_channel_context, and use it as the RX slot stride
so the code is correct regardless of the two sizes.
Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v6:
- None.
Changes in v5:
- No code changes since v4 (resend as a standalone thread).
Changes in v4:
- Reworded the changelog as a latent-correctness fix (both message
sizes are 0x1000, so the swap has no observable overflow) and dropped
the note about hoisting the queue dimensions above
mana_hwc_create_cq().
drivers/net/ethernet/microsoft/mana/hw_channel.c | 7 ++++---
include/net/mana/hw_channel.h | 1 +
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 409e20caeccdcccec0f8972c95db69ebff7ce30c..3f011ebbe7b3e3de2665bfa164c678c89b9b3005 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -263,7 +263,7 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
/* Select the RX work request for virtual address and for reposting. */
rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
- rx_req_idx = (sge->address - rq_base_addr) / hwc->max_req_msg_size;
+ rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size;
if (rx_req_idx >= hwc_rxq->msg_buf->num_reqs) {
dev_err(hwc->dev, "HWC RX: wrong rx_req_idx=%llu, num_reqs=%u\n",
@@ -733,14 +733,14 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
goto out;
}
- err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_req_msg_size,
+ err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_resp_msg_size,
hwc->cq, &hwc->rxq);
if (err) {
dev_err(hwc->dev, "Failed to create HWC RQ: %d\n", err);
goto out;
}
- err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_resp_msg_size,
+ err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_req_msg_size,
hwc->cq, &hwc->txq);
if (err) {
dev_err(hwc->dev, "Failed to create HWC SQ: %d\n", err);
@@ -749,6 +749,7 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
hwc->num_inflight_msg = q_depth;
hwc->max_req_msg_size = max_req_msg_size;
+ hwc->max_resp_msg_size = max_resp_msg_size;
return 0;
out:
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net v6 2/7] net: mana: fix HWC RQ/SQ buffer size swap
2026-08-11 2:38 ` [PATCH net v6 2/7] net: mana: fix HWC RQ/SQ buffer size swap Long Li
@ 2026-08-12 23:46 ` Jakub Kicinski
2026-08-13 0:47 ` [EXTERNAL] " Long Li
0 siblings, 1 reply; 21+ messages in thread
From: Jakub Kicinski @ 2026-08-12 23:46 UTC (permalink / raw)
To: longli
Cc: Jakub Kicinski, kotaranov, davem, pabeni, edumazet, andrew+netdev,
jgg, leon, haiyangz, kys, wei.liu, decui, shradhagupta, horms,
ernis, stephen, netdev, linux-rdma, linux-hyperv, linux-kernel
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
The patch swaps the message sizes used to allocate the HWC RQ and SQ so the
RQ (which receives responses) is sized by max_resp_msg_size and the SQ
(which sends requests) by max_req_msg_size. It also stores
max_resp_msg_size in struct hw_channel_context and uses it as the RX slot
stride in mana_hwc_rx_event_handler(). The commit message notes both sizes
are 0x1000 today, so the previous swap had no observable effect.
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 409e20caeccdc..3f011ebbe7b3e 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -733,14 +733,14 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
> goto out;
> }
>
> - err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_req_msg_size,
> + err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_resp_msg_size,
> hwc->cq, &hwc->rxq);
> if (err) {
> dev_err(hwc->dev, "Failed to create HWC RQ: %d\n", err);
> goto out;
> }
>
> - err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_resp_msg_size,
> + err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_req_msg_size,
> hwc->cq, &hwc->txq);
> if (err) {
> dev_err(hwc->dev, "Failed to create HWC SQ: %d\n", err);
> @@ -749,6 +749,7 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
>
> hwc->num_inflight_msg = q_depth;
> hwc->max_req_msg_size = max_req_msg_size;
> + hwc->max_resp_msg_size = max_resp_msg_size;
[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at how these
two fields are produced and consumed, the hardware-reported sizes appear to
be dead: is the negotiated-size half of this interface still expected to do
something?
mana_hwc_establish_channel() hands the device-reported values back to its
caller:
*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;
and mana_hwc_create_channel() forwards them:
err = mana_hwc_test_channel(gc->hwc.driver_data,
HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH,
max_req_msg_size, max_resp_msg_size);
but the body of mana_hwc_test_channel() references neither
max_req_msg_size nor max_resp_msg_size, and nothing else applies them to
the already-created queues or to hw_channel_context. So the fields set
here in mana_hwc_init_queues() permanently hold the bootstrap constants
HW_CHANNEL_MAX_REQUEST_SIZE / HW_CHANNEL_MAX_RESPONSE_SIZE (both 0x1000),
which matches the commit message.
Two peer consumers do read hwc->max_req_msg_size as if it were the
hardware-negotiated request bound. mana_gd_create_dma_region() in
gdma_main.c uses it to reject oversized commands:
if (req_msg_size > hwc->max_req_msg_size)
return -EINVAL;
and mana_ib_gd_create_dma_region() in drivers/infiniband/hw/mana/main.c
sizes both its request buffer and its page-list capacity from it:
max_pgs_create_cmd =
(hwc->max_req_msg_size - sizeof(*create_req)) / sizeof(u64);
...
request_buf = kzalloc(hwc->max_req_msg_size, GFP_KERNEL);
If a device ever reported HWC_INIT_DATA_MAX_REQUEST smaller than 0x1000
(recorded into hwc->hwc_init_max_req_msg_size by
mana_hwc_init_event_handler()), would those callers keep building up to 4KB
commands that the hardware then rejects, and would a larger reported value
simply be ignored? There is no memory-safety consequence today since the
SQ buffer and that bound are the same 0x1000 value after this patch, but
would it be worth either applying the reported sizes or dropping the unused
mana_hwc_test_channel() parameters?
^ permalink raw reply [flat|nested] 21+ messages in thread* RE: [EXTERNAL] Re: [PATCH net v6 2/7] net: mana: fix HWC RQ/SQ buffer size swap
2026-08-12 23:46 ` Jakub Kicinski
@ 2026-08-13 0:47 ` Long Li
0 siblings, 0 replies; 21+ messages in thread
From: Long Li @ 2026-08-13 0:47 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Konstantin Taranov, davem@davemloft.net, pabeni@redhat.com,
edumazet@google.com, andrew+netdev@lunn.ch, jgg@ziepe.ca,
leon@kernel.org, Haiyang Zhang, KY Srinivasan, wei.liu@kernel.org,
Dexuan Cui, shradhagupta@linux.microsoft.com, horms@kernel.org,
ernis@linux.microsoft.com, stephen@networkplumber.org,
netdev@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
>
> 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
>
> The patch swaps the message sizes used to allocate the HWC RQ and SQ so the
> RQ (which receives responses) is sized by max_resp_msg_size and the SQ
> (which sends requests) by max_req_msg_size. It also stores
> max_resp_msg_size in struct hw_channel_context and uses it as the RX slot
> stride in mana_hwc_rx_event_handler(). The commit message notes both
> sizes are 0x1000 today, so the previous swap had no observable effect.
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > index 409e20caeccdc..3f011ebbe7b3e 100644
> > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
>
> [ ... ]
>
> > @@ -733,14 +733,14 @@ static int mana_hwc_init_queues(struct
> hw_channel_context *hwc, u16 q_depth,
> > goto out;
> > }
> >
> > - err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth,
> max_req_msg_size,
> > + err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth,
> max_resp_msg_size,
> > hwc->cq, &hwc->rxq);
> > if (err) {
> > dev_err(hwc->dev, "Failed to create HWC RQ: %d\n", err);
> > goto out;
> > }
> >
> > - err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth,
> max_resp_msg_size,
> > + err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth,
> max_req_msg_size,
> > hwc->cq, &hwc->txq);
> > if (err) {
> > dev_err(hwc->dev, "Failed to create HWC SQ: %d\n", err); @@
> -749,6
> > +749,7 @@ static int mana_hwc_init_queues(struct hw_channel_context
> > *hwc, u16 q_depth,
> >
> > hwc->num_inflight_msg = q_depth;
> > hwc->max_req_msg_size = max_req_msg_size;
> > + hwc->max_resp_msg_size = max_resp_msg_size;
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but while looking at how these two
> fields are produced and consumed, the hardware-reported sizes appear to be
> dead: is the negotiated-size half of this interface still expected to do
> something?
>
> mana_hwc_establish_channel() hands the device-reported values back to its
> caller:
>
> *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;
>
> and mana_hwc_create_channel() forwards them:
>
> err = mana_hwc_test_channel(gc->hwc.driver_data,
>
> HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH,
> max_req_msg_size, max_resp_msg_size);
>
> but the body of mana_hwc_test_channel() references neither
> max_req_msg_size nor max_resp_msg_size, and nothing else applies them to
> the already-created queues or to hw_channel_context. So the fields set here in
> mana_hwc_init_queues() permanently hold the bootstrap constants
> HW_CHANNEL_MAX_REQUEST_SIZE / HW_CHANNEL_MAX_RESPONSE_SIZE
> (both 0x1000), which matches the commit message.
>
> Two peer consumers do read hwc->max_req_msg_size as if it were the
> hardware-negotiated request bound. mana_gd_create_dma_region() in
> gdma_main.c uses it to reject oversized commands:
>
> if (req_msg_size > hwc->max_req_msg_size)
> return -EINVAL;
>
> and mana_ib_gd_create_dma_region() in drivers/infiniband/hw/mana/main.c
> sizes both its request buffer and its page-list capacity from it:
>
> max_pgs_create_cmd =
> (hwc->max_req_msg_size - sizeof(*create_req)) / sizeof(u64);
> ...
> request_buf = kzalloc(hwc->max_req_msg_size, GFP_KERNEL);
>
> If a device ever reported HWC_INIT_DATA_MAX_REQUEST smaller than 0x1000
> (recorded into hwc->hwc_init_max_req_msg_size by
> mana_hwc_init_event_handler()), would those callers keep building up to 4KB
> commands that the hardware then rejects, and would a larger reported value
> simply be ignored? There is no memory-safety consequence today since the
> SQ buffer and that bound are the same 0x1000 value after this patch, but
> would it be worth either applying the reported sizes or dropping the unused
> mana_hwc_test_channel() parameters?
Thanks. You're right, and it's pre-existing — this patch only fixes which of the two (today equal) sizes maps to the RQ vs SQ and the RX slot stride.
hwc->max_req_msg_size / hwc->max_resp_msg_size are set from the bootstrap constants: mana_hwc_create_channel() calls mana_hwc_init_queues() with HW_CHANNEL_MAX_REQUEST_SIZE / HW_CHANNEL_MAX_RESPONSE_SIZE (both 0x1000). The device-reported values from mana_hwc_establish_channel() only reach mana_hwc_test_channel() , which ignores them, so the consumers you found use the 0x1000 constant. With both equal there's no memory-safety consequence; a smaller reported size would get commands rejected by the device, a larger one would be capped conservatively — neither is reachable on current firmware.
I'd rather not fix this in this series: feeding the reported sizes back is a runtime behaviour change that needs its own justification and testing, and this series is scoped to the reliability fixes. It's a good candidate for a separate patch.
Long
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net v6 3/7] net: mana: free HWC comp_buf after destroying the EQ
2026-08-11 2:38 [PATCH net v6 0/7] net: mana: HW channel reliability and hardening fixes Long Li
2026-08-11 2:38 ` [PATCH net v6 1/7] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy Long Li
2026-08-11 2:38 ` [PATCH net v6 2/7] net: mana: fix HWC RQ/SQ buffer size swap Long Li
@ 2026-08-11 2:38 ` Long Li
2026-08-12 23:46 ` Jakub Kicinski
2026-08-11 2:38 ` [PATCH net v6 4/7] net: mana: validate hardware-supplied values in the HWC RX path Long Li
` (3 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Long Li @ 2026-08-11 2:38 UTC (permalink / raw)
To: Long Li, Konstantin Taranov, Jakub Kicinski, David S . Miller,
Paolo Abeni, Eric Dumazet, Andrew Lunn, Jason Gunthorpe,
Leon Romanovsky, Haiyang Zhang, K . Y . Srinivasan, Wei Liu,
Dexuan Cui, shradhagupta, Simon Horman, ernis, stephen
Cc: netdev, linux-rdma, linux-hyperv, linux-kernel
mana_hwc_destroy_cq() freed hwc_cq->comp_buf and destroyed the CQ before
the EQ. That was unsafe while the EQ was still registered: the EQ
interrupt handler reaches comp_buf via mana_hwc_comp_event() and the CQ
object (hwc->cq->gdma_cq) via mana_hwc_init_event_handler(), so a late
EQE dispatched after the free could touch freed memory.
Destroy the EQ first. mana_gd_destroy_queue() on the EQ deregisters its
IRQ and waits out in-flight handlers, fencing all EQE dispatch; only then
free the CQ and comp_buf.
Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v6:
- None.
Changes in v5:
- No code changes since v4 (resend as a standalone thread).
Changes in v4:
- No functional change since v3; the teardown-ordering guarantees this
patch relies on are made explicit in patch 5.
.../net/ethernet/microsoft/mana/hw_channel.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 3f011ebbe7b3e3de2665bfa164c678c89b9b3005..19896bb5ce1a4e365a3a8363b78b005242f84e8b 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -384,14 +384,24 @@ static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)
static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
{
- kfree(hwc_cq->comp_buf);
+ /* Destroy the EQ before the CQ. mana_gd_destroy_queue() on the EQ
+ * deregisters its IRQ and waits out in-flight handlers, fencing all
+ * EQE dispatch — both the completion path and HWC init/reconfig
+ * events. Freeing the CQ first would leave the EQ live and able to
+ * dispatch an event that dereferences hwc->cq->gdma_cq (e.g.
+ * mana_hwc_init_event_handler()) after it has been freed.
+ */
+ if (hwc_cq->gdma_eq)
+ mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
+ /* comp_buf is reached only by mana_hwc_comp_event(), invoked from
+ * the now-fenced EQ handler, so it is safe to free once the EQ and
+ * CQ are gone.
+ */
if (hwc_cq->gdma_cq)
mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
- if (hwc_cq->gdma_eq)
- mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
-
+ kfree(hwc_cq->comp_buf);
kfree(hwc_cq);
}
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net v6 3/7] net: mana: free HWC comp_buf after destroying the EQ
2026-08-11 2:38 ` [PATCH net v6 3/7] net: mana: free HWC comp_buf after destroying the EQ Long Li
@ 2026-08-12 23:46 ` Jakub Kicinski
2026-08-13 0:52 ` [EXTERNAL] " Long Li
0 siblings, 1 reply; 21+ messages in thread
From: Jakub Kicinski @ 2026-08-12 23:46 UTC (permalink / raw)
To: longli
Cc: Jakub Kicinski, kotaranov, davem, pabeni, edumazet, andrew+netdev,
jgg, leon, haiyangz, kys, wei.liu, decui, shradhagupta, horms,
ernis, stephen, netdev, linux-rdma, linux-hyperv, linux-kernel
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 reorders mana_hwc_destroy_cq() so the EQ is destroyed first, then the
CQ, and only afterwards hwc_cq->comp_buf is freed. The stated reason is
that mana_gd_destroy_queue() on the EQ deregisters the IRQ and waits out
in-flight handlers, fencing EQE dispatch before handler-reachable memory is
released.
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 3f011ebbe7b3e..19896bb5ce1a4 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -384,14 +384,24 @@ static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)
>
> static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
> {
> - kfree(hwc_cq->comp_buf);
> + /* Destroy the EQ before the CQ. mana_gd_destroy_queue() on the EQ
> + * deregisters its IRQ and waits out in-flight handlers, fencing all
> + * EQE dispatch — both the completion path and HWC init/reconfig
> + * events. Freeing the CQ first would leave the EQ live and able to
> + * dispatch an event that dereferences hwc->cq->gdma_cq (e.g.
> + * mana_hwc_init_event_handler()) after it has been freed.
> + */
> + if (hwc_cq->gdma_eq)
> + mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
[Severity: Medium]
This isn't a bug introduced by this patch, but the ordering in the only
caller does not yet match the invariant the new comment states. Does the
comment's claim that this point fences "all EQE dispatch — both the
completion path and HWC init/reconfig events" hold for the WQ objects the
same handlers touch?
At this commit mana_hwc_destroy_channel() frees the WQs before it gets
here:
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->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 msg_buf (the DMA buffer plus reqs[]), the
gdma_wq and the hwc_wq itself, and hwc->txq / hwc->rxq are left dangling
rather than set to NULL. The EQ is still on gic->eq_list and the HWC CQ is
still published in gc->cq_table at that point, since the only dispatch
fence (mana_gd_deregister_irq() -> list_del_rcu() + synchronize_rcu()) is
reached from the mana_gd_destroy_queue() call added here.
So can an EQE arriving in that window reach freed memory through
mana_gd_process_eqe() -> mana_hwc_comp_event() -> mana_hwc_rx_event_handler(),
which dereferences hwc->rxq and then:
if (WARN_ON_ONCE(hwc_rxq->gdma_wq->id != gdma_rxq_id))
return;
and later hwc_rxq->msg_buf->mem_info.dma_handle and
hwc_rxq->msg_buf->reqs[]? The tx side has only a NULL check:
WARN_ON_ONCE(!hwc_txq || hwc_txq->gdma_wq->id != gdma_txq_id);
which a freed-but-non-NULL pointer passes. mana_hwc_init_event_handler()
similarly touches hwc->rxq->gdma_wq->id and hwc->txq->gdma_wq.
For what it's worth, a later commit in this series, "net: mana: fix HWC
teardown safety with setup_active flag and destroy ordering", reorders
mana_hwc_destroy_channel() to call mana_hwc_destroy_cq() before
mana_hwc_destroy_wq() for txq and rxq, so the final tree state does not
have this window. Given this patch carries a Fixes: tag and would be a
stable backport candidate on its own, would it be worth folding the caller
reordering in here, or noting the dependency in the changelog?
>
> + /* comp_buf is reached only by mana_hwc_comp_event(), invoked from
> + * the now-fenced EQ handler, so it is safe to free once the EQ and
> + * CQ are gone.
> + */
> if (hwc_cq->gdma_cq)
> mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
>
> - if (hwc_cq->gdma_eq)
> - mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
> -
> + kfree(hwc_cq->comp_buf);
> kfree(hwc_cq);
> }
[Severity: Medium]
This is a pre-existing issue, but moving the EQ destroy to the front of
this function changes which ring is handed back to the DMA allocator first
on the failure path. Is that safe when the shared-memory teardown was
skipped?
mana_hwc_destroy_channel() gates the teardown on max_num_cqs and drops the
return value:
/* 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)
mana_smc_teardown_hwc(&gc->shm_channel, false);
If mana_smc_setup_hwc() succeeded (MST entries active, device programmed
with the HWC ring addresses) but mana_hwc_establish_channel() then timed
out waiting on hwc_init_eqe_comp, max_num_cqs stays zero, the teardown is
skipped, and mana_hwc_destroy_cq() still runs. The EQ ring is then the
first HWC ring released, via mana_gd_destroy_queue() ->
mana_gd_free_memory() -> dma_free_coherent().
For the HWC EQ, eq.disable_needed is false, so mana_gd_destroy_eq() issues
no device-side DISABLE_QUEUE, and mana_gd_deregister_irq() fences only the
driver's handlers. Can the device still post an EQE into those pages after
they are freed? shm_channel.c notes the dependency:
/* Waiting for the hardware to invalidate the MST entries before the
* driver frees the queue memory */
The same later commit, "net: mana: fix HWC teardown safety with
setup_active flag and destroy ordering", replaces the max_num_cqs gate with
hwc->setup_active set before mana_smc_setup_hwc(), and on teardown failure
returns early and leaks the HWC resources instead of freeing memory the
device may still write to. Should the ordering change here wait for that
gate, or at least mention the ordering dependency between the two patches?
^ permalink raw reply [flat|nested] 21+ messages in thread* RE: [EXTERNAL] Re: [PATCH net v6 3/7] net: mana: free HWC comp_buf after destroying the EQ
2026-08-12 23:46 ` Jakub Kicinski
@ 2026-08-13 0:52 ` Long Li
0 siblings, 0 replies; 21+ messages in thread
From: Long Li @ 2026-08-13 0:52 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Konstantin Taranov, davem@davemloft.net, pabeni@redhat.com,
edumazet@google.com, andrew+netdev@lunn.ch, jgg@ziepe.ca,
leon@kernel.org, Haiyang Zhang, KY Srinivasan, wei.liu@kernel.org,
Dexuan Cui, shradhagupta@linux.microsoft.com, horms@kernel.org,
ernis@linux.microsoft.com, stephen@networkplumber.org,
netdev@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
>
> 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 reorders mana_hwc_destroy_cq() so the EQ is destroyed first, then the
> CQ, and only afterwards hwc_cq->comp_buf is freed. The stated reason is that
> mana_gd_destroy_queue() on the EQ deregisters the IRQ and waits out in-
> flight handlers, fencing EQE dispatch before handler-reachable memory is
> released.
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > index 3f011ebbe7b3e..19896bb5ce1a4 100644
> > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > @@ -384,14 +384,24 @@ static void mana_hwc_comp_event(void *ctx,
> > struct gdma_queue *q_self)
> >
> > static void mana_hwc_destroy_cq(struct gdma_context *gc, struct
> > hwc_cq *hwc_cq) {
> > - kfree(hwc_cq->comp_buf);
> > + /* Destroy the EQ before the CQ. mana_gd_destroy_queue() on the
> EQ
> > + * deregisters its IRQ and waits out in-flight handlers, fencing all
> > + * EQE dispatch — both the completion path and HWC init/reconfig
> > + * events. Freeing the CQ first would leave the EQ live and able to
> > + * dispatch an event that dereferences hwc->cq->gdma_cq (e.g.
> > + * mana_hwc_init_event_handler()) after it has been freed.
> > + */
> > + if (hwc_cq->gdma_eq)
> > + mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but the ordering in the only caller
> does not yet match the invariant the new comment states. Does the
> comment's claim that this point fences "all EQE dispatch — both the
> completion path and HWC init/reconfig events" hold for the WQ objects the
> same handlers touch?
>
> At this commit mana_hwc_destroy_channel() frees the WQs before it gets
> here:
>
> 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->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 msg_buf (the DMA buffer plus reqs[]), the
> gdma_wq and the hwc_wq itself, and hwc->txq / hwc->rxq are left dangling
> rather than set to NULL. The EQ is still on gic->eq_list and the HWC CQ is still
> published in gc->cq_table at that point, since the only dispatch fence
> (mana_gd_deregister_irq() -> list_del_rcu() + synchronize_rcu()) is reached
> from the mana_gd_destroy_queue() call added here.
>
> So can an EQE arriving in that window reach freed memory through
> mana_gd_process_eqe() -> mana_hwc_comp_event() ->
> mana_hwc_rx_event_handler(), which dereferences hwc->rxq and then:
>
> if (WARN_ON_ONCE(hwc_rxq->gdma_wq->id != gdma_rxq_id))
> return;
>
> and later hwc_rxq->msg_buf->mem_info.dma_handle and hwc_rxq->msg_buf-
> >reqs[]? The tx side has only a NULL check:
>
> WARN_ON_ONCE(!hwc_txq || hwc_txq->gdma_wq->id !=
> gdma_txq_id);
>
> which a freed-but-non-NULL pointer passes. mana_hwc_init_event_handler()
> similarly touches hwc->rxq->gdma_wq->id and hwc->txq->gdma_wq.
>
> For what it's worth, a later commit in this series, "net: mana: fix HWC teardown
> safety with setup_active flag and destroy ordering", reorders
> mana_hwc_destroy_channel() to call mana_hwc_destroy_cq() before
> mana_hwc_destroy_wq() for txq and rxq, so the final tree state does not have
> this window. Given this patch carries a Fixes: tag and would be a stable
> backport candidate on its own, would it be worth folding the caller reordering
> in here, or noting the dependency in the changelog?
>
> >
> > + /* comp_buf is reached only by mana_hwc_comp_event(), invoked
> from
> > + * the now-fenced EQ handler, so it is safe to free once the EQ and
> > + * CQ are gone.
> > + */
> > if (hwc_cq->gdma_cq)
> > mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
> >
> > - if (hwc_cq->gdma_eq)
> > - mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
> > -
> > + kfree(hwc_cq->comp_buf);
> > kfree(hwc_cq);
> > }
>
> [Severity: Medium]
> This is a pre-existing issue, but moving the EQ destroy to the front of this
> function changes which ring is handed back to the DMA allocator first on the
> failure path. Is that safe when the shared-memory teardown was skipped?
>
> mana_hwc_destroy_channel() gates the teardown on max_num_cqs and drops
> the return value:
>
> /* 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)
> mana_smc_teardown_hwc(&gc->shm_channel, false);
>
> If mana_smc_setup_hwc() succeeded (MST entries active, device programmed
> with the HWC ring addresses) but mana_hwc_establish_channel() then timed
> out waiting on hwc_init_eqe_comp, max_num_cqs stays zero, the teardown is
> skipped, and mana_hwc_destroy_cq() still runs. The EQ ring is then the first
> HWC ring released, via mana_gd_destroy_queue() ->
> mana_gd_free_memory() -> dma_free_coherent().
>
> For the HWC EQ, eq.disable_needed is false, so mana_gd_destroy_eq() issues
> no device-side DISABLE_QUEUE, and mana_gd_deregister_irq() fences only the
> driver's handlers. Can the device still post an EQE into those pages after they
> are freed? shm_channel.c notes the dependency:
>
> /* Waiting for the hardware to invalidate the MST entries before the
> * driver frees the queue memory */
>
> The same later commit, "net: mana: fix HWC teardown safety with
> setup_active flag and destroy ordering", replaces the max_num_cqs gate with
> hwc->setup_active set before mana_smc_setup_hwc(), and on teardown
> hwc->failure
> returns early and leaks the HWC resources instead of freeing memory the
> device may still write to. Should the ordering change here wait for that gate, or
> at least mention the ordering dependency between the two patches?
Thanks. Both windows are pre-existing: patch 3 only reorders the EQ/CQ/comp_buf destroy inside mana_hwc_destroy_cq() and doesn't touch the caller ordering or the teardown gate, so it's no worse than the base at this commit. As you note, "net: mana: fix HWC teardown safety with setup_active flag and destroy ordering" completes both — it reorders mana_hwc_destroy_channel() to destroy the CQ (fencing the EQ) before the WQs, and replaces the max_num_cqs gate with setup_active + leak-on-teardown-failure.
The two patches are part of the same series and are applied together, so the final tree has no such window. I'd prefer to leave patch 3 as is.
Long
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net v6 4/7] net: mana: validate hardware-supplied values in the HWC RX path
2026-08-11 2:38 [PATCH net v6 0/7] net: mana: HW channel reliability and hardening fixes Long Li
` (2 preceding siblings ...)
2026-08-11 2:38 ` [PATCH net v6 3/7] net: mana: free HWC comp_buf after destroying the EQ Long Li
@ 2026-08-11 2:38 ` Long Li
2026-08-12 23:46 ` Jakub Kicinski
2026-08-11 2:38 ` [PATCH net v6 5/7] net: mana: fix HWC teardown safety with setup_active flag and destroy ordering Long Li
` (2 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Long Li @ 2026-08-11 2:38 UTC (permalink / raw)
To: Long Li, Konstantin Taranov, Jakub Kicinski, David S . Miller,
Paolo Abeni, Eric Dumazet, Andrew Lunn, Jason Gunthorpe,
Leon Romanovsky, Haiyang Zhang, K . Y . Srinivasan, Wei Liu,
Dexuan Cui, shradhagupta, Simon Horman, ernis, stephen
Cc: netdev, linux-rdma, linux-hyperv, linux-kernel
mana_hwc_rx_event_handler() consumed lengths and indices taken straight
from device DMA without validation. A buggy firmware or a malicious host
(in a confidential VM, where the DMA buffer is shared) could drive a
wrong or reused in-flight request to completion or index out of bounds.
Validate before use:
- snapshot the device-supplied inline_oob_size_div4 (read once through
its u32 flags word with READ_ONCE(), as it is a bit-field) and reject
any value other than the one the driver programs
(INLINE_OOB_SMALL_SIZE / 4), so a corrupted OOB size cannot move the
SGE out of the WQE before it is dereferenced;
- snapshot sge->address with READ_ONCE() and validate and use only the
snapshot, so the value that is bounds-checked is the value that is
used (the DMA buffer is host-writable in a confidential VM);
- match the SGE address against the address the driver posted for that
slot, not just an in-range index -- an in-range but wrong SGE would
otherwise truncate onto a neighbouring slot and read a stale response;
- reject a resp_len larger than the RX buffer.
As defence in depth, mana_hwc_handle_resp() also bounds-checks hwc_msg_id
before indexing the inflight bitmap and caller_ctx. Its only caller
already rejects the same range with the value it passes by value, so this
is a guard at the indexing site, not a reachable out-of-bounds.
Repost the RX WQE on every validation early-return that can still
identify its slot. The paths that cannot -- an unexpected OOB size, an
out-of-range index, or an SGE address matching no posted slot --
intentionally leak a single WQE rather than risk reposting the wrong one.
Because the HWC RQ depth is never replenished, count those leaks and,
once they exhaust the posted depth, log the terminal state and shorten
the command timeout so callers fail fast instead of draining silently.
A short response is no longer rejected in the handler: it reaches
mana_hwc_handle_resp(), whose mana_hwc_verify_resp_msg() fails it with
-EPROTO and completes the waiting sender, so a single malformed response
cannot convert into a channel-wide timeout.
Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v6:
- None.
Changes in v5:
- No code changes since v4 (resend as a standalone thread).
Changes in v4:
- Removed the short-response early return so a malformed response
reaches verify_resp_msg() -> -EPROTO and completes the sender instead
of hanging it.
- Account leaked RX WQEs and trip hwc_timeout on RQ exhaustion.
- Read inline_oob_size_div4 (through its u32 flags word, as it is a
bit-field) and sge->address with READ_ONCE() and reject any value
other than the one the driver programs.
- Reframed the msg_id check as defense in depth in the changelog.
.../net/ethernet/microsoft/mana/hw_channel.c | 116 ++++++++++++++++--
include/net/mana/hw_channel.h | 6 +
2 files changed, 111 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 19896bb5ce1a4e365a3a8363b78b005242f84e8b..5db8cfe2d84432940cc97d814f2cd6933a92caf9 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -83,6 +83,19 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
struct hwc_caller_ctx *ctx;
int err;
+ /* Defence in depth: the sole caller, mana_hwc_rx_event_handler(),
+ * already rejects msg_id >= hwc->num_inflight_msg with the value it
+ * passes here by value, so this cannot be reached out of range. Keep
+ * the guard at the indexing site so the bitmap and caller_ctx array
+ * are never indexed without a bound in view.
+ */
+ if (msg_id >= hwc->num_inflight_msg) {
+ dev_err(hwc->dev, "hwc_rx: msg_id %u >= max %u\n",
+ msg_id, hwc->num_inflight_msg);
+ mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
+ return;
+ }
+
if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
dev_err(hwc->dev, "hwc_rx: invalid msg_id = %u\n", msg_id);
mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
@@ -90,6 +103,18 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
}
ctx = hwc->caller_ctx + msg_id;
+
+ /* Reject responses larger than the RX DMA buffer — the SGE
+ * limits what hardware can DMA, so an oversized resp_len
+ * indicates a firmware bug. Fail rather than silently
+ * truncating.
+ */
+ if (resp_len > rx_req->buf_len) {
+ dev_err(hwc->dev, "HWC RX: resp_len %u > buf_len %u\n",
+ resp_len, rx_req->buf_len);
+ resp_len = 0;
+ }
+
err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
if (err)
goto out;
@@ -237,18 +262,39 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
}
}
+/* An RX WQE whose SGE the handler cannot trust is deliberately not
+ * reposted: reposting a slot we may have mis-identified could double-post
+ * a buffer the device still owns. Each such leak permanently lowers the
+ * RQ's posted depth, so once the whole depth is gone the channel can no
+ * longer receive responses. Make that terminal state explicit -- log it
+ * once and shorten the command timeout so callers fail fast -- rather than
+ * letting every later command drain its full timeout against a dead RQ.
+ */
+static void mana_hwc_rx_leak_wqe(struct hw_channel_context *hwc)
+{
+ if (++hwc->rx_leaked_wqe == hwc->rxq->queue_depth) {
+ dev_err(hwc->dev,
+ "HWC RX: RQ exhausted after %u leaked WQEs; channel unusable\n",
+ hwc->rx_leaked_wqe);
+ hwc->hwc_timeout = 1;
+ }
+}
+
static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
const struct hwc_rx_oob *rx_oob)
{
struct hw_channel_context *hwc = ctx;
struct hwc_wq *hwc_rxq = hwc->rxq;
struct hwc_work_request *rx_req;
+ struct gdma_wqe oob_snapshot;
struct gdma_resp_hdr *resp;
struct gdma_wqe *dma_oob;
struct gdma_queue *rq;
struct gdma_sge *sge;
u64 rq_base_addr;
u64 rx_req_idx;
+ u64 sge_addr;
+ u32 oob_div4;
u16 msg_id;
u8 *wqe;
@@ -259,28 +305,76 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
wqe = mana_gd_get_wqe_ptr(rq, rx_oob->wqe_offset / GDMA_WQE_BU_SIZE);
dma_oob = (struct gdma_wqe *)wqe;
- sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 * 4);
-
- /* Select the RX work request for virtual address and for reposting. */
+ /* inline_oob_size_div4 lives in device-accessible RQ memory (shared
+ * and host-writable in a confidential VM), so snapshot it once and
+ * validate and use only the snapshot. It is a bit-field, which
+ * READ_ONCE() cannot take the size of, so read the u32 flags word it
+ * shares through the union and extract the field from the local copy.
+ * The driver programs INLINE_OOB_SMALL_SIZE for every HWC RQ WQE via
+ * mana_gd_post_work_request(), so the only valid value is
+ * INLINE_OOB_SMALL_SIZE / 4, which puts the SGE at wqe + 16 inside
+ * this WQE's own BU. Reject anything else -- the slot cannot be
+ * trusted, so leak this RX WQE rather than repost the wrong one.
+ */
+ oob_snapshot.flags = READ_ONCE(dma_oob->flags);
+ oob_div4 = oob_snapshot.inline_oob_size_div4;
+ if (oob_div4 != INLINE_OOB_SMALL_SIZE / 4) {
+ dev_err(hwc->dev, "HWC RX: unexpected inline_oob_size_div4=%u\n",
+ oob_div4);
+ mana_hwc_rx_leak_wqe(hwc);
+ return;
+ }
+ sge = (struct gdma_sge *)(wqe + 8 + oob_div4 * 4);
+
+ /* Recover the originating RX slot from the SGE address. Snapshot it
+ * once, for the same shared-memory reason: of the three terms only
+ * sge_addr comes from device memory; rq_base_addr and
+ * max_resp_msg_size are driver-private. An in-range but wrong SGE
+ * would otherwise truncate onto a neighbouring slot, letting us read
+ * a stale response that could complete the wrong, reused in-flight
+ * request. Require the index in range AND the address to exactly
+ * match the value the driver posted for that slot.
+ */
+ sge_addr = READ_ONCE(sge->address);
rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
- rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size;
-
- if (rx_req_idx >= hwc_rxq->msg_buf->num_reqs) {
- dev_err(hwc->dev, "HWC RX: wrong rx_req_idx=%llu, num_reqs=%u\n",
- rx_req_idx, hwc_rxq->msg_buf->num_reqs);
+ rx_req_idx = (sge_addr - rq_base_addr) / hwc->max_resp_msg_size;
+
+ if (rx_req_idx >= hwc_rxq->queue_depth) {
+ /* Cannot identify the slot, so we cannot safely repost this
+ * WQE; leak it. An out-of-range index means a corrupted SGE
+ * from hardware or host tampering.
+ */
+ dev_err(hwc->dev, "HWC RX: SGE idx %llu out of range\n",
+ rx_req_idx);
+ mana_hwc_rx_leak_wqe(hwc);
return;
}
rx_req = &hwc_rxq->msg_buf->reqs[rx_req_idx];
+ if (sge_addr != (u64)rx_req->buf_sge_addr) {
+ /* In-range index but the address does not match what the
+ * driver posted for that slot; the same unrecoverable case,
+ * so leak this WQE rather than repost the wrong one.
+ */
+ dev_err(hwc->dev, "HWC RX: invalid SGE address %llx (idx=%llu)\n",
+ sge_addr, rx_req_idx);
+ mana_hwc_rx_leak_wqe(hwc);
+ return;
+ }
+
resp = (struct gdma_resp_hdr *)rx_req->buf_va;
- /* Read msg_id once from DMA buffer to prevent TOCTOU:
- * DMA memory is shared/unencrypted in CVMs - host can
- * modify it between reads.
+ /* Read msg_id once from the DMA buffer to prevent TOCTOU: DMA memory
+ * is shared/unencrypted in CVMs, so the host can modify it between
+ * reads. A short response is not rejected here; it is handed to
+ * mana_hwc_handle_resp() below, whose mana_hwc_verify_resp_msg()
+ * fails it with -EPROTO and completes the waiting sender, so one
+ * malformed response cannot stall the whole channel.
*/
msg_id = READ_ONCE(resp->response.hwc_msg_id);
if (msg_id >= hwc->num_inflight_msg) {
dev_err(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
+ mana_hwc_post_rx_wqe(hwc_rxq, rx_req);
return;
}
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 73671f479399ac296cf472ec6449e5e6b00a8515..787c6f96d5b576c0911e777bcc76673bba0dfb50 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -200,6 +200,12 @@ struct hw_channel_context {
u32 pf_dest_vrcq_id;
u32 hwc_timeout;
+ /* Count of RX WQEs deliberately not reposted after an untrusted SGE
+ * (see mana_hwc_rx_leak_wqe()); once it reaches the RQ depth the
+ * channel can no longer receive responses.
+ */
+ u32 rx_leaked_wqe;
+
struct hwc_caller_ctx *caller_ctx;
};
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net v6 4/7] net: mana: validate hardware-supplied values in the HWC RX path
2026-08-11 2:38 ` [PATCH net v6 4/7] net: mana: validate hardware-supplied values in the HWC RX path Long Li
@ 2026-08-12 23:46 ` Jakub Kicinski
2026-08-13 1:20 ` [EXTERNAL] " Long Li
0 siblings, 1 reply; 21+ messages in thread
From: Jakub Kicinski @ 2026-08-12 23:46 UTC (permalink / raw)
To: longli
Cc: Jakub Kicinski, kotaranov, davem, pabeni, edumazet, andrew+netdev,
jgg, leon, haiyangz, kys, wei.liu, decui, shradhagupta, horms,
ernis, stephen, netdev, linux-rdma, linux-hyperv, linux-kernel
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 values in the HWC RX completion
path: the inline OOB size and the SGE address are snapshotted and checked
before use, the recovered RX slot index is bounds checked and the SGE
address must match the one the driver posted for that slot, and an
oversized response length is rejected. Untrustworthy completions no
longer repost their RX WQE; those leaks are counted and, once they reach
the RQ depth, the command timeout is shortened.
No Link: tags are present in the commit header.
One question about the changelog before the diff:
[Severity: Medium]
> A short response is no longer rejected in the handler: it reaches
> mana_hwc_handle_resp(), whose mana_hwc_verify_resp_msg() fails it with
> -EPROTO and completes the waiting sender, so a single malformed response
> cannot convert into a channel-wide timeout.
Is this paragraph describing a change against the kernel tree, or against
an earlier revision of the patch? In the parent commit,
mana_hwc_rx_event_handler() has no response-length check between
resp = (struct gdma_resp_hdr *)rx_req->buf_va;
and the msg_id read, and this diff does not remove such a check, so there
is no short-response rejection in the handler being relaxed here.
Does the stated guarantee hold in the resulting code? hwc_msg_id sits at
bytes 10-11 of struct gdma_msg_hdr:
include/net/mana/gdma.h:
struct gdma_msg_hdr {
u32 hdr_type;
u32 msg_type;
u16 msg_version;
u16 hwc_msg_id;
u32 msg_size;
};
A response shorter than 12 bytes therefore does not contain the field the
handler reads. If the bytes read there are out of range, the handler
returns early and mana_hwc_verify_resp_msg() is never reached, so no
sender is completed. Could the paragraph be reworded to describe what the
code actually guarantees?
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 19896bb5ce1a4..5db8cfe2d8443 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -83,6 +83,19 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
> struct hwc_caller_ctx *ctx;
> int err;
>
> + /* Defence in depth: the sole caller, mana_hwc_rx_event_handler(),
> + * already rejects msg_id >= hwc->num_inflight_msg with the value it
> + * passes here by value, so this cannot be reached out of range. Keep
> + * the guard at the indexing site so the bitmap and caller_ctx array
> + * are never indexed without a bound in view.
> + */
> + if (msg_id >= hwc->num_inflight_msg) {
> + dev_err(hwc->dev, "hwc_rx: msg_id %u >= max %u\n",
> + msg_id, hwc->num_inflight_msg);
> + mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> + return;
> + }
> +
> if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
> dev_err(hwc->dev, "hwc_rx: invalid msg_id = %u\n", msg_id);
> mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
[Severity: High]
This isn't a bug introduced by this patch, but while the msg_id guard is
being added here, is the bare test_bit() on hwc->inflight_msg_res.map
enough to own the slot? At this commit the bit is tested without
inflight_msg_res.lock, and mana_hwc_send_request() releases the slot on
its timeout path:
out:
mana_hwc_put_msg_index(hwc, msg_id);
return err;
so a late or replayed response can find the bit set again for a reused
slot and then write through the previous caller's output_buf:
ctx = hwc->caller_ctx + msg_id;
...
memcpy(ctx->output_buf, resp_msg, resp_len);
The new mana_hwc_rx_leak_wqe() latch below makes the abandon-and-reuse
path much easier to reach. For completeness: this race is closed later in
the same series by "net: mana: fix stale HWC response after command
timeout", which adds per-slot lock, refcount and responded state and
NULLs output_buf under the lock on timeout, so no change is needed here if
the ordering of the series is kept.
[ ... ]
> @@ -237,18 +262,39 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
> }
> }
>
> +/* An RX WQE whose SGE the handler cannot trust is deliberately not
> + * reposted: reposting a slot we may have mis-identified could double-post
> + * a buffer the device still owns. Each such leak permanently lowers the
> + * RQ's posted depth, so once the whole depth is gone the channel can no
> + * longer receive responses. Make that terminal state explicit -- log it
> + * once and shorten the command timeout so callers fail fast -- rather than
> + * letting every later command drain its full timeout against a dead RQ.
> + */
> +static void mana_hwc_rx_leak_wqe(struct hw_channel_context *hwc)
> +{
> + if (++hwc->rx_leaked_wqe == hwc->rxq->queue_depth) {
> + dev_err(hwc->dev,
> + "HWC RX: RQ exhausted after %u leaked WQEs; channel unusable\n",
> + hwc->rx_leaked_wqe);
> + hwc->hwc_timeout = 1;
> + }
> +}
[Severity: High]
Is hwc->rxq->queue_depth ever greater than 1 here? The header defines:
include/net/mana/hw_channel.h:
#define HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH 1
and that is the only depth used:
mana_hwc_create_channel()
mana_hwc_init_queues(hwc, HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH, ...)
mana_hwc_create_wq(..., q_depth, ...)
hwc_wq->queue_depth = q_depth;
If so, ++hwc->rx_leaked_wqe == hwc->rxq->queue_depth is true on the first
call and hwc->hwc_timeout = 1 is latched immediately.
Does that mean one single unattributable completion, from any of the three
new early returns, permanently reduces every later HWC command to
wait_for_completion_timeout(&ctx->comp_event,
msecs_to_jiffies(hwc->hwc_timeout))
with hwc_timeout == 1, returning -ETIMEDOUT? The inputs that decide those
returns (rx_oob->wqe_offset, the WQE flags word, sge->address) all come
from the device-writable memory this patch treats as untrusted, and none of
the three paths establishes that a posted WQE was actually consumed, so a
single forged or stale completion would appear to disable vport
configuration, queries, RDMA setup and teardown for the life of the
device.
[Severity: High]
This isn't a bug introduced by this patch, but does the new
interrupt-context path make the following teardown behaviour reachable
from device input? Once hwc_timeout is 1, mana_gd_destroy_queue() still
frees the memory whether or not the destroy-region command succeeded:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_destroy_queue() {
...
mana_gd_destroy_dma_region(gc, gmi->dma_region_handle);
mana_gd_free_memory(gmi);
kfree(queue);
}
mana_gd_destroy_dma_region() is itself an HWC command, so with the timeout
latched every unbind frees DMA regions whose device-side mappings were
never torn down. mana_hwc_send_request() already collapses hwc_timeout to
1 after a genuine timeout, and this error return has always been ignored,
but previously that state required a real hardware stall and a failing
command in the log. Should the leak accounting request a service reset
instead of silently forcing the fail-fast timeout from an interrupt?
[Severity: Medium]
Is hwc_timeout a safe place to record this terminal state? The field has
other writers, and a host-supplied reconfig event overwrites it:
drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_init_event_handler() {
...
case HWC_DATA_CFG_HWC_TIMEOUT:
hwc->hwc_timeout = val;
...
}
Because the leak helper tests with == rather than >=, the shortening can
never be applied a second time, so after such an event every later command
drains its full timeout against a dead RQ again, which is the behaviour
the comment above the helper says it avoids.
In the other direction, the store is unconditional, unlike the existing
guard in mana_hwc_send_request():
if (hwc->hwc_timeout > 1)
hwc->hwc_timeout = 1;
so it can raise hwc_timeout from the 0 sentinel that mana_serv_reset()
sets:
/* HWC is not responding in this case, so don't wait */
hwc->hwc_timeout = 0;
which mana_need_log() also consumes:
if (hwc && hwc->hwc_timeout == 0)
return false;
Would a separate sticky flag (and hwc->rx_leaked_wqe being reset when the
channel is re-established) express this state better? As it stands
rx_leaked_wqe is never reset and no recovery is requested.
[Severity: Medium]
Should this store be annotated? mana_hwc_rx_leak_wqe() runs in HWC
interrupt context:
mana_gd_intr() -> EQ handler -> mana_hwc_comp_event()
-> mana_hwc_rx_event_handler() -> mana_hwc_rx_leak_wqe()
while the same field is read and read-modify-written from process context
with plain accesses in mana_hwc_send_request(), written by
mana_gd_query_hwc_timeout() through a request/response round trip, read
unlocked by mana_need_log() and used as a 0 sentinel by mana_serv_reset().
There is no common lock and no READ_ONCE()/WRITE_ONCE() here, so the
process-context update can be lost and the 0 sentinel can be resurrected
to 1 mid-reset. A structurally identical field in the same series,
hwc_init_max_num_cqs, is annotated with WRITE_ONCE()/READ_ONCE() and a
comment; was the omission here intentional?
> +
> static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
> const struct hwc_rx_oob *rx_oob)
> {
[ ... ]
> @@ -259,28 +305,76 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
> wqe = mana_gd_get_wqe_ptr(rq, rx_oob->wqe_offset / GDMA_WQE_BU_SIZE);
> dma_oob = (struct gdma_wqe *)wqe;
>
> - sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 * 4);
> -
> - /* Select the RX work request for virtual address and for reposting. */
> + /* inline_oob_size_div4 lives in device-accessible RQ memory (shared
> + * and host-writable in a confidential VM), so snapshot it once and
> + * validate and use only the snapshot. It is a bit-field, which
> + * READ_ONCE() cannot take the size of, so read the u32 flags word it
> + * shares through the union and extract the field from the local copy.
> + * The driver programs INLINE_OOB_SMALL_SIZE for every HWC RQ WQE via
> + * mana_gd_post_work_request(), so the only valid value is
> + * INLINE_OOB_SMALL_SIZE / 4, which puts the SGE at wqe + 16 inside
> + * this WQE's own BU. Reject anything else -- the slot cannot be
> + * trusted, so leak this RX WQE rather than repost the wrong one.
> + */
> + oob_snapshot.flags = READ_ONCE(dma_oob->flags);
> + oob_div4 = oob_snapshot.inline_oob_size_div4;
> + if (oob_div4 != INLINE_OOB_SMALL_SIZE / 4) {
> + dev_err(hwc->dev, "HWC RX: unexpected inline_oob_size_div4=%u\n",
> + oob_div4);
> + mana_hwc_rx_leak_wqe(hwc);
> + return;
> + }
> + sge = (struct gdma_sge *)(wqe + 8 + oob_div4 * 4);
> +
> + /* Recover the originating RX slot from the SGE address. Snapshot it
> + * once, for the same shared-memory reason: of the three terms only
> + * sge_addr comes from device memory; rq_base_addr and
> + * max_resp_msg_size are driver-private. An in-range but wrong SGE
> + * would otherwise truncate onto a neighbouring slot, letting us read
> + * a stale response that could complete the wrong, reused in-flight
> + * request. Require the index in range AND the address to exactly
> + * match the value the driver posted for that slot.
> + */
> + sge_addr = READ_ONCE(sge->address);
> rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
> - rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size;
> -
> - if (rx_req_idx >= hwc_rxq->msg_buf->num_reqs) {
> - dev_err(hwc->dev, "HWC RX: wrong rx_req_idx=%llu, num_reqs=%u\n",
> - rx_req_idx, hwc_rxq->msg_buf->num_reqs);
> + rx_req_idx = (sge_addr - rq_base_addr) / hwc->max_resp_msg_size;
> +
> + if (rx_req_idx >= hwc_rxq->queue_depth) {
> + /* Cannot identify the slot, so we cannot safely repost this
> + * WQE; leak it. An out-of-range index means a corrupted SGE
> + * from hardware or host tampering.
> + */
> + dev_err(hwc->dev, "HWC RX: SGE idx %llu out of range\n",
> + rx_req_idx);
> + mana_hwc_rx_leak_wqe(hwc);
> return;
> }
[Severity: Low]
Why was the bound changed from the array's own counter to a field of a
different structure? The index is used immediately afterwards to index
hwc_rxq->msg_buf->reqs[], which is declared as:
include/net/mana/hw_channel.h:
u32 num_reqs;
struct hwc_work_request reqs[] __counted_by(num_reqs);
so num_reqs is the annotated bound for that array, while queue_depth is a
u16 in struct hwc_wq. The two are equal only by construction:
mana_hwc_create_wq() sets hwc_wq->queue_depth = q_depth and separately
calls mana_hwc_alloc_dma_buf(hwc, q_depth, ...), which allocates
reqs[q_depth] and sets dma_buf->num_reqs = q_depth. Nothing ties them
together, num_reqs becomes write-only in this file, and the change isn't
mentioned in the changelog even though every other added validation is.
Would keeping the check against msg_buf->num_reqs (and keeping num_reqs in
the error message) be preferable?
>
> rx_req = &hwc_rxq->msg_buf->reqs[rx_req_idx];
> + if (sge_addr != (u64)rx_req->buf_sge_addr) {
> + /* In-range index but the address does not match what the
> + * driver posted for that slot; the same unrecoverable case,
> + * so leak this WQE rather than repost the wrong one.
> + */
> + dev_err(hwc->dev, "HWC RX: invalid SGE address %llx (idx=%llu)\n",
> + sge_addr, rx_req_idx);
> + mana_hwc_rx_leak_wqe(hwc);
> + return;
> + }
> +
[Severity: Medium]
This isn't a problem introduced by this patch, but does the address match
establish the invariant the helper's comment relies on ("reposting a slot
we may have mis-identified could double-post a buffer the device still
owns")? The check only shows that the WQE named by the device-supplied
completion offset still contains the OOB size and SGE address the driver
once wrote there, and that offset is masked into the ring rather than
rejected:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_get_wqe_ptr() {
u32 offset = (wqe_offset * GDMA_WQE_BU_SIZE) & (wq->queue_size - 1);
WARN_ON_ONCE((offset + GDMA_WQE_BU_SIZE) > wq->queue_size);
...
}
RQ WQE memory keeps those driver-written values after the device consumes
the WQE, and mana_hwc_comp_event() dispatches on completions[i].is_sq
alone, without looking at rx_oob->vendor_err or eom, so a spurious, error
or replayed completion naming a still-owned slot passes both new checks
and gets reposted. The HWC RQ is created with spec.monitor_avl_buf =
false, so mana_gd_post_work_request() does no free-space check and
wq->head simply advances past the number of buffers the driver owns. Is
some per-slot posted/consumed state needed for the address match to mean
"this WQE was just consumed for this buffer"?
> resp = (struct gdma_resp_hdr *)rx_req->buf_va;
>
> - /* Read msg_id once from DMA buffer to prevent TOCTOU:
> - * DMA memory is shared/unencrypted in CVMs - host can
> - * modify it between reads.
> + /* Read msg_id once from the DMA buffer to prevent TOCTOU: DMA memory
> + * is shared/unencrypted in CVMs, so the host can modify it between
> + * reads. A short response is not rejected here; it is handed to
> + * mana_hwc_handle_resp() below, whose mana_hwc_verify_resp_msg()
> + * fails it with -EPROTO and completes the waiting sender, so one
> + * malformed response cannot stall the whole channel.
> */
> msg_id = READ_ONCE(resp->response.hwc_msg_id);
> if (msg_id >= hwc->num_inflight_msg) {
> dev_err(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
> + mana_hwc_post_rx_wqe(hwc_rxq, rx_req);
> return;
> }
[Severity: Low]
Should these messages be rate limited? All four rejection paths in this
handler use plain dev_err() from hardirq context with device-supplied
values ("unexpected inline_oob_size_div4=%u", "SGE idx %llu out of range",
"invalid SGE address %llx (idx=%llu)", "wrong msg_id=%u"). This path
previously returned without reposting, which bounded how often a host
could drive it; with the added mana_hwc_post_rx_wqe() a host that keeps
fabricating completions can repeat it indefinitely and flood the log,
which is unpleasant with a serial console. dev_err_ratelimited(), or the
counter scheme used for the leak accounting, would avoid that.
^ permalink raw reply [flat|nested] 21+ messages in thread* RE: [EXTERNAL] Re: [PATCH net v6 4/7] net: mana: validate hardware-supplied values in the HWC RX path
2026-08-12 23:46 ` Jakub Kicinski
@ 2026-08-13 1:20 ` Long Li
0 siblings, 0 replies; 21+ messages in thread
From: Long Li @ 2026-08-13 1:20 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Konstantin Taranov, davem@davemloft.net, pabeni@redhat.com,
edumazet@google.com, andrew+netdev@lunn.ch, jgg@ziepe.ca,
leon@kernel.org, Haiyang Zhang, KY Srinivasan, wei.liu@kernel.org,
Dexuan Cui, shradhagupta@linux.microsoft.com, horms@kernel.org,
ernis@linux.microsoft.com, stephen@networkplumber.org,
netdev@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
> 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 values in the HWC RX completion
> path: the inline OOB size and the SGE address are snapshotted and checked
> before use, the recovered RX slot index is bounds checked and the SGE address
> must match the one the driver posted for that slot, and an oversized response
> length is rejected. Untrustworthy completions no longer repost their RX WQE;
> those leaks are counted and, once they reach the RQ depth, the command
> timeout is shortened.
>
> No Link: tags are present in the commit header.
>
> One question about the changelog before the diff:
>
> [Severity: Medium]
>
> > A short response is no longer rejected in the handler: it reaches
> > mana_hwc_handle_resp(), whose mana_hwc_verify_resp_msg() fails it with
> > -EPROTO and completes the waiting sender, so a single malformed
> > response cannot convert into a channel-wide timeout.
>
> Is this paragraph describing a change against the kernel tree, or against an
> earlier revision of the patch? In the parent commit,
> mana_hwc_rx_event_handler() has no response-length check between
>
> resp = (struct gdma_resp_hdr *)rx_req->buf_va;
>
> and the msg_id read, and this diff does not remove such a check, so there is no
> short-response rejection in the handler being relaxed here.
>
> Does the stated guarantee hold in the resulting code? hwc_msg_id sits at
> bytes 10-11 of struct gdma_msg_hdr:
>
> include/net/mana/gdma.h:
> struct gdma_msg_hdr {
> u32 hdr_type;
> u32 msg_type;
> u16 msg_version;
> u16 hwc_msg_id;
> u32 msg_size;
> };
>
> A response shorter than 12 bytes therefore does not contain the field the
> handler reads. If the bytes read there are out of range, the handler returns
> early and mana_hwc_verify_resp_msg() is never reached, so no sender is
> completed. Could the paragraph be reworded to describe what the code
> actually guarantees?
You're right -- a <12-byte response has no hwc_msg_id, so the handler returns early at the msg_id bound check and never reaches mana_hwc_verify_resp_msg(). That paragraph is dropped in v7; the changelog no longer claims a short response is handled downstream.
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > index 19896bb5ce1a4..5db8cfe2d8443 100644
> > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > @@ -83,6 +83,19 @@ static void mana_hwc_handle_resp(struct
> hw_channel_context *hwc, u32 resp_len,
> > struct hwc_caller_ctx *ctx;
> > int err;
> >
> > + /* Defence in depth: the sole caller, mana_hwc_rx_event_handler(),
> > + * already rejects msg_id >= hwc->num_inflight_msg with the value it
> > + * passes here by value, so this cannot be reached out of range. Keep
> > + * the guard at the indexing site so the bitmap and caller_ctx array
> > + * are never indexed without a bound in view.
> > + */
> > + if (msg_id >= hwc->num_inflight_msg) {
> > + dev_err(hwc->dev, "hwc_rx: msg_id %u >= max %u\n",
> > + msg_id, hwc->num_inflight_msg);
> > + mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> > + return;
> > + }
> > +
> > if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
> > dev_err(hwc->dev, "hwc_rx: invalid msg_id = %u\n", msg_id);
> > mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
>
> [Severity: High]
>
> This isn't a bug introduced by this patch, but while the msg_id guard is being
> added here, is the bare test_bit() on hwc->inflight_msg_res.map enough to
> own the slot? At this commit the bit is tested without inflight_msg_res.lock,
> and mana_hwc_send_request() releases the slot on its timeout path:
>
> out:
> mana_hwc_put_msg_index(hwc, msg_id);
> return err;
>
> so a late or replayed response can find the bit set again for a reused slot and
> then write through the previous caller's output_buf:
>
> ctx = hwc->caller_ctx + msg_id;
> ...
> memcpy(ctx->output_buf, resp_msg, resp_len);
>
> The new mana_hwc_rx_leak_wqe() latch below makes the abandon-and-reuse
> path much easier to reach. For completeness: this race is closed later in the
> same series by "net: mana: fix stale HWC response after command timeout",
> which adds per-slot lock, refcount and responded state and NULLs output_buf
> under the lock on timeout, so no change is needed here if the ordering of the
> series is kept.
Agreed, and thanks for confirming the fix. The stale-response patch adds a per-slot lock, a refcount and a "responded" flag, and NULLs output_buf under the lock on timeout, so the reuse race is closed in the final tree. No change here.
>
> [ ... ]
>
> > @@ -237,18 +262,39 @@ static void mana_hwc_init_event_handler(void
> *ctx, struct gdma_queue *q_self,
> > }
> > }
> >
> > +/* An RX WQE whose SGE the handler cannot trust is deliberately not
> > + * reposted: reposting a slot we may have mis-identified could
> > +double-post
> > + * a buffer the device still owns. Each such leak permanently lowers
> > +the
> > + * RQ's posted depth, so once the whole depth is gone the channel can
> > +no
> > + * longer receive responses. Make that terminal state explicit --
> > +log it
> > + * once and shorten the command timeout so callers fail fast --
> > +rather than
> > + * letting every later command drain its full timeout against a dead RQ.
> > + */
> > +static void mana_hwc_rx_leak_wqe(struct hw_channel_context *hwc) {
> > + if (++hwc->rx_leaked_wqe == hwc->rxq->queue_depth) {
> > + dev_err(hwc->dev,
> > + "HWC RX: RQ exhausted after %u leaked WQEs;
> channel unusable\n",
> > + hwc->rx_leaked_wqe);
> > + hwc->hwc_timeout = 1;
> > + }
> > +}
>
> [Severity: High]
>
> Is hwc->rxq->queue_depth ever greater than 1 here? The header defines:
>
> include/net/mana/hw_channel.h:
> #define HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH 1
>
> and that is the only depth used:
>
> mana_hwc_create_channel()
> mana_hwc_init_queues(hwc,
> HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH, ...)
> mana_hwc_create_wq(..., q_depth, ...)
> hwc_wq->queue_depth = q_depth;
>
> If so, ++hwc->rx_leaked_wqe == hwc->rxq->queue_depth is true on the first
> call and hwc->hwc_timeout = 1 is latched immediately.
>
> Does that mean one single unattributable completion, from any of the three
> new early returns, permanently reduces every later HWC command to
>
> wait_for_completion_timeout(&ctx->comp_event,
> msecs_to_jiffies(hwc->hwc_timeout))
>
> with hwc_timeout == 1, returning -ETIMEDOUT? The inputs that decide those
> returns (rx_oob->wqe_offset, the WQE flags word, sge->address) all come
> from the device-writable memory this patch treats as untrusted, and none of
> the three paths establishes that a posted WQE was actually consumed, so a
> single forged or stale completion would appear to disable vport configuration,
> queries, RDMA setup and teardown for the life of the device.
Disagree.
Depth is 1 today, so yes, one un-attributable completion latches it. That is intended: the three paths leak the WQE rather than repost it, and at depth 1 the RQ then has no posted buffer, so it genuinely cannot receive another response -- shortening the timeout just makes later commands fail fast against a dead RQ instead of each draining the full timeout. The alternative, reposting a WQE we could not attribute, is worse: it could double-post a buffer the device still owns.
>
> [Severity: High]
>
> This isn't a bug introduced by this patch, but does the new interrupt-context
> path make the following teardown behaviour reachable from device input?
> Once hwc_timeout is 1, mana_gd_destroy_queue() still frees the memory
> whether or not the destroy-region command succeeded:
>
> drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_destroy_queue
> () {
> ...
> mana_gd_destroy_dma_region(gc, gmi->dma_region_handle);
> mana_gd_free_memory(gmi);
> kfree(queue);
> }
>
> mana_gd_destroy_dma_region() is itself an HWC command, so with the
> timeout latched every unbind frees DMA regions whose device-side mappings
> were never torn down. mana_hwc_send_request() already collapses
> hwc_timeout to
> 1 after a genuine timeout, and this error return has always been ignored, but
> previously that state required a real hardware stall and a failing command in the
> log. Should the leak accounting request a service reset instead of silently
> forcing the fail-fast timeout from an interrupt?
Disagree.
mana_gd_destroy_queue() freeing the region regardless of the command result is pre-existing and independent of this patch, so I'd keep it out of this series. Requesting a service reset from hardirq is a heavier, separate design choice; the fail-fast timeout is the minimal response to a permanently-exhausted RQ. There are other reset mechanism used by the driver.
>
> [Severity: Medium]
>
> Is hwc_timeout a safe place to record this terminal state? The field has other
> writers, and a host-supplied reconfig event overwrites it:
>
> drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_init_event_h
> andler() {
> ...
> case HWC_DATA_CFG_HWC_TIMEOUT:
> hwc->hwc_timeout = val;
> ...
> }
>
> Because the leak helper tests with == rather than >=, the shortening can never
> be applied a second time, so after such an event every later command drains its
> full timeout against a dead RQ again, which is the behaviour the comment
> above the helper says it avoids.
>
> In the other direction, the store is unconditional, unlike the existing guard in
> mana_hwc_send_request():
>
> if (hwc->hwc_timeout > 1)
> hwc->hwc_timeout = 1;
>
> so it can raise hwc_timeout from the 0 sentinel that mana_serv_reset()
> sets:
>
> /* HWC is not responding in this case, so don't wait */
> hwc->hwc_timeout = 0;
>
> which mana_need_log() also consumes:
>
> if (hwc && hwc->hwc_timeout == 0)
> return false;
>
> Would a separate sticky flag (and hwc->rx_leaked_wqe being reset when the
> channel is re-established) express this state better? As it stands rx_leaked_wqe
> is never reset and no recovery is requested.
Disagree.
The harmful direction -- resurrecting the serv_reset() 0 sentinel -- is guarded in v7: the store is "if (hwc->hwc_timeout > 1) hwc->hwc_timeout = 1;", matching mana_hwc_send_request(). The == vs >= and the reconfig overwrite are benign for what is a terminal state, so I'd rather not add a separate sticky field for it.
>
> [Severity: Medium]
>
> Should this store be annotated? mana_hwc_rx_leak_wqe() runs in HWC
> interrupt context:
>
> mana_gd_intr() -> EQ handler -> mana_hwc_comp_event()
> -> mana_hwc_rx_event_handler() -> mana_hwc_rx_leak_wqe()
>
> while the same field is read and read-modify-written from process context with
> plain accesses in mana_hwc_send_request(), written by
> mana_gd_query_hwc_timeout() through a request/response round trip, read
> unlocked by mana_need_log() and used as a 0 sentinel by mana_serv_reset().
> There is no common lock and no READ_ONCE()/WRITE_ONCE() here, so the
> process-context update can be lost and the 0 sentinel can be resurrected to 1
> mid-reset. A structurally identical field in the same series,
> hwc_init_max_num_cqs, is annotated with WRITE_ONCE()/READ_ONCE() and
> a comment; was the omission here intentional?
Fix in v7.
>
> > +
> > static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
> > const struct hwc_rx_oob *rx_oob) {
>
> [ ... ]
>
> > @@ -259,28 +305,76 @@ static void mana_hwc_rx_event_handler(void *ctx,
> u32 gdma_rxq_id,
> > wqe = mana_gd_get_wqe_ptr(rq, rx_oob->wqe_offset /
> GDMA_WQE_BU_SIZE);
> > dma_oob = (struct gdma_wqe *)wqe;
> >
> > - sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 *
> 4);
> > -
> > - /* Select the RX work request for virtual address and for reposting. */
> > + /* inline_oob_size_div4 lives in device-accessible RQ memory (shared
> > + * and host-writable in a confidential VM), so snapshot it once and
> > + * validate and use only the snapshot. It is a bit-field, which
> > + * READ_ONCE() cannot take the size of, so read the u32 flags word it
> > + * shares through the union and extract the field from the local copy.
> > + * The driver programs INLINE_OOB_SMALL_SIZE for every HWC RQ
> WQE via
> > + * mana_gd_post_work_request(), so the only valid value is
> > + * INLINE_OOB_SMALL_SIZE / 4, which puts the SGE at wqe + 16 inside
> > + * this WQE's own BU. Reject anything else -- the slot cannot be
> > + * trusted, so leak this RX WQE rather than repost the wrong one.
> > + */
> > + oob_snapshot.flags = READ_ONCE(dma_oob->flags);
> > + oob_div4 = oob_snapshot.inline_oob_size_div4;
> > + if (oob_div4 != INLINE_OOB_SMALL_SIZE / 4) {
> > + dev_err(hwc->dev, "HWC RX: unexpected
> inline_oob_size_div4=%u\n",
> > + oob_div4);
> > + mana_hwc_rx_leak_wqe(hwc);
> > + return;
> > + }
> > + sge = (struct gdma_sge *)(wqe + 8 + oob_div4 * 4);
> > +
> > + /* Recover the originating RX slot from the SGE address. Snapshot it
> > + * once, for the same shared-memory reason: of the three terms only
> > + * sge_addr comes from device memory; rq_base_addr and
> > + * max_resp_msg_size are driver-private. An in-range but wrong SGE
> > + * would otherwise truncate onto a neighbouring slot, letting us read
> > + * a stale response that could complete the wrong, reused in-flight
> > + * request. Require the index in range AND the address to exactly
> > + * match the value the driver posted for that slot.
> > + */
> > + sge_addr = READ_ONCE(sge->address);
> > rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
> > - rx_req_idx = (sge->address - rq_base_addr) / hwc-
> >max_resp_msg_size;
> > -
> > - if (rx_req_idx >= hwc_rxq->msg_buf->num_reqs) {
> > - dev_err(hwc->dev, "HWC RX: wrong rx_req_idx=%llu,
> num_reqs=%u\n",
> > - rx_req_idx, hwc_rxq->msg_buf->num_reqs);
> > + rx_req_idx = (sge_addr - rq_base_addr) / hwc->max_resp_msg_size;
> > +
> > + if (rx_req_idx >= hwc_rxq->queue_depth) {
> > + /* Cannot identify the slot, so we cannot safely repost this
> > + * WQE; leak it. An out-of-range index means a corrupted SGE
> > + * from hardware or host tampering.
> > + */
> > + dev_err(hwc->dev, "HWC RX: SGE idx %llu out of range\n",
> > + rx_req_idx);
> > + mana_hwc_rx_leak_wqe(hwc);
> > return;
> > }
>
> [Severity: Low]
>
> Why was the bound changed from the array's own counter to a field of a
> different structure? The index is used immediately afterwards to index
> hwc_rxq->msg_buf->reqs[], which is declared as:
>
> include/net/mana/hw_channel.h:
> u32 num_reqs;
> struct hwc_work_request reqs[] __counted_by(num_reqs);
>
> so num_reqs is the annotated bound for that array, while queue_depth is a
> u16 in struct hwc_wq. The two are equal only by construction:
> mana_hwc_create_wq() sets hwc_wq->queue_depth = q_depth and
> separately calls mana_hwc_alloc_dma_buf(hwc, q_depth, ...), which allocates
> reqs[q_depth] and sets dma_buf->num_reqs = q_depth. Nothing ties them
> together, num_reqs becomes write-only in this file, and the change isn't
> mentioned in the changelog even though every other added validation is.
> Would keeping the check against msg_buf->num_reqs (and keeping num_reqs
> in the error message) be preferable?
Yes -- fixed in v7. The bound is back to hwc_rxq->msg_buf->num_reqs (the __counted_by bound for reqs[]), and num_reqs is in the error message.
>
> >
> > rx_req = &hwc_rxq->msg_buf->reqs[rx_req_idx];
> > + if (sge_addr != (u64)rx_req->buf_sge_addr) {
> > + /* In-range index but the address does not match what the
> > + * driver posted for that slot; the same unrecoverable case,
> > + * so leak this WQE rather than repost the wrong one.
> > + */
> > + dev_err(hwc->dev, "HWC RX: invalid SGE address %llx
> (idx=%llu)\n",
> > + sge_addr, rx_req_idx);
> > + mana_hwc_rx_leak_wqe(hwc);
> > + return;
> > + }
> > +
>
> [Severity: Medium]
>
> This isn't a problem introduced by this patch, but does the address match
> establish the invariant the helper's comment relies on ("reposting a slot we
> may have mis-identified could double-post a buffer the device still owns")?
> The check only shows that the WQE named by the device-supplied completion
> offset still contains the OOB size and SGE address the driver once wrote there,
> and that offset is masked into the ring rather than
> rejected:
>
> drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_get_wqe_ptr()
> {
> u32 offset = (wqe_offset * GDMA_WQE_BU_SIZE) & (wq->queue_size -
> 1);
>
> WARN_ON_ONCE((offset + GDMA_WQE_BU_SIZE) > wq->queue_size);
> ...
> }
>
> RQ WQE memory keeps those driver-written values after the device consumes
> the WQE, and mana_hwc_comp_event() dispatches on completions[i].is_sq
> alone, without looking at rx_oob->vendor_err or eom, so a spurious, error or
> replayed completion naming a still-owned slot passes both new checks and
> gets reposted. The HWC RQ is created with spec.monitor_avl_buf = false, so
> mana_gd_post_work_request() does no free-space check and
> wq->head simply advances past the number of buffers the driver owns. Is
> some per-slot posted/consumed state needed for the address match to mean
> "this WQE was just consumed for this buffer"?
>
> > resp = (struct gdma_resp_hdr *)rx_req->buf_va;
> >
> > - /* Read msg_id once from DMA buffer to prevent TOCTOU:
> > - * DMA memory is shared/unencrypted in CVMs - host can
> > - * modify it between reads.
> > + /* Read msg_id once from the DMA buffer to prevent TOCTOU: DMA
> memory
> > + * is shared/unencrypted in CVMs, so the host can modify it between
> > + * reads. A short response is not rejected here; it is handed to
> > + * mana_hwc_handle_resp() below, whose
> mana_hwc_verify_resp_msg()
> > + * fails it with -EPROTO and completes the waiting sender, so one
> > + * malformed response cannot stall the whole channel.
> > */
> > msg_id = READ_ONCE(resp->response.hwc_msg_id);
> > if (msg_id >= hwc->num_inflight_msg) {
> > dev_err(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
> > + mana_hwc_post_rx_wqe(hwc_rxq, rx_req);
> > return;
> > }
>
> [Severity: Low]
>
> Should these messages be rate limited? All four rejection paths in this handler
> use plain dev_err() from hardirq context with device-supplied values
> ("unexpected inline_oob_size_div4=%u", "SGE idx %llu out of range", "invalid
> SGE address %llx (idx=%llu)", "wrong msg_id=%u"). This path previously
> returned without reposting, which bounded how often a host could drive it;
> with the added mana_hwc_post_rx_wqe() a host that keeps fabricating
> completions can repeat it indefinitely and flood the log, which is unpleasant
> with a serial console. dev_err_ratelimited(), or the counter scheme used for
> the leak accounting, would avoid that.
Good point -- fixed in v7. The reposting paths (wrong msg_id, and the msg_id/resp_len rejections in mana_hwc_handle_resp()) now use dev_err_ratelimited(), so a host can't flood the log. The three leak paths don't repost and are bounded by the RQ depth, so they keep a single dev_err().
Thanks,
Long
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net v6 5/7] net: mana: fix HWC teardown safety with setup_active flag and destroy ordering
2026-08-11 2:38 [PATCH net v6 0/7] net: mana: HW channel reliability and hardening fixes Long Li
` (3 preceding siblings ...)
2026-08-11 2:38 ` [PATCH net v6 4/7] net: mana: validate hardware-supplied values in the HWC RX path Long Li
@ 2026-08-11 2:38 ` Long Li
2026-08-12 23:46 ` Jakub Kicinski
2026-08-11 2:38 ` [PATCH net v6 6/7] net: mana: fix stale HWC response after command timeout Long Li
2026-08-11 2:38 ` [PATCH net v6 7/7] net: mana: keep max_num_cqs immutable once cq_table is allocated Long Li
6 siblings, 1 reply; 21+ messages in thread
From: Long Li @ 2026-08-11 2:38 UTC (permalink / raw)
To: Long Li, Konstantin Taranov, Jakub Kicinski, David S . Miller,
Paolo Abeni, Eric Dumazet, Andrew Lunn, Jason Gunthorpe,
Leon Romanovsky, Haiyang Zhang, K . Y . Srinivasan, Wei Liu,
Dexuan Cui, shradhagupta, Simon Horman, ernis, stephen
Cc: netdev, linux-rdma, linux-hyperv, linux-kernel
Three teardown hazards let the hardware touch memory the driver freed.
First, once mana_smc_setup_hwc() succeeds the device has active MST
entries and can DMA into the HWC queue buffers. If a later step in
mana_hwc_establish_channel() fails, the caller had no reliable way to
know teardown was required and could free those buffers while the
mappings were still live -- a DMA-after-free. max_num_cqs was used as a
"HWC is up" proxy, but it is only set when the init EQE arrives.
Add a setup_active flag, set the moment setup_hwc activates MST entries.
On a later failure establish_channel() just returns the error; the
caller's error path (mana_hwc_create_channel() -> destroy_channel())
performs the single teardown, gated on setup_active. Tearing down inline
as well would run teardown twice -- doubling the 60s hardware timeout on
failure and masking the original error code. max_num_cqs is no longer
reset: it is an immutable bound (see gdma.h) and cq_table == NULL is the
sole teardown signal.
Second, destroy_channel() freed the TXQ/RXQ buffers while the HWC EQ was
still on the interrupt dispatch list, so an in-flight interrupt could run
the handler against freed buffers:
CPU A (mana_gd_intr, hard IRQ) CPU B (destroy_channel)
---------------------------------- ------------------------------
free TXQ/RXQ DMA buffers
handler accesses RQ/TXQ buffers (EQ still registered)
Destroy the CQ first: mana_hwc_destroy_cq() -> mana_gd_deregister_irq()
removes the EQ via list_del_rcu() + synchronize_rcu(), after which no
handler can reach the queues; only then free the TXQ and RXQ.
Third, if mana_smc_teardown_hwc() itself fails the MST entries stay
live, yet destroy_channel() went on to free the CQ/RQ/TXQ buffers the
device can still DMA into -- a DMA-after-free on systems without an
IOMMU to fault the stale access. Leak the HWC resources on teardown
failure instead of freeing memory the hardware can still reach, and
keep setup_active set so the failure is not mistaken for a clean
teardown.
Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v6:
- Set setup_active before calling mana_smc_setup_hwc(): that call
activates the device MST entries before it can report a late failure,
so arming the flag afterwards left a window where the error path could
free buffers the device may still DMA into.
- On an unrecoverable teardown failure keep the HWC context reachable
and retry the teardown on the next bring-up instead of orphaning it.
- Dropped the gdma.h comment rewording (moved to patch 1).
Changes in v5:
- No code changes since v4 (resend as a standalone thread).
Changes in v4:
- Arm setup_active immediately after mana_smc_setup_hwc() succeeds.
- Destroy the EQ (IRQ deregister + drain) before the CQ.
- Dropped the redundant teardown in mana_hwc_establish_channel() that
caused a double hardware timeout and masked the original error code.
.../net/ethernet/microsoft/mana/hw_channel.c | 75 +++++++++++++++----
include/net/mana/hw_channel.h | 9 +++
2 files changed, 69 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 5db8cfe2d84432940cc97d814f2cd6933a92caf9..959886434d07fa32c62dacb041945a4587d3bb16 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)
@@ -783,6 +784,20 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
init_completion(&hwc->hwc_init_eqe_comp);
+ /* Arm setup_active before issuing the setup command.
+ * mana_smc_setup_hwc() hands the queue PFNs to the PF, activating
+ * MST entries so the device can DMA into our queue buffers, before
+ * it can report a later failure such as a possession-poll timeout.
+ * Recording it up front guarantees the error path
+ * (mana_hwc_create_channel() -> mana_hwc_destroy_channel()) still
+ * tears the HWC down instead of freeing buffers the device may still
+ * write to. Setting it for a rare pre-submission failure too is
+ * harmless -- the teardown is then a no-op the device ignores. Do
+ * not also tear down here: a second teardown would double the
+ * hardware timeout on failure and mask the original error code.
+ */
+ hwc->setup_active = true;
+
err = mana_smc_setup_hwc(&gc->shm_channel, false,
eq->mem_info.dma_handle,
cq->mem_info.dma_handle,
@@ -869,6 +884,20 @@ int mana_hwc_create_channel(struct gdma_context *gc)
u16 q_depth_max;
int err;
+ /* A previous teardown may have failed and deliberately left the old
+ * HWC context reachable (see mana_hwc_destroy_channel()). Retry the
+ * teardown now -- the device has since been reset -- before building
+ * a new channel, so we neither orphan the old context nor stack a
+ * second channel on one whose DESTROY_HWC never completed. If it is
+ * still failing, return an error that steers mana_serv_reset() to a
+ * full PCI rescan instead of silently leaking another generation.
+ */
+ if (gd->driver_data) {
+ mana_hwc_destroy_channel(gc);
+ if (gd->driver_data)
+ return -ETIMEDOUT;
+ }
+
hwc = kzalloc_obj(*hwc);
if (!hwc)
return -ENOMEM;
@@ -926,11 +955,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);
@@ -938,17 +994,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/hw_channel.h b/include/net/mana/hw_channel.h
index 787c6f96d5b576c0911e777bcc76673bba0dfb50..8340abd36af611c658fecb6f1604ce3d4aedbddc 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -206,6 +206,15 @@ struct hw_channel_context {
*/
u32 rx_leaked_wqe;
+ /* Set after mana_smc_setup_hwc() succeeds (hardware has active
+ * MST entries). Cleared only after mana_smc_teardown_hwc()
+ * succeeds, on both the recoverable establish_channel path and the
+ * terminal destroy_channel path. If teardown fails it stays set:
+ * establish_channel() skips its retry and destroy_channel() leaks
+ * the HWC rather than free buffers the device may still DMA into.
+ */
+ bool setup_active;
+
struct hwc_caller_ctx *caller_ctx;
};
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net v6 5/7] net: mana: fix HWC teardown safety with setup_active flag and destroy ordering
2026-08-11 2:38 ` [PATCH net v6 5/7] net: mana: fix HWC teardown safety with setup_active flag and destroy ordering Long Li
@ 2026-08-12 23:46 ` Jakub Kicinski
0 siblings, 0 replies; 21+ messages in thread
From: Jakub Kicinski @ 2026-08-12 23:46 UTC (permalink / raw)
To: longli
Cc: Jakub Kicinski, kotaranov, davem, pabeni, edumazet, andrew+netdev,
jgg, leon, haiyangz, kys, wei.liu, decui, shradhagupta, horms,
ernis, stephen, netdev, linux-rdma, linux-hyperv, linux-kernel
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 to record that
mana_smc_setup_hwc() has activated device MST entries, reorders
mana_hwc_destroy_channel() so the HWC CQ (and with it the EQ IRQ
deregistration) is destroyed before the TXQ/RXQ buffers are freed, and
changes destroy_channel() to leak the HWC resources when
mana_smc_teardown_hwc() fails.
A few questions on the new flag placement and on the leak-on-failure
policy.
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 5db8cfe2d8443..959886434d07f 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -783,6 +784,20 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
>
> init_completion(&hwc->hwc_init_eqe_comp);
>
> + /* Arm setup_active before issuing the setup command.
> + * mana_smc_setup_hwc() hands the queue PFNs to the PF, activating
> + * MST entries so the device can DMA into our queue buffers, before
> + * it can report a later failure such as a possession-poll timeout.
> + * Recording it up front guarantees the error path
> + * (mana_hwc_create_channel() -> mana_hwc_destroy_channel()) still
> + * tears the HWC down instead of freeing buffers the device may still
> + * write to. Setting it for a rare pre-submission failure too is
> + * harmless -- the teardown is then a no-op the device ignores. Do
> + * not also tear down here: a second teardown would double the
> + * hardware timeout on failure and mask the original error code.
> + */
> + hwc->setup_active = true;
> +
> err = mana_smc_setup_hwc(&gc->shm_channel, false,
> eq->mem_info.dma_handle,
> cq->mem_info.dma_handle,
[Severity: High]
Is the claim that "the teardown is then a no-op the device ignores" true
for the most likely pre-submission failure?
mana_smc_setup_hwc() fails at its very first step, before anything is
written to the shared memory aperture:
drivers/net/ethernet/microsoft/mana/shm_channel.c:mana_smc_setup_hwc() {
/* Ensure VF already has possession of shared memory */
err = mana_smc_poll_register(sc->base, false);
if (err) {
dev_err(sc->dev, "Timeout when setting up HWC: %d\n", err);
return err;
}
...
}
mana_smc_teardown_hwc() opens with the same poll:
drivers/net/ethernet/microsoft/mana/shm_channel.c:mana_smc_teardown_hwc() {
/* Ensure already has possession of shared memory */
err = mana_smc_poll_register(sc->base, false);
if (err) {
dev_err(sc->dev, "Timeout when tearing down HWC\n");
return err;
}
...
}
So when the possession poll times out, no ESTABLISH_HWC message ever
reached the device, but setup_active is already set, the error path runs
mana_hwc_destroy_channel(), the teardown fails for the identical reason,
and the new "leaking resources" branch returns having freed nothing.
Does that leak the hwc struct, the inflight bitmap, the EQ/CQ/RQ/SQ gdma
queues with their dma_alloc_coherent buffers, comp_buf and both msg_buf
DMA buffers, in a case where the old gc->max_num_cqs > 0 gate skipped the
teardown entirely and freed everything?
Would arming the flag only after mana_smc_setup_hwc() returns success, or
distinguishing "message submitted" from "poll failed before submission",
avoid recording a teardown obligation the device never acquired?
[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but while
reading mana_hwc_establish_channel(): the bound check
if (WARN_ON(cq->id >= gc->max_num_cqs))
return -EPROTO;
cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
...
rcu_assign_pointer(cq_table[cq->id], cq);
re-reads both gc->max_num_cqs and cq->id after the check, while
mana_hwc_init_event_handler() writes both from EQ interrupt context
(HWC_INIT_DATA_MAX_NUM_CQS and HWC_INIT_DATA_CQID).
The later patch in this series, "net: mana: keep max_num_cqs immutable
once cq_table is allocated", snapshots both values into locals with
READ_ONCE() and commits gc->max_num_cqs once, so the check-then-use is
gone by the end of the series. Noted here only for completeness.
> @@ -869,6 +884,20 @@ int mana_hwc_create_channel(struct gdma_context *gc)
> u16 q_depth_max;
> int err;
>
> + /* A previous teardown may have failed and deliberately left the old
> + * HWC context reachable (see mana_hwc_destroy_channel()). Retry the
> + * teardown now -- the device has since been reset -- before building
> + * a new channel, so we neither orphan the old context nor stack a
> + * second channel on one whose DESTROY_HWC never completed. If it is
> + * still failing, return an error that steers mana_serv_reset() to a
> + * full PCI rescan instead of silently leaking another generation.
> + */
> + if (gd->driver_data) {
> + mana_hwc_destroy_channel(gc);
> + if (gd->driver_data)
> + return -ETIMEDOUT;
> + }
> +
> hwc = kzalloc_obj(*hwc);
> if (!hwc)
> return -ENOMEM;
[Severity: High]
This retry is the only mechanism that can reclaim a retained generation.
Can it run on the probe failure path?
mana_hwc_destroy_channel() is void, so mana_gd_setup() cannot tell that
the teardown was incomplete and keeps unwinding, and mana_gd_probe() ends
at:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_probe() {
...
free_gc:
pci_set_drvdata(pdev, NULL);
vfree(gc);
...
}
That frees the gdma_context holding gd->driver_data and gc->shm_channel,
so DESTROY_HWC can never be issued for the retained context afterwards.
mana_gd_remove() also vfree()s gc after mana_gd_cleanup_device().
Since mana_gd_probe() schedules mana_dev_recovery_work every
MANA_SERVICE_PERIOD on -ETIMEDOUT, and mana_serv_reset() escalates to
mana_serv_rescan() (remove plus re-probe), does every cycle in which
DESTROY_HWC does not complete retain a fresh generation, paced by the PF?
For errnos other than -ETIMEDOUT/-EPROTO (for example the -ENOMEM from the
vcalloc in mana_hwc_establish_channel(), which now happens with
setup_active already armed) mana_serv_reset() does no rescan:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_serv_reset() {
ret = mana_gd_resume(pdev);
if (ret == -ETIMEDOUT || ret == -EPROTO) {
/* Perform PCI rescan on device if we failed on HWC */
dev_err(&pdev->dev, "MANA service: resume failed, rescanning\n");
mana_serv_rescan(pdev);
...
}
Is the retained generation then abandoned with no recovery at all?
Would a bounded quarantine work better here, keeping at most one retained
generation reclaimed by the next successful teardown, or propagating the
teardown error so an owner of gc stays alive until reclamation is safe?
> @@ -926,11 +955,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;
> + }
[Severity: Low]
This return is taken before mana_hwc_destroy_cq(), so the HWC EQ stays on
gic->eq_list and the CQ stays published in gc->cq_table while
mana_gd_cleanup_device() goes on to mana_gd_remove_irqs().
This does not appear to be a use-after-free. The HWC EQ registration takes
no gdma_irq_context reference:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_register_irq() {
/* The caller acquired a GIC reference via mana_gd_get_gic().
* That refcount prevents mana_gd_put_gic() from erasing this
* irq_contexts entry concurrently.
*/
gic = xa_load(&gc->irq_contexts, msix_index);
...
}
so mana_gd_put_gic() reaches refcount_dec_and_test() and calls free_irq(),
which unregisters mana_gd_intr() and waits out in-flight handlers before
any vfree(gc). In the window before that, an interrupt would only touch
the EQ/CQ/RQ/TXQ buffers this branch deliberately did not free.
What is left is stale state: the EQ's list node ends up pointing into a
kfree()d gic, and on a later retried teardown after a resume whose
mana_gd_setup() failed before mana_gd_setup_hwc_irqs(),
mana_gd_deregister_irq() can hit
msix_index = queue->eq.msix_index;
if (WARN_ON(msix_index >= gc->num_msix_usable))
return;
with num_msix_usable == 0 and skip the list_del_rcu()/synchronize_rcu()
before the EQ is freed. Would it be reasonable to deregister the EQ IRQ
before taking the leak branch, so the retained buffers are unreachable
from the interrupt path even in the failure case?
> +
> + /* 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);
[ ... ]
> diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
> index 787c6f96d5b57..8340abd36af61 100644
> --- a/include/net/mana/hw_channel.h
> +++ b/include/net/mana/hw_channel.h
> @@ -206,6 +206,15 @@ struct hw_channel_context {
> */
> u32 rx_leaked_wqe;
>
> + /* Set after mana_smc_setup_hwc() succeeds (hardware has active
> + * MST entries). Cleared only after mana_smc_teardown_hwc()
> + * succeeds, on both the recoverable establish_channel path and the
> + * terminal destroy_channel path. If teardown fails it stays set:
> + * establish_channel() skips its retry and destroy_channel() leaks
> + * the HWC rather than free buffers the device may still DMA into.
> + */
> + bool setup_active;
> +
[Severity: Low]
This isn't a runtime bug, but this comment describes different code than
what the patch implements, and it also contradicts the in-code comment
added to mana_hwc_establish_channel():
"Set after mana_smc_setup_hwc() succeeds" - mana_hwc_establish_channel()
sets it before the call and leaves it set when the call fails.
"Cleared only after mana_smc_teardown_hwc() succeeds, on both the
recoverable establish_channel path and the terminal destroy_channel path" -
mana_hwc_establish_channel() has no mana_smc_teardown_hwc() call and never
clears the flag; the single teardown call site and the single clear are
both in mana_hwc_destroy_channel().
"establish_channel() skips its retry" - mana_hwc_establish_channel() has no
retry.
Are these leftovers from the earlier version of the patch that tore down
inline? Could the comment be updated to match the current placement, so a
later reader does not move the assignment below mana_smc_setup_hwc() and
reopen the window this patch closes?
> struct hwc_caller_ctx *caller_ctx;
> };
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net v6 6/7] net: mana: fix stale HWC response after command timeout
2026-08-11 2:38 [PATCH net v6 0/7] net: mana: HW channel reliability and hardening fixes Long Li
` (4 preceding siblings ...)
2026-08-11 2:38 ` [PATCH net v6 5/7] net: mana: fix HWC teardown safety with setup_active flag and destroy ordering Long Li
@ 2026-08-11 2:38 ` Long Li
2026-08-12 23:46 ` Jakub Kicinski
2026-08-11 2:38 ` [PATCH net v6 7/7] net: mana: keep max_num_cqs immutable once cq_table is allocated Long Li
6 siblings, 1 reply; 21+ messages in thread
From: Long Li @ 2026-08-11 2:38 UTC (permalink / raw)
To: Long Li, Konstantin Taranov, Jakub Kicinski, David S . Miller,
Paolo Abeni, Eric Dumazet, Andrew Lunn, Jason Gunthorpe,
Leon Romanovsky, Haiyang Zhang, K . Y . Srinivasan, Wei Liu,
Dexuan Cui, shradhagupta, Simon Horman, ernis, stephen
Cc: netdev, linux-rdma, linux-hyperv, linux-kernel
The HWC freed a message slot (mana_hwc_put_msg_index) the instant
mana_hwc_send_request() timed out, while the hardware command was still
pending and caller_ctx.output_buf still pointed at the caller's response
buffer. A late response then raced two ways:
- handle_resp() runs in CQ interrupt context and memcpy()'d into
output_buf after the sender had returned and its buffer was gone.
- the freed slot was reused by the next request, so the stale
response completed the wrong command with another request's data.
Give each caller_ctx a spinlock, a refcount and an -EINPROGRESS
sentinel (and change caller_ctx::error from u32 to int so it holds
the negative errno values, including the sentinel, without relying on
unsigned wraparound):
- The sender publishes output_buf under the slot lock and NULLs it
under the same lock on timeout/exit, so handle_resp() (also under
the lock) skips the copy once the sender is gone.
- The slot is released only when both the sender and handle_resp()
have dropped their reference, so a msg_id whose response is still
outstanding is never handed to a new request.
- Both references are taken up front in mana_hwc_get_msg_index(),
under the same lock that publishes the slot, so a stale, duplicate
or early response that arrives before the sender posts drops only
the response-side reference and cannot release the slot out from
under the sender. A per-slot "responded" flag drops the payload of
any such extra response.
- On a genuine timeout the channel is marked hwc_timed_out and further
mana_hwc_get_msg_index() callers fail with -ETIMEDOUT instead of
reusing a slot whose response may still arrive. The flag is read
with READ_ONCE() outside the bitmap lock and written with
WRITE_ONCE() under it.
Replace the counting semaphore with a waitqueue + bitmap so a slot held
past a timeout does not deadlock admission and timed-out waiters can be
released.
Because the timeout latch keys off wait_for_completion_timeout()
returning immediately, a zero hwc_timeout would time out every command
at once and latch the whole channel. Ignore a device-reported zero from
both sources that feed hwc_timeout -- the HWC_DATA_CFG_HWC_TIMEOUT
reconfig event and the GDMA_QUERY_HWC_TIMEOUT response -- and keep the
positive default instead.
Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v6:
- Initialise the caller_ctx refcount/state before publishing the
inflight bitmap bit, so a racing or forged response cannot observe an
uninitialised slot.
- In mana_hwc_handle_resp(), honour a response only while the sender
still owns the slot (output_buf published and not yet reclaimed), so a
premature response cannot free the slot while its command is still in
flight.
- Do not latch hwc_timed_out for the deliberate no-wait teardown
(hwc_timeout == 0), applied on both the admission gate and the
post-wait check in mana_hwc_get_msg_index(); route the genuine-timeout
path through the slot-release path so it drops both references.
Changes in v5:
- No code changes since v4 (resend as a standalone thread).
Changes in v4:
- Take both the sender and response-side references up front in
mana_hwc_get_msg_index() (refcount initialised to 2, under the lock
that publishes the slot) so an early/stale/forged response cannot
free the slot before the sender posts; the pre-post error path
latches ->responded to avoid a double drop.
- Changed caller_ctx::error from u32 to int so it holds the negative
-EINPROGRESS sentinel and errno values directly.
- Reject a zero firmware-supplied HWC timeout in the query path as well
as the reconfig path.
- Access hwc_timed_out with READ_ONCE()/WRITE_ONCE(); comment and
changelog fixes.
.../net/ethernet/microsoft/mana/gdma_main.c | 7 +-
.../net/ethernet/microsoft/mana/hw_channel.c | 264 +++++++++++++++---
include/net/mana/hw_channel.h | 27 +-
3 files changed, 257 insertions(+), 41 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index d40f25a1a74a739315716a4066987f1137de88d9..d4c7426750016fd21e88a15e071fd2fb3da67ebe 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 959886434d07fa32c62dacb041945a4587d3bb16..759b65040a159339a5ab9a2eb95acaaa2e452f53 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -7,25 +7,77 @@
#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);
+ /* Reject new admissions once the channel has latched a genuine
+ * timeout -- but not for a deliberate no-wait teardown, where
+ * mana_serv_reset() sets hwc_timeout = 0 to best-effort post
+ * the teardown commands. Without this exception an earlier
+ * timeout would block those teardown commands here before the
+ * hwc_timeout == 0 path in mana_hwc_send_request() can run.
+ */
+ if (hwc->hwc_timed_out && hwc->hwc_timeout != 0) {
+ 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;
+
+ ctx = &hwc->caller_ctx[index];
+ reinit_completion(&ctx->comp_event);
+ /* Initialise the slot before publishing its inflight
+ * bit below. The response-side reference is taken
+ * here, under r->lock, so a stale or duplicate response
+ * that lands before mana_hwc_send_request() posts the
+ * request cannot drop the refcount to zero and free the
+ * slot under the sender. One reference is the sender's;
+ * the other is released by mana_hwc_handle_resp().
+ */
+ refcount_set(&ctx->refcnt, 2);
+ ctx->responded = false;
+ ctx->msg_id = index;
+ ctx->error = -EINPROGRESS;
+ /* Publish the slot last. mana_hwc_handle_resp() honours
+ * a response only after the sender sets ctx->output_buf
+ * (under ctx->lock, after this function returns), so the
+ * initialisation above is always visible before any
+ * response is acted on.
+ */
+ bitmap_set(r->map, index, 1);
+ spin_unlock_irqrestore(&r->lock, flags);
+ break;
+ }
+ spin_unlock_irqrestore(&r->lock, flags);
- bitmap_set(hwc->inflight_msg_res.map, index, 1);
+ wait_event(hwc->msg_waitq,
+ (READ_ONCE(hwc->hwc_timed_out) &&
+ READ_ONCE(hwc->hwc_timeout) != 0) ||
+ !bitmap_full(r->map, r->size));
- spin_unlock_irqrestore(&r->lock, flags);
+ /* Same no-wait teardown exception as the entry gate above:
+ * when hwc_timeout == 0 do not bail on the latch, wait for a
+ * slot to free so the best-effort teardown command can still
+ * be posted instead of spinning here.
+ */
+ if (READ_ONCE(hwc->hwc_timed_out) &&
+ READ_ONCE(hwc->hwc_timeout) != 0)
+ return -ETIMEDOUT;
+ }
*msg_id = index;
-
return 0;
}
@@ -35,10 +87,17 @@ static void mana_hwc_put_msg_index(struct hw_channel_context *hwc, u16 msg_id)
unsigned long flags;
spin_lock_irqsave(&r->lock, flags);
- bitmap_clear(hwc->inflight_msg_res.map, msg_id, 1);
+ bitmap_clear(r->map, msg_id, 1);
spin_unlock_irqrestore(&r->lock, flags);
- up(&hwc->sema);
+ wake_up(&hwc->msg_waitq);
+}
+
+static void hwc_ctx_put(struct hw_channel_context *hwc,
+ struct hwc_caller_ctx *ctx)
+{
+ if (refcount_dec_and_test(&ctx->refcnt))
+ mana_hwc_put_msg_index(hwc, ctx->msg_id);
}
static int mana_hwc_verify_resp_msg(const struct hwc_caller_ctx *caller_ctx,
@@ -116,22 +175,41 @@ 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;
-
- ctx->status_code = resp_msg->status;
+ spin_lock(&ctx->lock);
+
+ /* Honour a response only while the sender is actively waiting on
+ * this slot -- that is, it has published ctx->output_buf and not yet
+ * reclaimed it. A NULL output_buf means the sender has not posted
+ * its request yet (so this is a premature, stale or forged response
+ * that must not complete the slot and let it be freed while the real
+ * request is still in flight) or it already timed out and took
+ * ownership back. ctx->responded drops a second, duplicate response.
+ * In all these cases drop the response without touching the refcount
+ * or the completion; the genuine response, the sender or the teardown
+ * path still balances the references.
+ */
+ 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,
@@ -218,7 +296,12 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
switch (type) {
case HWC_DATA_CFG_HWC_TIMEOUT:
- hwc->hwc_timeout = val;
+ /* A zero timeout would make every command time out
+ * immediately and latch hwc_timed_out, disabling the
+ * channel. Ignore it and keep the positive default.
+ */
+ if (val)
+ hwc->hwc_timeout = val;
break;
case HWC_DATA_HW_LINK_CONNECT:
@@ -732,7 +815,7 @@ static int mana_hwc_init_inflight_msg(struct hw_channel_context *hwc,
{
int err;
- sema_init(&hwc->sema, num_msg);
+ init_waitqueue_head(&hwc->msg_waitq);
err = mana_gd_alloc_res_map(num_msg, &hwc->inflight_msg_res);
if (err)
@@ -762,8 +845,10 @@ static int mana_hwc_test_channel(struct hw_channel_context *hwc, u16 q_depth,
if (!ctx)
return -ENOMEM;
- for (i = 0; i < q_depth; ++i)
+ for (i = 0; i < q_depth; ++i) {
+ spin_lock_init(&ctx[i].lock);
init_completion(&ctx[i].comp_event);
+ }
hwc->caller_ctx = ctx;
@@ -774,6 +859,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;
@@ -1023,13 +1111,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];
@@ -1041,8 +1135,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)
@@ -1058,43 +1155,134 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
dest_vrcq = hwc->pf_dest_vrcq_id;
}
+ /* handle_resp()'s reference was taken in mana_hwc_get_msg_index(),
+ * so hardware responding immediately after the doorbell ring cannot
+ * release the slot before this sender is done with it.
+ */
err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
if (err) {
dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n", err);
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);
+
+ /* 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;
+ }
+
+ err = -ETIMEDOUT;
+
+ /* A deliberate no-wait send -- mana_serv_reset() sets
+ * hwc_timeout = 0 when the HWC is already unresponsive and it
+ * only needs to best-effort post the teardown commands -- is
+ * expected to expire here. Do not latch hwc_timed_out for it:
+ * that would make mana_hwc_get_msg_index() reject the remaining
+ * teardown commands before they are even posted. Release the
+ * slot through the out: path so the next command can reuse it,
+ * matching the pre-refcount behaviour where every command was
+ * posted and only the wait was skipped.
+ */
+ if (wait_ms == 0)
+ goto out;
- /* Reduce further waiting if HWC no response */
+ /* 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;
- err = -ETIMEDOUT;
+ spin_lock_irqsave(&hwc->inflight_msg_res.lock, flags);
+ WRITE_ONCE(hwc->hwc_timed_out, true);
+ spin_unlock_irqrestore(&hwc->inflight_msg_res.lock, flags);
+ wake_up_all(&hwc->msg_waitq);
+
+ /* Release the slot through out:, which also drops the
+ * response-side reference taken in mana_hwc_get_msg_index().
+ * A late response for this slot cannot drop it -- once the
+ * sender NULLs output_buf, mana_hwc_handle_resp() early-returns
+ * without touching the refcount -- so the sender must free it
+ * here, otherwise the slot bit would leak until channel
+ * teardown.
+ */
goto out;
}
- 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);
+ /* Reached by the pre-post error paths (request never submitted), by
+ * the deliberate no-wait teardown, and by a genuine timeout (request
+ * posted, but no valid response arrived). In every case the sender
+ * must drop the response-side reference taken in
+ * mana_hwc_get_msg_index() and its own. Guard against a stale or
+ * forged response that raced in first: latch ->responded under the
+ * lock so any later handle_resp() is a no-op, and drop the response-
+ * side reference here only if handle_resp() has not already done so.
+ */
+ ctx = hwc->caller_ctx + msg_id;
+ spin_lock_irqsave(&ctx->lock, flags);
+ ctx->output_buf = NULL;
+ drop_resp_ref = !ctx->responded;
+ ctx->responded = true;
+ spin_unlock_irqrestore(&ctx->lock, flags);
+ if (drop_resp_ref)
+ refcount_dec(&ctx->refcnt);
+ hwc_ctx_put(hwc, ctx);
+done:
return err;
}
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 8340abd36af611c658fecb6f1604ce3d4aedbddc..23bf83e2a3ec6a5b19ab54db0a65ae41b74ad74c 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -171,8 +171,25 @@ struct hwc_caller_ctx {
void *output_buf;
u32 output_buflen;
- u32 error; /* Linux error code */
+ int error; /* Linux error code (negative errno or 0) */
u32 status_code;
+
+ /* Protects output_buf against concurrent access from
+ * handle_resp() (CQ interrupt) and the sender timeout path.
+ */
+ spinlock_t lock;
+
+ /* Tracks sender + handle_resp ownership. The last put
+ * (refcount reaches 0) releases the bitmap slot.
+ */
+ refcount_t refcnt;
+ u16 msg_id;
+
+ /* Set under lock by the first handle_resp() for this slot so a
+ * duplicate or replayed response is dropped instead of consuming
+ * the response-side reference a second time.
+ */
+ bool responded;
};
struct hw_channel_context {
@@ -193,8 +210,9 @@ struct hw_channel_context {
struct hwc_wq *txq;
struct hwc_cq *cq;
- struct semaphore sema;
struct gdma_resource inflight_msg_res;
+ /* Waitqueue for senders blocked on a full inflight bitmap. */
+ wait_queue_head_t msg_waitq;
u32 pf_dest_vrq_id;
u32 pf_dest_vrcq_id;
@@ -206,6 +224,11 @@ struct hw_channel_context {
*/
u32 rx_leaked_wqe;
+ /* Set on first HWC timeout. Causes get_msg_index() to return
+ * -ETIMEDOUT instead of waiting, draining all queued senders.
+ */
+ bool hwc_timed_out;
+
/* Set after mana_smc_setup_hwc() succeeds (hardware has active
* MST entries). Cleared only after mana_smc_teardown_hwc()
* succeeds, on both the recoverable establish_channel path and the
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net v6 6/7] net: mana: fix stale HWC response after command timeout
2026-08-11 2:38 ` [PATCH net v6 6/7] net: mana: fix stale HWC response after command timeout Long Li
@ 2026-08-12 23:46 ` Jakub Kicinski
0 siblings, 0 replies; 21+ messages in thread
From: Jakub Kicinski @ 2026-08-12 23:46 UTC (permalink / raw)
To: longli
Cc: Jakub Kicinski, kotaranov, davem, pabeni, edumazet, andrew+netdev,
jgg, leon, haiyangz, kys, wei.liu, decui, shradhagupta, horms,
ernis, stephen, netdev, linux-rdma, linux-hyperv, linux-kernel
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 adds a per-slot spinlock, a refcount and an -EINPROGRESS sentinel to
struct hwc_caller_ctx so a late HWC response cannot memcpy() into a
returned sender's buffer or complete a recycled msg_id. The counting
semaphore guarding slot admission is replaced with a waitqueue plus the
inflight bitmap, and a new hwc_timed_out latch rejects further slot
requests after a genuine command timeout.
A few questions below about the new ownership rules, the latch, and the
matching comments/changelog text.
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index d40f25a1a74a7..d4c7426750016 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;
> }
[Severity: Medium]
The changelog justifies this hunk and the HWC_DATA_CFG_HWC_TIMEOUT hunk
with:
"Because the timeout latch keys off wait_for_completion_timeout()
returning immediately, a zero hwc_timeout would time out every command
at once and latch the whole channel."
Can a zero hwc_timeout actually reach the latch? In
mana_hwc_send_request() the zero case is exempted before hwc_timed_out is
written:
if (wait_ms == 0)
goto out;
...
WRITE_ONCE(hwc->hwc_timed_out, true);
and the new admission gate plus the wait_event() condition in
mana_hwc_get_msg_index() also skip the latch when hwc_timeout == 0. The
comment added to mana_hwc_init_event_handler() repeats the same rationale
("A zero timeout would make every command time out immediately and latch
hwc_timed_out, disabling the channel").
The effect of a device-reported zero looks different: every command turns
into a fire-and-forget post returning -ETIMEDOUT with an unfilled response
buffer.
Could the stated reason for silently overriding a firmware-supplied
timeout be restated to match what the code does?
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 959886434d07f..759b65040a159 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -7,25 +7,77 @@
> #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);
> + /* Reject new admissions once the channel has latched a genuine
> + * timeout -- but not for a deliberate no-wait teardown, where
> + * mana_serv_reset() sets hwc_timeout = 0 to best-effort post
> + * the teardown commands. Without this exception an earlier
> + * timeout would block those teardown commands here before the
> + * hwc_timeout == 0 path in mana_hwc_send_request() can run.
> + */
> + if (hwc->hwc_timed_out && hwc->hwc_timeout != 0) {
> + spin_unlock_irqrestore(&r->lock, flags);
> + return -ETIMEDOUT;
> + }
[Severity: Medium]
This gate is a two-variable predicate, but only hwc_timed_out is written
under r->lock. hwc_timeout is written with plain stores and no lock by:
mana_hwc_init_event_handler() /* HWC_DATA_CFG_HWC_TIMEOUT, EQ interrupt */
mana_hwc_rx_leak_wqe() /* hwc->hwc_timeout = 1, EQ interrupt */
mana_hwc_send_request() /* if (hwc->hwc_timeout > 1) ... = 1, outside
inflight_msg_res.lock */
mana_serv_reset() /* hwc->hwc_timeout = 0 */
mana_hwc_destroy_channel() /* hwc->hwc_timeout = 0 */
None of them takes inflight_msg_res.lock and none uses WRITE_ONCE() to
pair with the new READ_ONCE() readers, so the two halves of the gate are
never updated together.
Can that give these outcomes?
- mana_serv_reset() sets hwc_timeout = 0 precisely so the remaining
teardown commands bypass the latch; an interrupt-context store of a
non-zero hwc_timeout (reconfig event or mana_hwc_rx_leak_wqe()) makes
the gate true again and every remaining teardown command is rejected
with -ETIMEDOUT.
- A teardown sender that passed this gate with hwc_timeout == 0 then
reads wait_ms = hwc->hwc_timeout locklessly; if the interrupt store
lands in between it takes the genuine-timeout branch and latches
hwc_timed_out for the whole channel.
>
> - 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;
> +
> + ctx = &hwc->caller_ctx[index];
> + reinit_completion(&ctx->comp_event);
> + /* Initialise the slot before publishing its inflight
> + * bit below. The response-side reference is taken
> + * here, under r->lock, so a stale or duplicate response
> + * that lands before mana_hwc_send_request() posts the
> + * request cannot drop the refcount to zero and free the
> + * slot under the sender. One reference is the sender's;
> + * the other is released by mana_hwc_handle_resp().
> + */
> + refcount_set(&ctx->refcnt, 2);
> + ctx->responded = false;
> + ctx->msg_id = index;
> + ctx->error = -EINPROGRESS;
> + /* Publish the slot last. mana_hwc_handle_resp() honours
> + * a response only after the sender sets ctx->output_buf
> + * (under ctx->lock, after this function returns), so the
> + * initialisation above is always visible before any
> + * response is acted on.
> + */
> + bitmap_set(r->map, index, 1);
> + spin_unlock_irqrestore(&r->lock, flags);
> + break;
> + }
> + spin_unlock_irqrestore(&r->lock, flags);
>
> - bitmap_set(hwc->inflight_msg_res.map, index, 1);
> + wait_event(hwc->msg_waitq,
> + (READ_ONCE(hwc->hwc_timed_out) &&
> + READ_ONCE(hwc->hwc_timeout) != 0) ||
> + !bitmap_full(r->map, r->size));
[Severity: Medium]
hwc_timeout is now part of this wait predicate, but is anything waking
msg_waitq when hwc_timeout changes?
The only wakeups added are mana_hwc_put_msg_index() and the wake_up_all()
that follows the latch in mana_hwc_send_request().
A sender that sleeps here while hwc_timed_out is already set and
hwc_timeout == 0 (the mana_serv_reset() window) has a false exit
condition. When hwc_timeout later becomes non-zero, from
mana_hwc_init_event_handler() or mana_hwc_rx_leak_wqe(), the predicate
becomes true but no wake_up() is issued, so the task stays in
TASK_UNINTERRUPTIBLE sleep until some slot happens to be freed.
mana_hwc_destroy_channel() also sets hwc->hwc_timeout = 0 and tears
everything down without waking msg_waitq.
>
> - spin_unlock_irqrestore(&r->lock, flags);
> + /* Same no-wait teardown exception as the entry gate above:
> + * when hwc_timeout == 0 do not bail on the latch, wait for a
> + * slot to free so the best-effort teardown command can still
> + * be posted instead of spinning here.
> + */
> + if (READ_ONCE(hwc->hwc_timed_out) &&
> + READ_ONCE(hwc->hwc_timeout) != 0)
> + return -ETIMEDOUT;
> + }
>
> *msg_id = index;
> -
> return 0;
> }
>
> @@ -35,10 +87,17 @@ static void mana_hwc_put_msg_index(struct hw_channel_context *hwc, u16 msg_id)
> unsigned long flags;
>
> spin_lock_irqsave(&r->lock, flags);
> - bitmap_clear(hwc->inflight_msg_res.map, msg_id, 1);
> + bitmap_clear(r->map, msg_id, 1);
> spin_unlock_irqrestore(&r->lock, flags);
>
> - up(&hwc->sema);
> + wake_up(&hwc->msg_waitq);
> +}
> +
> +static void hwc_ctx_put(struct hw_channel_context *hwc,
> + struct hwc_caller_ctx *ctx)
> +{
> + if (refcount_dec_and_test(&ctx->refcnt))
> + mana_hwc_put_msg_index(hwc, ctx->msg_id);
> }
>
> static int mana_hwc_verify_resp_msg(const struct hwc_caller_ctx *caller_ctx,
> @@ -116,22 +175,41 @@ 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;
> -
> - ctx->status_code = resp_msg->status;
> + spin_lock(&ctx->lock);
> +
> + /* Honour a response only while the sender is actively waiting on
> + * this slot -- that is, it has published ctx->output_buf and not yet
> + * reclaimed it. A NULL output_buf means the sender has not posted
> + * its request yet (so this is a premature, stale or forged response
> + * that must not complete the slot and let it be freed while the real
> + * request is still in flight) or it already timed out and took
> + * ownership back. ctx->responded drops a second, duplicate response.
> + * In all these cases drop the response without touching the refcount
> + * or the completion; the genuine response, the sender or the teardown
> + * path still balances the references.
> + */
> + if (!ctx->output_buf || ctx->responded) {
> + spin_unlock(&ctx->lock);
> + mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> + return;
> + }
> + ctx->responded = true;
[Severity: High]
This is a pre-existing issue and not introduced by this patch (the
baseline also freed the slot on timeout and correlated responses purely by
hwc_msg_id), but the changelog says the misdelivery case is now closed.
Is it?
Responses are still matched only by the reusable msg_id.
mana_hwc_verify_resp_msg() checks lengths only, and this gate accepts any
response whose slot has output_buf published and responded == false; no
activity_id or msg_type is compared against the posted request.
With q_depth == HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH == 1:
sender A: mana_hwc_post_tx_wqe() posts the command, wait_ms == 0 expires,
"if (wait_ms == 0) goto out;" NULLs output_buf, drops both
references, mana_hwc_put_msg_index() clears bit 0
sender B: mana_hwc_get_msg_index() hands back index 0 again,
refcount_set(&ctx->refcnt, 2), ctx->responded = false,
reinit_completion(), then publishes its own output_buf
device: late response for A's command arrives with msg_id 0
mana_hwc_handle_resp() sees output_buf != NULL and !responded,
memcpy()s into B's buffer, stores status_code, complete()
Two further variants of the same correlation gap:
ctx->output_buf is published before the request is copied, tagged with
req_msg->req.hwc_msg_id and posted, so a duplicate or delayed response
landing in that window is accepted and completes the slot before the
hardware has seen the command.
The earlier test_bit(msg_id, hwc->inflight_msg_res.map) check in
mana_hwc_handle_resp() is done without ctx->lock, so the slot can be
released by the old sender and re-armed by a new one between that test and
this spin_lock(&ctx->lock).
>
> - 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,
> @@ -218,7 +296,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:
[ ... ]
> @@ -1041,8 +1135,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)
> @@ -1058,43 +1155,134 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
> dest_vrcq = hwc->pf_dest_vrcq_id;
> }
>
> + /* handle_resp()'s reference was taken in mana_hwc_get_msg_index(),
> + * so hardware responding immediately after the doorbell ring cannot
> + * release the slot before this sender is done with it.
> + */
> err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
> if (err) {
> dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n", err);
> 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);
> +
> + /* 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;
> + }
> +
> + err = -ETIMEDOUT;
> +
> + /* A deliberate no-wait send -- mana_serv_reset() sets
> + * hwc_timeout = 0 when the HWC is already unresponsive and it
> + * only needs to best-effort post the teardown commands -- is
> + * expected to expire here. Do not latch hwc_timed_out for it:
> + * that would make mana_hwc_get_msg_index() reject the remaining
> + * teardown commands before they are even posted. Release the
> + * slot through the out: path so the next command can reuse it,
> + * matching the pre-refcount behaviour where every command was
> + * posted and only the wait was skipped.
> + */
> + if (wait_ms == 0)
> + goto out;
[Severity: Medium]
This isn't a bug introduced by this patch (the baseline released the slot
immediately on timeout too), but does releasing the slot right after the
doorbell also release the TX request buffer while the device may still own
it?
mana_hwc_post_tx_wqe() hands the bus address of
txq->msg_buf->reqs[msg_id].buf_va to the device:
sge->address = (u64)req->buf_sge_addr;
...
err = mana_gd_post_and_ring(hwc_txq->gdma_wq, &req->wqe_req, NULL);
and mana_hwc_tx_event_handler() is WARN-only, so there is no per-WQE SQ
completion tracking; the buffer is only known to be consumed when the
response arrives. On this path (and after a genuine timeout once
mana_serv_reset() sets hwc_timeout = 0) the next command takes the same
msg_id and memcpy()s a new request over that buffer, so firmware can
DMA-read a mixture of two requests.
The changelog's invariant, "a msg_id whose response is still outstanding
is never handed to a new request", does not seem to cover this TX-buffer
ownership aspect.
>
> - /* Reduce further waiting if HWC no response */
> + /* 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;
>
> - err = -ETIMEDOUT;
> + spin_lock_irqsave(&hwc->inflight_msg_res.lock, flags);
> + WRITE_ONCE(hwc->hwc_timed_out, true);
> + spin_unlock_irqrestore(&hwc->inflight_msg_res.lock, flags);
> + wake_up_all(&hwc->msg_waitq);
[Severity: High]
Is hwc_timed_out ever cleared again? Grepping the tree it is only ever
written true, so after one command timeout every later
mana_hwc_get_msg_index() returns -ETIMEDOUT and the command is not posted
at all — no TX WQE and no doorbell.
The device-side teardown commands (GDMA_DESTROY_DMA_REGION,
GDMA_DISABLE_QUEUE, GDMA_DEREGISTER_DEVICE, the MANA WQ/RQ-object and
vport destroy commands) all travel over the HWC, and their callers free
the memory regardless of the error:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_destroy_queue() {
...
mana_gd_destroy_dma_region(gc, gmi->dma_region_handle);
mana_gd_free_memory(gmi);
kfree(queue);
}
Can this leave the device with DMA regions, memory keys and queues still
registered in its MST-PTE tables, holding DMA rights over pages just
returned to the page allocator, with no invalidating command even
attempted?
Before this patch the same command was still posted and only the wait was
shortened to 1 ms, so a merely slow or transiently unresponsive device
still processed the teardown. The one escape from the latch
(hwc_timeout == 0) is set only by mana_serv_reset(), so ordinary
remove/unbind/suspend paths do not get it.
The driver's own setup_active comment in mana_hwc_destroy_channel()
deliberately leaks HWC buffers rather than free memory the device may
still DMA into; should the teardown commands get a similar exemption from
the latch?
> +
> + /* Release the slot through out:, which also drops the
> + * response-side reference taken in mana_hwc_get_msg_index().
> + * A late response for this slot cannot drop it -- once the
> + * sender NULLs output_buf, mana_hwc_handle_resp() early-returns
> + * without touching the refcount -- so the sender must free it
> + * here, otherwise the slot bit would leak until channel
> + * teardown.
> + */
> goto out;
> }
>
> - 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);
[ ... ]
> out:
> - mana_hwc_put_msg_index(hwc, msg_id);
> + /* Reached by the pre-post error paths (request never submitted), by
> + * the deliberate no-wait teardown, and by a genuine timeout (request
> + * posted, but no valid response arrived). In every case the sender
> + * must drop the response-side reference taken in
> + * mana_hwc_get_msg_index() and its own. Guard against a stale or
> + * forged response that raced in first: latch ->responded under the
> + * lock so any later handle_resp() is a no-op, and drop the response-
> + * side reference here only if handle_resp() has not already done so.
> + */
> + ctx = hwc->caller_ctx + msg_id;
> + spin_lock_irqsave(&ctx->lock, flags);
> + ctx->output_buf = NULL;
> + drop_resp_ref = !ctx->responded;
> + ctx->responded = true;
> + spin_unlock_irqrestore(&ctx->lock, flags);
> + if (drop_resp_ref)
> + refcount_dec(&ctx->refcnt);
> + hwc_ctx_put(hwc, ctx);
> +done:
> return err;
> }
[Severity: Medium]
The changelog states:
"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."
Does the out: path keep that invariant? Both the no-wait teardown
(wait_ms == 0) and the genuine timeout reach out: with the request already
posted, and out: force-drops the response-side reference itself before
dropping the sender's, so hwc_ctx_put() -> mana_hwc_put_msg_index() clears
the inflight bit while the command is still outstanding.
Reuse of that msg_id is then blocked only by the
"hwc_timed_out && hwc_timeout != 0" gate in mana_hwc_get_msg_index(),
which is deliberately bypassed exactly when hwc_timeout == 0, i.e. the
mana_serv_reset() window.
Two related documentation points:
The mana_hwc_handle_resp() comment says "the genuine response, the sender
or the teardown path still balances the references", but
mana_hwc_destroy_channel() does no refcount handling at all, it just does
kfree(hwc->caller_ctx).
The changelog says a stale or early response "drops only the
response-side reference". In the code an early response with
output_buf == NULL returns without touching the refcount at all.
> diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
> index 8340abd36af61..23bf83e2a3ec6 100644
> --- a/include/net/mana/hw_channel.h
> +++ b/include/net/mana/hw_channel.h
> @@ -171,8 +171,25 @@ struct hwc_caller_ctx {
> void *output_buf;
> u32 output_buflen;
>
> - u32 error; /* Linux error code */
> + int error; /* Linux error code (negative errno or 0) */
> u32 status_code;
> +
> + /* Protects output_buf against concurrent access from
> + * handle_resp() (CQ interrupt) and the sender timeout path.
> + */
> + spinlock_t lock;
> +
> + /* Tracks sender + handle_resp ownership. The last put
> + * (refcount reaches 0) releases the bitmap slot.
> + */
> + refcount_t refcnt;
> + u16 msg_id;
> +
> + /* Set under lock by the first handle_resp() for this slot so a
> + * duplicate or replayed response is dropped instead of consuming
> + * the response-side reference a second time.
> + */
> + bool responded;
> };
[Severity: Low]
This isn't a bug, but the responded comment says it is "Set under lock by
the first handle_resp() for this slot", while mana_hwc_send_request()'s
out: path sets it too:
drop_resp_ref = !ctx->responded;
ctx->responded = true;
on paths where no hardware response was received at all: pre-post
failures where the request was never submitted, the no-wait teardown, and
genuine timeouts. Should the comment say the flag means "response handled
or response-side reference cancelled"?
>
> struct hw_channel_context {
[ ... ]
> @@ -206,6 +224,11 @@ struct hw_channel_context {
> */
> u32 rx_leaked_wqe;
>
> + /* Set on first HWC timeout. Causes get_msg_index() to return
> + * -ETIMEDOUT instead of waiting, draining all queued senders.
> + */
> + bool hwc_timed_out;
> +
[Severity: Low]
This isn't a bug either, but the comment describes the reject and drain as
unconditional. The admission gate, the wait_event() condition and the
post-wait recheck in mana_hwc_get_msg_index() all qualify the latch with
hwc_timeout != 0.
With hwc_timeout == 0, the state mana_serv_reset() installs on a live
channel, new senders are still admitted, and senders already parked in
wait_event() are not released by the wake_up_all() that follows the latch:
they re-evaluate the condition, find it false and sleep again waiting for
a free bitmap slot. The flag is also not set for the wait_ms == 0 expiry.
Could the comment mention that qualification?
> /* 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net v6 7/7] net: mana: keep max_num_cqs immutable once cq_table is allocated
2026-08-11 2:38 [PATCH net v6 0/7] net: mana: HW channel reliability and hardening fixes Long Li
` (5 preceding siblings ...)
2026-08-11 2:38 ` [PATCH net v6 6/7] net: mana: fix stale HWC response after command timeout Long Li
@ 2026-08-11 2:38 ` Long Li
2026-08-12 23:47 ` Jakub Kicinski
6 siblings, 1 reply; 21+ messages in thread
From: Long Li @ 2026-08-11 2:38 UTC (permalink / raw)
To: Long Li, Konstantin Taranov, Jakub Kicinski, David S . Miller,
Paolo Abeni, Eric Dumazet, Andrew Lunn, Jason Gunthorpe,
Leon Romanovsky, Haiyang Zhang, K . Y . Srinivasan, Wei Liu,
Dexuan Cui, shradhagupta, Simon Horman, ernis, stephen
Cc: netdev, linux-rdma, linux-hyperv, linux-kernel
mana_hwc_init_event_handler() applied every HWC_INIT_DATA_MAX_NUM_CQS
event straight to gc->max_num_cqs. That handler stays live for the whole
channel lifetime -- it also services runtime reconfig and link events --
so it is not confined to the initial bootstrap.
gc->cq_table is allocated once, sized to the max_num_cqs seen at bootstrap,
and every reader (mana_gd_process_eqe(), mana_create_rxq() and
mana_create_txq()) bounds-checks a CQ index against gc->max_num_cqs before
indexing gc->cq_table. A device -- or a malicious host in a confidential
VM -- that sends a later HWC_INIT_DATA_MAX_NUM_CQS with a larger value
inflates the bound past the allocation. This includes an event timed to
land while mana_hwc_establish_channel() is between reading the count and
publishing cq_table. A subsequent out-of-range CQ id then passes the
bounds check and indexes cq_table out of bounds: an out-of-bounds read in
the EQ fast path, or an out-of-bounds pointer write in
mana_create_rxq()/mana_create_txq(), corrupting guest kernel memory.
Stop writing gc->max_num_cqs from the event handler. Store the reported
value in hwc_init_max_num_cqs, and let mana_hwc_establish_channel() commit
it to gc->max_num_cqs once, from the same snapshot that sizes cq_table.
The handler store uses WRITE_ONCE() and the establish-time read uses
READ_ONCE(), since the two run concurrently (EQ interrupt vs process
context); the single, non-reloadable read is what guarantees the value
that sizes cq_table is the same one published as the bound, even across
the sleeping vcalloc(). gc->max_num_cqs then always matches the
allocation and no later event can change the bound after the table is
published, so the existing bounds checks are sufficient.
Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v6:
- None.
Changes in v5:
- No code changes since v4 (resend as a standalone thread).
Changes in v4:
- New patch in v4, split out of the v3 teardown-safety work in response
to review: gc->max_num_cqs is set once when cq_table is allocated and
never reset, so a spoofed post-init HWC event cannot inflate the
bound past the allocation.
.../net/ethernet/microsoft/mana/hw_channel.c | 34 ++++++++++++++++---
include/net/mana/hw_channel.h | 1 +
2 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 759b65040a159339a5ab9a2eb95acaaa2e452f53..a29955e0473aa63fde6aa6bca7c85d5c3f423a73 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -262,7 +262,15 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
break;
case HWC_INIT_DATA_MAX_NUM_CQS:
- gd->gdma_context->max_num_cqs = val;
+ /* Store, don't apply: mana_hwc_establish_channel()
+ * commits this to gc->max_num_cqs once, together
+ * with sizing cq_table, so a spoofed post-init event
+ * cannot inflate the bound past the allocation.
+ * WRITE_ONCE() pairs with the READ_ONCE() there:
+ * this store runs in EQ interrupt context,
+ * concurrently with that process-context read.
+ */
+ WRITE_ONCE(hwc->hwc_init_max_num_cqs, val);
break;
case HWC_INIT_DATA_PDID:
@@ -868,6 +876,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);
@@ -902,15 +912,29 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
*max_req_msg_size = hwc->hwc_init_max_req_msg_size;
*max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;
- /* Both were set in mana_hwc_init_event_handler(). */
- if (WARN_ON(cq->id >= gc->max_num_cqs))
+ /* Snapshot the device-reported CQ count and CQ id into locals and
+ * use only the locals below, so the same value that sizes cq_table
+ * also bounds and indexes it -- even across the sleeping vcalloc().
+ * Both fields are written by mana_hwc_init_event_handler() from EQ
+ * interrupt context: hwc_init_max_num_cqs under WRITE_ONCE() (paired
+ * here), and cq->id as an ordinary store. READ_ONCE() keeps each
+ * read tear-free and, crucially, non-reloadable, so a spoofed
+ * post-init event cannot make the WARN_ON() pass against one value
+ * while the allocation or the index uses another.
+ */
+ num_cqs = READ_ONCE(hwc->hwc_init_max_num_cqs);
+ cq_id = READ_ONCE(cq->id);
+
+ if (WARN_ON(cq_id >= num_cqs))
return -EPROTO;
- cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
+ cq_table = vcalloc(num_cqs, sizeof(*cq_table));
if (!cq_table)
return -ENOMEM;
- rcu_assign_pointer(cq_table[cq->id], cq);
+ gc->max_num_cqs = num_cqs;
+
+ rcu_assign_pointer(cq_table[cq_id], cq);
/* Publish the fully-initialised table last; pairs with the
* rcu_dereference(gc->cq_table) in mana_gd_process_eqe().
*/
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 23bf83e2a3ec6a5b19ab54db0a65ae41b74ad74c..c275150baf490bc5565a78611a4029ff6d6baf02 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -203,6 +203,7 @@ struct hw_channel_context {
u16 hwc_init_q_depth_max;
u32 hwc_init_max_req_msg_size;
u32 hwc_init_max_resp_msg_size;
+ u32 hwc_init_max_num_cqs;
struct completion hwc_init_eqe_comp;
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net v6 7/7] net: mana: keep max_num_cqs immutable once cq_table is allocated
2026-08-11 2:38 ` [PATCH net v6 7/7] net: mana: keep max_num_cqs immutable once cq_table is allocated Long Li
@ 2026-08-12 23:47 ` Jakub Kicinski
0 siblings, 0 replies; 21+ messages in thread
From: Jakub Kicinski @ 2026-08-12 23:47 UTC (permalink / raw)
To: longli
Cc: Jakub Kicinski, kotaranov, davem, pabeni, edumazet, andrew+netdev,
jgg, leon, haiyangz, kys, wei.liu, decui, shradhagupta, horms,
ernis, stephen, netdev, linux-rdma, linux-hyperv, linux-kernel
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: keep max_num_cqs immutable once cq_table is allocated
The HWC init event handler no longer writes gc->max_num_cqs directly. The
device-reported CQ count is staged in hwc->hwc_init_max_num_cqs and
mana_hwc_establish_channel() commits it to gc->max_num_cqs once, from the
same snapshot that sizes and indexes cq_table.
No Link: tags present in the commit header.
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 759b65040a159..a29955e0473aa 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -262,7 +262,15 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
> break;
>
> case HWC_INIT_DATA_MAX_NUM_CQS:
> - gd->gdma_context->max_num_cqs = val;
> + /* Store, don't apply: mana_hwc_establish_channel()
> + * commits this to gc->max_num_cqs once, together
> + * with sizing cq_table, so a spoofed post-init event
> + * cannot inflate the bound past the allocation.
> + * WRITE_ONCE() pairs with the READ_ONCE() there:
> + * this store runs in EQ interrupt context,
> + * concurrently with that process-context read.
> + */
> + WRITE_ONCE(hwc->hwc_init_max_num_cqs, val);
> break;
>
> case HWC_INIT_DATA_PDID:
[ ... ]
> @@ -868,6 +876,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);
> @@ -902,15 +912,29 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
> *max_req_msg_size = hwc->hwc_init_max_req_msg_size;
> *max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;
>
> - /* Both were set in mana_hwc_init_event_handler(). */
> - if (WARN_ON(cq->id >= gc->max_num_cqs))
> + /* Snapshot the device-reported CQ count and CQ id into locals and
> + * use only the locals below, so the same value that sizes cq_table
> + * also bounds and indexes it -- even across the sleeping vcalloc().
> + * Both fields are written by mana_hwc_init_event_handler() from EQ
> + * interrupt context: hwc_init_max_num_cqs under WRITE_ONCE() (paired
> + * here), and cq->id as an ordinary store. READ_ONCE() keeps each
> + * read tear-free and, crucially, non-reloadable, so a spoofed
> + * post-init event cannot make the WARN_ON() pass against one value
> + * while the allocation or the index uses another.
> + */
> + num_cqs = READ_ONCE(hwc->hwc_init_max_num_cqs);
> + cq_id = READ_ONCE(cq->id);
> +
> + if (WARN_ON(cq_id >= num_cqs))
> return -EPROTO;
[Severity: Medium]
This isn't a bug introduced by this patch, the equivalent
WARN_ON(cq->id >= gc->max_num_cqs) exists at baseline, but should this
WARN_ON() stay on a condition that is fully device-controlled?
Both operands come from untrusted HWC bootstrap events handled in
mana_hwc_init_event_handler(): cq_id from HWC_INIT_DATA_CQID and num_cqs
from HWC_INIT_DATA_MAX_NUM_CQS.
hwc is kzalloc'd in mana_hwc_create_channel(), so if the host simply never
sends HWC_INIT_DATA_MAX_NUM_CQS then hwc_init_max_num_cqs stays 0 and the
condition is unconditionally true.
On a guest booted with panic_on_warn=1, does that turn a malformed or
missing device response into a kernel panic instead of the intended -EPROTO
rejection? Would a rate-limited dev_err() plus -EPROTO be preferable here?
>
> - cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
> + cq_table = vcalloc(num_cqs, sizeof(*cq_table));
> if (!cq_table)
> return -ENOMEM;
[Severity: Medium]
This isn't a bug introduced by this patch either, baseline already did
vcalloc(gc->max_num_cqs, ...) from the same device-reported value, but is
there a reason not to add an upper sanity bound on num_cqs while the value
is being snapshotted here?
The value field of the init event is 24 bits wide:
include/net/mana/hw_channel.h:
struct {
u32 value : 24;
u32 type : 8;
};
so a host can report up to 16,777,215 entries, which is roughly 128 MiB of
zeroed vmalloc memory held for the channel lifetime on a 64-bit guest.
A large count paired with a small, valid cq_id passes the cq_id >= num_cqs
check above.
vcalloc() uses array_size(), so there is no integer wrap, and an allocation
failure returns -ENOMEM cleanly; the concern is only the memory pressure on
a small confidential guest.
>
> - rcu_assign_pointer(cq_table[cq->id], cq);
> + gc->max_num_cqs = num_cqs;
> +
> + rcu_assign_pointer(cq_table[cq_id], cq);
[Severity: Medium]
This isn't a bug introduced by this patch, the pieces below all exist at
baseline f1b3416, but does the snapshot invariant documented above hold for
the index as well?
The comment says "the same value that sizes cq_table also bounds and
indexes it", yet only hwc_init_max_num_cqs is staged. The sibling case in
the same switch still applies a device-supplied CQ id straight to the live
queue object:
case HWC_INIT_DATA_CQID:
hwc->cq->gdma_cq->id = val;
break;
That handler stays registered for the whole channel lifetime, so after
cq_table[cq_id] is published, a later HWC_INIT_DATA_CQID event can make
cq->id differ from the index the CQ was installed at.
Once that happens, does mana_gd_process_eqe() drop every HWC completion for
the published index?
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_process_eqe() {
...
if (WARN_ON_ONCE(cq->type != GDMA_CQ || cq->id != cq_id))
break;
...
}
If so, all HWC commands would time out and latch hwc_timed_out, leaving the
control channel disabled after a single spoofed event.
The teardown side recomputes the slot from the live queue->id rather than
from the index used at publish time:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_unpublish_cq() {
u32 id = queue->id;
...
if (!cq_table || id >= gc->max_num_cqs)
return false;
if (rcu_access_pointer(cq_table[id]) != queue)
return false;
...
}
mana_gd_destroy_cq() treats false as "already unpublished" and returns
without synchronize_rcu(), and mana_gd_destroy_queue() then kfree()s the
queue. Can a mutated cq->id therefore leave a freed pointer in the still
published cq_table?
Teardown ordering appears to keep that stale slot unreachable today:
mana_hwc_destroy_cq() destroys the HWC EQ before the CQ, and every caller of
mana_hwc_destroy_channel() runs mana_rdma_remove()/mana_remove() first, so
no live EQ dispatches into the slot before rcu_replace_pointer(gc->cq_table,
NULL) + synchronize_rcu() + vfree(). So the observable effect looks like
the stalled control channel plus a WARN_ON_ONCE splat, with the freed
pointer surviving only by that ordering.
Would it make sense to stage HWC_INIT_DATA_CQID the same way, or to key
unpublish off the index the CQ was published at?
> /* Publish the fully-initialised table last; pairs with the
> * rcu_dereference(gc->cq_table) in mana_gd_process_eqe().
> */
> diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
> index 23bf83e2a3ec6..c275150baf490 100644
> --- a/include/net/mana/hw_channel.h
> +++ b/include/net/mana/hw_channel.h
> @@ -203,6 +203,7 @@ struct hw_channel_context {
> u16 hwc_init_q_depth_max;
> u32 hwc_init_max_req_msg_size;
> u32 hwc_init_max_resp_msg_size;
> + u32 hwc_init_max_num_cqs;
[Severity: Low]
Unrelated to the new field, but a few lines further down in the same struct
the setup_active comment added by the earlier teardown-safety patch in this
series does not match the code in three places:
include/net/mana/hw_channel.h:
/* 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;
mana_hwc_establish_channel() sets the flag before the setup call, not after
it succeeds, and its own inline comment says that is deliberate:
hwc->setup_active = true;
err = mana_smc_setup_hwc(&gc->shm_channel, false, ...);
mana_smc_teardown_hwc() has a single caller in the driver,
mana_hwc_destroy_channel(), which is also the only place setup_active is
cleared; establish_channel() explicitly does not tear down ("Do not also
tear down here").
There is no retry in establish_channel(); the teardown retry lives in
mana_hwc_create_channel():
if (gd->driver_data) {
mana_hwc_destroy_channel(gc);
if (gd->driver_data)
return -ETIMEDOUT;
}
Could the comment be updated to describe the actual set point, the single
clear site, and where the retry lives?
^ permalink raw reply [flat|nested] 21+ messages in thread