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); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
