[PATCH 2/2] USB: core: Fix deadlock in port "disable" sysfs attribute

[PATCH 2/2] USB: core: Fix deadlock in port "disable" sysfs attribute

[Alan Stern]

The show and store callback routines for the "disable" sysfs attribute
file in port.c acquire the device lock for the port's parent hub
device.  This can cause problems if another process has locked the hub
to remove it or change its configuration:

	Removing the hub or changing its configuration requires the
	hub interface to be removed, which requires the port device
	to be removed, and device_del() waits until all outstanding
	sysfs attribute callbacks for the ports have returned.  The
	lock can't be released until then.

	But the disable_show() or disable_store() routine can't return
	until after it has acquired the lock.

The resulting deadlock can be avoided by calling
sysfs_break_active_protection().  This will cause the sysfs core not
to wait for the attribute's callback routine to return, allowing the
removal to proceed.  The disadvantage is that after making this call,
there is no guarantee that the hub structure won't be deallocated at
any moment.  To prevent this, we have to acquire a reference to it
first by calling hub_get().

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Cc: <stable@vger.kernel.org>	# Needs the previous patch in this series

---

 drivers/usb/core/port.c |   38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

Index: usb-devel/drivers/usb/core/port.c
===================================================================
--- usb-devel.orig/drivers/usb/core/port.c
+++ usb-devel/drivers/usb/core/port.c
@@ -55,11 +55,22 @@ static ssize_t disable_show(struct devic
 	u16 portstatus, unused;
 	bool disabled;
 	int rc;
+	struct kernfs_node *kn;
 
+	hub_get(hub);
 	rc = usb_autopm_get_interface(intf);
 	if (rc < 0)
-		return rc;
+		goto out_hub_get;
 
+	/*
+	 * Prevent deadlock if another process is concurrently
+	 * trying to unregister hdev.
+	 */
+	kn = sysfs_break_active_protection(&dev->kobj, &attr->attr);
+	if (!kn) {
+		rc = -ENODEV;
+		goto out_autopm;
+	}
 	usb_lock_device(hdev);
 	if (hub->disconnected) {
 		rc = -ENODEV;
@@ -69,9 +80,13 @@ static ssize_t disable_show(struct devic
 	usb_hub_port_status(hub, port1, &portstatus, &unused);
 	disabled = !usb_port_is_power_on(hub, portstatus);
 
-out_hdev_lock:
+ out_hdev_lock:
 	usb_unlock_device(hdev);
+	sysfs_unbreak_active_protection(kn);
+ out_autopm:
 	usb_autopm_put_interface(intf);
+ out_hub_get:
+	hub_put(hub);
 
 	if (rc)
 		return rc;
@@ -89,15 +104,26 @@ static ssize_t disable_store(struct devi
 	int port1 = port_dev->portnum;
 	bool disabled;
 	int rc;
+	struct kernfs_node *kn;
 
 	rc = kstrtobool(buf, &disabled);
 	if (rc)
 		return rc;
 
+	hub_get(hub);
 	rc = usb_autopm_get_interface(intf);
 	if (rc < 0)
-		return rc;
+		goto out_hub_get;
 
+	/*
+	 * Prevent deadlock if another process is concurrently
+	 * trying to unregister hdev.
+	 */
+	kn = sysfs_break_active_protection(&dev->kobj, &attr->attr);
+	if (!kn) {
+		rc = -ENODEV;
+		goto out_autopm;
+	}
 	usb_lock_device(hdev);
 	if (hub->disconnected) {
 		rc = -ENODEV;
@@ -118,9 +144,13 @@ static ssize_t disable_store(struct devi
 	if (!rc)
 		rc = count;
 
-out_hdev_lock:
+ out_hdev_lock:
 	usb_unlock_device(hdev);
+	sysfs_unbreak_active_protection(kn);
+ out_autopm:
 	usb_autopm_put_interface(intf);
+ out_hub_get:
+	hub_put(hub);
 
 	return rc;
 }

Re: [PATCH 2/2] USB: core: Fix deadlock in port "disable" sysfs attribute

[Greg KH]

On Fri, Mar 15, 2024 at 01:06:33PM -0400, Alan Stern wrote:
> The show and store callback routines for the "disable" sysfs attribute
> file in port.c acquire the device lock for the port's parent hub
> device.  This can cause problems if another process has locked the hub
> to remove it or change its configuration:
> 
> 	Removing the hub or changing its configuration requires the
> 	hub interface to be removed, which requires the port device
> 	to be removed, and device_del() waits until all outstanding
> 	sysfs attribute callbacks for the ports have returned.  The
> 	lock can't be released until then.
> 
> 	But the disable_show() or disable_store() routine can't return
> 	until after it has acquired the lock.
> 
> The resulting deadlock can be avoided by calling
> sysfs_break_active_protection().  This will cause the sysfs core not
> to wait for the attribute's callback routine to return, allowing the
> removal to proceed.  The disadvantage is that after making this call,
> there is no guarantee that the hub structure won't be deallocated at
> any moment.  To prevent this, we have to acquire a reference to it
> first by calling hub_get().
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Cc: <stable@vger.kernel.org>	# Needs the previous patch in this series

What "previous patch"?  I don't see this as a series even on
lore.kernel.org.

confused,

greg k-h

Re: [PATCH 2/2] USB: core: Fix deadlock in port "disable" sysfs attribute

[Greg KH]

On Tue, Mar 26, 2024 at 10:22:32AM +0100, Greg KH wrote:
> On Fri, Mar 15, 2024 at 01:06:33PM -0400, Alan Stern wrote:
> > The show and store callback routines for the "disable" sysfs attribute
> > file in port.c acquire the device lock for the port's parent hub
> > device.  This can cause problems if another process has locked the hub
> > to remove it or change its configuration:
> > 
> > 	Removing the hub or changing its configuration requires the
> > 	hub interface to be removed, which requires the port device
> > 	to be removed, and device_del() waits until all outstanding
> > 	sysfs attribute callbacks for the ports have returned.  The
> > 	lock can't be released until then.
> > 
> > 	But the disable_show() or disable_store() routine can't return
> > 	until after it has acquired the lock.
> > 
> > The resulting deadlock can be avoided by calling
> > sysfs_break_active_protection().  This will cause the sysfs core not
> > to wait for the attribute's callback routine to return, allowing the
> > removal to proceed.  The disadvantage is that after making this call,
> > there is no guarantee that the hub structure won't be deallocated at
> > any moment.  To prevent this, we have to acquire a reference to it
> > first by calling hub_get().
> > 
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > Cc: <stable@vger.kernel.org>	# Needs the previous patch in this series
> 
> What "previous patch"?  I don't see this as a series even on
> lore.kernel.org.

Ah, found it, you sent it only to me for some reason, and not the lists.

thanks,

greg k-h

Re: [PATCH 2/2] USB: core: Fix deadlock in port "disable" sysfs attribute

[Alan Stern]

On Tue, Mar 26, 2024 at 10:23:25AM +0100, Greg KH wrote:
> On Tue, Mar 26, 2024 at 10:22:32AM +0100, Greg KH wrote:
> > On Fri, Mar 15, 2024 at 01:06:33PM -0400, Alan Stern wrote:
> > > The show and store callback routines for the "disable" sysfs attribute
> > > file in port.c acquire the device lock for the port's parent hub
> > > device.  This can cause problems if another process has locked the hub
> > > to remove it or change its configuration:
> > > 
> > > 	Removing the hub or changing its configuration requires the
> > > 	hub interface to be removed, which requires the port device
> > > 	to be removed, and device_del() waits until all outstanding
> > > 	sysfs attribute callbacks for the ports have returned.  The
> > > 	lock can't be released until then.
> > > 
> > > 	But the disable_show() or disable_store() routine can't return
> > > 	until after it has acquired the lock.
> > > 
> > > The resulting deadlock can be avoided by calling
> > > sysfs_break_active_protection().  This will cause the sysfs core not
> > > to wait for the attribute's callback routine to return, allowing the
> > > removal to proceed.  The disadvantage is that after making this call,
> > > there is no guarantee that the hub structure won't be deallocated at
> > > any moment.  To prevent this, we have to acquire a reference to it
> > > first by calling hub_get().
> > > 
> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > > Cc: <stable@vger.kernel.org>	# Needs the previous patch in this series
> > 
> > What "previous patch"?  I don't see this as a series even on
> > lore.kernel.org.
> 
> Ah, found it, you sent it only to me for some reason, and not the lists.

Oops.  My apologies... maybe I'm getting rusty at this.  That was 
definitely a mistake, not intentional.

Alan Stern