On Tue, Apr 16, 2024 at 05:05:55PM +0800, Sam Sun wrote:
> On Mon, Apr 15, 2024 at 10:47 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > Actually, I've got a completely different patch which I think will fix
> > the problem you encountered. Instead of using mutual exclusion to
> > avoid the race, it prevents the two routines from being called at the
> > same time so the race can't occur in the first place. It also should
> > guarantee the usb_hub_to_struct_hub() doesn't return NULL when
> > disable_store() calls it.
> >
> > Can you try the patch below, instead of (not along with) the first
> > patch? Thanks.
> >
> > Alan Stern
> I tried this patch and it worked. I agree this patch is better and it
> avoids introducing new locks.
It turns out that patch is no good. The reason is mentioned in the
changelog for commit 543d7784b07f ("USB: fix race between hub_disconnect
and recursively_mark_NOTATTACHED"); it says that the port devices have to
be removed _after_ maxchild has been set to 0.
So okay, here's a third attempt to fix the problem. This doesn't try to
avoid the race or anything like that. Instead it just adds checks for
usb_hub_to_struct_hub() returning NULL. That should be enough to prevent
the invalid pointer dereference you encountered.
This should be tested by itself, without either of the first two patches.
Alan Stern
Index: usb-devel/drivers/usb/core/port.c
===================================================================
--- usb-devel.orig/drivers/usb/core/port.c
+++ usb-devel/drivers/usb/core/port.c
@@ -51,13 +51,15 @@ static ssize_t disable_show(struct devic
struct usb_port *port_dev = to_usb_port(dev);
struct usb_device *hdev = to_usb_device(dev->parent->parent);
struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
- struct usb_interface *intf = to_usb_interface(hub->intfdev);
+ struct usb_interface *intf = to_usb_interface(dev->parent);
int port1 = port_dev->portnum;
u16 portstatus, unused;
bool disabled;
int rc;
struct kernfs_node *kn;
+ if (!hub)
+ return -ENODEV;
hub_get(hub);
rc = usb_autopm_get_interface(intf);
if (rc < 0)
@@ -101,12 +103,14 @@ static ssize_t disable_store(struct devi
struct usb_port *port_dev = to_usb_port(dev);
struct usb_device *hdev = to_usb_device(dev->parent->parent);
struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
- struct usb_interface *intf = to_usb_interface(hub->intfdev);
+ struct usb_interface *intf = to_usb_interface(dev->parent);
int port1 = port_dev->portnum;
bool disabled;
int rc;
struct kernfs_node *kn;
+ if (!hub)
+ return -ENODEV;
rc = kstrtobool(buf, &disabled);
if (rc)
return rc;
