linux-kernel - Re: [PATCH] usb: hub: add I/O error retry & reset routine

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 16 Nov 2018 11:21:57 -0500 (EST)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Nicolas Saenz Julienne <nsaenzjulienne@...e.de>
cc:     linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        <linux-usb@...r.kernel.org>
Subject: Re: [PATCH] usb: hub: add I/O error retry & reset routine

On Fri, 16 Nov 2018, Nicolas Saenz Julienne wrote:

> Hi Alan,
> thanks for the review.
> 
> On Thu, 2018-11-15 at 14:24 -0500, Alan Stern wrote:
> > On Thu, 15 Nov 2018, Nicolas Saenz Julienne wrote:
> > 
> > > An URB submission error in the HUB's endpoint completion function
> > > renders the whole HUB device unresponsive. This patch introduces a
> > > routine that retries the submission for 1s to then, as a last
> > resort,
> > > reset the whole device.
> > > 
> > > The implementation is based on usbhid/hid_core.c's, which
> > implements the
> > > same functionality. It was tested with the help of BCC's error
> > injection
> > > tool (inject.py).
> > > 
> > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@...e.de>
> > 
> > Why do you think this is needed?  Are you experiencing these 
> > sorts of URB submission errors?
> 
> Sorry, I should have been more explicit on where I come from. I've been
> playing around injecting atomic allocation errors on the USB stack. For
> example any URB submission marked with GFP_ATOMIC that ends up into
> xhci will allocate some memory with that flag.
> 
> Most subsystems, after facing a burst of memory allocation failures,
> seem to recover well (usbnet/hid/uas/alsa/serial). But I found out that
> it's not the case for USB hubs: In the event of detecting a new device
> the hub will complete an int URB which was previously sent to it. The
> event data is saved by the host and the URB is resubmitted for further
> event passing. In the case that URB submission failed, we lose all
> further events.

Ah, I see.

> It is indeed pretty hard to find this issue in the wild, since you have
> to time plugging or unplugging an USB device with the system running
> out of memory. But I don't think it's unrealistic to think it might
> happen.
> 
> As I comment in the patch description, I'm injecting the errors using
> BCC and eBPF's function override capabilities.  
> 
> > 
> > Why do you handle only errors during submission but not during
> > completion?  And if you keep on getting errors during submission, why
> > would resetting the hub make any difference?
> 
> Well, as far as I know, errors during completion are handled. The error
> is marked in hub->error, which later-on, in hub-event(), triggers a
> device reset. 
> 
> While implementing the solution I took into account the hub's
> completion error processing behavior and HID's implementation of the
> submission error handling (see hid_irq_in() in usbhid/hid-core.c). My
> rationale was that since both HID and hub are USB devices with a
> similar behavior there was no point in reinventing a mechanism. That
> said I have no spec data to back the "1s retry window to then reset the
> device".
> 
> One could argue that in the event of an error having a timer running
> forever is not the best design. It has to stop sometime. If that's the
> case, the HUB will be in a unknown state, i.e. a device might have
> disappeared. Resetting the hub will at least unbind all the USB devices
> attached to it and retry the enumeration. Regardless of the
> enumeration's success we'll at least be in a "safe" state.

Well, the timer will get deleted when the hub is unplugged.  If that
doesn't happen, we can assume that the hub has retained its state and
so a reset isn't necessary.

I would just keep retrying at, say, 1-second intervals.  Don't even 
bother with the exponential slowdown that the HID driver does.

> > The patch doesn't delete the io_retry timer when the hub is removed.
> 
> Right, that was silly of me...

You could even rename the timer to something like irq_urb_retry; I 
think that would be a more accurate description of what it does.

So fix those things up, simplify the retries, and expand the patch
description -- then resubmit.  :-)

Alan Stern