Re: [PATCH net-next v2 1/7] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy

[sashiko-bot@kernel.org]

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 <longli@microsoft.com>

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/20260721234339.1476932-1-longli@microsoft.com?part=1