[PATCH v2 1/3] HID: nintendo-switch2-usb: Fix potential deadlock on teardown

[PATCH v2 1/3] HID: nintendo-switch2-usb: Fix potential deadlock on teardown

[Vicki Pfau]

As reported by Sashiko, there is a narrow window in which cancel_work_sync
on the message in work can block due to the work waiting on the mutex that
the canceling thread holds. By moving the pointer clearing and unlocking
the mutex before calling cfg->shutdown we can avoid this deadlock.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20261006061619.2005967-1-vi%40endrift.com?part=1
Fixes: 9417132df63e ("HID: nintendo: Add preliminary Switch 2 controller driver")
Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-nintendo.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 4d40f67f3e64..d235de6868a2 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -4292,6 +4292,7 @@ int switch2_controller_attach_cfg(const char *phys, struct switch2_cfg_intf *cfg
 	mutex_lock(&ns2->lock);
 	if (ns2->cfg) {
 		ret = -EBUSY;
+		mutex_unlock(&ns2->lock);
 		goto out;
 	}
 	cfg->parent = ns2;
@@ -4301,15 +4302,16 @@ int switch2_controller_attach_cfg(const char *phys, struct switch2_cfg_intf *cfg
 		ret = switch2_init_controller(ns2);
 
 	if (ret < 0) {
+		ns2->cfg = NULL;
+		mutex_unlock(&ns2->lock);
 		if (cfg->shutdown)
 			cfg->shutdown(cfg);
 		cfg->parent = NULL;
-		ns2->cfg = NULL;
+	} else {
+		mutex_unlock(&ns2->lock);
 	}
 
 out:
-	mutex_unlock(&ns2->lock);
-
 	if (ret < 0)
 		kref_put(&ns2->refcount, switch2_kref_put);
 	return ret;
-- 
2.54.0

[PATCH v2 2/3] 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 | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/hid-nintendo-switch2-usb.c b/drivers/hid/hid-nintendo-switch2-usb.c
index 8b3b6841035d..f53cbf0a3e15 100644
--- a/drivers/hid/hid-nintendo-switch2-usb.c
+++ b/drivers/hid/hid-nintendo-switch2-usb.c
@@ -52,12 +52,10 @@ static void switch2_bulk_in(struct urb *urb)
 {
 	struct switch2_usb *ns2_usb = urb->context;
 	int i;
-	bool schedule = false;
 	unsigned long flags;
 
 	switch (urb->status) {
 	case 0:
-		schedule = true;
 		break;
 	case -ECONNRESET:
 	case -ENOENT:
@@ -71,16 +69,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 (urb->status == 0 && !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 +111,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)
@@ -295,6 +288,9 @@ static void switch2_usb_message_in_work(struct work_struct *work)
 
 	spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
 	for (i = 0; i < NS2_IN_URBS; i++) {
+		if (ns2_usb->shutdown)
+			break;
+
 		urb = &ns2_usb->bulk_in[i];
 		if (urb->state != NS2_URB_IN)
 			continue;
@@ -311,7 +307,7 @@ static void switch2_usb_message_in_work(struct work_struct *work)
 		}
 
 		spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
-		if (!urb->urb)
+		if (ns2_usb->shutdown || !urb->urb)
 			/* We're shutting down and this URB was freed */
 			break;
 
-- 
2.54.0

[PATCH v2 3/3] HID: nintendo-switch2-usb: Clean up urb in probe error path

[Vicki Pfau]

In switch2_usb_probe the error path if the initial urb submission fails
forgets to unanchor the urb. This patch adds that in.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20261111032125.2888320-1-vi%40endrift.com?part=1
Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-nintendo-switch2-usb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hid/hid-nintendo-switch2-usb.c b/drivers/hid/hid-nintendo-switch2-usb.c
index f53cbf0a3e15..8524cc68d5a2 100644
--- a/drivers/hid/hid-nintendo-switch2-usb.c
+++ b/drivers/hid/hid-nintendo-switch2-usb.c
@@ -427,6 +427,8 @@ static int switch2_usb_probe(struct usb_interface *intf, const struct usb_device
 	ns2_usb->bulk_in[0].state = NS2_URB_OUT;
 	usb_anchor_urb(ns2_usb->bulk_in[0].urb, &ns2_usb->bulk_in_anchor);
 	ret = usb_submit_urb(ns2_usb->bulk_in[0].urb, GFP_ATOMIC);
+	if (ret < 0)
+		usb_unanchor_urb(ns2_usb->bulk_in[0].urb);
 	spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
 
 	if (ret < 0)
-- 
2.54.0

Re: [PATCH v2 3/3] HID: nintendo-switch2-usb: Clean up urb in probe error path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] HID: nintendo-switch2-usb: missing Fixes trailer for
  switch2_usb_probe() fix
--

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

> HID: nintendo-switch2-usb: Clean up urb in probe error path
>
> In switch2_usb_probe the error path if the initial urb submission fails
> forgets to unanchor the urb. This patch adds that in.
>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://sashiko.dev/#/patchset/20261111032125.2888320-1-vi%40endrift.com?part=1
> Signed-off-by: Vicki Pfau <vi@endrift.com>

[Severity: Low]
Since this fixes a probe error path bug, should it include a Fixes tag?

  Fixes: 9417132df63e ("HID: nintendo: Add preliminary Switch 2 controller driver")

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