Re: [PATCH wireless] wifi: rt2x00: avoid full teardown before work setup in probe

[Stanislaw Gruszka <stf_xl@wp.pl>]

Hi, 

On Fri, Jun 19, 2026 at 03:31:04PM +0800, Runyu Xiao wrote:
> rt2x00lib_probe_dev() uses the full rt2x00lib_remove_dev() teardown for
> all probe failures. However, drv_data allocation and workqueue allocation
> can fail before intf_work, autowakeup_work and sleep_work have been
> initialized.
> 
> Do not enter the full remove path until the probe has reached the point
> where those work items are set up. Return directly for drv_data allocation
> failure, and use a small early cleanup path for workqueue allocation
> failure.
>
> This issue was found by our static analysis tool and then confirmed by
> manual review of rt2x00lib_probe_dev() and rt2x00lib_remove_dev(). The
> early probe exits should not call a common teardown path that assumes the
> later work setup has already completed.
> 
> A QEMU PoC forced alloc_ordered_workqueue() to fail before the work
> initializers are reached. The resulting fail path entered
> rt2x00lib_remove_dev(), and DEBUG_OBJECTS reported invalid work drains with
> rt2x00lib_probe_dev() and rt2x00lib_remove_dev() in the stack.

Thanks for finding and fixing those bugs. The patch looks fine, but I think
it could be a bit simpler, see the comments below.

> Fixes: 1ebbc48520a0 ("rt2x00: Introduce concept of driver data in struct rt2x00_dev.")
> Fixes: 0439f5367c8d ("rt2x00: Move TX/RX work into dedicated workqueue")
> Cc: stable@vger.kernel.org
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
> ---
>  drivers/net/wireless/ralink/rt2x00/rt2x00dev.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
> index f8a6f9c968a1..847b64e586f6 100644
> --- a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
> +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
> @@ -1382,7 +1382,7 @@ int rt2x00lib_probe_dev(struct rt2x00_dev *rt2x00dev)
>  			                      GFP_KERNEL);
>  		if (!rt2x00dev->drv_data) {
>  			retval = -ENOMEM;
> -			goto exit;
> +			return retval;

This can be just "return -ENOMEM;"

>  		}
>  	}
>  
> @@ -1416,7 +1416,7 @@ int rt2x00lib_probe_dev(struct rt2x00_dev *rt2x00dev)
>  	    alloc_ordered_workqueue("%s", 0, wiphy_name(rt2x00dev->hw->wiphy));
>  	if (!rt2x00dev->workqueue) {
>  		retval = -ENOMEM;
> -		goto exit;
> +		goto exit_free_drv_data;
>  	}
>  
>  	INIT_WORK(&rt2x00dev->intf_work, rt2x00lib_intf_scheduled);
I think should be sufficient to move INIT_*WORK's lines before
alloc_ordered_workqueue() to avoid cancel_*_work_sync() on uninitialized data.
And other de-init code from rt2x00lib_remove_dev() should work fine at this
point.

Regards
Stanislaw
> @@ -1488,6 +1488,14 @@ int rt2x00lib_probe_dev(struct rt2x00_dev *rt2x00dev)
>  exit:
>  	rt2x00lib_remove_dev(rt2x00dev);
>  
> +	return retval;
> +
> +exit_free_drv_data:
> +	clear_bit(DEVICE_STATE_PRESENT, &rt2x00dev->flags);
> +
> +	kfree(rt2x00dev->drv_data);
> +	rt2x00dev->drv_data = NULL;
> +
>  	return retval;
>  }
>  EXPORT_SYMBOL_GPL(rt2x00lib_probe_dev);
> -- 
> 2.34.1
>