Re: [PATCH v11] Input: xpad - overhaul device data for wireless devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Input: xpad - double read of volatile jiffies in xpad_presence_work()
- [Medium] Input: xpad - unlocked mod_delayed_work() race in xpad_presence_work()
- [Medium] Input: xpad - stale wireless pad_info across xpad_suspend() and xpad_resume()
--

commit 1369f7a52af03b86ab596cabb5b216cf2f7e0a71
Author: Sanjay Govind <sanjay.govind9@gmail.com>

Input: xpad - overhaul device data for wireless devices

This commit updates the Xbox 360 wireless controller driver to populate input
device identification from link and capabilities reports rather than using
static receiver-derived defaults. It defers input registration until
capabilities discovery completes or a 500ms timeout expires, protecting the
discovery state with a spinlock and tracking controller generations across
disconnect and suspend.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 810e52782ae89..82408e1c0e044 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c

[ ... ]

> @@ -993,6 +1037,54 @@ static void xpad_presence_work(struct work_struct *work)
>  		 */
>  		xpad_deinit_input(xpad);
>  	}
> +
> +	if (!info.present || xpad->input_created)
> +		return;
> +
> +	/*
> +	 * The work may already be running when a new pad connects, so its
> +	 * original delayed execution cannot be relied on to finish discovery.
> +	 */
> +	scoped_guard(spinlock_irqsave, &xpad->x360w_lock) {
> +		if (!xpad->pad_info.present || xpad->pad_info.gen != info.gen)
> +			return;
> +
> +		if (!xpad->pad_info.discovery_ready &&
> +		    time_before(jiffies, xpad->pad_info.discovery_deadline)) {
> +			delay = xpad->pad_info.discovery_deadline - jiffies;
> +		} else {
> +			xpad->pad_info.discovery_ready = true;
> +			info = xpad->pad_info;
> +		}
> +	}
> +
> +	if (delay) {
> +		mod_delayed_work(system_percpu_wq, &xpad->work, delay);
> +		return;
> +	}
> +
> +	if (info.discovery_ready) {

[Severity: Medium]
In xpad_presence_work() in drivers/input/joystick/xpad.c, jiffies is read
twice when checking the discovery deadline:

	if (!xpad->pad_info.discovery_ready &&
	    time_before(jiffies, xpad->pad_info.discovery_deadline)) {
		delay = xpad->pad_info.discovery_deadline - jiffies;
	}

Because jiffies is volatile, the compiler emits two separate loads. Even with
local interrupts disabled under xpad->x360w_lock, another CPU can advance
jiffies between the time_before() check and the subtraction.

If jiffies is discovery_deadline - 1 during time_before() and ticks to
discovery_deadline before the subtraction, time_before() succeeds so
info.discovery_ready remains false, while delay evaluates to 0. Both
if (delay) and if (info.discovery_ready) then evaluate to false, causing
xpad_presence_work() to return without rescheduling xpad->work or calling
xpad_init_input(). If the controller does not send a capabilities report,
will it ever be registered?

Similarly, if jiffies advances by two or more ticks between the two reads,
could discovery_deadline - jiffies underflow unsigned long and schedule
xpad->work far in the future? Snapshotting jiffies into a local variable
before time_before() and the subtraction would avoid both issues.

[ ... ]

> @@ -1009,19 +1101,102 @@ static void xpad_presence_work(struct work_struct *work)

[ ... ]

> +	/* Capabilities report */
> +	if (len >= 21 && data[0] == 0x00 && data[1] == 0x05 && data[5] == 0x12) {
> +		discovery_complete = false;
> +
> +		scoped_guard(spinlock_irqsave, &xpad->x360w_lock) {
> +			if (info->present && info->link_report_received &&
> +			    !info->discovery_ready) {
> +				info->flags |= data[20];
> +				/*
> +				 * Some vendors encode identification in the stick
> +				 * capabilities. Only trust it when the VID matches
> +				 * the link report.
> +				 */
> +				if (info->vid &&
> +				    get_unaligned_le16(data + 10) == info->vid &&
> +				    get_unaligned_le16(data + 12)) {
> +					info->pid = get_unaligned_le16(data + 12);
> +					info->version = get_unaligned_le16(data + 14);
> +				}
> +
> +				info->discovery_ready = true;
> +				discovery_complete = true;
> +			}
> +		}
> +
> +		if (discovery_complete)
> +			mod_delayed_work(system_percpu_wq, &xpad->work, 0);
>  	}

[Severity: Medium]
Can mod_delayed_work() in xpad_presence_work() race with
xpad360w_process_packet() and overwrite an immediate 0-delay schedule with a
stale timeout delay?

Consider the following sequence in drivers/input/joystick/xpad.c:

CPU0 (xpad_presence_work)
  scoped_guard(spinlock_irqsave, &xpad->x360w_lock) {
      /* sees !discovery_ready and time_before(jiffies, discovery_deadline) */
      delay = xpad->pad_info.discovery_deadline - jiffies;
  }
  /* lock is released */

CPU1 (xpad_irq_in -> xpad360w_process_packet)
  /* capabilities report arrives (or pad disconnects) */
  info->discovery_ready = true;
  mod_delayed_work(system_percpu_wq, &xpad->work, 0);

CPU0 (xpad_presence_work)
  if (delay) {
      mod_delayed_work(system_percpu_wq, &xpad->work, delay);
      return;
  }

Because xpad_presence_work() is still running when CPU1 queues xpad->work
with delay 0, the work item is pending when CPU0 calls mod_delayed_work()
outside xpad->x360w_lock. CPU0 then replaces the 0-delay timer with the stale
non-zero delay, stalling controller registration or disconnect teardown until
the discovery timeout expires.

Should mod_delayed_work() be called while holding xpad->x360w_lock in
xpad_presence_work() and in the capabilities completion path of
xpad360w_process_packet(), matching the presence-change path?

[ ... ]

> @@ -2271,14 +2533,38 @@ static int xpad_suspend(struct usb_interface *intf, pm_message_t message)
>  		 */
>  		xpad360w_stop_input(xpad);
>  
> +		scoped_guard(spinlock_irqsave, &xpad->x360w_lock) {
> +			info = xpad->pad_info;
> +		}
> +
>  		/*
>  		 * The wireless adapter is going off now, so the
>  		 * gamepads are going to become disconnected.
>  		 * Unless explicitly disabled, power them down
>  		 * so they don't just sit there flashing.
>  		 */
> -		if (auto_poweroff && xpad->pad_present)
> +		if (auto_poweroff && info.present)
>  			xpad360w_poweroff_controller(xpad);
> +
> +		/* Finish teardown if suspend cancelled work for an old pad. */
> +		if (xpad->input_created &&
> +		    (!info.present || xpad->input_gen != info.gen)) {
> +			RCU_INIT_POINTER(xpad->x360w_dev, NULL);
> +			synchronize_rcu();
> +			xpad_deinit_input(xpad);
> +		}
> +
> +		/*
> +		 * A cancelled discovery must be restarted by the resume inquiry.
> +		 * Clear its metadata so late reports cannot seed the next pad.
> +		 */
> +		if (!xpad->input_created) {
> +			scoped_guard(spinlock_irqsave, &xpad->x360w_lock) {
> +				xpad->pad_info = (struct xpad360w_pad_info) {
> +					.gen = xpad->pad_info.gen,
> +				};
> +			}
> +		}
>  	} else {

[Severity: Medium]
As reported by Sashiko on v10 [1], what happens across xpad_suspend() and
xpad_resume() in drivers/input/joystick/xpad.c when a wireless controller is
already registered before suspend (xpad->input_created is true and
xpad->input_gen == info.gen) and is replaced while xpad->irq_in is stopped?

In xpad_suspend(), when xpad->input_created is true and info.present is true
with matching generation, the function stops xpad->irq_in and powers off the
controller via xpad360w_poweroff_controller(), but skips both the teardown
block and the !xpad->input_created reset block. As a result,
xpad->pad_info.present and xpad->pad_info.discovery_ready remain true.

If xpad_resume() subsequently runs without an interface unbind/rebind (for
example during hibernation freeze/thaw or if port suspend fails during system
suspend) and calls xpad360w_start_input() -> xpad_inquiry_pad_presence()
while a different controller is connected to that slot,
xpad360w_process_packet() sees info->present == true and
info->discovery_ready == true:

	if (info->present != present) {
		...
	}

	/* Link report */
	if (len >= 26 && data[0] == 0x00 && data[1] == 0x0F) {
		...
		scoped_guard(spinlock_irqsave, &xpad->x360w_lock) {
			if (info->present && !info->discovery_ready) {

Wouldn't this cause the replacement controller's presence, link, and
capabilities reports to be ignored so it continues using the previous
controller's input_dev and identity?

[1] https://lore.kernel.org/r/20261021221009.361992-3-sanjay.govind9@gmail.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103021550.1054584-3-sanjay.govind9@gmail.com?part=1