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

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

Pre-existing issues:
- [High] Missing teardown synchronization for dynamically allocated service tasks leads to Use-After-Free during device removal.
--

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

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

Two teardown hazards let the hardware touch memory the driver freed.
First, if establish_channel fails, the caller had no reliable way to know
teardown was required and could free buffers while mappings were still live.
Second, destroy_channel freed TXQ/RXQ buffers while the HWC EQ was still on
the interrupt dispatch list.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 68236727aee8b..b26c2122ebf52 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -878,11 +902,62 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
>  	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.
> +	/* Tear down the HWC if setup_hwc previously activated MST entries.
> +	 * This is the definitive flag — unlike max_num_cqs which depends
> +	 * on the init EQE arriving.
>  	 */
> -	if (gc->max_num_cqs > 0)
> -		mana_smc_teardown_hwc(&gc->shm_channel, false);
> +	if (hwc->setup_active) {
> +		int td_err = mana_smc_teardown_hwc(&gc->shm_channel, false);

[Severity: High]
This is a pre-existing issue, but is there a missing synchronization step for
dynamically allocated service tasks during device teardown?

When a hardware event (like GDMA_EQE_HWC_RESET_REQUEST) occurs, the EQ
interrupt handler calls mana_schedule_serv_work(), which allocates a
mana_serv_work structure and schedules it on the system workqueue:

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

If a device unbind or module removal occurs concurrently, mana_gd_remove()
is called, which frees the context without flushing or canceling these
dynamically allocated tasks:

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

When the scheduled mana_serv_func() eventually executes, it accesses the
freed gdma_context (gc) and hw_channel_context (hwc):

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_serv_reset() {
    ...
    struct gdma_context *gc = pci_get_drvdata(pdev);
    struct hw_channel_context *hwc;
    ...
    hwc = gc->hwc.driver_data;
}

Could this race condition result in a Use-After-Free during teardown?

> +
> +		if (td_err) {
> +			dev_err(gc->dev, "HWC teardown failed: %d, issuing FLR\n",
> +				td_err);
[ ... ]

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