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

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

Pre-existing issues:
- [High] A timed-out HWC request recycles its msg_id immediately, which can cause a delayed hardware response to corrupt the memory of a subsequent request that reused the same msg_id.
- [High] The dynamically allocated `mana_serv_work` item is not cancelled during device removal, leading to a race condition where the work item can access a freed `gdma_context` (`gc`).
--

--- Patch [5]: [PATCH net v7 5/7] net: mana: fix HWC teardown safety with setup_active flag and destroy ordering ---
commit f7262210dd3ba2257a4ad8d7840cc3065a491ae1
Author: Long Li <[email protected]>

net: mana: fix HWC teardown safety with setup_active flag and destroy ordering

This patch addresses teardown hazards that could allow the hardware to access
freed memory. It introduces a setup_active flag to ensure teardowns only occur
when necessary, reorders queue destruction to prevent interrupts from accessing
freed buffers, and intentionally leaks resources if the teardown itself fails.

> 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

[ ... ]

> @@ -837,6 +844,16 @@ int mana_hwc_create_channel(struct gdma_context *gc)
>  	u16 q_depth_max;
>  	int err;
>  
> +	/* A previous teardown may have failed and left the old context
> +	 * reachable.  Retry it before building a new channel; if it still
> +	 * fails, return an error so mana_serv_reset() does a full PCI rescan.
> +	 */

[Severity: High]
This isn't a bug introduced by this patch, but since mana_serv_reset() is
mentioned here, is there a use-after-free risk during device removal?

If an error triggers mana_schedule_serv_work(), it schedules a dynamically
allocated work item on the system workqueue:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_schedule_serv_work() {
    ...
    INIT_WORK(&mns_wk->serv_work, mana_serv_func);
    schedule_work(&mns_wk->serv_work);
    ...
}

If the driver is unbound concurrently, mana_gd_remove() frees the gdma_context
without waiting for or cancelling this work item:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_remove() {
    ...
    vfree(gc);
    ...
}

Could the pending work item then execute mana_serv_reset(), read the context
via pci_get_drvdata(), and dereference the freed gc pointer?

[ ... ]

> @@ -914,6 +953,12 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
>  	 */
>  	gc->max_num_cqs = 0;
>  
> +	if (hwc->txq)
> +		mana_hwc_destroy_wq(hwc, hwc->txq);
> +
> +	if (hwc->rxq)
> +		mana_hwc_destroy_wq(hwc, hwc->rxq);
> +
>  	kfree(hwc->caller_ctx);
>  	hwc->caller_ctx = NULL;

[Severity: High]
This is a pre-existing issue, but while reviewing the caller context cleanup,
is there a memory corruption risk if a hardware channel request times out?

If a request times out in mana_hwc_send_request(), the msg_id is immediately
recycled:

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_send_request() {
    ...
out:
    mana_hwc_put_msg_index(hwc, msg_id);
    return err;
}

If another thread allocates the same msg_id, and the hardware later sends a
delayed response for the original timed-out request, could
mana_hwc_handle_resp() overwrite the new request's memory?

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_handle_resp() {
    ...
    memcpy(ctx->output_buf, resp_msg, resp_len);
    ...
    complete(&ctx->comp_event);
}

Could this delayed response blindly overwrite the newly allocated caller's
output_buf and prematurely wake it up?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5