From: M. Vefa Bicakci <[email protected]>
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: <[email protected]> # 5.8: 41160802ab8e: USB: Simplify USB ID table match
Cc: <[email protected]> # 5.8
Cc: Bastien Nocera <[email protected]>
Cc: Valentina Manea <[email protected]>
Cc: Shuah Khan <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Alan Stern <[email protected]>
Cc: <[email protected]>
Reported-by: Andrey Konovalov <[email protected]>
Tested-by: Andrey Konovalov <[email protected]>
Acked-by: Shuah Khan <[email protected]>
Signed-off-by: M. Vefa Bicakci <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
drivers/usb/usbip/stub_dev.c | 6 ------
1 file changed, 6 deletions(-)
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -461,11 +461,6 @@ static void stub_disconnect(struct usb_d
return;
}
-static bool usbip_match(struct usb_device *udev)
-{
- return true;
-}
-
#ifdef CONFIG_PM
/* These functions need usb_port_suspend and usb_port_resume,
@@ -491,7 +486,6 @@ struct usb_device_driver stub_driver = {
.name = "usbip-host",
.probe = stub_probe,
.disconnect = stub_disconnect,
- .match = usbip_match,
#ifdef CONFIG_PM
.suspend = stub_suspend,
.resume = stub_resume,
On 05/10/2020 18.26, Greg Kroah-Hartman wrote:
> From: M. Vefa Bicakci <[email protected]>
>
> 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: <[email protected]> # 5.8: 41160802ab8e: USB: Simplify USB ID table match
> Cc: <[email protected]> # 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 just wanted to bring this to your attention.
Thank you,
Vefa
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 <[email protected]>
> >
> > 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: <[email protected]> # 5.8: 41160802ab8e: USB: Simplify USB ID table match
> > Cc: <[email protected]> # 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?
thanks,
greg k-h
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 <[email protected]>
>>>
>>> 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: <[email protected]> # 5.8: 41160802ab8e: USB: Simplify USB ID table match
>>> Cc: <[email protected]> # 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.
Thank you,
Vefa
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 <[email protected]>
> > > >
> > > > 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: <[email protected]> # 5.8: 41160802ab8e: USB: Simplify USB ID table match
> > > > Cc: <[email protected]> # 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...
thanks,
greg k-h
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 <[email protected]>
>>>>>
>>>>> 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: <[email protected]> # 5.8: 41160802ab8e: USB: Simplify USB ID table match
>>>>> Cc: <[email protected]> # 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