On Wed, Apr 17, 2024 at 12:35 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > 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. > I checked the commit you mentioned. Maybe your first fix is all we need to fix the problem? At least no race would occur for hdev->maxchild and usb_set_intfdata(). > 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; > I applied this patch and it also can fix the warning. I am not sure which one is better. Best, Yue