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

["M. Vefa Bicakci" <m.v.b@runbox.com>]

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.
>>
> 
> 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.

Hello Shuah,

Sorry for the delayed reply, and thank you for your interest! I realize
that I did not add notes to the patch series regarding the changes between
v1 and v2, and I forgot to label the second patch series as v2.

Patches 2/2 and 3/3 are the same, as you have mentioned. I was addressing
Bastien's code review comments for patch 1/2, and I split that patch into
two separate patches, which is why the second patch series had 3 patches
as opposed to 2.

I realize that you are missing the context; here is a link to the thread:
   https://lore.kernel.org/linux-usb/359d080c-5cbb-250a-0ebd-aaba5f5c530d@runbox.com/T/

I can copy all patches to you as well, if you would be interested.

All this to say, I am sorry about this small mess, and I will rectify this
with patches I publish in the future.

>> 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.

> thanks,
> -- Shuah

Thank you!

Vefa