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
