Message-ID: <20231020083125.1.I3e5f7abcbf6f08d392e31a5826b7f234df662276@changeid>
Date:   Fri, 20 Oct 2023 08:31:38 -0700
From:   Douglas Anderson <dianders@...omium.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Alan Stern <stern@...land.harvard.edu>,
        linux-usb@...r.kernel.org
Cc:     Grant Grundler <grundler@...omium.org>,
        Hayes Wang <hayeswang@...ltek.com>,
        Douglas Anderson <dianders@...omium.org>,
        Bastien Nocera <hadess@...ess.net>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Flavio Suligoi <f.suligoi@...m.it>,
        Hans de Goede <hdegoede@...hat.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Ivan Orlov <ivan.orlov0322@...il.com>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Ray Chi <raychi@...gle.com>,
        Ricardo Cañuelo <ricardo.canuelo@...labora.com>,
        Rob Herring <robh@...nel.org>, Roy Luo <royluo@...gle.com>,
        Stanley Chang <stanley_chang@...ltek.com>,
        Vincent Mailhol <mailhol.vincent@...adoo.fr>,
        linux-kernel@...r.kernel.org
Subject: [PATCH] usb: core: Prevent infinite loops when usb_reset_device() unbinds/binds

When we call usb_reset_device() and a driver doesn't implement
pre_reset() and post_reset() methods then the USB core will attempt to
unbind and rebind the driver in order to make reset work. This is a
great general solution, but it has the potential to loop forever.
Specifically, if the USB device is in a state that the USB device
driver issues another usb_reset_device() after each rebind then we'll
just continually unbind and rebind with no end.

It's difficult to address this condition in a USB device driver
because it's hard for the driver to keep state across each
unbind/bind. Various tricks could be done by keeping static globals,
but these are difficult to make reliable if there are multiple USB
devices using the same driver at the same time.

Let's solve this problem in the USB core. If we notice that we're
doing an unbind/bind for usb_reset_device() several times in a short
period of time, we'll eventually give up. For now, we'll allow 3
resets in a short period of time (and continue to allow an unbounded
number if they are spaced out). We'll say that any unbind/rebind reset
happened within 5 seconds of the previous one that it counts.

This patch is written in response to review comments for a patch to
the r8152 driver [1]. The problem it is solving is not actually seen
in practice but it seems plausible that it could happen.

[1] https://lore.kernel.org/r/20231012122458.v3.5.Ib2affdbfdc2527aaeef9b46d4f23f7c04147faeb@changeid

Signed-off-by: Douglas Anderson <dianders@...omium.org>
---

 drivers/usb/core/driver.c  | 38 ++++++++++++++++++++++++++++++++------
 drivers/usb/core/hub.c     |  2 +-
 drivers/usb/core/message.c |  2 +-
 drivers/usb/core/usb.h     |  2 +-
 include/linux/usb.h        |  7 +++++++
 5 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index f58a0299fb3b..5849cc21eb3f 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1104,21 +1104,47 @@ void usb_deregister(struct usb_driver *driver)
 }
 EXPORT_SYMBOL_GPL(usb_deregister);
 
+#define CONSECUTIVE_RESET_JIFFIES	(HZ * 5)
+#define MAX_CONSECUTIVE_RESETS		3
+
 /* Forced unbinding of a USB interface driver, either because
  * it doesn't support pre_reset/post_reset/reset_resume or
  * because it doesn't support suspend/resume.
  *
  * The caller must hold @intf's device's lock, but not @intf's lock.
  */
-void usb_forced_unbind_intf(struct usb_interface *intf)
+void usb_forced_unbind_intf(struct usb_interface *intf, bool for_reset)
 {
 	struct usb_driver *driver = to_usb_driver(intf->dev.driver);
 
 	dev_dbg(&intf->dev, "forced unbind\n");
 	usb_driver_release_interface(driver, intf);
 
-	/* Mark the interface for later rebinding */
-	intf->needs_binding = 1;
+	/*
+	 * If we're doing an unbind/rebind for a device that doesn't support
+	 * reset then make an attempt to avoid looking the reset over and over.
+	 *
+	 * NOTE: with jiffies wraparound it is nominally possible that we could
+	 * falsely detect that two resets are consecutive if we get a reset at
+	 * just the right time. Given that 1000 HZ w/ 32-bit jiffies wraps in
+	 * ~50 days, this is highly unlikely. In any case, it should be nearly
+	 * impossible to hit this many times in a row.
+	 */
+	if (for_reset && time_in_range(jiffies, jiffies,
+				       intf->last_reset_rebind_jiffies +
+				       CONSECUTIVE_RESET_JIFFIES))
+		intf->consecutive_reset_rebind_count++;
+	else
+		intf->consecutive_reset_rebind_count = 0;
+
+	if (for_reset)
+		intf->last_reset_rebind_jiffies = jiffies;
+
+	if (intf->consecutive_reset_rebind_count >= MAX_CONSECUTIVE_RESETS)
+		dev_warn(&intf->dev, "Too many resets in a row; giving up\n");
+	else
+		/* Mark the interface for later rebinding */
+		intf->needs_binding = 1;
 }
 
 /*
@@ -1138,7 +1164,7 @@ static void unbind_marked_interfaces(struct usb_device *udev)
 		for (i = 0; i < config->desc.bNumInterfaces; ++i) {
 			intf = config->interface[i];
 			if (intf->dev.driver && intf->needs_binding)
-				usb_forced_unbind_intf(intf);
+				usb_forced_unbind_intf(intf, false);
 		}
 	}
 }
@@ -1157,7 +1183,7 @@ static void usb_rebind_intf(struct usb_interface *intf)
 
 	/* Delayed unbind of an existing driver */
 	if (intf->dev.driver)
-		usb_forced_unbind_intf(intf);
+		usb_forced_unbind_intf(intf, false);
 
 	/* Try to rebind the interface */
 	if (!intf->dev.power.is_prepared) {
@@ -1226,7 +1252,7 @@ static void unbind_no_pm_drivers_interfaces(struct usb_device *udev)
 			if (intf->dev.driver) {
 				drv = to_usb_driver(intf->dev.driver);
 				if (!drv->suspend || !drv->resume)
-					usb_forced_unbind_intf(intf);
+					usb_forced_unbind_intf(intf, false);
 			}
 		}
 	}
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 0ff47eeffb49..54f60413940f 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -6229,7 +6229,7 @@ int usb_reset_device(struct usb_device *udev)
 						USB_INTERFACE_BOUND)
 					unbind = 1;
 				if (unbind)
-					usb_forced_unbind_intf(cintf);
+					usb_forced_unbind_intf(cintf, true);
 			}
 		}
 	}
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 077dfe48d01c..8e18912a6a6b 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1793,7 +1793,7 @@ void usb_deauthorize_interface(struct usb_interface *intf)
 		intf->authorized = 0;
 		device_unlock(dev);
 
-		usb_forced_unbind_intf(intf);
+		usb_forced_unbind_intf(intf, false);
 	}
 
 	device_unlock(dev->parent);
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 60363153fc3f..105bd6e2b69d 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -76,7 +76,7 @@ extern const struct usb_device_id *usb_device_match_id(struct usb_device *udev,
 				const struct usb_device_id *id);
 extern bool usb_driver_applicable(struct usb_device *udev,
 				  struct usb_device_driver *udrv);
-extern void usb_forced_unbind_intf(struct usb_interface *intf);
+extern void usb_forced_unbind_intf(struct usb_interface *intf, bool for_reset);
 extern void usb_unbind_and_rebind_marked_interfaces(struct usb_device *udev);
 
 extern void usb_hub_release_all_ports(struct usb_device *hdev,
diff --git a/include/linux/usb.h b/include/linux/usb.h
index a21074861f91..f554ef7d62e6 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -212,6 +212,10 @@ enum usb_wireless_status {
  * @reset_ws: Used for scheduling resets from atomic context.
  * @resetting_device: USB core reset the device, so use alt setting 0 as
  *	current; needs bandwidth alloc after reset.
+ * @last_reset_rebind_jiffies: The jiffies count last time we did an
+ *	unbind/rebind as part of reset.
+ * @consecutive_reset_rebind_count: The number of times in a row we've done a
+ *	rebind for reset soon after doing one.
  *
  * USB device drivers attach to interfaces on a physical device.  Each
  * interface encapsulates a single high level function, such as feeding
@@ -268,6 +272,9 @@ struct usb_interface {
 	struct device dev;		/* interface specific device info */
 	struct device *usb_dev;
 	struct work_struct reset_ws;	/* for resets in atomic context */
+
+	unsigned long last_reset_rebind_jiffies;
+	unsigned int consecutive_reset_rebind_count;
 };
 
 #define to_usb_interface(__dev)	container_of_const(__dev, struct usb_interface, dev)
-- 
2.42.0.758.gaed0368e0e-goog

Powered by blists - more mailing lists