| Message ID | 20201005142115.000911358@linuxfoundation.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 05/10/2020 18.26, Greg Kroah-Hartman wrote: > From: M. Vefa Bicakci <m.v.b@runbox.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@vger.kernel.org> # 5.8: 41160802ab8e: USB: Simplify USB ID table match > Cc: <stable@vger.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 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 <m.v.b@runbox.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@vger.kernel.org> # 5.8: 41160802ab8e: USB: Simplify USB ID table match > > Cc: <stable@vger.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? 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 <m.v.b@runbox.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@vger.kernel.org> # 5.8: 41160802ab8e: USB: Simplify USB ID table match >>> Cc: <stable@vger.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. 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 <m.v.b@runbox.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@vger.kernel.org> # 5.8: 41160802ab8e: USB: Simplify USB ID table match > > > > Cc: <stable@vger.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... 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 <m.v.b@runbox.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@vger.kernel.org> # 5.8: 41160802ab8e: USB: Simplify USB ID table match >>>>> Cc: <stable@vger.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
--- 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,