Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Shuah Khan <skhan@linuxfoundation.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