Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] net/mana: use-after-free in mana_gd_process_eqe() due to missing RCU grace period for CQ - [Medium] net/mana: memory leak of software structures on HWC teardown failure - [Low] net/mana: missing WRITE_ONCE in mana_gd_destroy_cq creates data race --
commit 361a37c1499976f2c320b5cabc5e1273595fd5dd Author: Long Li <[email protected]> net: mana: prepare HWC ownership for safe reinitialization Dynamic HWC queue sizing tears down the bootstrap queues and establishes a second channel before publishing it. Prepare the existing HWC ownership and teardown paths so that sequence cannot free or reuse state still reachable by either the PF or an EQ handler. Link: https://lore.kernel.org/r/[email protected] Link: https://lore.kernel.org/r/[email protected] Link: https://lore.kernel.org/r/[email protected] > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c > b/drivers/net/ethernet/microsoft/mana/gdma_main.c > index ae0ed700b3b93..69da65a405a46 100644 > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c [ ... ] > @@ -765,11 +766,16 @@ static void mana_gd_process_eqe(struct gdma_queue *eq) > switch (type) { > case GDMA_EQE_COMPLETION: > cq_id = eqe->details[0] & 0xFFFFFF; > - if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs)) > + /* The IRQ handler's RCU read-side section protects the table > + * until HWC teardown has fenced its EQ and waited for readers. > + */ > + cq_table = smp_load_acquire(&gc->cq_table); > + if (!cq_table || cq_id >= READ_ONCE(gc->max_num_cqs)) > break; > > - cq = gc->cq_table[cq_id]; > - if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id)) > + cq = READ_ONCE(cq_table[cq_id]); [Severity: High] During HWC teardown, the dedicated HWC EQ is fenced (which includes a synchronize_rcu() wait), and then the HWC CQ is destroyed. The CQ memory is freed without a subsequent RCU grace period. Can a concurrently running IRQ handler for a different active EQ (like an ENET EQ) read the CQ pointer from cq_table just before it is cleared, and dereference it after it is freed here? Since the PF is untrusted, could a malicious or compromised PF send an HWC CQ completion event to a different EQ? The synchronize_rcu() in the EQ teardown doesn't protect the CQ because the CQ is removed from cq_table after that wait. [Severity: Low] This problem wasn't introduced by this patch, but by upgrading the reader to READ_ONCE() here, it highlights that mana_gd_destroy_cq() clears the CQ table entry using a plain write: gc->cq_table[id] = NULL; For normal networking queues, this happens before the EQ is fenced, so the EQ handler may concurrently execute the READ_ONCE(cq_table[cq_id]). Should this writer be updated to WRITE_ONCE() to prevent a formal data race? > + if (!cq || WARN_ON_ONCE(cq->type != GDMA_CQ || > + cq->id != cq_id)) > break; > > if (cq->cq.callback) [ ... ] > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > index 3bca4b683134b..b1d972968f0d7 100644 > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c [ ... ] > @@ -808,30 +885,56 @@ int mana_hwc_create_channel(struct gdma_context *gc) [ ... ] > void mana_hwc_destroy_channel(struct gdma_context *gc) > { > struct hw_channel_context *hwc = gc->hwc.driver_data; > + struct gdma_queue **old_cq_table; > + int err; > > 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. > + /* An unacknowledged destroy leaves the PF's mappings live. Fence > + * software dispatch, but retain every PF-visible allocation. > */ > - if (gc->max_num_cqs > 0) { > - mana_smc_teardown_hwc(&gc->shm_channel, false); > - gc->max_num_cqs = 0; > + err = mana_smc_teardown_hwc(&gc->shm_channel, false, > + &hwc->setup_active); > + if (err) { > + dev_err(hwc->dev, > + "HWC teardown failed: %d, retaining PF-visible > resources\n", > + err); > + mana_hwc_fence_channel(gc, hwc); > + return; > } [Severity: Medium] If mana_smc_teardown_hwc() fails, mana_hwc_destroy_channel() returns early to prevent the PF from writing to reallocated DMA buffers. However, does this early return skip the deallocation of CPU-side software structures like hwc, hwc->caller_ctx, and gc->cq_table? Since these structures are pure software allocations and not DMA-mapped PF-visible memory, should they be freed even if the hardware is unresponsive? During driver unload (mana_gd_remove), vfree(gc) is eventually called, which destroys the parent gdma_context and permanently leaks the retained gc->hwc.driver_data (hwc) and gc->cq_table on this error path. > > + /* Fence the EQ before releasing any state its handlers can reach. */ > + if (hwc->cq) > + mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq); > + -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
