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

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

New issues:
- [Medium] Mixing `goto` error handling with `scoped_guard` cleanup helpers in the same function.
- [Medium] Incomplete refactoring and unfulfilled commit message claims regarding lock replacements.

Pre-existing issues:
- [High] Race condition in teardown sequence allows re-scheduling of canceled work, leading to Use-After-Free.
--

--- Patch [2]: [PATCH v3 2/4] HID: wiimote: replace spinlock pairs with scoped_guard ---
commit 20054d5c329a400384a729c4fe3021c525d804b5
Author: Rafael Passos <[email protected]>

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.

The multi line spinlock blocks were replaced by braced scoped_guard,
and one-liners by a scoped_guard without braces nor indentation.

> There are two cases left in this driver using lock/unlock, because
> guard would make the code more complex than current implementation.

[Severity: Medium]
Is this statement accurate? It appears that drivers/hid/hid-wiimote-modules.c
was mostly skipped during this refactoring, leaving over 30 manual
spin_lock_irqsave() and spin_lock_irq() calls on wdata->state.lock in
functions like wiimod_rumble_worker() and wiimod_battery_get_property().

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 48830f2ffcb50..762b3c383194e 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
[ ... ]
> @@ -675,11 +673,8 @@ static void wiimote_modules_load(struct wiimote_data *wdata,
>  			goto error;
>  	}
>  
> -	spin_lock_irq(&wdata->state.lock);
> -	wdata->state.devtype = devtype;
> -	spin_unlock_irq(&wdata->state.lock);
> -
>  	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
> +		wdata->state.devtype = devtype;
>  		/* after loading modules, set the Player ID LED cycling from 1 to 4*/
>  		wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
>  	}

[Severity: Medium]
Does this violate the cleanup helper guidelines? This function uses a
"goto error" label for manual error handling earlier in the function,
but now introduces a scoped_guard(). The subsystem guidelines typically
forbid mixing goto-based error handling and scope-based cleanup helpers
in the same function to avoid confusing ownership semantics. This also
happens in wiimote_init_detect() and wiimote_init_check().

[ ... ]
> @@ -1774,14 +1747,11 @@ 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);

[Severity: High]
This is a pre-existing issue, but does the teardown sequence later in this
function have a race condition? 

In wiimote_destroy(), cancel_work_sync() is called on wdata->queue.worker
before the hardware transport is stopped via hid_hw_stop():

	cancel_work_sync(&wdata->queue.worker);
	hid_hw_close(wdata->hdev);
	hid_hw_stop(wdata->hdev);
	kfree(wdata);

If an incoming hardware event arrives in the window after cancel_work_sync()
but before hid_hw_stop(), it can be processed by wiimote_hid_event(). This
could trigger a status reply that queues data and re-schedules the worker via
schedule_work(&wdata->queue.worker).

Since the worker can be re-enqueued after it was intended to be canceled,
could it execute after kfree(wdata) and cause a use-after-free? Should
the hardware source be stopped before canceling works that the hardware
can schedule?

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