Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Missing pointer verification in `mana_gd_unpublish_cq()` and `mana_ib_remove_cq_cb()` causes them to incorrectly clear valid, concurrent CQs (double-unpublish race) and inadvertently unpublish CQ 0 on error paths. --
--- Patch [1]: [PATCH net v5 1/7] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy --- commit 882f7728097f3bfd4b17e15e31d6c14542d507c2 Author: Long Li <[email protected]> Subject: net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy This patch puts the gc->cq_table under RCU protection to prevent a use-after-free race between the EQ interrupt handler and concurrent CQ teardowns. It also introduces a two-pass batched teardown to optimize synchronize_rcu() usage during network device teardowns. > diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c > index f2547989f4229..2bf4be21cede7 100644 > --- a/drivers/infiniband/hw/mana/cq.c > +++ b/drivers/infiniband/hw/mana/cq.c [ ... ] > @@ -149,23 +157,49 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, > struct mana_ib_cq *cq) > 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) > { [ ... ] > + /* Removers for a given CQ are serialized by the IB core, so the slot > + * is read and cleared without rcu_read_lock() or atomicity: a CQ is > + * never torn down while a live QP references it (cq->usecnt), nor > + * while the QP-create that installed the entry is still running (that > + * create holds a reference on the CQ uobject across its error path, > + * before usecnt is taken). Any double-remove is therefore sequential > + * -- the later caller sees the NULL stored below and returns. > + */ > + gdma_cq = rcu_dereference_protected(cq_table[cq->queue.id], true); > + if (!gdma_cq) > + return; /* already removed by a prior teardown path */ > + > + rcu_assign_pointer(cq_table[cq->queue.id], NULL); [Severity: High] Does this RDMA cleanup path need to verify that the gdma_cq read from the table actually matches the expected one (such as cq->queue.kmem)? If a hardware ID was reused by a concurrent operation, could this clear a newly created and published CQ? > + > + /* Wait for in-flight EQ handlers that may have loaded the old > + * pointer via rcu_dereference() to finish before freeing. > + */ > + synchronize_rcu(); > + kfree(gdma_cq); > } > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c > b/drivers/net/ethernet/microsoft/mana/gdma_main.c > index e8b7ffb47eb98..7714040d1df42 100644 > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c [ ... ] > @@ -1050,18 +1052,41 @@ 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; > + > + if (!rcu_access_pointer(cq_table[id])) > + return false; > + > + rcu_assign_pointer(cq_table[id], NULL); > + return true; > +} [Severity: High] Should this check if the pointer actually matches the queue being unpublished? If a hardware ID is 0 on a failure path, could this unintentionally clear the slot for whatever valid CQ currently holds ID 0? Verifying that rcu_access_pointer(cq_table[id]) equals the queue variable could prevent blindly clearing the wrong CQ. > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c > b/drivers/net/ethernet/microsoft/mana/mana_en.c > index 92bb55935c1c4..5d215981bba83 100644 > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c [ ... ] > @@ -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); [Severity: High] Does this create a race window during the two-pass teardown? Since mana_deinit_cq() calls mana_gd_destroy_cq(), which in turn calls mana_gd_unpublish_cq() a second time for the same queue, could a newly created RDMA CQ that recycled this ID during the synchronize_rcu() wait be incorrectly destroyed here? If mana_gd_unpublish_cq() only checks for a non-NULL slot, it seems like it might blindly clear the newly published CQ on this second pass. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
