On Mon, 3 Mar 2014, Dan Williams wrote:
> > In general I agree, and I like the compartmentalization of only needing
> > to take the lock in hub.c. But I still think we have a hole with a
> > scenario like the following (granted, this should never happen in
> > current code...):
> >
> > CPU1 CPU2
> > usb_remove_hcd(hcdA)
> > ...
> > mutex_lock(peer_lock)
> > hdevA->maxchild = 0;
> > mutex_unlock(peer_lock)
> > usb_add_hcd(hcdB)
> > ...
> > mutex_lock(peer_lock)
> > hdevA = hcdA->self.root_hub
> > usb_put_dev(hdevA) // free
> > hubA = usb_hub_to_struct_hub(hdevA) // use after free
>
> ...and if this is a hole then so is the hcdA->self.root_hub. It makes
> sense that when the host controller breaks the peering relationship at
> the root it should hold the lock and clear ->shared_hcd and
> ->primary_hcd.
Yes, you jogged my thinking. In addition to this hole, there is
another section of the patch I completely forgot about. The following
needs to be added on to what I posted before.
Alan Stern
Index: usb-3.14/drivers/usb/core/hub.c
===================================================================
--- usb-3.14.orig/drivers/usb/core/hub.c
+++ usb-3.14/drivers/usb/core/hub.c
@@ -4564,6 +4564,8 @@ static void hub_port_connect_change(stru
*/
status = 0;
+ mutex_lock(&usb_port_peer_mutex);
+
/* We mustn't add new devices if the parent hub has
* been disconnected; we would race with the
* recursively_mark_NOTATTACHED() routine.
@@ -4574,14 +4576,17 @@ static void hub_port_connect_change(stru
else
hub->ports[port1 - 1]->child = udev;
spin_unlock_irq(&device_state_lock);
+ mutex_unlock(&usb_port_peer_mutex);
/* Run it through the hoops (find a driver, etc) */
if (!status) {
status = usb_new_device(udev);
if (status) {
+ mutex_lock(&usb_port_peer_mutex);
spin_lock_irq(&device_state_lock);
hub->ports[port1 - 1]->child = NULL;
spin_unlock_irq(&device_state_lock);
+ mutex_unlock(&usb_port_peer_mutex);
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
