On Tue, 2014-02-25 at 13:19 -0500, Alan Stern wrote:
> On Mon, 24 Feb 2014, Dan Williams wrote:
>
> > On Mon, 2014-02-24 at 19:01 -0800, Dan Williams wrote:
> > > As mentioned in the comments on patch 2, while ->peer is being modified
> > > we don't want usb_port_runtime_{suspend|resume} to run. Introduce
> > > pre_modify_peers() and post_modify_peers() to close that hole.
> >
> > ...thinking about it further, when we unlink the peers it might be the
> > final put on the device, so {pre|post}_modify_peers() need to take and
> > drop a device reference.
>
> This is getting ridiculous.
>
> Look, we guarantee that the peer relation is dropped when either of the
> devices is unregistered. Therefore there's no need to take a reference
> to the peer device.
Ugh, yes. Reference counts alone don't save us from this scenario:
CPU1 CPU2
mutex_lock(peer_lock)
unlink_peers(portA, portC)
mutex_unlock(peer_lock)
mutex_lock(peer_lock)
device_unregister(portA)
link_peers(portB, portA)
mutex_unlock(peer_lock)
> This will cause a problem only if something else goes wrong. That's
> why I suggested, in patch 2, that when a peering error is detected, you
> print out the erroneous pointer rather than assuming it points to a
> valid port structure.
...but even with the invalid port flag portA can theoretically still be
a stale pointer.
> This is also why I suggested that you close the race in
> usb_hub_remove_port_device: to make sure that the guarantee really is
> enforced.
You proposed a flag on the port_dev once it goes invalid. It seems to
me the flag to check if a port is valid is parent_hub->disconnected.
However, in the case above, I believe find_port_by_location() and
find_default_peer() need to prevent the "link_peers(portB, portA)"
event. When these routines walk down the tree they should be taking
usb_lock_device() to make sure they are not racing with disconnect
operations.
Locking while traversing the list and otherwise checking for
hub->disconnected closes the race. We can't solve the race at the
port_dev level alone. Agreed?
--
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
