| Message ID | 20231020083125.1.I3e5f7abcbf6f08d392e31a5826b7f234df662276@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: core: Prevent infinite loops when usb_reset_device() unbinds/binds | expand |
On Fri, Oct 20, 2023 at 08:31:38AM -0700, Douglas Anderson wrote: > 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. How about just adding appropriate pre_reset() and post_reset() methods? This is precisely what they are meant for. Then the the unbind/rebind loop wouldn't ever get started. Alan Stern
Hi, On Fri, Oct 20, 2023 at 8:46 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Fri, Oct 20, 2023 at 08:31:38AM -0700, Douglas Anderson wrote: > > 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. > > How about just adding appropriate pre_reset() and post_reset() methods? > This is precisely what they are meant for. Then the the unbind/rebind > loop wouldn't ever get started. Right, and we have pre_reset() and post_reset() in the r1852 driver. The issue is that we are seeing occasional control transfer errors while the r8152 driver is still running its probe() routine and we want to reset in response to those. It is relatively difficult to have the pre_reset() and post_reset() methods work properly if failures happen and probe() hasn't finished yet. The current proposal I have for the r8152 driver is to have the pre_reset() routine return -EIO if we saw errors during probe, which tells the USB core to unbind/rebind us. This gets us to a known good state. If we need to do a reset later on (after probe finished successfully) then pre_reset() and post_reset() can avoid the unbind/bind. The worry was that this could cause an infinite loop. Hence this patch. ;-) -Doug
On Fri, Oct 20, 2023 at 08:59:42AM -0700, Doug Anderson wrote: > Hi, > > On Fri, Oct 20, 2023 at 8:46 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Fri, Oct 20, 2023 at 08:31:38AM -0700, Douglas Anderson wrote: > > > 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. > > > > How about just adding appropriate pre_reset() and post_reset() methods? > > This is precisely what they are meant for. Then the the unbind/rebind > > loop wouldn't ever get started. > > Right, and we have pre_reset() and post_reset() in the r1852 driver. > The issue is that we are seeing occasional control transfer errors > while the r8152 driver is still running its probe() routine and we > want to reset in response to those. It is relatively difficult to have > the pre_reset() and post_reset() methods work properly if failures > happen and probe() hasn't finished yet. Why is that? > The current proposal I have > for the r8152 driver is to have the pre_reset() routine return -EIO if > we saw errors during probe, which tells the USB core to unbind/rebind > us. This gets us to a known good state. Don't you also get to a known good state if pre_reset() and post_reset() both return 0? Then there's no unbinding, so the driver can just jump back to the start of its probe() routine. Or fail the probe, if there have been too many errors. > If we need to do a reset later > on (after probe finished successfully) then pre_reset() and > post_reset() can avoid the unbind/bind. > > The worry was that this could cause an infinite loop. Hence this patch. ;-) With no unbinding/rebinding, any loops that occur will be entirely under the driver's control. Then it should easily be able to avoid looping forever. Alan Stern
Hi, On Fri, Oct 20, 2023 at 9:23 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Fri, Oct 20, 2023 at 08:59:42AM -0700, Doug Anderson wrote: > > Hi, > > > > On Fri, Oct 20, 2023 at 8:46 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > On Fri, Oct 20, 2023 at 08:31:38AM -0700, Douglas Anderson wrote: > > > > 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. > > > > > > How about just adding appropriate pre_reset() and post_reset() methods? > > > This is precisely what they are meant for. Then the the unbind/rebind > > > loop wouldn't ever get started. > > > > Right, and we have pre_reset() and post_reset() in the r1852 driver. > > The issue is that we are seeing occasional control transfer errors > > while the r8152 driver is still running its probe() routine and we > > want to reset in response to those. It is relatively difficult to have > > the pre_reset() and post_reset() methods work properly if failures > > happen and probe() hasn't finished yet. > > Why is that? > > > The current proposal I have > > for the r8152 driver is to have the pre_reset() routine return -EIO if > > we saw errors during probe, which tells the USB core to unbind/rebind > > us. This gets us to a known good state. > > Don't you also get to a known good state if pre_reset() and post_reset() > both return 0? Then there's no unbinding, so the driver can just jump > back to the start of its probe() routine. Or fail the probe, if there > have been too many errors. > > > If we need to do a reset later > > on (after probe finished successfully) then pre_reset() and > > post_reset() can avoid the unbind/bind. > > > > The worry was that this could cause an infinite loop. Hence this patch. ;-) > > With no unbinding/rebinding, any loops that occur will be entirely under > the driver's control. Then it should easily be able to avoid looping > forever. Hmmm, OK. Let's see. I guess the most annoying thing would be dealing with anything "devm". I guess in the case of the r8152 driver, though, there is no use of devm. Thus I should be able to do this. Let me give it a shot and see how it looks. -Doug
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)
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@chromium.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(-)