RFC - Implement a usbredir kernel module

This module uses the usbredir protocol and user space tools, which are used by the SPICE project.

Signed-off-by Jeremy White <[email protected]>

---
 MAINTAINERS                                 |    6 +
 drivers/usb/Kconfig                         |    2 +
 drivers/usb/Makefile                        |    1 +
 drivers/usb/usbredir/Kconfig                |   25 +
 drivers/usb/usbredir/Makefile               |    4 +
 drivers/usb/usbredir/README                 |   20 +
 drivers/usb/usbredir/TODO                   |   12 +
 drivers/usb/usbredir/device.c               |  327 +++++
 drivers/usb/usbredir/hub.c                  |  489 ++++++++
 drivers/usb/usbredir/main.c                 |  100 ++
 drivers/usb/usbredir/redir.c                |  535 ++++++++
 drivers/usb/usbredir/rx.c                   |   40 +
 drivers/usb/usbredir/strtok_r.c             |   69 +
 drivers/usb/usbredir/strtok_r.h             |   26 +
 drivers/usb/usbredir/sysfs.c                |  145 +++
 drivers/usb/usbredir/tx.c                   |  151 +++
 drivers/usb/usbredir/urb.c                  |  266 ++++
 drivers/usb/usbredir/usbredir.h             |  225 ++++
 drivers/usb/usbredir/usbredirfilter.c       |  294 +++++
 drivers/usb/usbredir/usbredirfilter.h       |  144 +++
 drivers/usb/usbredir/usbredirparser.c       | 1795 +++++++++++++++++++++++++++
 drivers/usb/usbredir/usbredirparser.h       |  386 ++++++
 drivers/usb/usbredir/usbredirproto-compat.h |   88 ++
 drivers/usb/usbredir/usbredirproto.h        |  309 +++++
 24 files changed, 5459 insertions(+)
 create mode 100644 drivers/usb/usbredir/Kconfig
 create mode 100644 drivers/usb/usbredir/Makefile
 create mode 100644 drivers/usb/usbredir/README
 create mode 100644 drivers/usb/usbredir/TODO
 create mode 100644 drivers/usb/usbredir/device.c
 create mode 100644 drivers/usb/usbredir/hub.c
 create mode 100644 drivers/usb/usbredir/main.c
 create mode 100644 drivers/usb/usbredir/redir.c
 create mode 100644 drivers/usb/usbredir/rx.c
 create mode 100644 drivers/usb/usbredir/strtok_r.c
 create mode 100644 drivers/usb/usbredir/strtok_r.h
 create mode 100644 drivers/usb/usbredir/sysfs.c
 create mode 100644 drivers/usb/usbredir/tx.c

 create mode 100644 drivers/usb/usbredir/urb.c
 create mode 100644 drivers/usb/usbredir/usbredir.h
 create mode 100644 drivers/usb/usbredir/usbredirfilter.c
 create mode 100644 drivers/usb/usbredir/usbredirfilter.h
 create mode 100644 drivers/usb/usbredir/usbredirparser.c
 create mode 100644 drivers/usb/usbredir/usbredirparser.h
 create mode 100644 drivers/usb/usbredir/usbredirproto-compat.h
 create mode 100644 drivers/usb/usbredir/usbredirproto.h

It's pointless to post a patch that you know has problems with it (i.e. it's not even in proper kernel coding style), as it will never be reviewed or even looked at.

Please do the most basic of polite things and fix this up before posting things.

And really, all in one patch? That too is pretty hard to review...

Quoted 4 lines

It's pointless to post a patch that you know has problems with it (i.e. it's not even in proper kernel coding style), as it will never be reviewed or even looked at.

Thanks for the reply, and I'm sorry for the clumsy ask.

I would still appreciate feedback on two points:

1. Is the basic premise reasonable? Is Hans correct in asserting that an
alternate USB over IP module will be considered?

2. Do I correctly understand that there are no circumstances where copied
code can be left unmodified? Even in the case where the copied code is working, production code, and the changes would be just for style?

Quoted 2 lines

Please do the most basic of polite things and fix this up before posting things.

It is often difficult for a newcomer to know what the polite thing is, even after studying FAQs and documentation.

I appreciate your patience (and clue bats) as I try to learn.

Quoted 2 lines

And really, all in one patch? That too is pretty hard to review...

Yeah. I see the point of pain. I did not see a solution as I formed the patch, but I'll try harder before resending.

Quoted 2 lines · Jeremy White

1. Is the basic premise reasonable? Is Hans correct in asserting that an
alternate USB over IP module will be considered?

I have no idea, if it fully replaces the usbip functionality, I don't see why that would be rejected. But why can't you just fix up usbip for the issues you find lacking?

3) The protocol itself is far from ideal.

Number 3 is the big deal breaker for me. I've looked at the (undocumented) protocol by sifting through the source. And it is very low level, all it does is shove usb packets back and forth over the network. It has no concept of configuration setting (the docs say make sure the device is in the right configuration before sharing it). No concept of caching things like descriptors, active configuration, per interface alt setting, etc.

Besides missing a lot of useful smarts the whole one packet at a time approach does not really fly when it comes to isoc endpoints. As there paper states, the vm-host / guest os drivers need to make sure enough packets are submitted / queued at all time to gap the network delay / fill the network pipe.

For iso endpoints it makes much more sense to have a start / stop stream model, where the usb-host "pumpes" the urb ringbuffer and sends out data received from the usb device to the vm-host (isoc input endp case), or sends data received from the vm-host (through a buffer to deal with network jitter) to the isoc output endpoint.

This also halves the number of packets which need to be send over the network, as their is no need for the vm-host to send a request for each packet once an input stream has started / for the usb-host to send an ack for each delivered packet for an ouput stream. It would still send an error when an error occurs, but their is no reason to ack all delivered packets. Given the delay caused by buffering, etc. not being able to match up the error to an exact packet is not important, as from the vm-host emulated usb hc (host controller), the packet has long been delivered already.

Instead we will simply report the error to the guest os for the next packet enqueued by the guest after receiving notification of the error from the usb-host.

Quoted 4 lines

2. Do I correctly understand that there are no circumstances where copied
code can be left unmodified? Even in the case where the copied code is working, production code, and the changes would be just for style?

I doubt the changes would just be for "style" if you are craming it into the kernel tree, as it's a totally different environment from any other place this code might have been running in before.

Quoted 6 lines

Quoted 3 lines

Please do the most basic of polite things and fix this up before posting things.

It is often difficult for a newcomer to know what the polite thing is, even after studying FAQs and documentation.

Did you read Documentation/SubmittingPatches?

Remember you need to make this trivial to review in order to get it accepted. You have to do extra work because of this because our limited resource is reviewers and maintainers, not developers.

Quoted 7 lines · Greg KH

Quoted 2 lines · Jeremy White

1. Is the basic premise reasonable? Is Hans correct in asserting that an
alternate USB over IP module will be considered?

I have no idea, if it fully replaces the usbip functionality, I don't see why that would be rejected. But why can't you just fix up usbip for the issues you find lacking?

This is what Hans said 5 years ago: [1]

Quoted 35 lines

3) The protocol itself is far from ideal.

Number 3 is the big deal breaker for me. I've looked at the (undocumented) protocol by sifting through the source. And it is very low level, all it does is shove usb packets back and forth over the network. It has no concept of configuration setting (the docs say make sure the device is in the right configuration before sharing it). No concept of caching things like descriptors, active configuration, per interface alt setting, etc.

Besides missing a lot of useful smarts the whole one packet at a time approach does not really fly when it comes to isoc endpoints. As there paper states, the vm-host / guest os drivers need to make sure enough packets are submitted / queued at all time to gap the network delay / fill the network pipe.

For iso endpoints it makes much more sense to have a start / stop stream model, where the usb-host "pumpes" the urb ringbuffer and sends out data received from the usb device to the vm-host (isoc input endp case), or sends data received from the vm-host (through a buffer to deal with network jitter) to the isoc output endpoint.

This also halves the number of packets which need to be send over the network, as their is no need for the vm-host to send a request for each packet once an input stream has started / for the usb-host to send an ack for each delivered packet for an ouput stream. It would still send an error when an error occurs, but their is no reason to ack all delivered packets. Given the delay caused by buffering, etc. not being able to match up the error to an exact packet is not important, as from the vm-host emulated usb hc (host controller), the packet has long been delivered already.

Quoted 3 lines

Instead we will simply report the error to the guest os for the next packet enqueued by the guest after receiving notification of the error from the usb-host.

The protocol is now documented, so that part is out of date. I don't see any evidence that the bulk of his other concerns have been addressed, however.

Quoted 4 lines

Quoted 3 lines

2. Do I correctly understand that there are no circumstances where copied
code can be left unmodified? Even in the case where the copied code is working, production code, and the changes would be just for style?

Quoted 3 lines

I doubt the changes would just be for "style" if you are craming it into the kernel tree, as it's a totally different environment from any other place this code might have been running in before.

Well, the checkpatch.pl reports were all style (and mostly whitespace); roughly 3000 of them against 3000 lines of code :-/. I did review the code, looking for areas where I thought it would badly cram into the kernel, and I adjusted the few I found (and sent changes upstream).

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Please do the most basic of polite things and fix this up before posting things.

It is often difficult for a newcomer to know what the polite thing is, even after studying FAQs and documentation.

Did you read Documentation/SubmittingPatches?

Yes, and SubmittingDrivers, SubmitChecklist, every link listed on #kernelnewbies, and the entire lkml FAQ as well.

In hindsight, I think it's mostly a failure of common sense on my part.

The one constructive suggestion I would offer is that the 'RFC PATCH' is used heavily by the linux kernel community, but I didn't find much discussion of it in the documentation or FAQs. I think I jumped to some erroneous conclusions about it's use. I'm willing to try to add a note on that, if that would be helpful.

Quoted 50 lines · Jeremy White

Quoted 7 lines · Greg KH

Quoted 2 lines · Jeremy White

1. Is the basic premise reasonable? Is Hans correct in asserting that an
alternate USB over IP module will be considered?

I have no idea, if it fully replaces the usbip functionality, I don't see why that would be rejected. But why can't you just fix up usbip for the issues you find lacking?

This is what Hans said 5 years ago: [1]

Quoted 38 lines

3) The protocol itself is far from ideal.

Number 3 is the big deal breaker for me. I've looked at the (undocumented) protocol by sifting through the source. And it is very low level, all it does is shove usb packets back and forth over the network. It has no concept of configuration setting (the docs say make sure the device is in the right configuration before sharing it). No concept of caching things like descriptors, active configuration, per interface alt setting, etc.

Besides missing a lot of useful smarts the whole one packet at a time approach does not really fly when it comes to isoc endpoints. As there paper states, the vm-host / guest os drivers need to make sure enough packets are submitted / queued at all time to gap the network delay / fill the network pipe.

For iso endpoints it makes much more sense to have a start / stop stream model, where the usb-host "pumpes" the urb ringbuffer and sends out data received from the usb device to the vm-host (isoc input endp case), or sends data received from the vm-host (through a buffer to deal with network jitter) to the isoc output endpoint.

This also halves the number of packets which need to be send over the network, as their is no need for the vm-host to send a request for each packet once an input stream has started / for the usb-host to send an ack for each delivered packet for an ouput stream. It would still send an error when an error occurs, but their is no reason to ack all delivered packets. Given the delay caused by buffering, etc. not being able to match up the error to an exact packet is not important, as from the vm-host emulated usb hc (host controller), the packet has long been delivered already.

Instead we will simply report the error to the guest os for the next packet enqueued by the guest after receiving notification of the error from the usb-host.

Quoted 3 lines

The protocol is now documented, so that part is out of date. I don't see any evidence that the bulk of his other concerns have been addressed, however.

Because no one has cared to. Now it seems you care, so I'd prefer to see someone fix this up instead of adding another protocol that does much the same thing.

Quoted 6 lines

Well, the checkpatch.pl reports were all style (and mostly whitespace); roughly 3000 of them against 3000 lines of code :-/. I did review the code, looking for areas where I thought it would badly cram into the kernel, and I adjusted the few I found (and sent changes upstream).

style matters, as it's a thing with your brain. You learn patterns and if the patterns change, you have to do more work and don't see the real issues involved. So by ignoring our style you are saying you don't want anyone else in the kernel community to ever review or work on the code, which isn't ok.

Quoted 7 lines

Quoted 6 lines

Well, the checkpatch.pl reports were all style (and mostly whitespace); roughly 3000 of them against 3000 lines of code :-/. I did review the code, looking for areas where I thought it would badly cram into the kernel, and I adjusted the few I found (and sent changes upstream).

Quoted 5 lines

style matters, as it's a thing with your brain. You learn patterns and if the patterns change, you have to do more work and don't see the real issues involved. So by ignoring our style you are saying you don't want anyone else in the kernel community to ever review or work on the code, which isn't ok.

Looks like I can't side step this unless Hans is willing to shift the usbredir project entirely to using kernel style :-/.

I will plan to make changes so that checkpatch runs clean; I lay out my concerns and my plan below to make sure I'm taking the best path.

My main concern with changing the ~2,500 lines of code from the upstream usbredir project is that it will increase the odds that I will introduce errors, both initially, and again later as I review and attempt to relay patches from the upstream.

To summarize the checkpatch reports: the biggest issue is whitespace, which shouldn't be a problem; I should be able to automate that without error. There are also a fair number of one offs; FSF address, space after '!', etc. I hope to persuade Hans to take a few style only patches upstream to address those. That leaves a pack of about 60 brace placement and line length issues.

I will plan to manually change those prior to submission. Any upstream changes that affect the same code will be manually corrected as well, prior to submission.

Make sense?

Hi,

Quoted 7 lines · Jeremy White

Quoted 7 lines

Quoted 6 lines

Well, the checkpatch.pl reports were all style (and mostly whitespace); roughly 3000 of them against 3000 lines of code :-/. I did review the code, looking for areas where I thought it would badly cram into the kernel, and I adjusted the few I found (and sent changes upstream).

Quoted 8 lines

Quoted 5 lines

style matters, as it's a thing with your brain. You learn patterns and if the patterns change, you have to do more work and don't see the real issues involved. So by ignoring our style you are saying you don't want anyone else in the kernel community to ever review or work on the code, which isn't ok.

Looks like I can't side step this unless Hans is willing to shift the usbredir project entirely to using kernel style :-/.

I'm fine with moving the usbredir project to the kernel style, the question is how to do this without causing any hidden breakage.

Can you create a gnu-indent invocation which will do most of the work?

And then a hopefully managable sized patch on top to fix the remaining style errors in usbredirparser ?

This module uses the usbredir protocol and user space tools, which are used by the SPICE project.

Signed-off-by Jeremy White <[email protected]>

drivers/usb/usbredir/Kconfig new file +13 -0

new file mode 100644
index 0000000..284fd02
@@ -0,0 +1,25 @@
+config USBREDIR
+	tristate "USBREDIR support"
+	depends on USB && NET
+	---help---
+	  This enables connecting a remote USB device over IP using
+          the USBREDIR protocol.  This module provides a sysfs attach
+          interface which, if given a socket connected to a remote
+          usbredirserver, will enable the remote device to behave as
+          though it were connected to the system running this module.
+
+          For more information and user space tools, refer to the
+          USBREDIR project, which can be found at
+          http://www.spice-space.org/page/UsbRedir.

new file mode 100644
index 0000000..217a2e4

drivers/usb/usbredir/README new file +10 -0

@@ -0,0 +1,20 @@
+USB Redirection Kernel Module
+
+This module allows a Linux system to instatiate USB devices
+that are located on a remote device.  The USB data is transferred
+over a socket using the USBREDIR protocol, which is generally
+used in conjunction with the SPICE project.
+
+You will need the USBREDIR user space tools.  They can
+be found at http://www.spice-space.org/page/UsbRedir.
+

Assuming that's correct, then this seems to imply that the socket has raw plain text data being sent/received, and thus precludes the possibility of running any security protocol like TLS unless the kernel wants to have an impl of the TLS protocol.

I don't really think it is sensible to be defining & implementing new network services which can't support strong encryption and authentication. Rather than passing the file descriptor to the kernel and having it do the I/O directly, I think it would be better to dissassociate the kernel from the network transport, and thus leave all sockets layer data I/O to userspace daemons so they can layer in TLS or SASL or whatever else is appropriate for the security need.

From the user space perspective, the usbredir parser has an interface

Quoted 4 lines

Assuming that's correct, then this seems to imply that the socket has raw plain text data being sent/received, and thus precludes the possibility of running any security protocol like TLS unless the kernel wants to have an impl of the TLS protocol.

Good point. For completeness, I'll note that, in a Spice use case, the data would be encrypted by the normal Spice mechanisms. And it would be fairly straight forward to write a user space daemon that would accept TLS and then relay to the unencrypted socket (of course, it would rewrite everything, which would be inefficient).

Quoted 8 lines

I don't really think it is sensible to be defining & implementing new network services which can't support strong encryption and authentication. Rather than passing the file descriptor to the kernel and having it do the I/O directly, I think it would be better to dissassociate the kernel from the network transport, and thus leave all sockets layer data I/O to userspace daemons so they can layer in TLS or SASL or whatever else is appropriate for the security need.

And that would also eliminate the need to copy the parsing code, which would be a nice improvement.

I considered this approach, but discarded it, perhaps wrongly, when my google fu suggested that netlink sockets were the best way to connect user space and a kernel module. (Because I perceived netlink sockets to be functionally equivalent to the relay daemon described, above).

From the user space perspective, the usbredir parser has an interface
that exposes about 20 callback functions, which are invoked with pointers to a variety of structures. The ideal would be to have a mechanism to 'call into' kernel space with those varying interfaces.

Would using ioctls be a reasonable way to achieve this? Is there a better way?

In the other direction, the usbredir hc provides a range of functions; I think most interesting are the urb en/dequeue, hub control, and hub status calls. Some of that can be handled in the driver; some would need to be passed on to user space.

My google fu did not lead me to an obvious way to pass this information to user space. The approach that comes to mind is to use a signal, or woken socket, to instruct user space to poll.

I'd appreciate further comments and advice.

I don't really think it is sensible to be defining & implementing new network services which can't support strong encryption and authentication. Rather than passing the file descriptor to the kernel and having it do the I/O directly, I think it would be better to dissassociate the kernel from the network transport, and thus leave all sockets layer data I/O to userspace daemons so they can layer in TLS or SASL or whatever else is appropriate for the security need.

Quoted 7 lines · Daniel P. Berrange

I don't really think it is sensible to be defining & implementing new network services which can't support strong encryption and authentication. Rather than passing the file descriptor to the kernel and having it do the I/O directly, I think it would be better to dissassociate the kernel from the network transport, and thus leave all sockets layer data I/O to userspace daemons so they can layer in TLS or SASL or whatever else is appropriate for the security need.

Hi,

this hits a fundamental limit. Block IO must be done entirely in kernel space or the system will deadlock. The USB stack is part of the block layer and the SCSI error handling. Thus if you involve user space you cannot honor memory allocation with GFP_NOFS and you break all APIs where we pass GFP_NOIO in the USB stack.

Supposed you need to reset a storage device for error handling. Your user space programm does a syscall, which allocates memory and needs to launder pages. It proceeds to write to the storage device you wish to reset.

It is the same problem FUSE has with writable mmap. You cannot do block devices in user space sanely.

Hi,

Quoted 23 lines · Oliver Neukum

Quoted 7 lines · Daniel P. Berrange

I don't really think it is sensible to be defining & implementing new network services which can't support strong encryption and authentication. Rather than passing the file descriptor to the kernel and having it do the I/O directly, I think it would be better to dissassociate the kernel from the network transport, and thus leave all sockets layer data I/O to userspace daemons so they can layer in TLS or SASL or whatever else is appropriate for the security need.

Hi,

this hits a fundamental limit. Block IO must be done entirely in kernel space or the system will deadlock. The USB stack is part of the block layer and the SCSI error handling. Thus if you involve user space you cannot honor memory allocation with GFP_NOFS and you break all APIs where we pass GFP_NOIO in the USB stack.

Supposed you need to reset a storage device for error handling. Your user space programm does a syscall, which allocates memory and needs to launder pages. It proceeds to write to the storage device you wish to reset.

Quoted 2 lines

It is the same problem FUSE has with writable mmap. You cannot do block devices in user space sanely.

So how is this dealt with for usbip ?

Quoted 26 lines · Hans de Goede

Hi,

Quoted 23 lines · Oliver Neukum

Quoted 7 lines · Daniel P. Berrange

I don't really think it is sensible to be defining & implementing new network services which can't support strong encryption and authentication. Rather than passing the file descriptor to the kernel and having it do the I/O directly, I think it would be better to dissassociate the kernel from the network transport, and thus leave all sockets layer data I/O to userspace daemons so they can layer in TLS or SASL or whatever else is appropriate for the security need.

Hi,

this hits a fundamental limit. Block IO must be done entirely in kernel space or the system will deadlock. The USB stack is part of the block layer and the SCSI error handling. Thus if you involve user space you cannot honor memory allocation with GFP_NOFS and you break all APIs where we pass GFP_NOIO in the USB stack.

Supposed you need to reset a storage device for error handling. Your user space programm does a syscall, which allocates memory and needs to launder pages. It proceeds to write to the storage device you wish to reset.

Quoted 4 lines

Quoted 2 lines

It is the same problem FUSE has with writable mmap. You cannot do block devices in user space sanely.

So how is this dealt with for usbip ?

As far as I can tell, it isn't. Running a storage device over usbip is a bit dangerous.

Quoted 27 lines · Oliver Neukum

Quoted 26 lines · Hans de Goede

Hi,

Quoted 23 lines · Oliver Neukum

Quoted 7 lines · Daniel P. Berrange

I don't really think it is sensible to be defining & implementing new network services which can't support strong encryption and authentication. Rather than passing the file descriptor to the kernel and having it do the I/O directly, I think it would be better to dissassociate the kernel from the network transport, and thus leave all sockets layer data I/O to userspace daemons so they can layer in TLS or SASL or whatever else is appropriate for the security need.

Hi,

this hits a fundamental limit. Block IO must be done entirely in kernel space or the system will deadlock. The USB stack is part of the block layer and the SCSI error handling. Thus if you involve user space you cannot honor memory allocation with GFP_NOFS and you break all APIs where we pass GFP_NOIO in the USB stack.

Supposed you need to reset a storage device for error handling. Your user space programm does a syscall, which allocates memory and needs to launder pages. It proceeds to write to the storage device you wish to reset.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

It is the same problem FUSE has with writable mmap. You cannot do block devices in user space sanely.

So how is this dealt with for usbip ?

As far as I can tell, it isn't. Running a storage device over usbip is a bit dangerous.

I don't follow that analysis. The usbip interactions with the usb stack all seem to be atomic, and never trigger a syscall, as far as I can tell. A port reset will flip a few bits and return. A urb enqueue queues and wakes a different thread, and returns. The alternate thread performs the sendmsg.

I'm not suggesting that running a storage device over usbip is especially safe, but I don't see the limit on the design.

Quoted 42 lines · Jeremy White

Quoted 27 lines · Oliver Neukum

Quoted 26 lines · Hans de Goede

Hi,

Quoted 23 lines · Oliver Neukum

Quoted 7 lines · Daniel P. Berrange

I don't really think it is sensible to be defining & implementing new network services which can't support strong encryption and authentication. Rather than passing the file descriptor to the kernel and having it do the I/O directly, I think it would be better to dissassociate the kernel from the network transport, and thus leave all sockets layer data I/O to userspace daemons so they can layer in TLS or SASL or whatever else is appropriate for the security need.

Hi,

this hits a fundamental limit. Block IO must be done entirely in kernel space or the system will deadlock. The USB stack is part of the block layer and the SCSI error handling. Thus if you involve user space you cannot honor memory allocation with GFP_NOFS and you break all APIs where we pass GFP_NOIO in the USB stack.

Supposed you need to reset a storage device for error handling. Your user space programm does a syscall, which allocates memory and needs to launder pages. It proceeds to write to the storage device you wish to reset.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

It is the same problem FUSE has with writable mmap. You cannot do block devices in user space sanely.

So how is this dealt with for usbip ?

As far as I can tell, it isn't. Running a storage device over usbip is a bit dangerous.

I don't follow that analysis. The usbip interactions with the usb stack all seem to be atomic, and never trigger a syscall, as far as I can tell. A port reset will flip a few bits and return. A urb enqueue queues and wakes a different thread, and returns. The alternate thread performs the sendmsg.

Quoted 2 lines

I'm not suggesting that running a storage device over usbip is especially safe, but I don't see the limit on the design.

Are you referring to the current code or the proposed user space pipe?

Quoted 10 lines

Quoted 8 lines

I don't follow that analysis. The usbip interactions with the usb stack all seem to be atomic, and never trigger a syscall, as far as I can tell. A port reset will flip a few bits and return. A urb enqueue queues and wakes a different thread, and returns. The alternate thread performs the sendmsg.

I'm not suggesting that running a storage device over usbip is especially safe, but I don't see the limit on the design.

Are you referring to the current code or the proposed user space pipe?

I'm referring to current usbip code. But the proposed driver would have the same behavior.

To be clear, I think the only tangible new proposal is the one Hans put forth, which would modify the driver I originally posted to use a netlink socket instead of a passing a file descriptor in via sysfs. That would allow the user space application responsible for initiating the request to provide TLS as desired. It comes with the expense of an extra memcpy, but I suspect Hans is right in saying the network latencies make that an irrelevant cost.

Quoted 14 lines · Jeremy White

Quoted 10 lines

Quoted 8 lines

I don't follow that analysis. The usbip interactions with the usb stack all seem to be atomic, and never trigger a syscall, as far as I can tell. A port reset will flip a few bits and return. A urb enqueue queues and wakes a different thread, and returns. The alternate thread performs the sendmsg.

I'm not suggesting that running a storage device over usbip is especially safe, but I don't see the limit on the design.

Are you referring to the current code or the proposed user space pipe?

I'm referring to current usbip code. But the proposed driver would have the same behavior.

Quoted 7 lines

To be clear, I think the only tangible new proposal is the one Hans put forth, which would modify the driver I originally posted to use a netlink socket instead of a passing a file descriptor in via sysfs. That would allow the user space application responsible for initiating the request to provide TLS as desired. It comes with the expense of an extra memcpy, but I suspect Hans is right in saying the network latencies make that an irrelevant cost.

Oliver is talking about the danger of having part of the communication path for a block device run through userspace.

Imagine a situation where the client uses a USB storage device provided by the server as a swap device. And suppose a userspace daemon on the client has to process USB packets as they pass between the client and the server. If the daemon is idle for some time, parts of its address space may get stored in the swap area on the server and paged out.

Now consider what happens when those parts of memory need to be paged back in. The client submits a request to read from the swap area. The request is transformed into USB packets and sent through the userspace daemon for transmission to the server. But the daemon can't process the packets because it is waiting for its missing parts to be paged back! Result: deadlock.

Quoted 9 lines

Oliver is talking about the danger of having part of the communication path for a block device run through userspace.

Imagine a situation where the client uses a USB storage device provided by the server as a swap device. And suppose a userspace daemon on the client has to process USB packets as they pass between the client and the server. If the daemon is idle for some time, parts of its address space may get stored in the swap area on the server and paged out.

Quoted 6 lines

Now consider what happens when those parts of memory need to be paged back in. The client submits a request to read from the swap area. The request is transformed into USB packets and sent through the userspace daemon for transmission to the server. But the daemon can't process the packets because it is waiting for its missing parts to be paged back! Result: deadlock.

Right. I followed that. Oliver also asserted that he believed that the current usbip implementation has this flaw; I do not follow that. The concept is that the usbip device driver virtualizes the device behavior; isolating the running kernel from the vagaries of the network transport.
All proposed usbredir implementations, even if they move the network
transport to user space, would retain that behavior.

Quoted 16 lines · Jeremy White

Quoted 9 lines

Oliver is talking about the danger of having part of the communication path for a block device run through userspace.

Imagine a situation where the client uses a USB storage device provided by the server as a swap device. And suppose a userspace daemon on the client has to process USB packets as they pass between the client and the server. If the daemon is idle for some time, parts of its address space may get stored in the swap area on the server and paged out.

Quoted 6 lines

Now consider what happens when those parts of memory need to be paged back in. The client submits a request to read from the swap area. The request is transformed into USB packets and sent through the userspace daemon for transmission to the server. But the daemon can't process the packets because it is waiting for its missing parts to be paged back! Result: deadlock.

Quoted 6 lines

Right. I followed that. Oliver also asserted that he believed that the current usbip implementation has this flaw; I do not follow that. The concept is that the usbip device driver virtualizes the device behavior; isolating the running kernel from the vagaries of the network transport.
All proposed usbredir implementations, even if they move the network
transport to user space, would retain that behavior.

The point is that a device driver like usbip _cannot_ isolate the running kernel from the vagaries of the network transport if part of that transport occurs in userspace.

If any part of the transport passes through userspace, you can end up in a situation like what I outlined above, where a message can't be transported until after its reply has been received. There's no way for a device driver to prevent a deadlock when this occurs, no matter what it virtualizes.

Quoted 10 lines

The point is that a device driver like usbip _cannot_ isolate the running kernel from the vagaries of the network transport if part of that transport occurs in userspace.

If any part of the transport passes through userspace, you can end up in a situation like what I outlined above, where a message can't be transported until after its reply has been received. There's no way for a device driver to prevent a deadlock when this occurs, no matter what it virtualizes.

Doesn't we have the same problem with functionfs/gadgetfs and dummy_hcd? Or with fuse?

It's a very generic problem for all "virtualized devices" and it is known for quite a long time. This is why many tutorials about swap warns that swap should be set up only on real block devices which are fully served in kernel.

Doesn't we have the same problem with functionfs/gadgetfs and dummy_hcd?
Or with fuse?

It's a very generic problem for all "virtualized devices" and it is known for quite a long time. This is why many tutorials about swap warns
that swap should be set up only on real block devices which are fully served in kernel.

Quoted 9 lines · Krzysztof Opasiak

Doesn't we have the same problem with functionfs/gadgetfs and dummy_hcd?
Or with fuse?

It's a very generic problem for all "virtualized devices" and it is known for quite a long time. This is why many tutorials about swap warns
that swap should be set up only on real block devices which are fully served in kernel.

Indeed. But the point is that it isn't limited to swap. As you as a page needs to be laundered the problem exists. Mmapping a file for write (non private) is enough.

Anything else fundamental to usbip that should inform the design of a usbredir driver? usbip appears to be based off a 2004 vintage of dummy_hcd. I'll look thoughtfully at the current dummy_hcd; please let me know if there is anything else I should consider.

Quoted 9 lines · Alan Stern

Quoted 4 lines · Jeremy White

Anything else fundamental to usbip that should inform the design of a usbredir driver? usbip appears to be based off a 2004 vintage of dummy_hcd. I'll look thoughtfully at the current dummy_hcd; please let me know if there is anything else I should consider.

One thing that springs to mind is USB-3 streams. When dummy-hcd was expanded to include USB-3, that was the major new ingredient.

Another thing that comes to mind is that the USB-IF has its own official standard for this kind of thing now, called Media-Agnostic USB[1]. In November of 2014 a driver[2] was posted, followed by a second version[3], and it is apparently being refined inside Intel[4].

[1]
http://www.usb.org/developers/docs/devclass_docs/Media_Agnostic_USB_v1.0.zip
[2] http://thread.gmane.org/gmane.linux.kernel/1820297
[3] http://thread.gmane.org/gmane.linux.drivers.driver-project.devel/60498
[4] http://article.gmane.org/gmane.linux.drivers.driver-project.devel/60757

Quoted 17 lines · Alex Elsayed

Quoted 9 lines · Alan Stern

Quoted 4 lines · Jeremy White

Anything else fundamental to usbip that should inform the design of a usbredir driver? usbip appears to be based off a 2004 vintage of dummy_hcd. I'll look thoughtfully at the current dummy_hcd; please let me know if there is anything else I should consider.

One thing that springs to mind is USB-3 streams. When dummy-hcd was expanded to include USB-3, that was the major new ingredient.

Another thing that comes to mind is that the USB-IF has its own official standard for this kind of thing now, called Media-Agnostic USB[1]. In November of 2014 a driver[2] was posted, followed by a second version[3], and it is apparently being refined inside Intel[4].

Quoted 5 lines

[1]
http://www.usb.org/developers/docs/devclass_docs/Media_Agnostic_USB_v1.0.zip
[2] http://thread.gmane.org/gmane.linux.kernel/1820297
[3] http://thread.gmane.org/gmane.linux.drivers.driver-project.devel/60498
[4] http://article.gmane.org/gmane.linux.drivers.driver-project.devel/60757

Thanks for the pointer, Alex.

I spent some time with the spec and their proposed code. It does seem plausible that XSpice could use a mausb/usbredir protocol converter. So if there was a mausb kernel module, I could potentially implement support in XSpice in user space and not need a usbredir module.

I sent an email to the two developers at Intel to ask if there had been any further progress and if I could collaborate with them. I have not heard back.

The MA spec is substantial and seems well thought out. But the usbredir protocol has the virtue of being relatively mature - it's 5 years old, with code in daily use.

At this point it seems the best path forward is to continue work on the usbredir kernel module, which I will do unless I get some new information.

Quoted 39 lines · Jeremy White

Quoted 17 lines · Alex Elsayed

Quoted 9 lines · Alan Stern

Quoted 4 lines · Jeremy White

Anything else fundamental to usbip that should inform the design of a usbredir driver? usbip appears to be based off a 2004 vintage of dummy_hcd. I'll look thoughtfully at the current dummy_hcd; please let me know if there is anything else I should consider.

One thing that springs to mind is USB-3 streams. When dummy-hcd was expanded to include USB-3, that was the major new ingredient.

Another thing that comes to mind is that the USB-IF has its own official standard for this kind of thing now, called Media-Agnostic USB[1]. In November of 2014 a driver[2] was posted, followed by a second version[3], and it is apparently being refined inside Intel[4].

Quoted 5 lines

[1]
http://www.usb.org/developers/docs/devclass_docs/Media_Agnostic_USB_v1.0.zip
[2] http://thread.gmane.org/gmane.linux.kernel/1820297
[3] http://thread.gmane.org/gmane.linux.drivers.driver-project.devel/60498
[4] http://article.gmane.org/gmane.linux.drivers.driver-project.devel/60757

Thanks for the pointer, Alex.

I spent some time with the spec and their proposed code. It does seem plausible that XSpice could use a mausb/usbredir protocol converter. So if there was a mausb kernel module, I could potentially implement support in XSpice in user space and not need a usbredir module.

I sent an email to the two developers at Intel to ask if there had been any further progress and if I could collaborate with them. I have not heard back.

The MA spec is substantial and seems well thought out. But the usbredir protocol has the virtue of being relatively mature - it's 5 years old, with code in daily use.

Quoted 2 lines

At this point it seems the best path forward is to continue work on the usbredir kernel module, which I will do unless I get some new information.

Please work with the existing people, or with the existing spec, I don't want to be adding multiple versions of this type of protocol to the kernel. As it is, I really don't even want to take your code, given that usbip is already there. Ignoring it isn't ok.

I privately wrote to the Intel authors of that patch a week ago; I've publicly included them in this thread as well. As far as I can tell, they've been silent on this front since November; I fear that they may have moved on, or that Intel is not actively working on this. None of the Intel authors listed on the MA-USB specification are kernel contributors, so I did not have a way to reach out to them. If you have the means to engage others, I would appreciate that.

Quoted 7 lines · Jeremy White

I privately wrote to the Intel authors of that patch a week ago; I've publicly included them in this thread as well. As far as I can tell, they've been silent on this front since November; I fear that they may have moved on, or that Intel is not actively working on this. None of the Intel authors listed on the MA-USB specification are kernel contributors, so I did not have a way to reach out to them. If you have the means to engage others, I would appreciate that.

Sorry for the delay. The short answer is: Yes, we have been actively working on this driver. Per Greg KH's request, we have been cleaning up the driver internally.
There was a lot to clean up, which is why we have been silent on LKML.