Re: [PATCH net-next v7 1/4] net: mana: prepare HWC ownership for safe reinitialization

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net/mana: use-after-free in mana_gd_process_eqe() due to missing RCU grace period for CQ
- [Medium] net/mana: memory leak of software structures on HWC teardown failure
- [Low] net/mana: missing WRITE_ONCE in mana_gd_destroy_cq creates data race
--

commit 361a37c1499976f2c320b5cabc5e1273595fd5dd
Author: Long Li <longli@microsoft.com>

net: mana: prepare HWC ownership for safe reinitialization

Dynamic HWC queue sizing tears down the bootstrap queues and establishes a
second channel before publishing it. Prepare the existing HWC ownership and
teardown paths so that sequence cannot free or reuse state still reachable
by either the PF or an EQ handler.

Link: https://lore.kernel.org/r/20260908035201.402424-2-longli@microsoft.com
Link: https://lore.kernel.org/r/20260908035201.402424-5-longli@microsoft.com
Link: https://lore.kernel.org/r/178910960115.219967.13830871915506436112@kernel.org

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index ae0ed700b3b93..69da65a405a46 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
[ ... ]
> @@ -765,11 +766,16 @@ 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))
> +		/* The IRQ handler's RCU read-side section protects the table
> +		 * until HWC teardown has fenced its EQ and waited for readers.
> +		 */
> +		cq_table = smp_load_acquire(&gc->cq_table);
> +		if (!cq_table || cq_id >= READ_ONCE(gc->max_num_cqs))
>  			break;
>  
> -		cq = gc->cq_table[cq_id];
> -		if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id))
> +		cq = READ_ONCE(cq_table[cq_id]);

[Severity: High]
During HWC teardown, the dedicated HWC EQ is fenced (which includes a
synchronize_rcu() wait), and then the HWC CQ is destroyed. The CQ memory is
freed without a subsequent RCU grace period. Can a concurrently running IRQ
handler for a different active EQ (like an ENET EQ) read the CQ pointer from
cq_table just before it is cleared, and dereference it after it is freed here?

Since the PF is untrusted, could a malicious or compromised PF send an HWC CQ
completion event to a different EQ? The synchronize_rcu() in the EQ teardown
doesn't protect the CQ because the CQ is removed from cq_table after that wait.

[Severity: Low]
This problem wasn't introduced by this patch, but by upgrading the reader to
READ_ONCE() here, it highlights that mana_gd_destroy_cq() clears the CQ table
entry using a plain write:

    gc->cq_table[id] = NULL;

For normal networking queues, this happens before the EQ is fenced, so the EQ
handler may concurrently execute the READ_ONCE(cq_table[cq_id]). Should this
writer be updated to WRITE_ONCE() to prevent a formal data race?

> +		if (!cq || WARN_ON_ONCE(cq->type != GDMA_CQ ||
> +					cq->id != cq_id))
>  			break;
>  
>  		if (cq->cq.callback)
[ ... ]
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 3bca4b683134b..b1d972968f0d7 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -808,30 +885,56 @@ int mana_hwc_create_channel(struct gdma_context *gc)
[ ... ]
>  void mana_hwc_destroy_channel(struct gdma_context *gc)
>  {
>  	struct hw_channel_context *hwc = gc->hwc.driver_data;
> +	struct gdma_queue **old_cq_table;
> +	int err;
>  
>  	if (!hwc)
>  		return;
>  
> -	/* gc->max_num_cqs is set in mana_hwc_init_event_handler(). If it's
> -	 * non-zero, the HWC worked and we should tear down the HWC here.
> +	/* An unacknowledged destroy leaves the PF's mappings live.  Fence
> +	 * software dispatch, but retain every PF-visible allocation.
>  	 */
> -	if (gc->max_num_cqs > 0) {
> -		mana_smc_teardown_hwc(&gc->shm_channel, false);
> -		gc->max_num_cqs = 0;
> +	err = mana_smc_teardown_hwc(&gc->shm_channel, false,
> +				    &hwc->setup_active);
> +	if (err) {
> +		dev_err(hwc->dev,
> +			"HWC teardown failed: %d, retaining PF-visible resources\n",
> +			err);
> +		mana_hwc_fence_channel(gc, hwc);
> +		return;
>  	}

[Severity: Medium]
If mana_smc_teardown_hwc() fails, mana_hwc_destroy_channel() returns early to
prevent the PF from writing to reallocated DMA buffers. However, does this
early return skip the deallocation of CPU-side software structures like hwc,
hwc->caller_ctx, and gc->cq_table?

Since these structures are pure software allocations and not DMA-mapped
PF-visible memory, should they be freed even if the hardware is unresponsive?

During driver unload (mana_gd_remove), vfree(gc) is eventually called, which
destroys the parent gdma_context and permanently leaks the retained
gc->hwc.driver_data (hwc) and gc->cq_table on this error path.

>  
> +	/* Fence the EQ before releasing any state its handlers can reach. */
> +	if (hwc->cq)
> +		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
> +

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