Re: [PATCH net v6 3/7] net: mana: free HWC comp_buf after destroying the EQ

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mana: free HWC comp_buf after destroying the EQ

This reorders mana_hwc_destroy_cq() so the EQ is destroyed first, then the
CQ, and only afterwards hwc_cq->comp_buf is freed.  The stated reason is
that mana_gd_destroy_queue() on the EQ deregisters the IRQ and waits out
in-flight handlers, fencing EQE dispatch before handler-reachable memory is
released.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 3f011ebbe7b3e..19896bb5ce1a4 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -384,14 +384,24 @@ static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)
>  
>  static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
>  {
> -	kfree(hwc_cq->comp_buf);
> +	/* Destroy the EQ before the CQ.  mana_gd_destroy_queue() on the EQ
> +	 * deregisters its IRQ and waits out in-flight handlers, fencing all
> +	 * EQE dispatch — both the completion path and HWC init/reconfig
> +	 * events.  Freeing the CQ first would leave the EQ live and able to
> +	 * dispatch an event that dereferences hwc->cq->gdma_cq (e.g.
> +	 * mana_hwc_init_event_handler()) after it has been freed.
> +	 */
> +	if (hwc_cq->gdma_eq)
> +		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);

[Severity: Medium]
This isn't a bug introduced by this patch, but the ordering in the only
caller does not yet match the invariant the new comment states.  Does the
comment's claim that this point fences "all EQE dispatch — both the
completion path and HWC init/reconfig events" hold for the WQ objects the
same handlers touch?

At this commit mana_hwc_destroy_channel() frees the WQs before it gets
here:

	if (gc->max_num_cqs > 0)
		mana_smc_teardown_hwc(&gc->shm_channel, false);

	if (hwc->txq)
		mana_hwc_destroy_wq(hwc, hwc->txq);

	if (hwc->rxq)
		mana_hwc_destroy_wq(hwc, hwc->rxq);

	if (hwc->cq)
		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);

mana_hwc_destroy_wq() releases msg_buf (the DMA buffer plus reqs[]), the
gdma_wq and the hwc_wq itself, and hwc->txq / hwc->rxq are left dangling
rather than set to NULL.  The EQ is still on gic->eq_list and the HWC CQ is
still published in gc->cq_table at that point, since the only dispatch
fence (mana_gd_deregister_irq() -> list_del_rcu() + synchronize_rcu()) is
reached from the mana_gd_destroy_queue() call added here.

So can an EQE arriving in that window reach freed memory through
mana_gd_process_eqe() -> mana_hwc_comp_event() -> mana_hwc_rx_event_handler(),
which dereferences hwc->rxq and then:

	if (WARN_ON_ONCE(hwc_rxq->gdma_wq->id != gdma_rxq_id))
		return;

and later hwc_rxq->msg_buf->mem_info.dma_handle and
hwc_rxq->msg_buf->reqs[]?  The tx side has only a NULL check:

	WARN_ON_ONCE(!hwc_txq || hwc_txq->gdma_wq->id != gdma_txq_id);

which a freed-but-non-NULL pointer passes.  mana_hwc_init_event_handler()
similarly touches hwc->rxq->gdma_wq->id and hwc->txq->gdma_wq.

For what it's worth, a later commit in this series, "net: mana: fix HWC
teardown safety with setup_active flag and destroy ordering", reorders
mana_hwc_destroy_channel() to call mana_hwc_destroy_cq() before
mana_hwc_destroy_wq() for txq and rxq, so the final tree state does not
have this window.  Given this patch carries a Fixes: tag and would be a
stable backport candidate on its own, would it be worth folding the caller
reordering in here, or noting the dependency in the changelog?

>  
> +	/* comp_buf is reached only by mana_hwc_comp_event(), invoked from
> +	 * the now-fenced EQ handler, so it is safe to free once the EQ and
> +	 * CQ are gone.
> +	 */
>  	if (hwc_cq->gdma_cq)
>  		mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
>  
> -	if (hwc_cq->gdma_eq)
> -		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
> -
> +	kfree(hwc_cq->comp_buf);
>  	kfree(hwc_cq);
>  }

[Severity: Medium]
This is a pre-existing issue, but moving the EQ destroy to the front of
this function changes which ring is handed back to the DMA allocator first
on the failure path.  Is that safe when the shared-memory teardown was
skipped?

mana_hwc_destroy_channel() gates the teardown on max_num_cqs and drops the
return value:

	/* 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.
	 */
	if (gc->max_num_cqs > 0)
		mana_smc_teardown_hwc(&gc->shm_channel, false);

If mana_smc_setup_hwc() succeeded (MST entries active, device programmed
with the HWC ring addresses) but mana_hwc_establish_channel() then timed
out waiting on hwc_init_eqe_comp, max_num_cqs stays zero, the teardown is
skipped, and mana_hwc_destroy_cq() still runs.  The EQ ring is then the
first HWC ring released, via mana_gd_destroy_queue() ->
mana_gd_free_memory() -> dma_free_coherent().

For the HWC EQ, eq.disable_needed is false, so mana_gd_destroy_eq() issues
no device-side DISABLE_QUEUE, and mana_gd_deregister_irq() fences only the
driver's handlers.  Can the device still post an EQE into those pages after
they are freed?  shm_channel.c notes the dependency:

	/* Waiting for the hardware to invalidate the MST entries before the
	 * driver frees the queue memory */

The same later commit, "net: mana: fix HWC teardown safety with
setup_active flag and destroy ordering", replaces the max_num_cqs gate with
hwc->setup_active set before mana_smc_setup_hwc(), and on teardown failure
returns early and leaks the HWC resources instead of freeing memory the
device may still write to.  Should the ordering change here wait for that
gate, or at least mention the ordering dependency between the two patches?