| Message ID | 20200917144151.355848-1-m.v.b@runbox.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] usbcore/driver: Fix specific driver selection | expand |
On 9/17/20 8:41 AM, M. Vefa Bicakci wrote: > Prior to this commit, the USB-IP subsystem's USB device driver match > function used to match all USB devices (by returning true > unconditionally). Unfortunately, this is not correct behaviour and is > likely the root cause of the bug reported by Andrey Konovalov. > > USB-IP should only match USB devices that the user-space asked the kernel > to handle via USB-IP, by writing to the match_busid sysfs file, which is > what this commit aims to achieve. This is done by making the match > function check that the passed in USB device was indeed requested by the > user-space to be handled by USB-IP. > I see two patches 2/2 and 3/3 back to back. What is the difference between 2/2 and 3/3 versions? They look identical. Please include changes if any from version to version to make it easier for me to review. > Reported-by: Andrey Konovalov <andreyknvl@google.com> > Fixes: 7a2f2974f2 ("usbip: Implement a match function to fix usbip") > Link: https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/ > 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> > --- > drivers/usb/usbip/stub_dev.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c > index 9d7d642022d1..3d9c8ff6762e 100644 > --- a/drivers/usb/usbip/stub_dev.c > +++ b/drivers/usb/usbip/stub_dev.c > @@ -463,7 +463,20 @@ static void stub_disconnect(struct usb_device *udev) > > static bool usbip_match(struct usb_device *udev) > { > - return true; > + bool match; > + struct bus_id_priv *busid_priv; > + const char *udev_busid = dev_name(&udev->dev); > + > + busid_priv = get_busid_priv(udev_busid); > + if (!busid_priv) > + return false; > + > + match = (busid_priv->status != STUB_BUSID_REMOV && > + busid_priv->status != STUB_BUSID_OTHER); > + > + put_busid_priv(busid_priv); > + > + return match; > } > > #ifdef CONFIG_PM > Did you happen to run the usbip test on this patch? If not, can you please run tools/testing/selftests/drivers/usb/usbip/usbip_test.sh and make sure there are no regressions. thanks, -- Shuah
On Fri, Sep 18, 2020 at 05:31:26PM +0300, M. Vefa Bicakci wrote: > Hello all, > > I noticed that applying this patch on its own to the kernel causes the following > unexpected behaviour: As soon as the usbip_host module is loaded, all of the > USB devices are re-probed() by their drivers, and this causes the USB devices > connected to my system to be momentarily unavailable. This happens because > *without* the third patch in this patch set, the match function for the usbip_host > device driver unconditionally returns true. > > The third patch in this patch set [1] makes this unexpected behaviour go > away, as it makes the usbip device driver's match function only match devices > that were requested by user-space to be used with USB-IP. > > Is this something to be concerned about? I was thinking of people who might be > using git-bisect, who might encounter this issue in an unexpected manner. > > As a potential solution, I can prepare another patch to revert commit > 7a2f2974f2 ("usbip: Implement a match function to fix usbip") so that this > unexpected behaviour will not be observed. This revert would be placed as > the first patch in the patch series. Yes, that sounds like a good solution. Alan Stern
On 9/18/20 8:31 AM, M. Vefa Bicakci wrote: > On 18/09/2020 12.26, M. Vefa Bicakci wrote: >> On 17/09/2020 18.21, Shuah Khan wrote: >>> On 9/17/20 8:41 AM, M. Vefa Bicakci wrote: >>>> Prior to this commit, the USB-IP subsystem's USB device driver match >>>> function used to match all USB devices (by returning true >>>> unconditionally). Unfortunately, this is not correct behaviour and is >>>> likely the root cause of the bug reported by Andrey Konovalov. >>>> >>>> USB-IP should only match USB devices that the user-space asked the >>>> kernel >>>> to handle via USB-IP, by writing to the match_busid sysfs file, >>>> which is >>>> what this commit aims to achieve. This is done by making the match >>>> function check that the passed in USB device was indeed requested by >>>> the >>>> user-space to be handled by USB-IP. > > [snipped by Vefa] > >>>> Reported-by: Andrey Konovalov <andreyknvl@google.com> >>>> Fixes: 7a2f2974f2 ("usbip: Implement a match function to fix usbip") >>>> Link: >>>> https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/ >>>> >>>> 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> >>>> --- >>>> drivers/usb/usbip/stub_dev.c | 15 ++++++++++++++- >>>> 1 file changed, 14 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/usb/usbip/stub_dev.c >>>> b/drivers/usb/usbip/stub_dev.c >>>> index 9d7d642022d1..3d9c8ff6762e 100644 >>>> --- a/drivers/usb/usbip/stub_dev.c >>>> +++ b/drivers/usb/usbip/stub_dev.c >>>> @@ -463,7 +463,20 @@ static void stub_disconnect(struct usb_device >>>> *udev) >>>> static bool usbip_match(struct usb_device *udev) >>>> { >>>> - return true; >>>> + bool match; >>>> + struct bus_id_priv *busid_priv; >>>> + const char *udev_busid = dev_name(&udev->dev); >>>> + >>>> + busid_priv = get_busid_priv(udev_busid); >>>> + if (!busid_priv) >>>> + return false; >>>> + >>>> + match = (busid_priv->status != STUB_BUSID_REMOV && >>>> + busid_priv->status != STUB_BUSID_OTHER); >>>> + >>>> + put_busid_priv(busid_priv); >>>> + >>>> + return match; >>>> } >>>> #ifdef CONFIG_PM >>>> >>> >>> Did you happen to run the usbip test on this patch? If not, can you >>> please run tools/testing/selftests/drivers/usb/usbip/usbip_test.sh >>> and make sure there are no regressions. >> >> Ah, this is a very good point! I have been testing the patches on >> Qubes OS, >> which uses usbip to forward USB devices between VMs. To be honest, I >> was not >> aware of the self-tests for usbip, and I will run the self-tests prior to >> publishing the next version of the patch series. > > Hello Shuah, > > I have just cleaned up the patches and run usbip_test.sh with a kernel > without > the patches in this series and with a kernel in this series. > > I noticed that there is a change in behaviour due to the fact that the new > match function (usbip_match) does not always return true. This causes the > stub device driver's probe() function to not get called at all, as the new > more selective match function will prevent the stub device driver from > being > considered as a potential driver for the device under consideration. > Yes. This is the behavior I am concerned about and hence the reason to use the usbip test to verify this doesn't happen. With the patch you have the usbip match behavior becomes restrictive which isn't desirable. > All of this results in the following difference in the logs of the > usbip_test.sh, > where the expected kernel log message "usbip-host 2-6: 2-6 is not in > match_busid table... skip!" > is not printed by a kernel that includes the patches in this series. > > --- unpatched_kernel_log.txt 2020-09-18 17:12:10.654000000 +0300 > +++ patched_kernel_log.txt 2020-09-18 17:12:10.654000000 +0300 > @@ -213,70 +213,69 @@ > |__ Port 1: Dev 2, If 0, Class=Human Interface Device, > Driver=usbhid, 480M > ============================================================== > modprobe usbip_host - does it work? > Should see -busid- is not in match_busid table... skip! dmesg > ============================================================== > -usbip-host 2-6: 2-6 is not in match_busid table... skip! > ============================================================== > > Do you find this change in behaviour unacceptable? Yeah. This behavior isn't acceptable. If no, I can remove this > test case from usbip_test.sh with the same patch. If yes, then there is > a need > for a different solution to resolve the unexpected negative interaction > between > Bastien's work on generic/specific USB device driver selection and usbip > functionality. > I would recommend finding a different solution. Now that you have the usbip test handy, you can verify and test for regressions. thanks, -- Shuah
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index c976ea9f9582..950044a6e77f 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -924,7 +924,7 @@ static int __usb_bus_reprobe_drivers(struct device *dev, void *data) udev = to_usb_device(dev); if (usb_device_match_id(udev, new_udriver->id_table) == NULL && - (!new_udriver->match || new_udriver->match(udev) != 0)) + (!new_udriver->match || new_udriver->match(udev) == 0)) return 0; ret = device_reprobe(dev);
This commit resolves a 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 in the logic that determines whether a device currently bound to a generic USB device driver should be re-probed by a more specific USB device driver or not. The code in __usb_bus_reprobe_drivers() used to have the following lines: if (usb_device_match_id(udev, new_udriver->id_table) == NULL && (!new_udriver->match || new_udriver->match(udev) != 0)) return 0; ret = device_reprobe(dev); As the reader will notice, the code checks whether the USB device in consideration matches the identifier table (id_table) of a specific USB device_driver (new_udriver), followed by a similar check, but this time with the USB device driver's match function. However, the match function's return value is not checked correctly. When match() returns zero, it means that the specific USB device driver is *not* applicable to the USB device in question, but the code then goes on to reprobe the device with the new USB device driver under consideration. All this to say, the logic is inverted. This bug was found by code inspection and instrumentation after Andrey Konovalov's report indicating USB/IP subsystem's misbehaviour with the generic USB device driver matching code. Reported-by: Andrey Konovalov <andreyknvl@google.com> Fixes: d5643d2249 ("USB: Fix device driver race") Link: https://lore.kernel.org/linux-usb/CAAeHK+zOrHnxjRFs=OE8T=O9208B9HP_oo8RZpyVOZ9AJ54pAA@mail.gmail.com/ Cc: <stable@vger.kernel.org> # 5.8 Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Bastien Nocera <hadess@hadess.net> Cc: <syzkaller@googlegroups.com> Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com> --- drivers/usb/core/driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: 871e6496207c6aa94134448779c77631a11bfa2e