Re: [PATCH] Prevent race condition between USB authorisation and USB discovery events

[Andrew Worsley]

On Tue, 18 Dec 2018 at 03:21, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, 17 Dec 2018, Andrew Worsley wrote:
>
> > A sysfs driven USB authorisation change can trigger a usb_set_configuration
> > while a hub_event worker thread is running. This can result in a USB device
> > being disabled just after it was configured and bringing down all the
> > devices and impacting hardware and user processes that were established on
> > top of this these interfaces. In some cases the USB disable never completed
> > and the whole system hung.
>
> Can you be more specific about this?  Disabling a USB device shouldn't
> cause these kinds of problems, regardless of whether or not the device
> was just configured.

I can't say too much about the hardware - but there was an SPI bus and
an i2c bus built
on top of USB devices on a bus. It appears the SPI bus shutdown was
the item that was prone
to problems when being torn down.

I understand it would be better if the dependant systems shutdown
cleanly, which it
did about 75% of the time but then it was rather messy sequence. So
the current system
is far from ideal. The crux of the issue is the usb_disable_device()
(line 1874 of drivers/usb/core/message.c)
call in usb_set_configuration() is applied to a configured device and
triggers the
tear down of all the devices built on that USB device and is very disruptive.

> > At my work I had an occasional hang due to this race condition. Roughly 1
> > in 50 boots had the race occurrence and 1 in 4 of those resulted in a hang.
> > This patch fixed the problem and I had no problems (spurious disables
> > or hangs) in 750+ boots.
>
> usb_authorize_device, usb_deauthorize_device, and hub_event all acquire
> the device mutex.  Why should adding another mutex make any difference?

I believe the new usb_authorize_mutex prevents the cascade of USB
device bus scans,
discoveries and probes from colliding with those caused a change in
USB bus authorisation.
The individual device / hub locks are not across the whole system,
only an individual
component hub/device so any logic that gives an orderly USB device
scanning and probing is
undermined. The idea of the mutex is to keep the USB device discovery
work when a device is
added/removed/powered up is separated from those that are caused by
authorisation changes.

I don't pretend to understand all the logic and sequencing of the
current code which works works
faultlessly when these threads are serialised by the mutex (750+
times) in boot ups which
build up the system and shutdowns which bring down the system.
Likewise if I endlessly run
the authorisation off / on by itself it is faultless over hundreds of
iterations.

> In fact there's an actual disadvantage: Making hub_event acquire a
> global mutex will prevent us from handling multiple hubs concurrently.
> Although we don't do this now, we might want to in the future.
>
> Alan Stern

This is true - I am not trying to serialise the hub_event() threads -
but to prevent the authorisation changes
from happening during the hub_event threads. So perhaps there would be
a way of allowing multiple
concurrent hub_events from happening but only one authorisation thread
at a time. Assuming multiple
hub_event()s are deemed ok. Something like a lock which allows
multiple readers but only a single exclusive
writer lock?

Andrew