Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Improper PCI device reset via `pcie_flr()` leaves PCI core state out of sync. Pre-existing issues: - [Critical] Use-After-Free in EQ interrupt handler because the CQ is destroyed before the EQ is deregistered. - [Critical] Tag reuse causing logical corruption when HWC commands time out. - [High] Race condition leading to Use-After-Free on module unbind due to un-cancelled `mana_serv_work`. -- --- Patch [5]: [PATCH net-next v2 5/7] net: mana: fix HWC teardown safety with setup_active flag and destroy ordering --- commit 7a0e58ab3c693baa720b6e21219cbe04fb7e9fcf Author: Long Li <[email protected]> net: mana: fix HWC teardown safety with setup_active flag and destroy ordering This patch adds a setup_active flag to ensure safe hardware channel teardown on initialization failures. It also reorders queue destruction to prevent DMA-after-free races during teardown. > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > index 515c45c4a890f..5bc873d548d58 100644 > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c [ ... ] > @@ -902,11 +926,58 @@ void mana_hwc_destroy_channel(struct gdma_context *gc) [ ... ] > + /* On systems without IOMMU, freeing DMA memory with > + * active hardware MST mappings risks memory corruption. > + * Issue FLR to force-reset the device and invalidate > + * all hardware state including MST entries. > + */ > + td_err = pcie_flr(to_pci_dev(gc->dev)); [Severity: High] Does using pcie_flr() here instead of pci_reset_function() leave the Linux PCI core completely unaware of the reset? Since pcie_flr() does not save and restore the PCI configuration space, such as BARs, MSI-X vectors, and Bus Master enable, could this leave the hardware's PCI configuration wiped and out of sync with the kernel's cached state, resulting in a wedged device? > + if (td_err) { [ ... ] > + /* 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); [Severity: Critical] This is a pre-existing issue, but does destroying the CQ before the EQ inside mana_hwc_destroy_cq() create a use-after-free window? In mana_hwc_destroy_cq(), the CQ structure is freed first: mana_hwc_destroy_cq() { if (hwc_cq->gdma_cq) mana_gd_destroy_queue(gc, hwc_cq->gdma_cq); ... } If the hardware posts an initialization event before the EQ is subsequently destroyed, could the interrupt handler execute: mana_hwc_init_event_handler() { ... case HWC_INIT_DATA_CQID: hwc->cq->gdma_cq->id = val; break; } and write directly into the newly freed gdma_cq structure memory? [ ... ] [Severity: High] This isn't a bug introduced by this patch, but is there a race condition leading to a use-after-free on module unbind due to un-cancelled service work? If a hardware reset event triggers mana_schedule_serv_work(), it queues work on the global system_wq. If the driver is unbound via sysfs, mana_gd_remove() frees the gdma_context via vfree(gc) without cancelling this pending work. Could the work then execute concurrently, calling mana_serv_reset() which fetches the freed context: mana_serv_reset() { struct gdma_context *gc = pci_get_drvdata(pdev); struct hw_channel_context *hwc = gc->hwc.driver_data; ... } and dereference the freed memory? [Severity: Critical] This is a pre-existing issue, but can tag reuse cause logical data corruption when hardware channel commands time out? When wait_for_completion_timeout() times out in mana_hwc_send_request(), the function calls mana_hwc_put_msg_index() to free the msg_id for immediate reuse. Since the hardware is not reset, the timed-out command is still pending. If another thread sends a new command and allocates the same msg_id, what happens when the hardware eventually responds to the original command? Could mana_hwc_handle_resp() match this late response to the new command: mana_hwc_handle_resp() { ctx = hwc->caller_ctx + msg_id; ... memcpy(ctx->output_buf, resp_msg, resp_len); complete(&ctx->comp_event); } and complete it with stale data? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
