Re: [PATCH v5 05/11] HID: steam: Coalesce rumble packets

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

New issues:
- [High] Dynamic device unregistration fails to cancel `coalesce_rumble_work`, causing an infinite loop and resource leak.
- [Medium] Lockless `delayed_work_pending()` check bypasses rate limiting, causing haptic reset bugs.

Pre-existing issues:
- [High] Teardown order in `steam_remove()` causes Use-After-Free by cancelling work items before unregistering the input device.
--

--- Patch [5]: [PATCH v5 05/11] HID: steam: Coalesce rumble packets ---
commit d8418fccc5859b59417d02482c0b768383aa61ef
Author: Vicki Pfau <[email protected]>

HID: steam: Coalesce rumble packets

This commit introduces a 20Hz rate limit for rumble packets sent to the Steam
Deck to prevent the hardware from resetting haptic patterns on every packet.
It implements this rate limiting by using a delayed work queue to coalesce 
rumble updates.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -603,10 +604,24 @@ static void steam_haptic_rumble_cb(struct work_struct *work)
>  {
>  	struct steam_device *steam = container_of(work, struct steam_device,
>  							rumble_work);
> +
>  	steam_haptic_rumble(steam, 0, steam->rumble_left,
>  		steam->rumble_right, 2, 0);
>  }
>  
> +static void steam_coalesce_rumble_cb(struct work_struct *work)
> +{
> +	struct steam_device *steam = container_of(to_delayed_work(work),
> +							struct steam_device,
> +							coalesce_rumble_work);
> +
> +	steam_haptic_rumble(steam, 0, steam->rumble_left,
> +		steam->rumble_right, 2, 0);
> +
> +	if (steam->rumble_left || steam->rumble_right)
> +		schedule_delayed_work(&steam->coalesce_rumble_work, HZ / 20);
> +}

[Severity: High]
Will this work item reschedule itself indefinitely if the device is dynamically
unregistered while a haptic effect is playing?

When the device is dynamically unregistered, such as during a wireless
disconnect or a mode switch, steam_unregister() destroys the input device
via steam_input_unregister().

However, steam_unregister() does not cancel coalesce_rumble_work, and the
input core does not proactively clear the rumble_left and rumble_right
magnitudes when an input device is destroyed. Because the input device is
gone, userspace can never send a 0-magnitude effect to stop the rumble,
leaving this worker to loop indefinitely every 50ms and leak CPU cycles
and USB bandwidth.

> +
>  #ifdef CONFIG_STEAM_FF
>  static int steam_play_effect(struct input_dev *dev, void *data,
>  				struct ff_effect *effect)
> @@ -616,6 +631,14 @@ static int steam_play_effect(struct input_dev *dev, void *data,
>  	steam->rumble_left = effect->u.rumble.strong_magnitude;
>  	steam->rumble_right = effect->u.rumble.weak_magnitude;
>  
> +	/*
> +	 * The interface gets somewhat overloaded when too many rumble
> +	 * packets are sent in a row, so Steam throttles it to 20 Hz
> +	 */
> +	if (delayed_work_pending(&steam->coalesce_rumble_work))
> +		return 0;
> +
> +	schedule_delayed_work(&steam->coalesce_rumble_work, HZ / 20);
> +	return schedule_work(&steam->rumble_work);
>  }

[Severity: Medium]
Could this lockless check of delayed_work_pending() defeat the intended
rate limiting?

The workqueue core clears the pending bit immediately before executing the
worker callback. Because steam_coalesce_rumble_cb() acquires a mutex and
performs synchronous USB I/O, its execution time can be relatively large.

If a userspace process updates force feedback effects while the callback is
actively executing, delayed_work_pending() will evaluate to false. This
causes steam_play_effect() to immediately queue another rumble_work, which
then executes right after the callback finishes, sending two packets
back-to-back and bypassing the 50ms throttle.

[ ... ]
> @@ -1444,6 +1469,7 @@ static void steam_remove(struct hid_device *hdev)
>  	cancel_delayed_work_sync(&steam->mode_switch);
>  	cancel_work_sync(&steam->work_connect);
>  	cancel_work_sync(&steam->rumble_work);
> +	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
>  	cancel_work_sync(&steam->unregister_work);
>  	steam->client_hdev = NULL;
>  	steam->client_opened = 0;

[Severity: High]
This is a pre-existing issue, but does the teardown order in steam_remove()
allow a use-after-free?

The works are cancelled before the input device is unregistered:

steam_remove() {
    ...
    cancel_work_sync(&steam->rumble_work);
    cancel_delayed_work_sync(&steam->coalesce_rumble_work);
    ...
    steam_unregister(steam);
}

If a userspace process continuously calls the EVIOCSFF ioctl to play
force-feedback effects while the device is being removed, the ioctl
path in steam_play_effect() can queue new rumble_work and
coalesce_rumble_work items after they have been cancelled.

Once steam_remove() completes, the devm-managed steam structure is freed,
but the newly queued work items will eventually execute and dereference
the freed memory.

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