Re: [PATCH 2/3] HID: wiimote: replace spinlock pairs with scoped_guard

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Functions wiimote_init_detect() and wiimote_init_check() mix goto-based cleanup labels with cleanup.h helpers (scoped_guard).

Pre-existing issues:
- [High] Race condition during device teardown allows `queue.worker` to be re-scheduled after cancellation, leading to a Use-After-Free on `wdata`.
--

--- Patch [2]: [PATCH 2/3] HID: wiimote: replace spinlock pairs with scoped_guard ---
commit b72167f4bfaf6c447e5bff82a564df82f7c2a54d
Author: Rafael Passos <rafael@rcpassos.me>

HID: wiimote: replace spinlock pairs with scoped_guard

Cleanup code replacing manual lock/unlock with scoped guards.
This does not change any behavior, but makes it safer to modify.

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -878,19 +867,19 @@ static void wiimote_init_detect(struct wiimote_data *wdata)
>  
>  	wiimote_cmd_acquire_noint(wdata);
>  
> -	spin_lock_irq(&wdata->state.lock);
> -	wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
> -	wiimote_cmd_set(wdata, WIIPROTO_REQ_SREQ, 0);
> -	wiiproto_req_status(wdata);
> -	spin_unlock_irq(&wdata->state.lock);
> +	scoped_guard(spinlock_irq, &wdata->state.lock) {
> +		wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
> +		wiimote_cmd_set(wdata, WIIPROTO_REQ_SREQ, 0);
> +		wiiproto_req_status(wdata);
> +	}
> +
>  
>  	ret = wiimote_cmd_wait_noint(wdata);
>  	if (ret)
>  		goto out_release;

[Severity: Low]
Is it acceptable to mix goto-based error handling with scoped_guard() in the
same function?

According to the cleanup.h documentation, the expectation is that a function
either uses goto-based cleanup or scope-based cleanup exclusively. Since
this patch introduces scoped_guard() while leaving goto out_release, does
this violate the subsystem guidelines for mixing cleanup styles? This same
pattern also appears further down in wiimote_init_check().

[ ... ]

> @@ -1767,14 +1745,12 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev)
>  
>  static void wiimote_destroy(struct wiimote_data *wdata)
>  {
> -	unsigned long flags;
> -
>  	wiidebug_deinit(wdata);
>  
>  	/* prevent init_worker from being scheduled again */
> -	spin_lock_irqsave(&wdata->state.lock, flags);
> -	wdata->state.flags |= WIIPROTO_FLAG_EXITING;
> -	spin_unlock_irqrestore(&wdata->state.lock, flags);
> +	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
> +	    wdata->state.flags |= WIIPROTO_FLAG_EXITING;
> +	}
>  
>  	cancel_work_sync(&wdata->init_worker);
>  	timer_shutdown_sync(&wdata->timer);
[ ... ]
> 	wiimote_mp_unload(wdata);
> 	wiimote_ext_unload(wdata);
> 	wiimote_modules_unload(wdata);
> 	cancel_work_sync(&wdata->queue.worker);
> 	hid_hw_close(wdata->hdev);
> 	hid_hw_stop(wdata->hdev);
> 
> 	kfree(wdata);
>  }

[Severity: High]
This is a pre-existing issue, but could this teardown sequence result in a
use-after-free on wdata?

Currently, cancel_work_sync(&wdata->queue.worker) is called before
hid_hw_stop(wdata->hdev). If an incoming status report arrives after the work
is canceled but before the HID device is stopped, wiimote_hid_event() could
handle it and call handler_status_K() -> wiiproto_req_drm() ->
wiimote_queue(), which unconditionally re-arms the work queue via
schedule_work(&wdata->queue.worker).

If the queue worker executes after wdata is freed by kfree(wdata) here,
would it dereference freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710153456.2093889-1-rafael@rcpassos.me?part=2