The EQ interrupt handler (mana_gd_process_eqe) looks up the completing CQ
in gc->cq_table[cq_id] and runs its callback. cq_table was a plain array,
read without RCU and freed without a grace period, so a concurrent CQ
teardown races the lookup into a use-after-free:
CPU A (mana_gd_intr, hard IRQ) CPU B (CQ destroy)
---------------------------------- ------------------------------
cq = gc->cq_table[cq_id]; // valid
gc->cq_table[id] = NULL;
kfree(cq); // freed
cq->cq.callback(ctx, cq); // use-after-free
Reference-count the CQ, like the driver's existing QP get/put. Mark
cq_table __rcu and look it up under the handler's rcu_read_lock():
mana_gd_get_cq() takes a reference with refcount_inc_not_zero() and
mana_gd_put_cq() drops it after the callback. Teardown clears the slot,
drops the publish reference, waits for any in-flight handler, then frees
the CQ with kfree_rcu().
On the RDMA destroy path (mana_ib_destroy_cq) clear the dispatch entry
before destroying the HW CQ. A late completion then finds an empty slot
and is dropped, and a cq_id the device recycles cannot alias the
outgoing entry.
The cq_id bound is hardened in a later patch.
Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network
Adapter (MANA)")
Signed-off-by: Long Li <[email protected]>
---
Changes since v6:
Reworked from the v6 lock-based RCU scheme to lockless reference
counting per review feedback (Leon Romanovsky):
- Dropped gc->cq_table_lock; lookups take a reference under RCU with
refcount_inc_not_zero() and the CQ is freed via kfree_rcu().
- publish/unpublish are lockless; the EQ-handler lookup uses
smp_load_acquire() paired with the release-store that publishes the
table.
- Reordered mana_ib_destroy_cq() to detach the software callback before
destroying the hardware CQ, closing a CQ-id recycle window.
- Retitled (was "RCU-protect gc->cq_table lookups against concurrent CQ
destroy").
drivers/infiniband/hw/mana/cq.c | 41 ++++---
.../net/ethernet/microsoft/mana/gdma_main.c | 100 +++++++++++++++---
.../net/ethernet/microsoft/mana/hw_channel.c | 29 +++--
drivers/net/ethernet/microsoft/mana/mana_en.c | 8 +-
include/net/mana/gdma.h | 23 +++-
5 files changed, 160 insertions(+), 41 deletions(-)
diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
index
f2547989f422901075fa19a1ba48daf3e9a1ec96..022c82479ef6c47d78bc64b638f18aa25bbd3308
100644
--- a/drivers/infiniband/hw/mana/cq.c
+++ b/drivers/infiniband/hw/mana/cq.c
@@ -108,11 +108,12 @@ int mana_ib_destroy_cq(struct ib_cq *ibcq, struct
ib_udata *udata)
mdev = container_of(ibdev, struct mana_ib_dev, ib_dev);
+ /* Detach the dispatch entry first, then stop the HW CQ and free the
+ * queue. A completion racing teardown then finds an empty slot, and
+ * a recycled cq_id cannot alias this CQ. Errors are logged inside.
+ */
mana_ib_remove_cq_cb(mdev, cq);
- /* Ignore return code as there is not much we can do about it.
- * The error message is printed inside.
- */
mana_ib_gd_destroy_cq(mdev, cq);
mana_ib_destroy_queue(mdev, &cq->queue);
@@ -132,12 +133,8 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct
mana_ib_cq *cq)
{
struct gdma_context *gc = mdev_to_gc(mdev);
struct gdma_queue *gdma_cq;
+ int err;
- if (cq->queue.id >= gc->max_num_cqs)
- return -EINVAL;
- /* Create CQ table entry, sharing a CQ between WQs is not supported */
- if (gc->cq_table[cq->queue.id])
- return -EINVAL;
if (cq->queue.kmem)
gdma_cq = cq->queue.kmem;
else
@@ -149,23 +146,41 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev,
struct mana_ib_cq *cq)
gdma_cq->type = GDMA_CQ;
gdma_cq->cq.callback = mana_ib_cq_handler;
gdma_cq->id = cq->queue.id;
- gc->cq_table[cq->queue.id] = gdma_cq;
- return 0;
+
+ err = mana_gd_publish_cq(gc, gdma_cq);
+ if (err && !cq->queue.kmem)
+ kfree(gdma_cq);
+
+ return err;
}
void mana_ib_remove_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
{
struct gdma_context *gc = mdev_to_gc(mdev);
+ struct gdma_queue __rcu **cq_table;
+ struct gdma_queue *gdma_cq;
- if (cq->queue.id >= gc->max_num_cqs || cq->queue.id == INVALID_QUEUE_ID)
+ if (cq->queue.id == INVALID_QUEUE_ID || cq->queue.id >= gc->max_num_cqs)
return;
if (cq->queue.kmem)
/* Then it will be cleaned and removed by the mana */
return;
- kfree(gc->cq_table[cq->queue.id]);
- gc->cq_table[cq->queue.id] = NULL;
+ rcu_read_lock();
+ cq_table = READ_ONCE(gc->cq_table);
+ gdma_cq = cq_table ? rcu_dereference(cq_table[cq->queue.id]) : NULL;
+ /* Match the CQ under RCU so the slot cannot be freed mid-check. */
+ if (gdma_cq && gdma_cq->cq.context != cq)
+ gdma_cq = NULL;
+ rcu_read_unlock();
+
+ if (!gdma_cq)
+ return;
+
+ /* Remove from the table, then free after a grace period. */
+ mana_gd_unpublish_cq(gc, gdma_cq);
+ kfree_rcu(gdma_cq, rcu);
}
int mana_ib_arm_cq(struct ib_cq *ibcq, enum ib_cq_notify_flags flags)
diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index
e8b7ffb47eb982d139b80bc4fb4bbb0ad5307962..b29e078b419b3c16326ad890c8e97401e1d3f3a9
100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -724,6 +724,9 @@ int mana_schedule_serv_work(struct gdma_context *gc, enum
gdma_eqe_type type)
return 0;
}
+static struct gdma_queue *mana_gd_get_cq(struct gdma_context *gc, u32 cq_id);
+static void mana_gd_put_cq(struct gdma_queue *cq);
+
static void mana_gd_process_eqe(struct gdma_queue *eq)
{
u32 head = eq->head % (eq->queue_size / GDMA_EQE_SIZE);
@@ -743,16 +746,16 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
switch (type) {
case GDMA_EQE_COMPLETION:
cq_id = eqe->details[0] & 0xFFFFFF;
- if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
- break;
-
- cq = gc->cq_table[cq_id];
- if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id))
+ cq = mana_gd_get_cq(gc, cq_id);
+ /* CQ already torn down: stale completion, drop it. */
+ if (!cq)
break;
- if (cq->cq.callback)
+ if (!WARN_ON_ONCE(cq->type != GDMA_CQ || cq->id != cq_id) &&
+ cq->cq.callback)
cq->cq.callback(cq->cq.context, cq);
+ mana_gd_put_cq(cq);
break;
case GDMA_EQE_TEST_EVENT:
@@ -1050,18 +1053,81 @@ static void mana_gd_create_cq(const struct
gdma_queue_spec *spec,
queue->cq.callback = spec->cq.callback;
}
-static void mana_gd_destroy_cq(struct gdma_context *gc,
- struct gdma_queue *queue)
+static struct gdma_queue *mana_gd_get_cq(struct gdma_context *gc, u32 cq_id)
{
- u32 id = queue->id;
+ struct gdma_queue __rcu **cq_table;
+ struct gdma_queue *cq = NULL;
- if (id >= gc->max_num_cqs)
- return;
+ /* IRQ reader: a stray completion can race the table publish in
+ * mana_hwc_establish_channel(), so the acquire pairs with its
+ * smp_store_release() to see a consistent table and bound.
+ */
+ cq_table = smp_load_acquire(&gc->cq_table);
+ if (cq_table && cq_id < gc->max_num_cqs) {
+ cq = rcu_dereference(cq_table[cq_id]);
+ /* Fails if the CQ is being torn down. */
+ if (cq && !refcount_inc_not_zero(&cq->cq.refcount))
+ cq = NULL;
+ }
+
+ return cq;
+}
- if (!gc->cq_table[id])
+static void mana_gd_put_cq(struct gdma_queue *cq)
+{
+ if (cq && refcount_dec_and_test(&cq->cq.refcount))
+ complete(&cq->cq.free);
+}
+
+int mana_gd_publish_cq(struct gdma_context *gc, struct gdma_queue *queue)
+{
+ struct gdma_queue __rcu **cq_table;
+
+ /* Only mana_gd_get_cq() (IRQ) races the table publish and needs the
+ * acquire; this control path does not.
+ */
+ cq_table = READ_ONCE(gc->cq_table);
+ if (!cq_table || queue->id >= gc->max_num_cqs)
+ return -EINVAL;
+
+ /* Sharing a CQ between WQs is not supported. */
+ if (rcu_access_pointer(cq_table[queue->id]))
+ return -EINVAL;
+
+ refcount_set(&queue->cq.refcount, 1);
+ init_completion(&queue->cq.free);
+ rcu_assign_pointer(cq_table[queue->id], queue);
+
+ return 0;
+}
+EXPORT_SYMBOL_NS(mana_gd_publish_cq, "NET_MANA");
+
+void mana_gd_unpublish_cq(struct gdma_context *gc, struct gdma_queue *queue)
+{
+ struct gdma_queue __rcu **cq_table;
+
+ /* Only mana_gd_get_cq() (IRQ) races the table publish and needs the
+ * acquire; this control path does not.
+ */
+ cq_table = READ_ONCE(gc->cq_table);
+ if (!cq_table || queue->id >= gc->max_num_cqs ||
+ rcu_access_pointer(cq_table[queue->id]) != queue)
return;
- gc->cq_table[id] = NULL;
+ rcu_assign_pointer(cq_table[queue->id], NULL);
+
+ /* Drop the publish reference and wait for any handler that already
+ * took one, so the caller can free the CQ.
+ */
+ mana_gd_put_cq(queue);
+ wait_for_completion(&queue->cq.free);
+}
+EXPORT_SYMBOL_NS(mana_gd_unpublish_cq, "NET_MANA");
+
+static void mana_gd_destroy_cq(struct gdma_context *gc,
+ struct gdma_queue *queue)
+{
+ mana_gd_unpublish_cq(gc, queue);
}
int mana_gd_create_hwc_queue(struct gdma_dev *gd,
@@ -1333,7 +1399,13 @@ void mana_gd_destroy_queue(struct gdma_context *gc,
struct gdma_queue *queue)
mana_gd_destroy_dma_region(gc, gmi->dma_region_handle);
mana_gd_free_memory(gmi);
- kfree(queue);
+ /* The EQ handler may still be looking this CQ up; free it after a
+ * grace period.
+ */
+ if (queue->type == GDMA_CQ)
+ kfree_rcu(queue, rcu);
+ else
+ kfree(queue);
}
EXPORT_SYMBOL_NS(mana_gd_destroy_queue, "NET_MANA");
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index
e3c24d50dad07c65be9e94129dc09af9264f9f8d..b5ed2dbce6ceb7f7a5196dfe5ba3534eb4c5d330
100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -674,6 +674,7 @@ static int mana_hwc_establish_channel(struct gdma_context
*gc, u16 *q_depth,
struct gdma_queue *sq = hwc->txq->gdma_wq;
struct gdma_queue *eq = hwc->cq->gdma_eq;
struct gdma_queue *cq = hwc->cq->gdma_cq;
+ struct gdma_queue __rcu **cq_table;
int err;
init_completion(&hwc->hwc_init_eqe_comp);
@@ -698,11 +699,19 @@ static int mana_hwc_establish_channel(struct gdma_context
*gc, u16 *q_depth,
if (WARN_ON(cq->id >= gc->max_num_cqs))
return -EPROTO;
- gc->cq_table = vcalloc(gc->max_num_cqs, sizeof(struct gdma_queue *));
- if (!gc->cq_table)
+ cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
+ if (!cq_table)
return -ENOMEM;
- gc->cq_table[cq->id] = cq;
+ /* Publish the initialised table; pairs with smp_load_acquire()
+ * in mana_gd_get_cq().
+ */
+ smp_store_release(&gc->cq_table, cq_table);
+
+ /* Publish the HWC CQ now that the table is in place. */
+ err = mana_gd_publish_cq(gc, cq);
+ if (WARN_ON(err))
+ return err;
return 0;
}
@@ -811,6 +820,7 @@ int mana_hwc_create_channel(struct gdma_context *gc)
void mana_hwc_destroy_channel(struct gdma_context *gc)
{
struct hw_channel_context *hwc = gc->hwc.driver_data;
+ struct gdma_queue __rcu **old_cq_table;
if (!hwc)
return;
@@ -818,10 +828,8 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
/* gc->max_num_cqs is set in mana_hwc_init_event_handler(). If it's
* non-zero, the HWC worked and we should tear down the HWC here.
*/
- if (gc->max_num_cqs > 0) {
+ if (gc->max_num_cqs > 0)
mana_smc_teardown_hwc(&gc->shm_channel, false);
- gc->max_num_cqs = 0;
- }
if (hwc->txq)
mana_hwc_destroy_wq(hwc, hwc->txq);
@@ -832,6 +840,11 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
if (hwc->cq)
mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
+ /* Reset only after mana_hwc_destroy_cq() has cleared the CQ table
+ * slot, so it is not left dangling.
+ */
+ gc->max_num_cqs = 0;
+
kfree(hwc->caller_ctx);
hwc->caller_ctx = NULL;
@@ -848,8 +861,10 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
gc->hwc.driver_data = NULL;
gc->hwc.gdma_context = NULL;
- vfree(gc->cq_table);
+ old_cq_table = gc->cq_table;
gc->cq_table = NULL;
+ /* All EQs are gone, so no EQ handler can be using the table. */
+ vfree(old_cq_table);
}
int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
b/drivers/net/ethernet/microsoft/mana/mana_en.c
index
92bb55935c1c4e76e3912794eb3c4483fb331821..515b39f085c0d6519cc90a53b8d879c07e33264e
100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -2596,13 +2596,11 @@ static int mana_create_txq(struct mana_port_context
*apc,
cq->gdma_id = cq->gdma_cq->id;
- if (WARN_ON(cq->gdma_id >= gc->max_num_cqs)) {
+ if (WARN_ON(mana_gd_publish_cq(gc, cq->gdma_cq))) {
err = -EINVAL;
goto out;
}
- gc->cq_table[cq->gdma_id] = cq->gdma_cq;
-
mana_create_txq_debugfs(apc, i);
set_bit(NAPI_STATE_NO_BUSY_POLL, &cq->napi.state);
@@ -2905,13 +2903,11 @@ static struct mana_rxq *mana_create_rxq(struct
mana_port_context *apc,
if (err)
goto out;
- if (WARN_ON(cq->gdma_id >= gc->max_num_cqs)) {
+ if (WARN_ON(mana_gd_publish_cq(gc, cq->gdma_cq))) {
err = -EINVAL;
goto out;
}
- gc->cq_table[cq->gdma_id] = cq->gdma_cq;
-
netif_napi_add_weight_locked(ndev, &cq->napi, mana_poll, 1);
WARN_ON(xdp_rxq_info_reg(&rxq->xdp_rxq, ndev, rxq_idx,
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index
0c395917b2144ec4c2faafa5d6c7de7a452f1ebf..abf243358bf82e2428478cb3cf2f387d9cd9ea28
100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -333,6 +333,9 @@ struct gdma_queue {
u32 tail;
struct list_head entry;
+ /* For kfree_rcu(): CQs are looked up locklessly from the EQ handler. */
+ struct rcu_head rcu;
+
/* Extra fields specific to EQ/CQ. */
union {
struct {
@@ -352,6 +355,12 @@ struct gdma_queue {
void *context;
struct gdma_queue *parent; /* For CQ/EQ relationship */
+
+ /* Keep the CQ alive while the EQ handler runs its
+ * callback; teardown waits on @free.
+ */
+ refcount_t refcount;
+ struct completion free;
} cq;
};
};
@@ -418,7 +427,11 @@ struct gdma_context {
/* This maps a CQ index to the queue structure. */
unsigned int max_num_cqs;
- struct gdma_queue **cq_table;
+ /* Entries are published/cleared by CQ create/destroy and read
+ * locklessly by the EQ handler under RCU. max_num_cqs is the table
+ * size; NULL means the table is torn down.
+ */
+ struct gdma_queue __rcu **cq_table;
/* Protect eq_test_event and test_event_eq_id */
struct mutex eq_test_event_mutex;
@@ -496,6 +509,14 @@ int mana_gd_create_mana_wq_cq(struct gdma_dev *gd,
void mana_gd_destroy_queue(struct gdma_context *gc, struct gdma_queue *queue);
+/* Add a CQ to cq_table so the EQ handler can dispatch to it. Returns
+ * -EINVAL if the id is out of range or already in use.
+ */
+int mana_gd_publish_cq(struct gdma_context *gc, struct gdma_queue *queue);
+
+/* Remove a CQ from cq_table and wait for the EQ handler to stop using it. */
+void mana_gd_unpublish_cq(struct gdma_context *gc, struct gdma_queue *queue);
+
int mana_gd_poll_cq(struct gdma_queue *cq, struct gdma_comp *comp, int
num_cqe);
void mana_gd_ring_cq(struct gdma_queue *cq, u8 arm_bit);
--
2.43.0
