From: Tristan Madani <tristan@talencesecurity.com>

oxp_hid_remove() uses cancel_delayed_work() for all three delayed work
items.  cancel_delayed_work() only dequeues a pending work item without
waiting for a currently executing callback to finish.  If any of the
work callbacks (oxp_rgb_queue_fn, oxp_btn_queue_fn, oxp_mcu_init_fn) is
running at the time of removal, the callback continues executing
concurrently with hid_hw_close() and hid_hw_stop(), accessing the HID
device after it has been closed and stopped.

Use cancel_delayed_work_sync() instead to ensure that any in-progress
work callback completes before device teardown proceeds.

Fixes: 84910c459d65 ("HID: hid-oxp: Add OneXPlayer configuration driver")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 drivers/hid/hid-oxp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index d2ded6b08ce9e..1e691ebc1199e 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -1552,9 +1552,9 @@ static int oxp_hid_probe(struct hid_device *hdev,
 
 static void oxp_hid_remove(struct hid_device *hdev)
 {
-	cancel_delayed_work(&drvdata.oxp_rgb_queue);
-	cancel_delayed_work(&drvdata.oxp_btn_queue);
-	cancel_delayed_work(&drvdata.oxp_mcu_init);
+	cancel_delayed_work_sync(&drvdata.oxp_rgb_queue);
+	cancel_delayed_work_sync(&drvdata.oxp_btn_queue);
+	cancel_delayed_work_sync(&drvdata.oxp_mcu_init);
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
 }
-- 
2.47.3

On Fri, 4 Sep 2026, Tristan Madani wrote:

> From: Tristan Madani <tristan@talencesecurity.com>
> 
> oxp_hid_remove() uses cancel_delayed_work() for all three delayed work
> items.  cancel_delayed_work() only dequeues a pending work item without
> waiting for a currently executing callback to finish.  If any of the
> work callbacks (oxp_rgb_queue_fn, oxp_btn_queue_fn, oxp_mcu_init_fn) is
> running at the time of removal, the callback continues executing
> concurrently with hid_hw_close() and hid_hw_stop(), accessing the HID
> device after it has been closed and stopped.
> 
> Use cancel_delayed_work_sync() instead to ensure that any in-progress
> work callback completes before device teardown proceeds.
> 
> Fixes: 84910c459d65 ("HID: hid-oxp: Add OneXPlayer configuration driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tristan Madani <tristan@talencesecurity.com>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs

On September 4, 2026 3:58:00 AM PDT, Tristan Madani <tristmd@gmail.com> wrote:
>From: Tristan Madani <tristan@talencesecurity.com>
>
>oxp_hid_remove() uses cancel_delayed_work() for all three delayed work
>items.  cancel_delayed_work() only dequeues a pending work item without
>waiting for a currently executing callback to finish.  If any of the
>work callbacks (oxp_rgb_queue_fn, oxp_btn_queue_fn, oxp_mcu_init_fn) is
>running at the time of removal, the callback continues executing
>concurrently with hid_hw_close() and hid_hw_stop(), accessing the HID
>device after it has been closed and stopped.
>
>Use cancel_delayed_work_sync() instead to ensure that any in-progress
>work callback completes before device teardown proceeds.
>
>Fixes: 84910c459d65 ("HID: hid-oxp: Add OneXPlayer configuration driver")
>Cc: stable@vger.kernel.org
>Signed-off-by: Tristan Madani <tristan@talencesecurity.com>


Hi Tristan,

LGTM, thanks.

Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
>---
> drivers/hid/hid-oxp.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
>index d2ded6b08ce9e..1e691ebc1199e 100644
>--- a/drivers/hid/hid-oxp.c
>+++ b/drivers/hid/hid-oxp.c
>@@ -1552,9 +1552,9 @@ static int oxp_hid_probe(struct hid_device *hdev,
> 
> static void oxp_hid_remove(struct hid_device *hdev)
> {
>-	cancel_delayed_work(&drvdata.oxp_rgb_queue);
>-	cancel_delayed_work(&drvdata.oxp_btn_queue);
>-	cancel_delayed_work(&drvdata.oxp_mcu_init);
>+	cancel_delayed_work_sync(&drvdata.oxp_rgb_queue);
>+	cancel_delayed_work_sync(&drvdata.oxp_btn_queue);
>+	cancel_delayed_work_sync(&drvdata.oxp_mcu_init);
> 	hid_hw_close(hdev);
> 	hid_hw_stop(hdev);
> }

Hi Derek,

Thanks for the review.

One thing the Sashiko bot flagged, and I think it's valid: if
oxp_hid_probe() returns early on an unknown usage page (the
default: return 0 path), the delayed work items are never
initialized via INIT_DELAYED_WORK().  In that case,
cancel_delayed_work_sync() in remove() would call __flush_work()
on a zero-filled work struct, which hits WARN_ON(!work->func).

The non-sync cancel_delayed_work() didn't trigger this because it
never calls __flush_work().

Do you think this path is actually reachable in practice, or are
the usage page IDs exhaustive for all matched devices?  If it can
happen, I can send a v2 that guards the cancels.

Thanks,
Tristan

On September 4, 2026 2:17:02 PM PDT, Tristan Madani <tristmd@gmail.com> wrote:
>Hi Derek,
>
>Thanks for the review.
>
>One thing the Sashiko bot flagged, and I think it's valid: if
>oxp_hid_probe() returns early on an unknown usage page (the
>default: return 0 path), the delayed work items are never
>initialized via INIT_DELAYED_WORK().  In that case,
>cancel_delayed_work_sync() in remove() would call __flush_work()
>on a zero-filled work struct, which hits WARN_ON(!work->func).
>
>The non-sync cancel_delayed_work() didn't trigger this because it
>never calls __flush_work().
>
>Do you think this path is actually reachable in practice, or are
>the usage page IDs exhaustive for all matched devices?  If it can
>happen, I can send a v2 that guards the cancels.
>
If the usage page rejects init then I think we can safely assume that it will be an effective gate.

There's a series in the works ATM that will make drvdata unique per hid dev after the usage page checka, so we could also wait for that and if it's not fixed by that series then gate on drvdata not being NULL.

Cheers,
Derek

>Thanks,
>Tristan