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

> 
> 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
> hwc->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?

Thanks. Both windows are pre-existing: patch 3 only reorders the EQ/CQ/comp_buf destroy inside  mana_hwc_destroy_cq()  and doesn't touch the caller ordering or the teardown gate, so it's no worse than the base at this commit. As you note, "net: mana: fix HWC teardown safety with setup_active flag and destroy ordering" completes both — it reorders  mana_hwc_destroy_channel()  to destroy the CQ (fencing the EQ) before the WQs, and replaces the  max_num_cqs  gate with  setup_active  + leak-on-teardown-failure.

The two patches are part of the same series and are applied together, so the final tree has no such window. I'd prefer to leave patch 3 as is.

Long