Re: [PATCH net-next v7 02/13] net: mana: share the EQ pool across a queue-set swap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mana_ib_remove_cq_cb: Use-After-Free due to missing RCU synchronization
- [High] net: mana: Unbounded RTNL stall during queue teardown if EQ is dead
- [High] mana: NULL pointer dereference in mana_gd_process_eqe()
--

commit 5cede1970aed52e8e7cc0e961f84924a35708274
Author: Long Li <longli@microsoft.com>

net: mana: share the EQ pool across a queue-set swap

This patch makes the EQ pool port-owned so overlapping queue sets share EQs
instead of requiring new allocations. It initializes EQ callbacks and
throttles before IRQ publication, introduces a lockless RCU pattern for
CQ table publication, and modifies CQ teardown to flush events and wait
for RCU readers.

> diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
> index d4e5e3f912689..0b6ad2547e0ef 100644
> --- a/drivers/infiniband/hw/mana/cq.c
> +++ b/drivers/infiniband/hw/mana/cq.c
> @@ -155,7 +155,8 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq)
>  	gdma_cq->type = GDMA_CQ;
>  	gdma_cq->cq.callback = mana_ib_cq_handler;
>  	gdma_cq->id = cq->queue.id;
> -	gc->cq_table[cq->queue.id] = gdma_cq;
> +	/* Pairs with the acquire load in mana_gd_process_eqe(). */
> +	smp_store_release(&gc->cq_table[cq->queue.id], gdma_cq);
>  	return 0;
>  }

[Severity: High]
This patch converts the IB CQ table publication into a lockless RCU pattern,
but does the teardown path in mana_ib_remove_cq_cb() also need to be updated?

If a hardware EQ completion occurs concurrently while mana_ib_remove_cq_cb()
is tearing down the CQ, it calls kfree(gc->cq_table[cq->queue.id]) directly
without an RCU grace period. The hardirq handler (mana_gd_process_eqe)
reads this via smp_load_acquire() and dereferences the freed pointer,
leading to a use-after-free.

Should mana_ib_remove_cq_cb() use cmpxchg() and synchronize_rcu() for
teardown, similar to what was added for Ethernet CQs in
mana_gd_destroy_cq()?

[ ... ]
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index ae0ed700b3b93..078b3666947a4 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -768,7 +768,8 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
>  		if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
>  			break;
>  
> -		cq = gc->cq_table[cq_id];
> +		/* Match release publication of the CQ and its callback state. */
> +		cq = smp_load_acquire(&gc->cq_table[cq_id]);

[Severity: High]
Can gc->cq_table be NULL when this acquire load executes?

During channel initialization, mana_hwc_establish_channel() communicates
with the hardware. The hardware begins executing and sends
GDMA_EQE_HWC_INIT_DATA events to the EQ, including HWC_INIT_DATA_MAX_NUM_CQS,
which populates gc->max_num_cqs.

However, gc->cq_table is not allocated via vcalloc() until after the
hardware initialization completes and mana_hwc_establish_channel() resumes.
If the hardware sends a spurious or unexpected GDMA_EQE_COMPLETION event in
this window, the bounds check passes because gc->max_num_cqs is already set.
The handler then executes smp_load_acquire() before the table is actually
allocated, which would result in a NULL pointer dereference.

[ ... ]
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 7586972e78e09..4b2c8039b8fa3 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
[ ... ]
> @@ -2653,12 +2712,25 @@ static void mana_schedule_napi(void *context, struct gdma_queue *gdma_queue)
>  
>  static void mana_deinit_cq(struct mana_port_context *apc, struct mana_cq *cq)
>  {
> -	struct gdma_dev *gd = apc->ac->gdma_dev;
> +	struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
> +	struct gdma_queue *gdma_cq = cq->gdma_cq;
> +	struct gdma_queue *eq;
> +	int err;
>  
> -	if (!cq->gdma_cq)
> +	if (!gdma_cq)
>  		return;
>  
> -	mana_gd_destroy_queue(gd->gdma_context, cq->gdma_cq);
> +	eq = gdma_cq->cq.parent;
> +	if (gdma_cq->id < gc->max_num_cqs && eq &&
> +	    eq->id != INVALID_QUEUE_ID) {
> +		/* Flush queued events after WQ teardown, before removing this CQ. */
> +		err = mana_gd_test_eq(gc, eq);
> +		if (err && mana_en_need_log(apc, err))
> +			netdev_err(apc->ndev, "Failed to flush EQ %u for CQ %u: %d\n",
> +				   eq->id, gdma_cq->id, err);
> +	}
> +
> +	mana_gd_destroy_queue(gc, gdma_cq);
>  }

[Severity: High]
Could this sequential mana_gd_test_eq() call lead to an unbounded RTNL
stall if an EQ stops delivering interrupts?

If an EQ is dead but the hardware channel remains responsive,
mana_gd_test_eq() will wait its full 30-second timeout.

Since mana_deinit_cq() is called sequentially for every single TX and RX
queue during teardown (via mana_destroy_txq() and mana_destroy_rxqs()),
a typical device with 64 queues would experience a 30-second timeout
per queue. This could result in an unrecoverable stall of over an hour
while holding the global RTNL lock, blocking all networking management.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791710735.git.weh@linux.microsoft.com?part=2