| Message ID | 20200922110703.720960-1-m.v.b@runbox.com |
|---|---|
| Headers | show |
| Series | Fixes for usbip and specialised USB driver selection | expand |
On Tue, Sep 22, 2020 at 1:07 PM M. Vefa Bicakci <m.v.b@runbox.com> wrote: > > Hello all, > > This is the third version of the patch sets originally published in the > e-mail thread thread at [1]. As mentioned in the same e-mail thread with > the e-mail at [2], I was able to find a more acceptable solution to the > issue reported by Andrey Konovalov, where usbip takes over the > dummy_hcd-provided devices set up by the USB fuzzing instance of the > syzkaller fuzzer. > > In summary, the approach involves: > > * Removal of the usbip_match function. > * Fixing two bugs in the specialised USB driver selection code. > * Accommodating usbip by changing the logic in the specialised USB > driver selection code, while preserving legacy/previous behaviour. > > I have tested this patch set with Greg Kroah-Hartman's usb-next tree > based on v5.9-rc6 with the base commit mentioned below in this e-mail, > and I can report that usbip works as expected, with no regressions in > the usbip_test.sh self-test suite output compared to v4.14.119. I have > also verified that the apple-mfi-fastcharge driver is correctly used > when an iPhone is plugged in to my system. Finally, I can report that > Andrey Konovalov's "keyboard" test program making use of dummy_hcd, > found at [3], also works as expected. > > I would appreciate your comments. > > Thank you, > > Vefa > > [1] https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/ > [2] https://lore.kernel.org/linux-usb/9f332d7b-e33d-ebd0-3154-246fbfb69128@runbox.com/ > [3] https://github.com/xairy/raw-gadget > > Cc: Bastien Nocera <hadess@hadess.net> > Cc: Valentina Manea <valentina.manea.m@gmail.com> > Cc: Shuah Khan <shuah@kernel.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: <syzkaller@googlegroups.com> > > M. Vefa Bicakci (4): > Revert "usbip: Implement a match function to fix usbip" > usbcore/driver: Fix specific driver selection > usbcore/driver: Fix incorrect downcast > usbcore/driver: Accommodate usbip > > drivers/usb/core/driver.c | 50 ++++++++++++++++++++++++------------ > drivers/usb/usbip/stub_dev.c | 6 ----- > 2 files changed, 34 insertions(+), 22 deletions(-) Hi Vefa, This fixes the issue that I've been having. Tested-by: Andrey Konovalov <andreyknvl@google.com> for the series. Thank you!
On 22/09/2020 15.38, Andrey Konovalov wrote: > On Tue, Sep 22, 2020 at 1:07 PM M. Vefa Bicakci <m.v.b@runbox.com> wrote: >> >> Hello all, >> >> This is the third version of the patch sets originally published in the >> e-mail thread thread at [1]. As mentioned in the same e-mail thread with >> the e-mail at [2], I was able to find a more acceptable solution to the >> issue reported by Andrey Konovalov, where usbip takes over the >> dummy_hcd-provided devices set up by the USB fuzzing instance of the >> syzkaller fuzzer. >> >> In summary, the approach involves: >> >> * Removal of the usbip_match function. >> * Fixing two bugs in the specialised USB driver selection code. >> * Accommodating usbip by changing the logic in the specialised USB >> driver selection code, while preserving legacy/previous behaviour. >> >> I have tested this patch set with Greg Kroah-Hartman's usb-next tree >> based on v5.9-rc6 with the base commit mentioned below in this e-mail, >> and I can report that usbip works as expected, with no regressions in >> the usbip_test.sh self-test suite output compared to v4.14.119. I have >> also verified that the apple-mfi-fastcharge driver is correctly used >> when an iPhone is plugged in to my system. Finally, I can report that >> Andrey Konovalov's "keyboard" test program making use of dummy_hcd, >> found at [3], also works as expected. >> >> I would appreciate your comments. >> >> Thank you, >> >> Vefa >> >> [1] https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/ >> [2] https://lore.kernel.org/linux-usb/9f332d7b-e33d-ebd0-3154-246fbfb69128@runbox.com/ >> [3] https://github.com/xairy/raw-gadget >> >> Cc: Bastien Nocera <hadess@hadess.net> >> Cc: Valentina Manea <valentina.manea.m@gmail.com> >> Cc: Shuah Khan <shuah@kernel.org> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: Alan Stern <stern@rowland.harvard.edu> >> Cc: <syzkaller@googlegroups.com> >> >> M. Vefa Bicakci (4): >> Revert "usbip: Implement a match function to fix usbip" >> usbcore/driver: Fix specific driver selection >> usbcore/driver: Fix incorrect downcast >> usbcore/driver: Accommodate usbip >> >> drivers/usb/core/driver.c | 50 ++++++++++++++++++++++++------------ >> drivers/usb/usbip/stub_dev.c | 6 ----- >> 2 files changed, 34 insertions(+), 22 deletions(-) > > Hi Vefa, > > This fixes the issue that I've been having. > > Tested-by: Andrey Konovalov <andreyknvl@google.com> > > for the series. > > Thank you! Thank you as well, Andrey, for confirming that these patches resolve the negative interaction between usbip and dummy_hcd! I appreciate your effort in testing the patches. Vefa
On 9/22/20 5:07 AM, M. Vefa Bicakci wrote: > 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". > > Reported-by: Andrey Konovalov <andreyknvl@google.com> > Link: https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/ > Cc: <stable@vger.kernel.org> # 5.8: 41160802ab8e: USB: Simplify USB ID table match > Cc: <stable@vger.kernel.org> # 5.8 > Cc: Bastien Nocera <hadess@hadess.net> > Cc: Valentina Manea <valentina.manea.m@gmail.com> > Cc: Shuah Khan <shuah@kernel.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: <syzkaller@googlegroups.com> > Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com> > > --- > v3: New patch in the patch set. > > Note for stable tree maintainers: I have marked the following commit > as a dependency of this patch, because that commit resolves a bug that > the next commit in this patch set uncovers, where if a driver does > not have an id_table, then its match function is not considered for > execution at all. > commit 41160802ab8e ("USB: Simplify USB ID table match") > --- > drivers/usb/usbip/stub_dev.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c > index 9d7d642022d1..2305d425e6c9 100644 > --- a/drivers/usb/usbip/stub_dev.c > +++ b/drivers/usb/usbip/stub_dev.c > @@ -461,11 +461,6 @@ static void stub_disconnect(struct usb_device *udev) > 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, > Thank you for finding a solution that works for usbip Acked-by: Shuah Khan <skhan@linuxfoundation.org> thanks, -- Shuah
On Tue, Sep 22, 2020 at 02:07:02PM +0300, M. Vefa Bicakci wrote: > This commit resolves a minor bug in the selection/discovery of more > specific USB device drivers for devices that are currently bound to > generic USB device drivers. > > The bug is related to the way a candidate USB device driver is > compared against the generic USB device driver. The code in > is_dev_usb_generic_driver() assumes that the device driver in question > is a USB device driver by calling to_usb_device_driver(dev->driver) > to downcast; however I have observed that this assumption is not always > true, through code instrumentation. > > This commit avoids the incorrect downcast altogether by comparing > the USB device's driver (i.e., dev->driver) to the generic USB > device driver directly. This method was suggested by Alan Stern. > > This bug was found while investigating Andrey Konovalov's report > indicating usbip device driver misbehaviour with the recently merged > generic USB device driver selection feature. The report is linked > below. > > Fixes: d5643d2249 ("USB: Fix device driver race") Nit, this should have been: Fixes: d5643d2249b2 ("USB: Fix device driver race") I'll go fix it up as my scripts are rejecting it as-is... thanks, greg k-h
On 9/25/20 5:51 PM, Greg Kroah-Hartman wrote: > On Tue, Sep 22, 2020 at 02:07:02PM +0300, M. Vefa Bicakci wrote: >> This commit resolves a minor bug in the selection/discovery of more >> specific USB device drivers for devices that are currently bound to >> generic USB device drivers. >> >> The bug is related to the way a candidate USB device driver is >> compared against the generic USB device driver. The code in >> is_dev_usb_generic_driver() assumes that the device driver in question >> is a USB device driver by calling to_usb_device_driver(dev->driver) >> to downcast; however I have observed that this assumption is not always >> true, through code instrumentation. >> >> This commit avoids the incorrect downcast altogether by comparing >> the USB device's driver (i.e., dev->driver) to the generic USB >> device driver directly. This method was suggested by Alan Stern. >> >> This bug was found while investigating Andrey Konovalov's report >> indicating usbip device driver misbehaviour with the recently merged >> generic USB device driver selection feature. The report is linked >> below. >> >> Fixes: d5643d2249 ("USB: Fix device driver race") > > Nit, this should have been: > Fixes: d5643d2249b2 ("USB: Fix device driver race") > > I'll go fix it up as my scripts are rejecting it as-is... Noted; sorry for missing this. I will use 12 characters from now on. I also wanted to thank you for committing the patches. Vefa
On Fri, Sep 25, 2020 at 07:31:44PM +0300, M. Vefa Bicakci wrote: > On 9/25/20 5:51 PM, Greg Kroah-Hartman wrote: > > On Tue, Sep 22, 2020 at 02:07:02PM +0300, M. Vefa Bicakci wrote: > > > This commit resolves a minor bug in the selection/discovery of more > > > specific USB device drivers for devices that are currently bound to > > > generic USB device drivers. > > > > > > The bug is related to the way a candidate USB device driver is > > > compared against the generic USB device driver. The code in > > > is_dev_usb_generic_driver() assumes that the device driver in question > > > is a USB device driver by calling to_usb_device_driver(dev->driver) > > > to downcast; however I have observed that this assumption is not always > > > true, through code instrumentation. > > > > > > This commit avoids the incorrect downcast altogether by comparing > > > the USB device's driver (i.e., dev->driver) to the generic USB > > > device driver directly. This method was suggested by Alan Stern. > > > > > > This bug was found while investigating Andrey Konovalov's report > > > indicating usbip device driver misbehaviour with the recently merged > > > generic USB device driver selection feature. The report is linked > > > below. > > > > > > Fixes: d5643d2249 ("USB: Fix device driver race") > > > > Nit, this should have been: > > Fixes: d5643d2249b2 ("USB: Fix device driver race") > > > > I'll go fix it up as my scripts are rejecting it as-is... > > Noted; sorry for missing this. I will use 12 characters from now on. No worries. There's a nice git configuration line you can do that is documented in the submitting patches file in the kernel documentation directory, if you want to use that. I have a alias that does it easily as well as it gets annoying to have to type: git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n" A lot :) > I also wanted to thank you for committing the patches. Thanks for fixing this all up! greg k-h
On 9/22/20 7:06 AM, M. Vefa Bicakci wrote: > Hello all, > > This is the third version of the patch sets originally published in the > e-mail thread thread at [1]. As mentioned in the same e-mail thread with > the e-mail at [2], I was able to find a more acceptable solution to the > issue reported by Andrey Konovalov, where usbip takes over the > dummy_hcd-provided devices set up by the USB fuzzing instance of the > syzkaller fuzzer. > > In summary, the approach involves: > > * Removal of the usbip_match function. > * Fixing two bugs in the specialised USB driver selection code. > * Accommodating usbip by changing the logic in the specialised USB > driver selection code, while preserving legacy/previous behaviour. > > I have tested this patch set with Greg Kroah-Hartman's usb-next tree > based on v5.9-rc6 with the base commit mentioned below in this e-mail, > and I can report that usbip works as expected, with no regressions in > the usbip_test.sh self-test suite output compared to v4.14.119. I have > also verified that the apple-mfi-fastcharge driver is correctly used > when an iPhone is plugged in to my system. Finally, I can report that > Andrey Konovalov's "keyboard" test program making use of dummy_hcd, > found at [3], also works as expected. > > I would appreciate your comments. > > Thank you, > > Vefa > > [1] https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/ > [2] https://lore.kernel.org/linux-usb/9f332d7b-e33d-ebd0-3154-246fbfb69128@runbox.com/ > [3] https://github.com/xairy/raw-gadget > > Cc: Bastien Nocera <hadess@hadess.net> > Cc: Valentina Manea <valentina.manea.m@gmail.com> > Cc: Shuah Khan <shuah@kernel.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: <syzkaller@googlegroups.com> > > M. Vefa Bicakci (4): > Revert "usbip: Implement a match function to fix usbip" > usbcore/driver: Fix specific driver selection > usbcore/driver: Fix incorrect downcast > usbcore/driver: Accommodate usbip > > drivers/usb/core/driver.c | 50 ++++++++++++++++++++++++------------ > drivers/usb/usbip/stub_dev.c | 6 ----- > 2 files changed, 34 insertions(+), 22 deletions(-) > > > base-commit: 55be22adf11b48c80ea181366685ec91a4700b7e > Hi, I ran into this issue when trying to use two different FTDI serial TTL cables on my laptop, running 5.9-rc7. This patch set fixes the issue. Oddly, I was unable to reproduce the issue on another laptop, also running 5.9-rc7. Tested-by: Brooke Basile <brookebasile@gmail.com> Thank you! Brooke Basile
On 10/2/20 6:11 AM, Brooke Basile wrote: > On 9/22/20 7:06 AM, M. Vefa Bicakci wrote: >> Hello all, >> >> This is the third version of the patch sets originally published in the >> e-mail thread thread at [1]. As mentioned in the same e-mail thread with >> the e-mail at [2], I was able to find a more acceptable solution to the >> issue reported by Andrey Konovalov, where usbip takes over the >> dummy_hcd-provided devices set up by the USB fuzzing instance of the >> syzkaller fuzzer. >> >> In summary, the approach involves: >> >> * Removal of the usbip_match function. >> * Fixing two bugs in the specialised USB driver selection code. >> * Accommodating usbip by changing the logic in the specialised USB >> driver selection code, while preserving legacy/previous behaviour. >> >> I have tested this patch set with Greg Kroah-Hartman's usb-next tree >> based on v5.9-rc6 with the base commit mentioned below in this e-mail, >> and I can report that usbip works as expected, with no regressions in >> the usbip_test.sh self-test suite output compared to v4.14.119. I have >> also verified that the apple-mfi-fastcharge driver is correctly used >> when an iPhone is plugged in to my system. Finally, I can report that >> Andrey Konovalov's "keyboard" test program making use of dummy_hcd, >> found at [3], also works as expected. >> >> I would appreciate your comments. >> >> Thank you, >> >> Vefa >> >> [1] https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/ >> [2] https://lore.kernel.org/linux-usb/9f332d7b-e33d-ebd0-3154-246fbfb69128@runbox.com/ >> [3] https://github.com/xairy/raw-gadget >> >> Cc: Bastien Nocera <hadess@hadess.net> >> Cc: Valentina Manea <valentina.manea.m@gmail.com> >> Cc: Shuah Khan <shuah@kernel.org> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: Alan Stern <stern@rowland.harvard.edu> >> Cc: <syzkaller@googlegroups.com> >> >> M. Vefa Bicakci (4): >> Revert "usbip: Implement a match function to fix usbip" >> usbcore/driver: Fix specific driver selection >> usbcore/driver: Fix incorrect downcast >> usbcore/driver: Accommodate usbip >> >> drivers/usb/core/driver.c | 50 ++++++++++++++++++++++++------------ >> drivers/usb/usbip/stub_dev.c | 6 ----- >> 2 files changed, 34 insertions(+), 22 deletions(-) >> >> >> base-commit: 55be22adf11b48c80ea181366685ec91a4700b7e >> > > Hi, > > I ran into this issue when trying to use two different FTDI serial TTL cables on my laptop, running 5.9-rc7. > > This patch set fixes the issue. > > Oddly, I was unable to reproduce the issue on another laptop, also running 5.9-rc7. > > Tested-by: Brooke Basile <brookebasile@gmail.com> > > Thank you! > Brooke Basile Hello Brooke, Thank you for the feedback! Greg Kroah-Hartman has committed the patches to the usb-linus branch of the USB git tree about a week ago, so it may unfortunately be a bit late to add your Tested-by tag to the patch series. Nevertheless, I appreciate your success report! In case you are interested, the committed patches can be seen here: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/log/?h=usb-linus&id=3fce39601a1a34d940cf62858ee01ed9dac5d459 Thanks again, Vefa
On 10/2/20 5:00 AM, M. Vefa Bicakci wrote: > On 10/2/20 6:11 AM, Brooke Basile wrote: >> On 9/22/20 7:06 AM, M. Vefa Bicakci wrote: >>> Hello all, >>> >>> This is the third version of the patch sets originally published in the >>> e-mail thread thread at [1]. As mentioned in the same e-mail thread with >>> the e-mail at [2], I was able to find a more acceptable solution to the >>> issue reported by Andrey Konovalov, where usbip takes over the >>> dummy_hcd-provided devices set up by the USB fuzzing instance of the >>> syzkaller fuzzer. >>> >>> In summary, the approach involves: >>> >>> * Removal of the usbip_match function. >>> * Fixing two bugs in the specialised USB driver selection code. >>> * Accommodating usbip by changing the logic in the specialised USB >>> driver selection code, while preserving legacy/previous behaviour. >>> >>> I have tested this patch set with Greg Kroah-Hartman's usb-next tree >>> based on v5.9-rc6 with the base commit mentioned below in this e-mail, >>> and I can report that usbip works as expected, with no regressions in >>> the usbip_test.sh self-test suite output compared to v4.14.119. I have >>> also verified that the apple-mfi-fastcharge driver is correctly used >>> when an iPhone is plugged in to my system. Finally, I can report that >>> Andrey Konovalov's "keyboard" test program making use of dummy_hcd, >>> found at [3], also works as expected. >>> >>> I would appreciate your comments. >>> >>> Thank you, >>> >>> Vefa >>> >>> [1] >>> https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/ >>> >>> [2] >>> https://lore.kernel.org/linux-usb/9f332d7b-e33d-ebd0-3154-246fbfb69128@runbox.com/ >>> >>> [3] https://github.com/xairy/raw-gadget >>> >>> Cc: Bastien Nocera <hadess@hadess.net> >>> Cc: Valentina Manea <valentina.manea.m@gmail.com> >>> Cc: Shuah Khan <shuah@kernel.org> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> Cc: Alan Stern <stern@rowland.harvard.edu> >>> Cc: <syzkaller@googlegroups.com> >>> >>> M. Vefa Bicakci (4): >>> Revert "usbip: Implement a match function to fix usbip" >>> usbcore/driver: Fix specific driver selection >>> usbcore/driver: Fix incorrect downcast >>> usbcore/driver: Accommodate usbip >>> >>> drivers/usb/core/driver.c | 50 ++++++++++++++++++++++++------------ >>> drivers/usb/usbip/stub_dev.c | 6 ----- >>> 2 files changed, 34 insertions(+), 22 deletions(-) >>> >>> >>> base-commit: 55be22adf11b48c80ea181366685ec91a4700b7e >>> >> >> Hi, >> >> I ran into this issue when trying to use two different FTDI serial TTL >> cables on my laptop, running 5.9-rc7. >> >> This patch set fixes the issue. >> >> Oddly, I was unable to reproduce the issue on another laptop, also >> running 5.9-rc7. >> >> Tested-by: Brooke Basile <brookebasile@gmail.com> >> >> Thank you! >> Brooke Basile > > Hello Brooke, > > Thank you for the feedback! Greg Kroah-Hartman has committed the patches > to the usb-linus branch of the USB git tree about a week ago, so it may > unfortunately be a bit late to add your Tested-by tag to the patch series. > Nevertheless, I appreciate your success report! > > In case you are interested, the committed patches can be seen here: > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/log/?h=usb-linus&id=3fce39601a1a34d940cf62858ee01ed9dac5d459 > > > Thanks again, > > Vefa > Vefa, Ah, no worries at all! Sorry, I didn't see this on LKML so I assumed it hadn't been merged yet. Thanks for the link! Best, Brooke Basile