Message-ID: <aecd3780-1248-804d-2c74-550ccbaeb993@runbox.com>
Date:   Thu, 8 Oct 2020 05:37:02 -0400
From:   "M. Vefa Bicakci" <m.v.b@...box.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Bastien Nocera <hadess@...ess.net>,
        Valentina Manea <valentina.manea.m@...il.com>,
        Shuah Khan <shuah@...nel.org>,
        Alan Stern <stern@...land.harvard.edu>,
        syzkaller@...glegroups.com,
        Andrey Konovalov <andreyknvl@...gle.com>,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH 5.8 05/85] Revert "usbip: Implement a match function to
 fix usbip"

On 08/10/2020 05.25, Greg Kroah-Hartman wrote:
> On Thu, Oct 08, 2020 at 04:56:56AM -0400, M. Vefa Bicakci wrote:
>> On 07/10/2020 12.13, Greg Kroah-Hartman wrote:
>>> On Tue, Oct 06, 2020 at 04:26:27PM +0300, M. Vefa Bicakci wrote:
>>>> On 05/10/2020 18.26, Greg Kroah-Hartman wrote:
>>>>> From: M. Vefa Bicakci <m.v.b@...box.com>
>>>>>
>>>>> commit d6407613c1e2ef90213dee388aa16b6e1bd08cbc upstream.
>>>>>
>>>>> This commit reverts commit 7a2f2974f265 ("usbip: Implement a match
>>>>> function to fix usbip").
>>>>>
>>>>> In summary, commit d5643d2249b2 ("USB: Fix device driver race")
>>>>> inadvertently broke usbip functionality, which I resolved in an incorrect
>>>>> manner by introducing a match function to usbip, usbip_match(), that
>>>>> unconditionally returns true.
>>>>>
>>>>> However, the usbip_match function, as is, causes usbip to take over
>>>>> virtual devices used by syzkaller for USB fuzzing, which is a regression
>>>>> reported by Andrey Konovalov.
>>>>>
>>>>> Furthermore, in conjunction with the fix of another bug, handled by another
>>>>> patch titled "usbcore/driver: Fix specific driver selection" in this patch
>>>>> set, the usbip_match function causes unexpected USB subsystem behaviour
>>>>> when the usbip_host driver is loaded. The unexpected behaviour can be
>>>>> qualified as follows:
>>>>> - If commit 41160802ab8e ("USB: Simplify USB ID table match") is included
>>>>>      in the kernel, then all USB devices are bound to the usbip_host
>>>>>      driver, which appears to the user as if all USB devices were
>>>>>      disconnected.
>>>>> - If the same commit (41160802ab8e) is not in the kernel (as is the case
>>>>>      with v5.8.10) then all USB devices are re-probed and re-bound to their
>>>>>      original device drivers, which appears to the user as a disconnection
>>>>>      and re-connection of USB devices.
>>>>>
>>>>> Please note that this commit will make usbip non-operational again,
>>>>> until yet another patch in this patch set is merged, titled
>>>>> "usbcore/driver: Accommodate usbip".
>>>>>
>>>>> Cc: <stable@...r.kernel.org> # 5.8: 41160802ab8e: USB: Simplify USB ID table match
>>>>> Cc: <stable@...r.kernel.org> # 5.8
>>>>
>>>> Hello Greg,
>>>>
>>>> Sorry for the lateness of this e-mail.
>>>>
>>>> I had noted commit 41160802ab8e ("USB: Simplify USB ID table match") as a
>>>> prerequisite in the commit message, but I just realized that the commit
>>>> identifier refers to a commit in my local git tree, and not to the actual
>>>> commit in Linus Torvalds' git tree! I apologize for this mistake.
>>>>
>>>> Here is the correct commit identifier:
>>>>     0ed9498f9ecfde50c93f3f3dd64b4cd5414d9944 ("USB: Simplify USB ID table match")
>>>>
>>>> Perhaps this is why the prerequisite commit was not cherry-picked to the 5.8.y
>>>> branch.
>>>>
>>>> As a justification for the cherry-pick, commit 0ed9498f9ecf actually resolves
>>>> a bug. In summary, this commit works together with commit adb6e6ac20ee ("USB:
>>>> Also match device drivers using the ->match vfunc", which has been cherry-picked
>>>> as part of v5.8.6) and ensures that a USB driver's ->match function is also
>>>> called during the search for a more specialized/appropriate USB driver, in case
>>>> the driver in question does not have an id_table.
>>>>
>>>> If I am to be the devil's advocate, however, then given that there is only one
>>>> specialized USB device driver ("apple-mfi-fastcharge"), which conveniently has
>>>> an id_table, and also given that usbip no longer has a match function, I also
>>>> realize that it may not be crucial to cherry-pick 0ed9498f9ecf as a prerequisite
>>>> commit.
>>>
>>> I'm sorry, but I don't really understand.  Does 5.8.y need an additional
>>> patch here, or is all ok because I missed the above patch?
>>
>> No worries; sorry for not communicating clearly and for the delay.
>>
>> I meant to state that it would be good to have commit 0ed9498f9ecf ("USB: Simplify
>> USB ID table match") cherry picked to 5.8.y, as it fixes a bug, albeit a minor one.
> 
> What bug does it fix now?  Is usbip or the apple charger driver not
> working properly on 5.8.14?  If nothing is broken, no need to add this
> patch...

I had tried to describe the bug in my original e-mail; it involves not considering
the match functions of candidate USB drivers when determining whether a more
specialized driver exists for a USB device. The bug takes effect when a candidate
USB driver does not have an id_table, but has a match function.

To the best of my current understanding, however, the impact of the bug is
currently none/negligible, because the only specialized driver is currently
the Apple charger driver, and the Apple charger driver uses an id_table and
not a match function.

All this to say, given your last statement, and having thought about the whole
thing one more time, perhaps we do not need to cherry-pick the aforementioned
commit. Sorry for the noise!

Thank you,

Vefa

Powered by blists - more mailing lists