On 6/24/22 12:07 PM, Alan Stern wrote:
On Fri, Jun 24, 2022 at 10:31:06AM -0600, Shuah Khan wrote:
On 6/24/22 8:43 AM, Alan Stern wrote:
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 the future people will want to make other changes to
include/linux/usb.h and they will not be aware that those changes will
adversely affect usbip, because there is no documentation saying that
the values defined in usb.h are part of a user API. That will be a
problem, because those changes may be serious and important ones, not
just decorative or stylistic as in this case.
How often do these values change based on our past experience with these
fields?
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.
I agree with Hongren that values defined in include/linux/ should not be
part of a user API. There are two choices:
I agree with this in general. I don't think this is an explicit decision
to make them part of API. It is a consequence of simply copying the
transfer_flags. I am with you both on not being able to recognize the
impact until as this is rather obscure usage hidden away in the packets.
These defines aren't directly referenced.
Move the definitions into include/uapi/linux/, or
Wouldn't this be easier way to handle the change? With this option
the uapi will be well documented.
Add code to translate the values between the numbers used in
userspace and the numbers used in the kernel. (This is what
was done for urb->transfer_flags in devio.c:proc_do_submiturb()
near line 1862.)
I looked at the code and looks simple enough. I am okay going this route
if we see issues with the option 1.
thanks,
-- Shuah
