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