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

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