Re: [PATCH v3 1/6] HID: steam: Unregister battery on partial registration error

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

New issues:
- [Medium] Tearing down the battery on the error path without clearing `steam->serial_no` prevents battery registration on subsequent retries.

Pre-existing issues:
- [High] Concurrent execution of unregistration functions allows unprotected RCU pointer modifications, causing potential double frees.
--

--- Patch [1]: [PATCH v3 1/6] HID: steam: Unregister battery on partial registration error ---
commit 024061e54a475950f36c1803b7d20aeb74ac4f96
Author: Vicki Pfau <[email protected]>

HID: steam: Unregister battery on partial registration error

In a previous refactor the code for unregistering a battery if later
registration steps failed was accidentally left out. As a result, a
lingering power_supply object could get left over after the steam object
was torn down.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 6199f67f3c4cd..967f12c7a599a 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -1199,6 +1199,7 @@ static int steam_register(struct steam_device *steam)
>  steam_register_sensors_fail:
>  	steam_input_unregister(steam);
>  steam_register_input_fail:
> +	steam_battery_unregister(steam);

[Severity: Medium]
Will this error path leave the device in an inconsistent state on retry?

If steam_register() fails and steam_battery_unregister() is called,
steam->serial_no[0] is not cleared.

On a subsequent retry (e.g. from steam_work_connect_cb()), steam_register()
will see the serial number populated and skip the initialization block where
steam_battery_register() is called.

This results in the input devices registering successfully, but the battery
object being permanently missing until a full device disconnect.

[Severity: High]
This is a pre-existing issue, but does calling steam_battery_unregister()
here expose a race condition?

When a user holds the HID device open, hid_hw_close(hdev) in steam_remove()
doesn't drop ll_open_count to zero, leaving the interrupt URB active.

After cancel_work_sync(&steam->work_connect), the active URB can still
receive raw events and re-queue work_connect.

This allows a worker thread to execute steam_register() or
steam_unregister() concurrently with steam_remove().

Since steam_battery_unregister() only uses rcu_read_lock() for reads, both
threads can observe a non-NULL battery pointer:

drivers/hid/hid-steam.c:steam_battery_unregister() {
	rcu_read_lock();
	battery = rcu_dereference(steam->battery);
	rcu_read_unlock();

	if (!battery)
		return;
	RCU_INIT_POINTER(steam->battery, NULL);
	synchronize_rcu();
	power_supply_unregister(battery);
	devm_kfree(&steam->hdev->dev, steam->battery_desc.name);
}

Is it possible for both threads to proceed past the NULL check and
double-free the battery and its memory?

>  	return ret;
>  }

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