Re: [PATCH net v7 1/7] net: mana: reference-count CQs looked up from the EQ handler

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix for CQ ID recycle window on Ethernet and RDMA kernel CQs
--

--- Patch [1]: [PATCH net v7 1/7] net: mana: reference-count CQs looked up from the EQ handler ---
commit 26834a69bfec0a9b46df2c9aa1aa0f248272df13
Author: Long Li <[email protected]>

net: mana: reference-count CQs looked up from the EQ handler

This patch introduces reference counting for CQs looked up from the EQ
handler in the MANA driver to prevent a use-after-free. It also reorders
the RDMA teardown path to clear the dispatch entry before destroying the
hardware CQ, aiming to close a window where a recycled cq_id could alias
the outgoing entry.

> diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
> index f2547989f4229..022c82479ef6c 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);

[ ... ]

> @@ -149,23 +146,41 @@ 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)
>  {
>  	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;

[Severity: High]
This is a pre-existing issue, but does this early return leave the CQ ID
recycle window open for RDMA kernel CQs?

For kernel CQs, mana_ib_remove_cq_cb() returns early, bypassing the software
CQ unpublish step that was explicitly added to close the recycle window.
Because it skips this, the hardware CQ is destroyed via
mana_ib_gd_destroy_cq() before the queue is torn down.

If the hardware immediately frees the cq_id, a concurrent thread could
create a new CQ, be assigned the same cq_id, and fail with -EINVAL in
mana_gd_publish_cq() because the old CQ is still in gc->cq_table.

Also, does this same race exist in the Ethernet paths? Looking at
mana_destroy_rxq() in drivers/net/ethernet/microsoft/mana/mana_en.c,
the hardware object is destroyed before mana_deinit_cq() removes the
old software CQ from the dispatch table:

mana_destroy_rxq() {
	...
	if (rxq->rxobj != INVALID_MANA_HANDLE)
		mana_destroy_wq_obj(apc, GDMA_RQ, rxq->rxobj);

	mana_deinit_cq(apc, &rxq->rx_cq);
	...
}

A similar inverted destruction sequence seems to exist in mana_destroy_txq().
Could this lead to spurious queue creation failures and permanently detached
network interfaces after operations like MTU changes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1