Re: [PATCH] HID: microsoft: cancel the rumble work on removal

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Danish,

On Sat, Sep 26, 2026 at 08:12:20PM -0500, Danish Khateeb wrote:
> Commit 1cfc77a64b71 ("HID: microsoft: move FF initialization to
> .input_configured()") removed ms_remove_ff() along with ms_init_ff(),
> and with it the only cancel_work_sync() of ff_worker. Nothing waits for
> the rumble work any more before devres frees struct ms_data and the
> report buffer that the work fills in.
> 
> Removing the device queues the work itself: hid_hw_stop() unregisters
> the input device, and if an effect is playing, input_ff_flush() stops
> it through ms_play_effect(). The work usually runs before remove()
> returns, but nothing guarantees it.
> 
> Cancel the work again after hid_hw_stop(), when the input device is gone
> and nothing can queue it any more. Initialize it in ms_probe(), so that
> it can be cancelled for devices without force feedback too.
> 
> Fixes: 1cfc77a64b71 ("HID: microsoft: move FF initialization to .input_configured()")
> Assisted-by: LLM
> Signed-off-by: Danish Khateeb <danishkhateeb03@gmail.com>
> ---
> 
> Notes:
>     Tested in QEMU on a next-20260925 KASAN kernel with uhid devices bound
>     to hid-microsoft: an Xbox Wireless Controller (BT 045e:0b13) destroyed
>     while a rumble effect was playing and its evdev node was open (20
>     cycles), a Natural Ergonomic 4000 (no force feedback, 5 cycles), then
>     rmmod. Tracing shows the removal queuing ff_worker. There are no
>     warnings with or without this patch: I could not reproduce the
>     use-after-free, as the work always ran before remove() returned. A W=1
>     build is clean.
> 
>  drivers/hid/hid-microsoft.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
> index a7d3493a6141..1b527954937d 100644
> --- a/drivers/hid/hid-microsoft.c
> +++ b/drivers/hid/hid-microsoft.c
> @@ -335,7 +335,6 @@ static int ms_input_configured(struct hid_device *hdev, struct hid_input *hidinp
>  		return 0;
>  
>  	ms->hdev = hdev;
> -	INIT_WORK(&ms->ff_worker, ms_ff_worker);
>  
>  	ms->output_report_dmabuf = devm_kzalloc(&hdev->dev,
>  						sizeof(struct xb1s_ff_report),
> @@ -358,6 +357,7 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		return -ENOMEM;
>  
>  	ms->quirks = quirks;
> +	INIT_WORK(&ms->ff_worker, ms_ff_worker);
>  
>  	hid_set_drvdata(hdev, ms);
>  
> @@ -384,7 +384,11 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  }
>  static void ms_remove(struct hid_device *hdev)
>  {
> +	struct ms_data *ms = hid_get_drvdata(hdev);
> +
>  	hid_hw_stop(hdev);
> +	/* Unregistering the input device stops rumble, which queues the work */
> +	cancel_work_sync(&ms->ff_worker);

I think this is too late: by this time the hardware is shut off so if
the work is still running and tries to access the hardware there may be
failures.

I think I will add support for "slow" effect playback to memoryless
handling so that the drivers do not need to worry about managing their
private work items.

Thanks.

-- 
Dmitry