linux-kernel - Re: [RFC] usb: host: xhci: Remove the watchdog timer and use command timer to watch stop endpoint command

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:   Thu, 1 Dec 2016 12:54:58 +0800
From:   Baolin Wang <baolin.wang@...aro.org>
To:     Mathias Nyman <mathias.nyman@...ux.intel.com>
Cc:     mathias.nyman@...el.com, Greg KH <gregkh@...uxfoundation.org>,
        USB <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Mark Brown <broonie@...nel.org>
Subject: Re: [RFC] usb: host: xhci: Remove the watchdog timer and use command
 timer to watch stop endpoint command

On 30 November 2016 at 22:09, Mathias Nyman
<mathias.nyman@...ux.intel.com> wrote:
> On 30.11.2016 11:02, Baolin Wang wrote:
>>
>> If the hardware never responds to the stop endpoint command, the
>> URBs will never be completed, and we might hang the USB subsystem.
>> The original watchdog timer is used to watch if one stop endpoint
>> command is timeout, if timeout, then the watchdog timer will set
>> XHCI_STATE_DYING, try to halt the xHCI host, and give back all
>> pending URBs.
>>
>> But now we already have one command timer to control command timeout,
>> thus we can also use the command timer to watch the stop endpoint
>> command, instead of one duplicate watchdog timer which need to be
>> removed.
>>
>> Meanwhile we don't need the 'stop_cmds_pending' flag to identy if
>> this is the last stop endpoint command of one endpoint. Since we
>> can make sure we only set one stop endpoint command for one endpoint
>> by 'EP_HALT_PENDING' flag in xhci_urb_dequeue() function. Thus remove
>> this flag.
>>
>> We also need to clean up the command queue before trying to halt the
>> xHCI host in xhci_stop_endpoint_command_timeout() function.
>
>
> This isn't a bad idea.
>
> There are anyway some corner cases and details that need to be
> checked, such as suspend (which will clear the command queue), module unload
> and abrupt host removal (like pci hotplug removal of host controller)
> we need to make sure we can trust the command timer to always return the
> canceled URB

Yes, you are right, we need to check these carefully.

Suspend process, module unload and abrupt host removal, they all will
issue usb_disconnect() firstly before clear the command queue, it will
check URBs for every endpoint by
usb_disconnect()--->usb_disable_device()--->usb_disable_endpoint(),
which will make sure every URBs of endpoints will be cancelled by the
stop endpoint command responding or the timeout function of stop
endpoint command (xhci_stop_endpoint_command_timeout()) in
usb_hcd_flush_endpoint(). From that point, we can make sure the
command timer will be useful to handle stop endpoint command timeout.
Please correct me if I said something wrong. Thanks.

-- 
Baolin.wang
Best Regards