Re: [PATCH v2 2/4] HID: wiimote: replace spinlock pairs with scoped_guard
From: sashiko-bot@kernel.org
Date: 2026-07-15 21:39:19
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: New issues: - [Medium] Functions mix `goto`-based error handling cleanup labels with scope-based cleanup (`scoped_guard()`), violating the cleanup subsystem architectural rule. - [Low] Incorrect 4-space indentation within `scoped_guard` blocks instead of standard kernel tabs. Pre-existing issues: - [High] Use-after-free of `wdata` because `queue.worker` can be rescheduled after it is cancelled in `wiimote_destroy()`. --
--- Patch [2]: [PATCH v2 2/4] HID: wiimote: replace spinlock pairs with scoped_guard ---commit 343c163a7f9f1368f62e298a4bac0953c054d9f3
Author: Rafael Passos [off-list ref]
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.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c index 91c2cec35f6eb..d37d740a7162f 100644 --- a/drivers/hid/hid-wiimote-core.c +++ b/drivers/hid/hid-wiimote-core.c@@ -362,13 +363,12 @@ void wiiproto_req_rmem(struct wiimote_data *wdata, bool eeprom, __u32 offset, int wiimote_cmd_write(struct wiimote_data *wdata, __u32 offset, const __u8 *wmem, __u8 size) {
[ ... ]
- spin_lock_irqsave(&wdata->state.lock, flags);
- wiimote_cmd_set(wdata, WIIPROTO_REQ_WMEM, 0);
- wiiproto_req_wreg(wdata, offset, wmem, size);
- spin_unlock_irqrestore(&wdata->state.lock, flags);
+ scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+ wiimote_cmd_set(wdata, WIIPROTO_REQ_WMEM, 0);
+ wiiproto_req_wreg(wdata, offset, wmem, size);
+ }[Severity: Low] this isn't a bug, but did the indentation get changed to 4 spaces here instead of standard kernel tabs? [ ... ]
quoted hunk ↗ jump to hunk
@@ -885,19 +874,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: Medium] Does this mix goto-based cleanup labels with scope-based cleanup? The cleanup guidelines state that for a given routine, all resources requiring cleanup should be converted to scope-based cleanup, or none of them should be. This function (as well as wiimote_init_check() and wiimote_modules_load()) still uses goto-based cleanup like goto out_release; and goto error; combined with the new scoped_guard() declarations. [ ... ]
quoted hunk ↗ jump to hunk
@@ -1120,16 +1108,15 @@ static void wiimote_init_hotplug(struct wiimote_data *wdata) wiimote_cmd_acquire_noint(wdata); - spin_lock_irq(&wdata->state.lock); - - /* get state snapshot that we will then work on */ - flags = wdata->state.flags; + scoped_guard(spinlock_irq, &wdata->state.lock) { - /* disable event forwarding temporarily */ - wdata->state.flags &= ~WIIPROTO_FLAG_EXT_ACTIVE; - wdata->state.flags &= ~WIIPROTO_FLAG_MP_ACTIVE; + /* get state snapshot that we will then work on */ + flags = wdata->state.flags;
[Severity: Low] this isn't a bug, but it looks like the contents of this scoped_guard block are also indented with 4 spaces instead of a tab. [ ... ]
quoted hunk ↗ jump to hunk
@@ -1774,14 +1749,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 isn't a bug introduced by this patch, but does the teardown sequence
in wiimote_destroy() have a use-after-free race condition?
Looking at the rest of wiimote_destroy(), cancel_work_sync(&wdata->queue.worker)
is called before hid_hw_stop(wdata->hdev). If a HID interrupt arrives
immediately after the worker is cancelled:
HID core calls wiimote_hid_event() ->
handler_status_K() ->
wiiproto_req_drm() ->
wiimote_queue()
wiimote_queue() then calls schedule_work(&wdata->queue.worker) without
checking if the device is exiting.
Since hid_hw_stop() hasn't been called yet, the new worker is queued. After
hid_hw_stop() and kfree(wdata) complete, the queued worker can execute and
dereference the freed wdata.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715212501.3920033-1-rafael@rcpassos.me?part=2