[PATCH] HID: nintendo-switch2-usb: Improve shutdown logic with inbound message work

[PATCH] HID: nintendo-switch2-usb: Improve shutdown logic with inbound message work

[Vicki Pfau]

When detaching the USB interface the driver marks the interface as shutdown
to prevent work that may be executing in parallel from attempting to use
the URBs before freeing the URBs. There is theoretically a very narrow
window in which the work could be scheduled while the shutdown is running
in a separate thread, starting to execute before the shutdown cancels the
work.

By ensuring the scheduling happens within the locked scope and adding an
additional shutdown check, this closes the window entirely.

Reported-by: Noah Weinberger <noahweinberger2003@proton.me>
Link: https://lore.kernel.org/linux-input/a2dc00e7-9cab-4315-87a4-795f256c1e03@endrift.com/T/
Fixes: 9417132df63e ("HID: nintendo: Add preliminary Switch 2 controller driver")
Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-nintendo-switch2-usb.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-nintendo-switch2-usb.c b/drivers/hid/hid-nintendo-switch2-usb.c
index 8b3b6841035d..80c002a98d5a 100644
--- a/drivers/hid/hid-nintendo-switch2-usb.c
+++ b/drivers/hid/hid-nintendo-switch2-usb.c
@@ -71,16 +71,14 @@ static void switch2_bulk_in(struct urb *urb)
 	}
 
 	spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
-	if (ns2_usb->shutdown)
-		schedule = false;
-
 	for (i = 0; i < NS2_IN_URBS; i++) {
 		int err;
 		struct switch2_urb *ns2_urb;
 
 		if (ns2_usb->bulk_in[i].urb == urb) {
-			if (schedule) {
+			if (!ns2_usb->shutdown) {
 				ns2_usb->bulk_in[i].state = NS2_URB_IN;
+				schedule_work(&ns2_usb->message_in_work);
 				continue;
 			} else {
 				ns2_usb->bulk_in[i].state = NS2_URB_FREE;
@@ -115,9 +113,6 @@ static void switch2_bulk_in(struct urb *urb)
 		}
 	}
 	spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
-
-	if (schedule)
-		schedule_work(&ns2_usb->message_in_work);
 }
 
 static void switch2_bulk_out(struct urb *urb)
@@ -294,6 +289,10 @@ static void switch2_usb_message_in_work(struct work_struct *work)
 	unsigned long flags;
 
 	spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
+	if (ns2_usb->shutdown) {
+		spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
+		return;
+	}
 	for (i = 0; i < NS2_IN_URBS; i++) {
 		urb = &ns2_usb->bulk_in[i];
 		if (urb->state != NS2_URB_IN)
-- 
2.54.0

Re: [PATCH] HID: nintendo-switch2-usb: Improve shutdown logic with inbound message work

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] HID: nintendo-switch2-usb: unused schedule and error bug in
  switch2_bulk_in()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] HID: nintendo: AB-BA deadlock in switch2_usb_shutdown() [1]
--

commit 6f6e1e4dbbc575c0862f97bc1dde9a1e20a88deb
Author: Vicki Pfau <vi@endrift.com>

HID: nintendo-switch2-usb: Improve shutdown logic with inbound message work

This commit moves the scheduling of message_in_work inside bulk_in_lock
in switch2_bulk_in() and adds a shutdown check in
switch2_usb_message_in_work().

Link: https://lore.kernel.org/linux-input/a2dc00e7-9cab-4315-87a4-795f256c1e03@endrift.com/T/

> diff --git a/drivers/hid/hid-nintendo-switch2-usb.c b/drivers/hid/hid-nintendo-switch2-usb.c
> index 623473f3d2161..5174a5c2dbeca 100644
> --- a/drivers/hid/hid-nintendo-switch2-usb.c
> +++ b/drivers/hid/hid-nintendo-switch2-usb.c
> @@ -71,16 +71,14 @@ static void switch2_bulk_in(struct urb *urb)
>  	}
>  
>  	spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
> -	if (ns2_usb->shutdown)
> -		schedule = false;
> -
>  	for (i = 0; i < NS2_IN_URBS; i++) {
>  		int err;
>  		struct switch2_urb *ns2_urb;
>  
>  		if (ns2_usb->bulk_in[i].urb == urb) {
> -			if (schedule) {
> +			if (!ns2_usb->shutdown) {
>  				ns2_usb->bulk_in[i].state = NS2_URB_IN;
> +				schedule_work(&ns2_usb->message_in_work);
>  				continue;
>  			} else {
>  				ns2_usb->bulk_in[i].state = NS2_URB_FREE;
> @@ -115,9 +113,6 @@ static void switch2_bulk_in(struct urb *urb)
>  		}
>  	}
>  	spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
> -
> -	if (schedule)
> -		schedule_work(&ns2_usb->message_in_work);
>  }

[Severity: High]
Does replacing the schedule check with !ns2_usb->shutdown cause failed
URBs to be processed as valid input and leave schedule unused?

Earlier in switch2_bulk_in(), schedule is only set to true when
urb->status is 0, while unhandled non-zero status codes such as -EPROTO
fall through the default branch with schedule set to false:

drivers/hid/hid-nintendo-switch2-usb.c:switch2_bulk_in() {
	bool schedule = false;
	unsigned long flags;

	switch (urb->status) {
	case 0:
		schedule = true;
		break;
	...
	default:
		dev_dbg(&ns2_usb->udev->dev, "unknown input urb status: %d\n", urb->status);
		break;
	}

Without checking schedule here, a failed URB hitting the default branch is
marked NS2_URB_IN and queued for switch2_usb_message_in_work(), passing
stale transfer_buffer data to switch2_receive_command() instead of marking
the URB NS2_URB_FREE and resubmitting it.  Removing all reads of schedule
also triggers a -Wunused-but-set-variable compiler warning.

[ ... ]

> @@ -294,6 +289,10 @@ static void switch2_usb_message_in_work(struct work_struct *work)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
> +	if (ns2_usb->shutdown) {
> +		spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
> +		return;
> +	}
>  	for (i = 0; i < NS2_IN_URBS; i++) {
>  		urb = &ns2_usb->bulk_in[i];
>  		if (urb->state != NS2_URB_IN)

[Severity: High]
This problem wasn't introduced by this patch, but can
switch2_usb_message_in_work() and switch2_usb_shutdown() hit an ABBA
deadlock [1] when an inbound USB packet arrives during controller
disconnect?

CPU0                               kworker
----                               -------
mutex_lock(&ns2->lock)
                                   switch2_usb_message_in_work()
                                     spin_unlock_irqrestore(...)
                                     switch2_receive_command()
switch2_usb_shutdown()
  cancel_work_sync(...)
                                       mutex_lock(&ns2->lock)

 *** DEADLOCK ***

CPU0: switch2_controller_detach_cfg() -> switch2_usb_shutdown()
kworker: switch2_usb_message_in_work() -> switch2_receive_command()

switch2_controller_detach_cfg() (and the error path of
switch2_controller_attach_cfg()) calls cfg->shutdown() while holding
ns2->lock.  If switch2_usb_message_in_work() has already passed the
ns2_usb->shutdown check and dropped bulk_in_lock, it blocks on ns2->lock
in switch2_receive_command() while cancel_work_sync() waits for the work
to finish.

[1] https://sashiko.dev/#/bug/linux-6ed5071c-4448-42b6-82a2-03203abfe1da

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