Re: [PATCH net-next v2 5/7] net: mana: fix HWC teardown safety with setup_active flag and destroy ordering

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 [off-list ref]

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.

[ ... ]

[ ... ]

+			/* 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/20260721234339.1476932-1-longli@microsoft.com?part=5