linux-kernel - Re: [Patch v2] usb: core: stop USB enumeration if too many retries

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]

Message-ID: <YxYTRSSeNqooy7lz@rowland.harvard.edu>
Date:   Mon, 5 Sep 2022 11:18:29 -0400
From:   Alan Stern <stern@...land.harvard.edu>
To:     Ray Chi <raychi@...gle.com>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        mathias.nyman@...ux.intel.com,
        Albert Wang <albertccwang@...gle.com>,
        Badhri Jagan Sridharan <badhri@...gle.com>,
        Puma Hsu <pumahsu@...gle.com>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [Patch v2] usb: core: stop USB enumeration if too many retries

On Mon, Sep 05, 2022 at 04:36:16PM +0800, Ray Chi wrote:
> On Sat, Sep 3, 2022 at 1:07 AM Alan Stern <stern@...land.harvard.edu> wrote:
> > I don't understand.  If you don't know whether the accessory is broken,
> > how do you know whether to set the quirk?
> >
> > On the other hand, if you always set the quirk even before you know
> > whether the accessory is broken, why make it a quirk at all?  Why not
> > make it the normal behavior of the driver?
> >
> 
> Since our device has a watchdog mechanism, when the device connects to
> a broken accessory, the kernel panic will happen. This problem didn't happen
> in all USB Hosts, so I want to use the quirk to fix this problem for those hosts
> with a watchdog mechanism.

Okay.  So this shouldn't be a quirk; it should apply all the time to any 
hub where the host controller has this watchdog mechanism.

> > Why not set CONFIG_USB_FEW_INIT_RETRIES instead?
> >
> 
> https://source.android.com/docs/core/architecture/kernel/android-common
> According to Android Common Kernel, I can't only add this config to one project.
> In addition, it can't stop enumeration so that the timeout problem
> still happens.

This is the first time you have mentioned either the watchdog mechanism 
or the fact that this is intended for Android.  It would have been a lot 
better if both of these facts were included in the initial patch 
description.  You can't expect people to evaluate a new patch properly 
if they don't have a clear picture of what it was meant for.

> > might describe in detail a situation where the quirk is needed,
> > explaining what sort of behavior of the system would lead you to set the
> > quirk, and why.
> >
> 
> There is a kernel panic when the device connects to the broken accessory.
> I tried to modify the initial_descriptor_timeout. When the accessory is not
> working, the total time is 6.5s (get descriptor retry) + 5*2 seconds
> (set address of xhci timeout).
> The time is so long to cause kernel panic for the device. This is why I want to
> stop enumeration instead reducing the retries or timeout.

It sounds like what you need is a "quick initialization" option that 
will limit the timeout lengths and the numbers of retries, and will 
cause the system to ignore connections on a port once an initialization 
has failed.  There should also be a way to make the system stop ignoring 
a port, perhaps by writing to a sysfs file.

In addition, there should be an automatic algorithm to determine which 
hub ports this option will apply to.  I don't think you want it to be 
based on a quirk, because you shouldn't need to wait for a kernel panic 
before realizing that the quirk is needed -- that's why the algorithm 
has to be automatic.

Can you write a new patch that works more like this?

Alan Stern