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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message presents this patch only as preparation for
  dynamic HWC queue sizing.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  documents mana_gd_process_eqe() as an RCU-protected lockless reader of…

--

net: mana: prepare HWC ownership for safe reinitialization

This commit serializes the shared-memory ESTABLISH_HWC and DESTROY_HWC
transactions. When DESTROY_HWC is not acknowledged, it keeps the
PF-visible HWC resources, and it adds a final-disposal helper for when
the gdma_context is discarded. It also snapshots the bootstrap CQ count
and ID, publishes cq_table with release/acquire ordering, and waits for
RCU readers before freeing the HWC queues.

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
Link: https://lore.kernel.org/r/179196575238.434549.10775387794024020898@kernel.org
Link: https://lore.kernel.org/r/sashiko-outbox-170992@kernel.org

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index ae0ed700b3b93..d11068f1dc05f 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c

[ ... ]

> @@ -765,11 +767,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]);
> +		if (!cq || WARN_ON_ONCE(cq->type != GDMA_CQ ||
> +					cq->id != cq_id))
>  			break;
>  
>  		if (cq->cq.callback)

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. This
hunk now documents mana_gd_process_eqe() as an RCU-protected lockless
reader of gc->cq_table, though. Do the Ethernet and RDMA writers of the
same slots follow that model?

The reader loads cq from the slot, dereferences cq->type and cq->id,
and then calls cq->cq.callback(cq->cq.context, cq).

In drivers/infiniband/hw/mana/cq.c, mana_ib_remove_cq_cb() frees the
queue before it clears the slot, and it never waits for a grace period:

drivers/infiniband/hw/mana/cq.c:mana_ib_remove_cq_cb() {
    ...
	kfree(gc->cq_table[cq->queue.id]);
	gc->cq_table[cq->queue.id] = NULL;
}

mana_ib_destroy_cq() calls this before mana_ib_gd_destroy_cq(), so the
hardware CQ is still live at that point.

Ethernet CQs have a similar problem. In mana_gd_destroy_queue(),
mana_gd_destroy_cq() does WRITE_ONCE(gc->cq_table[id], NULL), and then
kfree(queue) runs straight away with no synchronize_rcu() in between.

Could an IRQ on another CPU that has already loaded the slot in
mana_gd_process_eqe() then read freed memory and call through a freed
cq->cq.callback?

The publish side looks similar. mana_ib_install_cq_cb() initializes
gdma_cq and then publishes it with a plain store:

	gdma_cq->id = cq->queue.id;
	gc->cq_table[cq->queue.id] = gdma_cq;

The txq and rxq creation paths in mana_en.c do the same:

	gc->cq_table[cq->gdma_id] = cq->gdma_cq;

Neither store has release ordering. Could a reader on a weakly ordered
CPU such as arm64 see a partly initialized CQ?

The commit message says "Individual Ethernet and RDMA CQ lifetime
handling remains separate". drivers/infiniband/hw/mana/cq.c is still
unchanged at the end of the series.

[ ... ]

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 3bca4b683134b..a1c1c6f32cafa 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -382,16 +386,49 @@ static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)
>  	mana_gd_ring_cq(q_self, SET_ARM_BIT);
>  }
>  
> -static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
> +static int mana_hwc_publish_cq(struct gdma_context *gc,
> +			       struct gdma_queue *cq)
>  {
> -	kfree(hwc_cq->comp_buf);
> +	struct gdma_queue **cq_table = READ_ONCE(gc->cq_table);
> +	u32 id = READ_ONCE(cq->id);
>  
> -	if (hwc_cq->gdma_cq)
> -		mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);

[ ... ]

> +static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
> +{
> +	/* Destroy the EQ first: it deregisters the IRQ and drains in-flight
> +	 * handlers, so none can touch the CQ after it is freed.
> +	 */
>  	if (hwc_cq->gdma_eq)
>  		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
>  
> +	/* Safe to free now that the EQ handler is fenced. */
> +	if (hwc_cq->gdma_cq)
> +		mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
> +
> +	kfree(hwc_cq->comp_buf);
>  	kfree(hwc_cq);
>  }

[Severity: Low]
This isn't a bug in the code, but the commit message describes the
patch only as preparation for dynamic HWC queue sizing. Several of the
changes appear to fix problems in the existing driver. There is no
Fixes: tag and no stable Cc, so stable kernels will not pick these
fixes up.

The baseline mana_hwc_destroy_cq() freed comp_buf and destroyed the CQ
before destroying the EQ, and destroying the EQ is what deregisters the
IRQ and runs synchronize_rcu(). The baseline mana_hwc_destroy_channel()
also destroyed txq and rxq before the CQ and EQ. A concurrent
mana_hwc_comp_event() or rx handler could therefore touch freed memory.

The baseline mana_hwc_destroy_channel() ignored the return value of
mana_smc_teardown_hwc(). It freed the HWC DMA memory even when
DESTROY_HWC was not acknowledged.

The baseline mana_hwc_init_event_handler() did:

	case HWC_INIT_DATA_MAX_NUM_CQS:
		gd->gdma_context->max_num_cqs = val;

on any such event. That handler stays installed for the life of the
channel. A later event could raise the bound above the vcalloc() size,
and mana_gd_process_eqe() would then read gc->cq_table out of bounds
in hardirq context.

The baseline also vfree()d gc->cq_table without waiting a grace period
for IRQ-side readers on other EQs.

Would it make sense to send these fixes separately, with Fixes: tags,
so they can be backported?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791998462.git.weh%40linux.microsoft.com