>
> 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: fix HWC teardown safety with setup_active flag and destroy
> ordering
>
> This adds a setup_active flag to struct hw_channel_context to record that
> mana_smc_setup_hwc() has activated device MST entries, reorders
> mana_hwc_destroy_channel() so the HWC CQ (and with it the EQ IRQ
> deregistration) is destroyed before the TXQ/RXQ buffers are freed, and
> changes destroy_channel() to leak the HWC resources when
> mana_smc_teardown_hwc() fails.
>
> A few questions on the new flag placement and on the leak-on-failure policy.
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > index 5db8cfe2d8443..959886434d07f 100644
> > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
>
> [ ... ]
>
> > @@ -783,6 +784,20 @@ static int mana_hwc_establish_channel(struct
> > gdma_context *gc, u16 *q_depth,
> >
> > init_completion(&hwc->hwc_init_eqe_comp);
> >
> > + /* Arm setup_active before issuing the setup command.
> > + * mana_smc_setup_hwc() hands the queue PFNs to the PF, activating
> > + * MST entries so the device can DMA into our queue buffers, before
> > + * it can report a later failure such as a possession-poll timeout.
> > + * Recording it up front guarantees the error path
> > + * (mana_hwc_create_channel() -> mana_hwc_destroy_channel()) still
> > + * tears the HWC down instead of freeing buffers the device may still
> > + * write to. Setting it for a rare pre-submission failure too is
> > + * harmless -- the teardown is then a no-op the device ignores. Do
> > + * not also tear down here: a second teardown would double the
> > + * hardware timeout on failure and mask the original error code.
> > + */
> > + hwc->setup_active = true;
> > +
> > err = mana_smc_setup_hwc(&gc->shm_channel, false,
> > eq->mem_info.dma_handle,
> > cq->mem_info.dma_handle,
>
> [Severity: High]
>
> Is the claim that "the teardown is then a no-op the device ignores" true for
> the
> most likely pre-submission failure?
>
> mana_smc_setup_hwc() fails at its very first step, before anything is written
> to
> the shared memory aperture:
>
> drivers/net/ethernet/microsoft/mana/shm_channel.c:mana_smc_setup_hwc(
> ) {
> /* Ensure VF already has possession of shared memory */
> err = mana_smc_poll_register(sc->base, false);
> if (err) {
> dev_err(sc->dev, "Timeout when setting up HWC: %d\n", err);
> return err;
> }
> ...
> }
>
> mana_smc_teardown_hwc() opens with the same poll:
>
> drivers/net/ethernet/microsoft/mana/shm_channel.c:mana_smc_teardown_h
> wc() {
> /* Ensure already has possession of shared memory */
> err = mana_smc_poll_register(sc->base, false);
> if (err) {
> dev_err(sc->dev, "Timeout when tearing down HWC\n");
> return err;
> }
> ...
> }
>
> So when the possession poll times out, no ESTABLISH_HWC message ever
> reached the device, but setup_active is already set, the error path runs
> mana_hwc_destroy_channel(), the teardown fails for the identical reason, and
> the new "leaking resources" branch returns having freed nothing.
>
> Does that leak the hwc struct, the inflight bitmap, the EQ/CQ/RQ/SQ gdma
> queues with their dma_alloc_coherent buffers, comp_buf and both msg_buf
> DMA buffers, in a case where the old gc->max_num_cqs > 0 gate skipped the
> teardown entirely and freed everything?
>
> Would arming the flag only after mana_smc_setup_hwc() returns success, or
> distinguishing "message submitted" from "poll failed before submission", avoid
> recording a teardown obligation the device never acquired?
The "no-op the device ignores" wording is dropped in v7. On the substance:
arming only after success would reopen the window this patch closes --
mana_smc_setup_hwc() activates the MST entries mid-call, so a failure after
activation but before it returns success would leave the flag clear and free
buffers the device can still DMA into. Arming before is the safe side. The cost
is over-leaking on a possession-poll timeout, where freeing would in fact be
safe; telling the two apart needs mana_smc_setup_hwc() to report whether it
passed the possession poll -- a shm_channel change I'd keep separate. The
result is a one-generation leak on a rare setup timeout, not corruption.
>
> [Severity: Medium]
>
> This is a pre-existing issue and not introduced by this patch, but while
> reading
> mana_hwc_establish_channel(): the bound check
>
> if (WARN_ON(cq->id >= gc->max_num_cqs))
> return -EPROTO;
>
> cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
> ...
> rcu_assign_pointer(cq_table[cq->id], cq);
>
> re-reads both gc->max_num_cqs and cq->id after the check, while
> mana_hwc_init_event_handler() writes both from EQ interrupt context
> (HWC_INIT_DATA_MAX_NUM_CQS and HWC_INIT_DATA_CQID).
>
> The later patch in this series, "net: mana: keep max_num_cqs immutable once
> cq_table is allocated", snapshots both values into locals with
> READ_ONCE() and commits gc->max_num_cqs once, so the check-then-use is
> gone by the end of the series. Noted here only for completeness.
Agreed, and as you note the immutable-max_num_cqs patch snapshots both with
READ_ONCE() and commits gc->max_num_cqs once, so the check-then-use is gone by
the end of the series. Nothing to change here.
>
> > @@ -869,6 +884,20 @@ int mana_hwc_create_channel(struct
> gdma_context *gc)
> > u16 q_depth_max;
> > int err;
> >
> > + /* A previous teardown may have failed and deliberately left the old
> > + * HWC context reachable (see mana_hwc_destroy_channel()). Retry
> the
> > + * teardown now -- the device has since been reset -- before building
> > + * a new channel, so we neither orphan the old context nor stack a
> > + * second channel on one whose DESTROY_HWC never completed. If it
> is
> > + * still failing, return an error that steers mana_serv_reset() to a
> > + * full PCI rescan instead of silently leaking another generation.
> > + */
> > + if (gd->driver_data) {
> > + mana_hwc_destroy_channel(gc);
> > + if (gd->driver_data)
> > + return -ETIMEDOUT;
> > + }
> > +
> > hwc = kzalloc_obj(*hwc);
> > if (!hwc)
> > return -ENOMEM;
>
> [Severity: High]
>
> This retry is the only mechanism that can reclaim a retained generation.
> Can it run on the probe failure path?
>
> mana_hwc_destroy_channel() is void, so mana_gd_setup() cannot tell that the
> teardown was incomplete and keeps unwinding, and mana_gd_probe() ends
> at:
>
> drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_probe() {
> ...
> free_gc:
> pci_set_drvdata(pdev, NULL);
> vfree(gc);
> ...
> }
>
> That frees the gdma_context holding gd->driver_data and gc->shm_channel, so
> DESTROY_HWC can never be issued for the retained context afterwards.
> mana_gd_remove() also vfree()s gc after mana_gd_cleanup_device().
>
> Since mana_gd_probe() schedules mana_dev_recovery_work every
> MANA_SERVICE_PERIOD on -ETIMEDOUT, and mana_serv_reset() escalates to
> mana_serv_rescan() (remove plus re-probe), does every cycle in which
> DESTROY_HWC does not complete retain a fresh generation, paced by the PF?
>
> For errnos other than -ETIMEDOUT/-EPROTO (for example the -ENOMEM from
> the vcalloc in mana_hwc_establish_channel(), which now happens with
> setup_active already armed) mana_serv_reset() does no rescan:
>
> drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_serv_reset() {
> ret = mana_gd_resume(pdev);
> if (ret == -ETIMEDOUT || ret == -EPROTO) {
> /* Perform PCI rescan on device if we failed on HWC */
> dev_err(&pdev->dev, "MANA service: resume failed,
> rescanning\n");
> mana_serv_rescan(pdev);
> ...
> }
>
> Is the retained generation then abandoned with no recovery at all?
>
> Would a bounded quarantine work better here, keeping at most one retained
> generation reclaimed by the next successful teardown, or propagating the
> teardown error so an owner of gc stays alive until reclamation is safe?
This comment will not be addressed in v7.
Correct that the retry is best-effort: it reclaims on the next create_channel,
which the common recovery path (serv_reset -> resume -> create) reaches, but a
probe failure that vfree()s gc, or a non-ETIMEDOUT/-EPROTO error that skips the
rescan, abandons the retained generation. That is the deliberate cost of
leaking rather than freeing buffers the device may DMA into -- a bounded memory
leak per failed teardown, not corruption. A bounded quarantine reclaimed by the
recovery worker would be an improvement, but it's a recovery-architecture
change beyond this fix; I'd do it separately.
>
> > @@ -926,11 +955,38 @@ void mana_hwc_destroy_channel(struct
> gdma_context *gc)
> > 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.
> > + /* Tear down the HWC if setup_hwc previously activated MST entries.
> > + * This is the definitive flag — unlike max_num_cqs which depends
> > + * on the init EQE arriving.
> > + *
> > + * If teardown fails the device may still have active MST entries
> > + * and can DMA into the HWC queue buffers. Freeing them would risk
> > + * memory corruption on systems without an IOMMU to fault the stale
> > + * DMA, so leak the HWC resources instead of handing the pages back
> > + * to the allocator. Keep setup_active set so the failure is not
> > + * mistaken for a clean teardown.
> > */
> > - if (gc->max_num_cqs > 0)
> > - mana_smc_teardown_hwc(&gc->shm_channel, false);
> > + if (hwc->setup_active) {
> > + int td_err = mana_smc_teardown_hwc(&gc->shm_channel,
> false);
> > +
> > + if (td_err) {
> > + dev_err(gc->dev,
> > + "HWC teardown failed: %d, leaking
> resources\n",
> > + td_err);
> > + return;
> > + }
> > +
> > + hwc->setup_active = false;
> > + }
>
> [Severity: Low]
>
> This return is taken before mana_hwc_destroy_cq(), so the HWC EQ stays on
> gic->eq_list and the CQ stays published in gc->cq_table while
> mana_gd_cleanup_device() goes on to mana_gd_remove_irqs().
>
> This does not appear to be a use-after-free. The HWC EQ registration takes no
> gdma_irq_context reference:
>
> drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_register_irq() {
> /* The caller acquired a GIC reference via mana_gd_get_gic().
> * That refcount prevents mana_gd_put_gic() from erasing this
> * irq_contexts entry concurrently.
> */
> gic = xa_load(&gc->irq_contexts, msix_index);
> ...
> }
>
> so mana_gd_put_gic() reaches refcount_dec_and_test() and calls free_irq(),
> which unregisters mana_gd_intr() and waits out in-flight handlers before any
> vfree(gc). In the window before that, an interrupt would only touch the
> EQ/CQ/RQ/TXQ buffers this branch deliberately did not free.
>
> What is left is stale state: the EQ's list node ends up pointing into a
> kfree()d gic,
> and on a later retried teardown after a resume whose
> mana_gd_setup() failed before mana_gd_setup_hwc_irqs(),
> mana_gd_deregister_irq() can hit
>
> msix_index = queue->eq.msix_index;
> if (WARN_ON(msix_index >= gc->num_msix_usable))
> return;
>
> with num_msix_usable == 0 and skip the list_del_rcu()/synchronize_rcu()
> before the EQ is freed. Would it be reasonable to deregister the EQ IRQ before
> taking the leak branch, so the retained buffers are unreachable from the
> interrupt path even in the failure case?
I'm fixing it in v7.
>
> > +
> > + /* Tear down the HWC CQ object first — mana_hwc_destroy_cq()
> > + * both unpublishes the CQ from cq_table (+synchronize_rcu) and
> > + * deregisters the HWC EQ from the interrupt handler list (via
> > + * mana_gd_deregister_irq + synchronize_rcu), guaranteeing no
> > + * interrupt handler can access RQ/TXQ buffers after this point.
> > + */
> > + if (hwc->cq)
> > + mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc-
> >cq);
> >
> > if (hwc->txq)
> > mana_hwc_destroy_wq(hwc, hwc->txq);
>
> [ ... ]
>
> > diff --git a/include/net/mana/hw_channel.h
> > b/include/net/mana/hw_channel.h index 787c6f96d5b57..8340abd36af61
> > 100644
> > --- a/include/net/mana/hw_channel.h
> > +++ b/include/net/mana/hw_channel.h
> > @@ -206,6 +206,15 @@ struct hw_channel_context {
> > */
> > u32 rx_leaked_wqe;
> >
> > + /* Set after mana_smc_setup_hwc() succeeds (hardware has active
> > + * MST entries). Cleared only after mana_smc_teardown_hwc()
> > + * succeeds, on both the recoverable establish_channel path and the
> > + * terminal destroy_channel path. If teardown fails it stays set:
> > + * establish_channel() skips its retry and destroy_channel() leaks
> > + * the HWC rather than free buffers the device may still DMA into.
> > + */
> > + bool setup_active;
> > +
>
> [Severity: Low]
>
> This isn't a runtime bug, but this comment describes different code than what
> the patch implements, and it also contradicts the in-code comment added to
> mana_hwc_establish_channel():
>
> "Set after mana_smc_setup_hwc() succeeds" - mana_hwc_establish_channel()
> sets it before the call and leaves it set when the call fails.
>
> "Cleared only after mana_smc_teardown_hwc() succeeds, on both the
> recoverable establish_channel path and the terminal destroy_channel path" -
> mana_hwc_establish_channel() has no mana_smc_teardown_hwc() call and
> never clears the flag; the single teardown call site and the single clear are
> both
> in mana_hwc_destroy_channel().
>
> "establish_channel() skips its retry" - mana_hwc_establish_channel() has no
> retry.
>
> Are these leftovers from the earlier version of the patch that tore down
> inline?
> Could the comment be updated to match the current placement, so a later
> reader does not move the assignment below mana_smc_setup_hwc() and
> reopen the window this patch closes?
Fixed in v7. The field comment now reads "set before the setup command, cleared
after teardown succeeds" and no longer mentions a clear on the establish path
or a retry in establish_channel; it matches the in-code comment and the
implementation.
Long
>
> > struct hwc_caller_ctx *caller_ctx;
> > };
