Re: [PATCH v6 part1 4/8] usb: assign default peer ports for root hubs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 2014-03-03 at 17:21 -0500, Alan Stern wrote:
> On Fri, 28 Feb 2014, Dan Williams wrote:
> 
> > Assume that the peer of a superspeed port is the port with the same id
> > on the shared_hcd root hub.  This identification scheme is required of
> > external hubs by the USB3 spec [1].  However, for root hubs, tier mismatch
> > may be in effect [2].  Tier mismatch can only be enumerated via platform
> > firmware.  For now, simply perform the nominal association.
> > 
> > Once the root hub is marked as unregistered we block attempts to walk
> > the ->shared_hcd pointer to find a root-peer port.
> > 
> > [1]: usb 3.1 section 10.3.3
> > [2]: xhci 1.1 appendix D
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> 
> Okay, I think the locking still isn't right.  Below is an example patch
> showing how it ought to work.  Basically, all the locking can be done
> in hub.c; port.c doesn't have to worry about it at all.
> 
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index 2518c3250750..e380b8a80830 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -2778,9 +2778,17 @@ void usb_remove_hcd(struct usb_hcd *hcd)
> >  		hcd->state = HC_STATE_QUIESCING;
> >  
> >  	dev_dbg(hcd->self.controller, "roothub graceful disconnect\n");
> > +
> > +	/*
> > +	 * Flush + disable peering operations, and atomically update the
> > +	 * hcd state relative to other root hub state
> > +	 * changes/evaluations
> > +	 */
> > +	mutex_lock(&usb_port_peer_mutex);
> >  	spin_lock_irq (&hcd_root_hub_lock);
> >  	hcd->rh_registered = 0;
> >  	spin_unlock_irq (&hcd_root_hub_lock);
> > +	mutex_unlock(&usb_port_peer_mutex);
> 
> Not needed.  We don't care whether the root hub device is registered; 
> we only care whether it qualifies as a hub according to the tests in 
> usb_hub_to_struct_hub.

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)
                                  peer_hdev = hdevA
  usb_put_dev(hdevA) // free
                                  hubA = usb_hub_to_struct_hub(hdevA) // use after free


Am I missing something that mitigates this?  I still think we need to
check against ->rh_registered.

Otherwise changes look good and I have folded this into the patch.

--
Dan


--
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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux