Re: [PATCH 3/3] usbip: Make the driver's match function specific

[Shuah Khan <skhan@linuxfoundation.org>]

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