On Mon, Aug 30, 2021 at 10:46:13AM -0400, Alan Stern wrote:

On Mon, Aug 30, 2021 at 09:56:50AM +0200, Johan Hovold wrote:

quoted

On Sat, Aug 28, 2021 at 09:58:25PM -0400, Alan Stern wrote:

quoted

This patch fixes the problem by converting the uninterruptible wait to
an interruptible one.  For the most part this won't affect calls to
usb_start_wait_urb(), because they are made by kernel threads and so
can't receive most signals.

But in some cases such calls may occur in user threads in contexts
other than usbfs ioctls.  A signal in these circumstances could cause
a USB transfer to fail when otherwise it wouldn't.  The outcome
wouldn't be too dreadful, since USB transfers can fail at any time and
the system is prepared to handle these failures gracefully.  In
theory, for example, a signal might cause a driver's probe routine to
fail; in practice if the user doesn't want a probe to fail then he
shouldn't send interrupt signals to the probing process.

While probe() triggered through sysfs or by module loading is one
example, the USB msg helpers are also called in a lot of other
user-thread contexts such as open() calls etc. It might even be that the
majority of these calls can be done from user threads (post
enumeration).

Could be.  It's not a well defined matter; it depends on what drivers 
are in use and how they are used.

Right, but the commit message seemed to suggest that these helpers being
run from interruptible threads was the exception.

Consider that a control message in a driver is likely to use the 
default USB_CTRL_[GS]ET_TIMEOUT value of 5 seconds.  Does it make sense 
to allow uninterruptible wait states to last as long as that?

Perhaps sometimes? I don't have a use case at hand, but I haven't
reviewed all drivers either.

The comment above usb_start_wait_urb() (which also needs to be updated,
by the way) even suggests that drivers should "implement their own
interruptible routines" so perhaps this has just gone unnoticed for 20
odd years. And the question then becomes, why didn't we use
interruptible sleep from the start?

And trying to answer that I find that that's precisely what we did, but
for some reason it was changed to uninterruptible sleep in v2.4.11
without a motivation (that I could easily find spelled out).

And to what extent does it matter if we make these delays 
interruptible?  A signal delivered during a system call will be fielded 
when the call returns if not earlier; the only difference will be that 
now some USB messages may be aborted.  For things like SIGINT or 
SIGTERM this seems reasonable.  (I'm not so sure about things like 
SIGALRM, SIGIO, or SIGSTOP, though.)

I'm not saying I'm necessarily against the change. It just seemed a bit
rushed to change the (stable) API like this while claiming it won't
affect most call sites.

quoted

quoted

Overall, then, making these delays interruptible seems to be an
acceptable risk.

Possibly, but changing the API like this to fix the usbfs ioctls seems
like using a bit of a too big hammer to me, especially when backporting
to stable.

Perhaps the stable backport could be delayed for a while (say, one 
release cycle).

That might work.

Do you have alternative suggestions?  I don't think we want special 
interruptible versions of usb_control_msg() and usb_bulk_msg() just for 
use by usbfs.

usbfs could carry a temporary local implementation as the documentation
for usb_start_wait_urb() currently suggests. I assume we can't limit the
usbfs timeouts.

Johan