> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: Wednesday, September 2, 2026 1:01 PM
> To: Long Li <[email protected]>
> Cc: [email protected]
> Subject: [EXTERNAL] Re: [PATCH net-next v4 4/4] net: mana: add dynamic
> HWC queue depth with reinit path
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
> to consider:
>
> Pre-existing issues:
> - [Critical] Use-after-free of `hwc_cq->comp_buf` during queue teardown
> because memory is freed before the EQ IRQ is deregistered.
> - [High] NULL pointer dereference on `gc->cq_table` during HWC reinit
> fallback on error paths because `gc->max_num_cqs` is updated
> asynchronously before table allocation.
> --
>
> commit b3057022f465825e22d893f907cd5cd5dd291b2f
> Author: Long Li <[email protected]>
>
> net: mana: add dynamic HWC queue depth with reinit path
>
> This patch dynamically adjusts the hardware channel (HWC) queue depth by
> tearing down the bootstrap channel and rebuilding it at a device-supported
> maximum depth. This allows more management commands to be in flight.
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
>
> [ ... ]
>
> > @@ -872,13 +916,64 @@ static int mana_hwc_init_queues(struct
> > hw_channel_context *hwc, u16 q_depth,
> [ ... ]
> > +static void mana_hwc_destroy_queues(struct hw_channel_context *hwc) {
> > + struct gdma_context *gc = hwc->gdma_dev->gdma_context;
> > +
> > + /* Destroy CQ first to deregister the EQ from the interrupt
> > + * handler list before freeing caller_ctx, TXQ, or RXQ memory.
> > + * A pending interrupt handler could still reach handle_resp()
> > + * which dereferences caller_ctx.
> > + */
> > + if (hwc->cq) {
> > + mana_hwc_destroy_cq(gc, hwc->cq);
>
> [Severity: Critical]
> This is a pre-existing issue, but since this patch dynamically tears down and
> reconstructs the active HWC queues on the main probe path, does it turn a
> teardown race into a reachable use-after-free regression?
>
> Inside mana_hwc_destroy_cq(), the memory for hwc_cq->comp_buf is freed
> before the EQ interrupt is deregistered:
>
> mana_hwc_destroy_cq() {
> kfree(hwc_cq->comp_buf);
>
> 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); ...
>
> If an EQ interrupt fires concurrently (or a stray interrupt occurs), the
> interrupt
> handler mana_gd_intr() calls the completion callback
> mana_hwc_comp_event(). That callback will read the freed comp_buf pointer
> and pass it to mana_gd_poll_cq(), causing out-of-bounds DMA completions
> to be written into the freed memory.
>
> > + hwc->cq = NULL;
> > + }
>
> [ ... ]
>
> > @@ -926,8 +1021,200 @@ int mana_hwc_create_channel(struct
> gdma_context
> > *gc)
> [ ... ]
> > +reinit_fallback:
> > + /* Restore bootstrap-depth channel so the device remains
> functional.
> > + * Free cq_table if it was allocated by a partially successful
> > + * establish attempt.
> > + */
> > + dev_warn(hwc->dev, "HWC reinit failed, falling back to bootstrap
> > +depth\n");
> > +
> > + mana_hwc_destroy_queues(hwc);
>
> [Severity: High]
> This is a pre-existing issue, but does this fallback path expose a null
> pointer
> dereference on gc->cq_table?
>
> If mana_hwc_establish_channel() fails after the firmware updates
> gc->max_num_cqs (via an EQ event) but before gc->cq_table is allocated
> (for example, if vcalloc fails or the doorbell is missing), we jump to
> reinit_fallback.
>
> In this path, mana_hwc_destroy_queues() is called before gc->max_num_cqs
> is cleared. This calls down into mana_gd_destroy_cq(), which assumes
> gc->cq_table is safely allocated if id < max_num_cqs:
>
> mana_gd_destroy_cq() {
> if (id >= gc->max_num_cqs)
> return;
> if (!gc->cq_table[id])
> return;
> ...
>
> This will dereference the null gc->cq_table pointer.
>
> > +
> > + old_cq_table = gc->cq_table;
> > + gc->cq_table = NULL;
> > + /* Clear the bound with the table, as above. */
> > + gc->max_num_cqs = 0;
> > + synchronize_rcu();
> > + vfree(old_cq_table);
>
> --
Those are both pre-existing issues. The patch set implements multi queue depth
support and doesn't attempt to fix the existing issues.
If needed, I will submit a separate patch to fix the existing issue. The patch
is not related to this patch series.
Long
