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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 72948C43334 for ; Fri, 24 Jun 2022 16:31:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229844AbiFXQbL (ORCPT ); Fri, 24 Jun 2022 12:31:11 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58168 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229605AbiFXQbK (ORCPT ); Fri, 24 Jun 2022 12:31:10 -0400 Received: from mail-io1-xd36.google.com (mail-io1-xd36.google.com [IPv6:2607:f8b0:4864:20::d36]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0B3FA50B22 for ; Fri, 24 Jun 2022 09:31:09 -0700 (PDT) Received: by mail-io1-xd36.google.com with SMTP id s17so3232315iob.7 for ; Fri, 24 Jun 2022 09:31:09 -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=42vqXHuiZ9lUJjbA2pfwnVdZIl0tCd1lnlfeS2c09o8=; b=RdMOSt9wBjBPkoVnwIYFV+VWkAATYKxJkmR6c2wP03QQrnPnEPVstOUHv72i6D0Eag 8SqoitFjIH/S8uacVOj2LyJdXIKFjWUpj+Lt6uhPElN+EdbdcBfpVVunGnZ3P9GeCMsV GFw++SB20sFA9doxngusBTyA8m1dXVk/ld/lA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=42vqXHuiZ9lUJjbA2pfwnVdZIl0tCd1lnlfeS2c09o8=; b=5xEGFryfH733NHuPDQGGkLS/+mmP9npC/Vx7r/GhXW3k80VEJjplOLTagGko5KPN+Q uEVvP5xfdoBC2rsf9OimWlsCDUnfGptQ86LUNKYhozp1kxam7SgThJyTk+3UpiUDTrq4 e9/hx+/RPDAsZDHgZUwUUp0j99/qFD3iXMcfj/0LyLhw8lMuUGl7a7o1fHZc5KoCebcc 2eRKqmccg8G0gTG85jk7u6uAmxfoeNwZW4efmWyF+4PBKIGYXl1wUvXSQuvXNGJ2MWHW ymAnDtqRbrSxoxeWQtvDO4bPbIFAibg8OzTl6mr/qW5mMrXw7xhOilPd6DEoChDXhB1I lcTQ== X-Gm-Message-State: AJIora/b5nF+rxl0CKVh1qZyJ+fYfBJWyhi5jBkLF/BWlsCQmal0xWbY FMnKWL8k44NqacvaiwOIK3Z/0Q== X-Google-Smtp-Source: AGRyM1sahCDCNcX7Yf0xMwht08Evwx8yKQuTO2jO/VzugNl12Q27ofkLUPw6J1t5eNZJ4g7Xw354jg== X-Received: by 2002:a6b:b842:0:b0:669:b35b:fb66 with SMTP id i63-20020a6bb842000000b00669b35bfb66mr8027574iof.108.1656088268262; Fri, 24 Jun 2022 09:31:08 -0700 (PDT) Received: from [192.168.1.128] ([38.15.45.1]) by smtp.gmail.com with ESMTPSA id b201-20020a6bb2d2000000b00674c8448c3csm1445557iof.6.2022.06.24.09.31.06 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 24 Jun 2022 09:31:07 -0700 (PDT) Subject: Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT To: Alan Stern Cc: Hongren Zenithal Zheng , Rhett Aultman , linux-usb@vger.kernel.org, linux-can , Oliver Neukum , Marc Kleine-Budde , Greg Kroah-Hartman , Vincent Mailhol , Shuah Khan , Shuah Khan References: <20220609204714.2715188-1-rhett.aultman@samsara.com> <20220610213335.3077375-1-rhett.aultman@samsara.com> <20220610213335.3077375-2-rhett.aultman@samsara.com> <143b863d-c86b-6678-44e6-38799391fa36@linuxfoundation.org> From: Shuah Khan Message-ID: Date: Fri, 24 Jun 2022 10:31:06 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org On 6/24/22 8:43 AM, Alan Stern wrote: > On Thu, Jun 23, 2022 at 11:45:13AM -0600, Shuah Khan wrote: >> On 6/23/22 11:30 AM, Hongren Zenithal Zheng wrote: >>> On Fri, Jun 10, 2022 at 05:33:35PM -0400, Rhett Aultman wrote: >>>> >>>> In order to have all the flags in numerical order, URB_DIR_IN is >>>> renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse >>>> value 0x0200. >>> >>>> #define URB_FREE_BUFFER 0x0100 /* Free transfer buffer with the URB */ >>>> +#define URB_FREE_COHERENT 0x0200 /* Free DMA memory of transfer buffer */ >>>> /* The following flags are used internally by usbcore and HCDs */ >>>> -#define URB_DIR_IN 0x0200 /* Transfer from device to host */ >>>> +#define URB_DIR_IN 0x0400 /* Transfer from device to host */ >>>> #define URB_DIR_OUT 0 >>>> #define URB_DIR_MASK URB_DIR_IN >>>> -- >>>> 2.30.2 >>>> >>> >>> I'm afraid this is a change of uapi as this field is, unfortunately, >>> exported by usbip to userspace as TCP packets. >>> >>> This may also cause incompatibility (surprisingly not for this case, >>> detailed below) between usbip server and client >>> when one kernel is using the new flags and the other one is not. >>> >>> If we do change this, we may need to bump usbip protocol version >>> accordingly. >>> >> >> >>> A copy of Alan Stern's suggestion here for reference >>>> I don't see anything wrong with this, except that it would be nice to keep >>>> the flag values in numerical order. In other words, set URB_FREE_COHERENT >>>> to 0x0200 and change URB_DIR_IN to 0x0400. >>>> >>>> Alan Stern >> >> Thank you Alan for this detailed analysis of uapi impacts and >> usbip host side and vhci incompatibilities. Userspace is going >> to be affected. In addition to the usbip tool in the kernel repo, >> there are other versions floating around that would break if we >> were to change the flags. >> >>> One way to solve this issue for usbip >>> is to add some boilerplate transform >>> from URB_* to USBIP_FLAGS_* >>> as it is de facto uapi now. >> >> It doesn't sound like a there is a compelling reason other than >> "it would be nice to keep the flag values in numerical order". >> >> I would not recommend this option. I am not seeing any value to adding >> change URB_* to USBIP_FLAGS_* layer without some serious techinical >> concerns. >> >>> >>> Another way is to use 0x0400 for FREE_COHERENT. >>> usbip will not take care of this bit as >>> it would be masked. >>> >> >> I would go with this option adding a clear comment with link to this >> discussion. >> >>> Cc Shuah Khan here since she is the maintainer >>> on usbip. >>> >> >> Thank you adding me to the discussion. > > I can see this causing more problems in the future. There's no hint in > include/linux/usb.h that any of the values it defines are part of a user > API. If they are, they should be moved to include/uapi/linux/usb/. > Please elaborate on more problems in the future. > In general, if a user program depends on kernel details that are not > designed to be part of a user API, you should expect that the program > will sometimes break from one kernel version to another. > > Yes, I know Linus insists that kernel changes should not cause > regressions in userspace, but the line has to be drawn somewhere. > Otherwise the kernel could never change at all. > I have had to change the usbip sysfs interface api in the past to address security bugs related to information leaks. I am not saying no. I am asking if there is a good reason to do this. So far I haven't heard one. thanks, -- Shuah