Re: [PATCH net-next 3/7] net: mana: free HWC comp_buf after destroying the EQ

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-after-free of TX and RX work queues during HWC channel destruction due to incorrect teardown ordering.
- [High] Data corruption and premature completion due to msg_id reuse after timeout in mana_hwc_send_request().
- [High] Use-after-free of gdma_context (gc) due to un-canceled service work during device removal.
--

--- Patch [3]: [PATCH net-next 3/7] net: mana: free HWC comp_buf after destroying the EQ ---

commit 0772412f368270fa1d8812b8355a2560f063690d
Author: Long Li [off-list ref]
Subject: net: mana: free HWC comp_buf after destroying the EQ

This patch resolves a use-after-free issue by deferring the release of
the hardware channel completion buffer until after the event queue has been
destroyed. This ensures no late interrupts can access the freed memory.

[ ... ]

 static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
 {

[ ... ]

 	if (hwc_cq->gdma_eq)
 		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
 
+	kfree(hwc_cq->comp_buf);
 	kfree(hwc_cq);
 }

[Severity: High]
This isn't a bug introduced by this patch, but while looking at teardown
ordering, could there be a similar use-after-free with the TX and RX work
queues?

In mana_hwc_destroy_channel(), the work queues are freed before the
completion queue:

mana_hwc_destroy_channel() {
    if (hwc->txq)
        mana_hwc_destroy_wq(hwc, hwc->txq);

    if (hwc->rxq)
        mana_hwc_destroy_wq(hwc, hwc->rxq);

    if (hwc->cq)
        mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
}

If an interrupt fires before the CQ and EQ are destroyed, will the
completion handler still attempt to dereference the already freed work
queues?

mana_hwc_rx_event_handler() {
    struct hw_channel_context *hwc = ctx;
    struct hwc_wq *hwc_rxq = hwc->rxq;
    ...
    rq = hwc_rxq->gdma_wq;
    ...
}


[Severity: High]
This is a pre-existing issue, but does mana_hwc_send_request() have a
race condition if a request times out?

If a timeout occurs, mana_hwc_send_request() releases the msg_id while the
hardware might still be processing it:

mana_hwc_send_request() {
    ...
    err = -ETIMEDOUT;
    goto out;
    ...
out:
    mana_hwc_put_msg_index(hwc, msg_id);
}

Could another caller subsequently allocate the same msg_id and assign its
own stack buffer? If the hardware then completes the original delayed
request, would the handler overwrite the new caller's buffer with the old
response?

mana_hwc_handle_resp() {
    ctx = hwc->caller_ctx + msg_id;
    ...
    memcpy(ctx->output_buf, resp_msg, resp_len);
    ...
    complete(&ctx->comp_event);
}


[Severity: High]
This is also a pre-existing issue, but could there be a use-after-free
during device removal if service work is pending?

In mana_gd_remove(), the gdma_context is freed without canceling pending
work on the system workqueue:

mana_gd_remove() {
    ...
    mana_gd_cleanup_device(pdev);
    xa_destroy(&gc->irq_contexts);
    pci_iounmap(pdev, gc->bar0_va);
    vfree(gc);
}

If the delayed work handler executes concurrently with or after device
removal, will it dereference the freed pointer?

mana_serv_reset() {
    struct gdma_context *gc = pci_get_drvdata(pdev);
    struct hw_channel_context *hwc;
    ...
    hwc = gc->hwc.driver_data;
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715032942.3945317-1-longli@microsoft.com?part=3