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] [day] [month] [year] [list]

Message-ID: <CAPBYUsD5peoF0WNQnjS3xc6R_tCXH3685AKdf3MbUhk8Tkxf_g@mail.gmail.com>
Date:   Thu, 8 Sep 2022 19:11:01 +0800
From:   Ray Chi <raychi@...gle.com>
To:     Alan Stern <stern@...land.harvard.edu>
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 5, 2022 at 11:18 PM Alan Stern <stern@...land.harvard.edu> wrote:
>
> 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.
>

I will remove the quirk and use the sysfs file to do.

> 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?
>

I had two ideas to determine whether the port should apply the option or not.
One is a timeout and the other one is using a retry count. If using
the retry count,
I think it is close to current retry definitions. Maybe we can modify
the design.
If I use the timeout, I need more time to think of a better way to do it for the
synchronization problem. Currently, they are rough ideas. I will
upload the commit
if I have a better solution to determine the behavior automatically.

I will upload a v3 patch using sysfs file to fix the current problem.

> Alan Stern

Thanks,
Ray