* [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-11 2:38 ` [PATCH net v6 2/7] net: mana: fix HWC RQ/SQ buffer size swap Long Li
` (5 subsequent siblings)
6 siblings, 1 reply; 10+ 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] 10+ 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
0 siblings, 1 reply; 10+ 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] 10+ 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; 10+ 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] 10+ 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-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, 0 replies; 10+ 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] 10+ 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-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, 0 replies; 10+ 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] 10+ 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-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, 0 replies; 10+ 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] 10+ 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-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, 0 replies; 10+ 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] 10+ 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-11 2:38 ` [PATCH net v6 7/7] net: mana: keep max_num_cqs immutable once cq_table is allocated Long Li
6 siblings, 0 replies; 10+ 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] 10+ 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
6 siblings, 0 replies; 10+ 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] 10+ messages in thread