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

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

Pre-existing issues:
- [Critical] The NAPI struct for an RX/TX queue is scheduled before it is fully initialized, leading to a NULL pointer dereference (Kernel Panic).
- [High] Dangling `cq_table` entries after RAW/RSS QP destruction cause memory leaks and prevent future QP creation (Cross-process DoS).
--

--- Patch [1]: [PATCH net v4 1/7] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy ---
commit 855b59e3efafe6690d7dd204468e072eef10cb49
Author: Long Li <[email protected]>

net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy

This commit puts the cq_table under RCU protection to prevent a
use-after-free race between the EQ interrupt handler and CQ teardown. It
ensures the slot is cleared and an RCU grace period elapses before freeing
the CQ memory.

> 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
> @@ -131,12 +131,20 @@ static void mana_ib_cq_handler(void *ctx, struct gdma_queue *gdma_cq)
>  int mana_ib_install_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)
> +	/* No rcu_read_lock(): install/remove run within the IB device
> +	 * lifetime, which mana_rdma_remove() (ib_unregister_device) drains
> +	 * before the base cq_table can be freed.  See gdma_context::cq_table
> +	 * in gdma.h for why "true" is sound.
> +	 */
> +	cq_table = rcu_dereference_protected(gc->cq_table, true);
> +	if (!cq_table || cq->queue.id >= gc->max_num_cqs)
>  		return -EINVAL;
> +
>  	/* Create CQ table entry, sharing a CQ between WQs is not supported */
> -	if (gc->cq_table[cq->queue.id])
> +	if (rcu_access_pointer(cq_table[cq->queue.id]))
[Severity: High]
This isn't a bug introduced by this patch, but could this fail permanently due
to a dangling pointer?

If a userspace application creates an RDMA QP with an existing CQ,
mana_ib_create_qp_raw() allocates a WQ/CQ pair and overwrites
send_cq->queue.id. When the QP is destroyed, mana_ib_destroy_qp_raw() tears
down the WQ/CQ in hardware but does not call mana_ib_remove_cq_cb().

This leaves a dangling pointer in cq_table. If the hardware reallocates the
same CQ ID to another process or queue, won't this check see the leaked entry
and prevent further QP creation?

>  		return -EINVAL;
>  	if (cq->queue.kmem)
>  		gdma_cq = cq->queue.kmem;

[ ... ]

> 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

[ ... ]

> @@ -2596,12 +2619,18 @@ static int mana_create_txq(struct mana_port_context *apc,
>  
>  		cq->gdma_id = cq->gdma_cq->id;
>  
> -		if (WARN_ON(cq->gdma_id >= gc->max_num_cqs)) {
> +		/* No rcu_read_lock(): mana_create_txq runs under RTNL during
> +		 * netdev bring-up, inside the netdev lifetime that
> +		 * mana_remove() drains before the base cq_table can be freed.
> +		 * See gdma_context::cq_table in gdma.h for why "true" is sound.
> +		 */
> +		cq_table = rcu_dereference_protected(gc->cq_table, true);
> +		if (WARN_ON(!cq_table || cq->gdma_id >= gc->max_num_cqs)) {
>  			err = -EINVAL;
>  			goto out;
>  		}
>  
> -		gc->cq_table[cq->gdma_id] = cq->gdma_cq;
> +		rcu_assign_pointer(cq_table[cq->gdma_id], cq->gdma_cq);
[Severity: Critical]
This is a pre-existing issue, but by publishing the CQ to cq_table before the
NAPI struct is initialized, does this open a race window that could lead to a
crash?

Once rcu_assign_pointer() publishes the CQ to cq_table, an early hardware
completion could trigger the EQ interrupt handler mana_gd_process_eqe(), which
can access the published CQ and call mana_schedule_napi().

>  
>  		mana_create_txq_debugfs(apc, i);
>  
> @@ -2621,25 +2650,20 @@ static int mana_create_txq(struct mana_port_context *apc,

[ ... ]

> @@ -2905,12 +2966,18 @@ static struct mana_rxq *mana_create_rxq(struct mana_port_context *apc,
>  	if (err)
>  		goto out;
>  
> -	if (WARN_ON(cq->gdma_id >= gc->max_num_cqs)) {
> +	/* No rcu_read_lock(): mana_create_rxq runs under RTNL during netdev
> +	 * bring-up, inside the netdev lifetime that mana_remove() drains
> +	 * before the base cq_table can be freed.  See gdma_context::cq_table
> +	 * in gdma.h for why "true" is sound.
> +	 */
> +	cq_table = rcu_dereference_protected(gc->cq_table, true);
> +	if (WARN_ON(!cq_table || cq->gdma_id >= gc->max_num_cqs)) {
>  		err = -EINVAL;
>  		goto out;
>  	}
>  
> -	gc->cq_table[cq->gdma_id] = cq->gdma_cq;
> +	rcu_assign_pointer(cq_table[cq->gdma_id], cq->gdma_cq);
[Severity: Critical]
This is a pre-existing issue, but since mana_push_wqe(rxq) has already posted
buffers to the hardware just before this, couldn't a network packet be
received at this point?

Since netif_napi_add_weight_locked() hasn't run yet, would an early hardware
interrupt cause the EQ handler to invoke napi_schedule() on an uninitialized
NAPI struct?

>  
>  	netif_napi_add_weight_locked(ndev, &cq->napi, mana_poll, 1);
>

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