Re: [PATCH v6 03/12] Input: xbox_gip - Add controllable LED support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Registering a managed LED classdev against an input device creates a circular dependency, leading to memory leaks, list corruption, and kernel crashes on device reconnect.

Pre-existing issues:
- [High] Repeatedly calling `devm_input_allocate_device()` against the USB interface without unbinding leaks `input_dev` structures on every device reconnect.
--

commit 170b2e9725cebf9b1d26ed7fb1bf1a2b52d452c6
Author: Vicki Pfau <vi@endrift.com>
Input: xbox_gip - Add controllable LED support

This commit adds controllable LED support for Xbox One controllers. It exposes 
both monochrome white and RGBW addressable LEDs as led class devices depending 
on the controller capabilities.

> diff --git a/drivers/input/joystick/gip/gip.h b/drivers/input/joystick/gip/gip.h
> index 605149834269f..69eef001d91ff 100644
> --- a/drivers/input/joystick/gip/gip.h
> +++ b/drivers/input/joystick/gip/gip.h
> @@ -234,6 +235,10 @@ struct gip_attachment {
>  	uint8_t seq_vendor;
>  
>  	uint8_t device_state;
> +	union {
> +		struct led_classdev standard;
> +		struct led_classdev_mc color;
> +	} guide_led;

[Severity: High]
Does embedding the led_classdev directly inside the long-lived
gip_attachment structure risk list corruption?

If the input device is reset and re-probed (for example, via a reset
message), the driver recreates the device and could call
devm_led_classdev_register() again on the exact same already-registered
led_classdev struct. This would corrupt the global leds_list and cause a
kernel crash.

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> index 0e23811a6ecc1..a5ed5f3bf3fed 100644
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c
[ ... ]
> +static int gip_guide_led_probe(struct gip_attachment *attachment, struct device *dev)
> +{
[ ... ]
> +		if (!rc)
> +			rc = devm_led_classdev_multicolor_register(dev,
> +				mc_cdev);

[Severity: High]
Can this create a circular dependency? 

By using devm_led_classdev_multicolor_register() in gip_guide_led_probe()
with the input_dev as the parent device, the LED child device takes a
reference to the input_dev. Since input_dev devres actions only execute when
its reference count drops to zero, the LED device is never automatically
unregistered, creating a circular reference and leaking memory.

[ ... ]
> +	} else {
> +		struct led_classdev *cdev = &attachment->guide_led.standard;
[ ... ]
> +		rc = devm_led_classdev_register(dev, cdev);

[Severity: High]
Similarly, does registering the standard LED classdev against the input device
here create the same circular dependency preventing automatic unregistration?

[ ... ]
> @@ -1157,6 +1268,10 @@ static int gip_setup_input_device(struct gip_attachment *attachment)
>  	if (rc)
>  		goto err_free_device;
>  
> +	rc = gip_guide_led_probe(attachment, &input->dev);
> +	if (rc)
> +		gip_err(attachment, "Failed to register LEDs: %d\n", rc);
> +
>  	return 0;

[Severity: High]
This isn't a bug introduced by this patch, but does the allocation of the
input device earlier in gip_setup_input_device() leak input_dev structures on
every reconnect?

The code uses devm_input_allocate_device() with the long-lived USB interface
as the parent. When the controller resets (by sending a reset packet),
the driver correctly calls input_unregister_device(), but the devm free action
remains attached to the USB interface's devres list. A new input_dev is then
allocated for the re-initialization, leaking an entire input_dev structure
each time until the USB dongle is physically unplugged.

[Severity: High]
Also, if a controller reset causes gip_setup_input_device() to run again, does
calling gip_guide_led_probe() register the LEDs a second time without
ensuring the previous LED instances were unregistered?

Combined with embedding the led_classdev inside the attachment, this seems
like it would trigger list corruption.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908032145.2118234-1-vi@endrift.com?part=3