Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] Concurrent QP creations sharing the same CQ cause a data race on `send_cq->queue.id`, leaking CQ table entries and triggering a double free of `gdma_cq` in the error path. - [High] The EQ event handler `mana_gd_process_eqe()` triggers a false positive `WARN_ON_ONCE(!cq)` when encountering stale EQEs for a recently destroyed CQ during normal teardown. - [High] `mana_gd_destroy_queue()` frees EQ queues immediately after `list_del_rcu()` without waiting for an RCU grace period, causing a use-after-free if an interrupt handler is concurrently traversing the EQ list. -- --- Patch [1]: [PATCH net-next v2 1/7] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy --- commit 4df31c26faa213971de40d92e7d0a48ab9e5d300 Author: Long Li <[email protected]> net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy This commit fixes a use-after-free race between the EQ interrupt handler and CQ teardown by placing the cq_table under RCU protection. It annotates the table, updates readers to use rcu_dereference, and ensures removers clear the slot and call synchronize_rcu() before freeing the CQ. > diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c > --- 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) > 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); > + > + /* 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); > } [Severity: Critical] This is a pre-existing issue, but does this actually prevent double frees during concurrent QP creation failures? If multiple concurrent ib_create_qp() calls share the same send_cq, the IB core takes a read lock. In mana_ib_create_qp_raw(), the shared send_cq->queue.id is locklessly overwritten: drivers/infiniband/hw/mana/qp.c:mana_ib_create_qp_raw() { ... qp->raw_sq.id = wq_spec.queue_index; send_cq->queue.id = cq_spec.queue_index; ... } If both threads fail after this point (e.g., due to an invalid user pointer), they both jump to the error path and call mana_ib_remove_cq_cb(). Since the removers are not serialized in this error path, couldn't they both read the same gdma_cq pointer, both clear it, wait for synchronize_rcu(), and then both call kfree(gdma_cq)? > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c > b/drivers/net/ethernet/microsoft/mana/gdma_main.c > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c [ ... ] > @@ -772,10 +773,11 @@ 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)) > + cq_table = rcu_dereference(gc->cq_table); > + if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs || !cq_table)) > break; > > - cq = gc->cq_table[cq_id]; > + cq = rcu_dereference(cq_table[cq_id]); > if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id)) > break; [Severity: High] This isn't a bug introduced by this patch, but could this WARN_ON_ONCE trigger as a false positive during normal teardown? During interface teardown, mana_deinit_cq() clears cq_table[id] and waits for an RCU grace period, but the associated EQ remains active until mana_destroy_eq() is called later. If the EQ already contains a completion event for the destroyed CQ, the interrupt handler will process the stale EQE, read a NULL pointer for cq, and hit this warning. Since panic_on_warn is common in cloud environments, could this crash the system during a standard interface teardown? [ ... ] > @@ -1082,15 +1084,28 @@ static void mana_gd_create_cq(const struct > gdma_queue_spec *spec, > static void mana_gd_destroy_cq(struct gdma_context *gc, > struct gdma_queue *queue) > { [ ... ] > + /* Wait for in-flight EQ handlers that may have loaded the old > + * pointer via rcu_dereference() to finish before the caller > + * frees the CQ memory. > + */ > + synchronize_rcu(); > } [Severity: High] This is a pre-existing issue, but does mana_gd_destroy_queue() need a similar synchronize_rcu() to prevent a use-after-free? While mana_gd_intr() traverses the EQ list under RCU: drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_intr() { rcu_read_lock(); list_for_each_entry_rcu(eq, &gic->eq_list, entry) mana_gd_process_eqe(eq); rcu_read_unlock(); } The teardown path removes the queue via list_del_rcu() but then immediately frees the memory without waiting for a grace period: drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_destroy_queue() { ... mana_gd_destroy_dma_region(gc, gmi->dma_region_handle); mana_gd_free_memory(gmi); kfree(queue); } Could a concurrent reader in mana_gd_intr() dereference the freed EQ if the memory is released before the RCU grace period ends? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
