From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BF819C43464 for ; Fri, 18 Sep 2020 15:49:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 70DCC2388E for ; Fri, 18 Sep 2020 15:49:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1600444156; bh=NlIi8x1ZdcUv50+9LS1T75WkFtnO7+s4MnjRrbmIeng=; h=Subject:To:Cc:References:From:Date:In-Reply-To:List-ID:From; b=QPvfAPrJbYIMqlEyyxAEa7+m5jq0w5ER+6WW3kVtMs/sSJO4gfTRybhdBZdY1tgh5 ILlW9A6dkOA4uQNK6lcGQqqQfZPavoOkXs6DZTi80XeglsoSR6QtXtZZSWNoq+kvVq ZbGTFwqzOXZbSjA+EZF+Iy1hoapDRkmYNx/sKh+U= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726130AbgIRPtQ (ORCPT ); Fri, 18 Sep 2020 11:49:16 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40060 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725941AbgIRPtP (ORCPT ); Fri, 18 Sep 2020 11:49:15 -0400 Received: from mail-oo1-xc43.google.com (mail-oo1-xc43.google.com [IPv6:2607:f8b0:4864:20::c43]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 92024C0613CE for ; Fri, 18 Sep 2020 08:49:15 -0700 (PDT) Received: by mail-oo1-xc43.google.com with SMTP id m25so1537711oou.0 for ; Fri, 18 Sep 2020 08:49:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linuxfoundation.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=3ZCvvmPS8NZtzcruP7g025J2xlGm6Ho0jKBIcAsBZw8=; b=FQcc71LXQzx71dmeeefd9NzMr960WhzjM+ML44qYvLf/kwepwfygyAnsLEa3l7CPmD QBB7oIcSwiVCNe086MFRQ4Ff/JJqxAbZQztXfinVbDFX7jrtNcCtJ4C1QxafYK+9d1Qw w5FYm+ijWZTrToN5m/arhmrFbwIRKgyKe9gm4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=3ZCvvmPS8NZtzcruP7g025J2xlGm6Ho0jKBIcAsBZw8=; b=pUl+jOMELZAnld5veEKuAjPpoCEcC8ubFRWYaedvXU1l+l0w9/E/D2CUrC3kGCO7af cNTyALS0pp+k/e92LBDtw13v7dn9VUI/sFQkIx0q2ncUOkpD1l+GD+oo0kxjwo5GA4NA ZG3yp9Tkwz0M/zLiOlwvIcTWMXVcPCpU081rZbUkWNHQxm68+WK+4+IvQ+4oaMtXNjEN HK7/2DN8lXt/gXYqRdE93gLyBookZsby0WLi9A8lsJZ3bR0Rryld11JV05QT/OrglAlY //YeZaTY1z040VSoeiE7+IL5rOIbGmsLhZKninbl5yWgAvfWbA9AOm8FrqY4PyF+0MIQ K2aw== X-Gm-Message-State: AOAM532yucMBA3tkEBz9a9GUBdHX0akgjf5tUlyvPha2hTqvxfzdurGc jfy7gTi+tREe++DHa7R2DqUW6Q== X-Google-Smtp-Source: ABdhPJzxEgRaV8FUuna5mSlVx3LEhW2+ch/5o+hyR0quWhQb+fauYLPUj1Ysu4SsHrEEoWKIX9DYRA== X-Received: by 2002:a4a:d04c:: with SMTP id x12mr24290524oor.61.1600444154765; Fri, 18 Sep 2020 08:49:14 -0700 (PDT) Received: from [192.168.1.112] (c-24-9-64-241.hsd1.co.comcast.net. [24.9.64.241]) by smtp.gmail.com with ESMTPSA id j24sm2518897otn.64.2020.09.18.08.49.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 18 Sep 2020 08:49:14 -0700 (PDT) Subject: Re: [PATCH 3/3] usbip: Make the driver's match function specific To: "M. Vefa Bicakci" , linux-usb@vger.kernel.org Cc: Andrey Konovalov , stable@vger.kernel.org, Bastien Nocera , Valentina Manea , Shuah Khan , Greg Kroah-Hartman , Alan Stern , syzkaller@googlegroups.com, Shuah Khan References: <20200917144151.355848-1-m.v.b@runbox.com> <20200917144151.355848-3-m.v.b@runbox.com> <45badff8-53e9-359d-4bf2-b0f71b910b2f@linuxfoundation.org> From: Shuah Khan Message-ID: Date: Fri, 18 Sep 2020 09:49:12 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.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 >>>> 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: # 5.8 >>>> Cc: Bastien Nocera >>>> Cc: Valentina Manea >>>> Cc: Shuah Khan >>>> Cc: Greg Kroah-Hartman >>>> Cc: Alan Stern >>>> Cc: >>>> Signed-off-by: M. Vefa Bicakci >>>> --- >>>>   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