Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ioctl(USB_RAW_IOCTL_EP_READ): Cannot send after transport endpoint shutdown when changing altinterface #9

Closed
RandomOnlineName opened this issue Aug 23, 2023 · 21 comments · Fixed by #14

Comments

@RandomOnlineName
Copy link

log
kmsg

This issue when passing through an xbox controller and with my raspberry pi 4B
When trying to run the program the program hangs for 5-10 second before crashing an printing the error
When switching interfaces the program tries to terminate endpoints but one of them hangs when reading and from what I can tell that causes the crash
The proxy works perfectly using dummy raw gadget and also another device (usb drive) which changes interfaces
A wireshark file as a result of proxying the controller is here

@AristoChen
Copy link
Owner

Hi,

Thanks for reporting the issue!

could you please provide the output of this command lsusb -v -d 045e:02ea?

@RandomOnlineName
Copy link
Author

Bus 001 Device 010: ID 045e:02ea Microsoft Corp. Xbox One S Controller
Couldn't open device, some information will be missing
Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass 255 Vendor Specific Class
bDeviceSubClass 71
bDeviceProtocol 208
bMaxPacketSize0 64
idVendor 0x045e Microsoft Corp.
idProduct 0x02ea Xbox One S Controller
bcdDevice 5.0d
iManufacturer 1 Microsoft
iProduct 2 Controller
iSerial 3 3033363030313331323636383530
bNumConfigurations 1
Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 0x0077
bNumInterfaces 3
bConfigurationValue 1
iConfiguration 0
bmAttributes 0xa0
(Bus Powered)
Remote Wakeup
MaxPower 500mA
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 0
bNumEndpoints 2
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 71
bInterfaceProtocol 208
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x02 EP 2 OUT
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 4
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 1
bNumEndpoints 2
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 71
bInterfaceProtocol 208
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x02 EP 2 OUT
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 2
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 0
bNumEndpoints 0
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 71
bInterfaceProtocol 208
iInterface 0
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 1
bNumEndpoints 2
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 71
bInterfaceProtocol 208
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x03 EP 3 OUT
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00e4 1x 228 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 2
bAlternateSetting 0
bNumEndpoints 0
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 71
bInterfaceProtocol 208
iInterface 0
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 2
bAlternateSetting 1
bNumEndpoints 2
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 71
bInterfaceProtocol 208
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x04 EP 4 OUT
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 0

@AristoChen
Copy link
Owner

AristoChen commented Sep 2, 2023

Hi @RandomOnlineName ,

Thanks for providing the info, could you please also provide the output of uname -a? and what is the OS that you were proxying to? is it Linux or Windows?

And I got a Xbox controller to test today, I do encounter similiar issue, and please try to switch to commit ID 6aecd455b2ef3d6734ab92105ca00c01464d4030 to test again

# in usb-proxy directory
$ git reset --hard 6aecd455b2ef3d6734ab92105ca00c01464d4030
$ make -j$(nproc)

with commit ID 6aecd455b2ef3d6734ab92105ca00c01464d4030, the xbox controller seems to be able to send some data and usb-proxy won't die, but does't look like it's working totally fine, I will spend more time on this issue when I have time, thanks!

@RandomOnlineName
Copy link
Author

uname -a outputs "Linux rasberrypi 5.15.89v7lAlex+ #3 SMP Tue Mar 21 18:08:19 GMT 2023 armv7l GNU/Linux"
I reset to top 6aecd45 but the same issue persists
The error above occurs when proxying to a windows laptop but another error happens when using both a linux laptop and a chromebook
"ioctl(USB_RAW_IOCTL_RUN): No such device" just after "Setup USB config successfully"

@xairy
Copy link
Contributor

xairy commented Sep 19, 2023

I think the problem is that the please_stop_eps logic doesn't really work with Raw Gadget. usb_raw_ep_read blocks until it receives some data. We need to kill the thread that calls usb_raw_ep_read when switching altsetting/interface (and also on a USB reset event, although Raw Gadget has no capability of reporting that yet). Otherwise, usb_raw_ep_read will fail with -ESHUTDOWN once usb_raw_ep_disable is called. It's actually surprising that proxy worked for some altsetting/interface-switching devices at all.

@xairy
Copy link
Contributor

xairy commented Sep 20, 2023

It's also possible that we need this Raw Gadget patch xairy/raw-gadget@60042be to avoid disabling the Raw Gadget device if the emulation code accidentally does an endpoint operation on a disabled endpoint.

@AristoChen
Copy link
Owner

Hi @xairy ,

Thanks for helping out on this issue!

I was awared that the please_stop_eps logic is not fully compatible with Raw Gadget previously, and I remember that there is a To-Do to make Raw Gadget operate in non_blocking mode, and I assume it would be working with the current please_stop_eps logic better.

I didn't try to implement the mechanism to kill the thread that calls usb_raw_ep_read when switching altsetting/interface previously, because IMHO I think it is a workaround and implementing non_blocking mode may be a better solution for this issue. I am 100% willing to try to implement it in Raw Gadget, but unfortunately I am not a kernel expert, so I don't know where and how to start :(

I will try that patch and update the result here when I have time. Thanks again for the help!

@xairy
Copy link
Contributor

xairy commented Sep 20, 2023

I don't think implementing the non-blocking mode will make that much of a difference: we will still need some way to wait for the transfer to complete and will still need a way to interrupt this waiting.

Using a signal to interrupt a blocking syscall is not that unusual of an approach. Technically, we don't even need to kill the thread. We can use any signal and add a no-op handler for it. The important part is that sending a signal will interrupt the usb_raw_ep_read call and make it return to userspace. And then the rest of the please_stop_eps logic should just work.

To make this work, we also need this patch btw xairy/raw-gadget@98f9a64, besides the one I linked above. If all this proves to work, I will send these patches upstream.

@AristoChen
Copy link
Owner

Hi @xairy , I see, thanks for the explanation!

Hi @RandomOnlineName , I might need your help, I think my Xbox controller behaves differently by comparing your log and my log, I never received the USB control request to change interface/altsetting, so please apply the patches mentioned above for Raw Gadget and apply the following for usb-proxy

diff --git a/proxy.cpp b/proxy.cpp
index e3dbd57..e139e3a 100644
--- a/proxy.cpp
+++ b/proxy.cpp
@@ -291,8 +291,15 @@ void terminate_eps(int fd, int config, int interface, int altsetting) {
        for (int i = 0; i < alt->interface.bNumEndpoints; i++) {
                struct raw_gadget_endpoint *ep = &alt->endpoints[i];
 
-               if (ep->thread_read && pthread_join(ep->thread_read, NULL)) {
-                       fprintf(stderr, "Error join thread_read\n");
+               if (ep->thread_read) {
+                       if (ep->thread_info.dir == "out") {
+                               pthread_kill(ep->thread_read, SIGINT);
+                               printf("Sent SIGINT to thread_read for EP%02x\n",
+                                               ep->thread_info.endpoint.bEndpointAddress);
+                       }
+                       if (pthread_join(ep->thread_read, NULL)) {
+                               fprintf(stderr, "Error join thread_read\n");
+                       }
                }
                if (ep->thread_write && pthread_join(ep->thread_write, NULL)) {
                        fprintf(stderr, "Error join thread_write\n");

I don't have a proper USB device to test this logic, hopefully it works, feel free to let me know if it doesn't, thanks

@RandomOnlineName
Copy link
Author

I will test this when I get back to my computer. Thank you for the help.

@RandomOnlineName
Copy link
Author

It causes an "ioctl(USB_RAW_IOCTL_READ): Interrupted system call" error with the same log otherwise

@xairy
Copy link
Contributor

xairy commented Oct 3, 2023

@RandomOnlineName I think you need to add a no-op handler for SIGINT.

@xairy
Copy link
Contributor

xairy commented Oct 21, 2023

I tested out the approach I suggested: xairy/raw-gadget@0732f53, it works. I used it for handling the reset event, but it should work the same for switching altsetting/interface.

@xairy
Copy link
Contributor

xairy commented Oct 21, 2023

A cleaner solution via pthread_cancel: xairy/raw-gadget@f380fc5.

On a side note, I found this article very useful to learn about pthread cancellation.

xairy added a commit to xairy/usb-proxy that referenced this issue Oct 21, 2023
Disable endpoint before attempting to join threads that might be
blocked on read/write Raw Gadget ioctls.

Hopefully fixes AristoChen#9.
xairy added a commit to xairy/usb-proxy that referenced this issue Oct 21, 2023
Disable endpoint before attempting to join threads that might be
blocked on read/write Raw Gadget ioctls.

Hopefully fixes AristoChen#9.
xairy added a commit to xairy/usb-proxy that referenced this issue Oct 22, 2023
Disable endpoint before attempting to join threads that might be
blocked on read/write Raw Gadget ioctls.

Hopefully fixes AristoChen#9.
@xairy
Copy link
Contributor

xairy commented Oct 22, 2023

Looked through usb-proxy code: #9 (comment) + pthread_cancel instead of pthread_kill should work, but there's a caveat: if the endpoint thread gets cancelled with data_mutex locked, this will result in a dead lock. Also if it gets cancelled while data is allocated, there will be a memory leak.

However, what I just realized is that we don't need to interrupt the ioctl via a signal. We can just disable the endpoint (before attempting to join the thread) and the blocking ioctl should exit with -ESHUTDOWN (or with -EBUSY if the endpoint is disabled before the ioctl is called).

@RandomOnlineName, could you check if this patch works for you (with Raw Gadget patches from its dev branch)?

Actually, scratch that: usb_ep_disable will fail due to waiting for urb completion.

@xairy
Copy link
Contributor

xairy commented Oct 24, 2023

Looks like interrupting blocked threads will not fix proxying the device. Something else is wrong: I don't think the host is supposed to send those USB_REQ_SET_INTERFACE requests at all. They make no sense: each interface already has the altsetting 0 set. xairy@b21a3a1 surfaces the issue.

xairy added a commit to xairy/usb-proxy that referenced this issue Oct 25, 2023
xairy added a commit to xairy/usb-proxy that referenced this issue Oct 26, 2023
@xairy
Copy link
Contributor

xairy commented Oct 26, 2023

Added support for reset handling: https://github.com/xairy/usb-proxy/tree/reset. Together with ignoring meaningless SET_INTERFACE requests, I can proxy an Xbox controller (connected to Windows; Linux works as is).

Requires Raw Gadget patches from dev branch. I've sent them upstream, but it's going to take some time before they are merged. I'll hold off sending a PR before the patches are at least in one of the USB trees.

@RandomOnlineName
Copy link
Author

I have tried https://github.com/xairy/usb-proxy/tree/reset and https://github.com/xairy/raw-gadget/tree/dev but get

event: disconnect
usb_raw_ep_read(): Cannot send after transport endpoint shutdown
usb_raw_ep_write(): Cannot send after transport endpoint shutdown

Could there be something different about our devices like kernel version or something else?

@xairy
Copy link
Contributor

xairy commented Oct 26, 2023

Ah, I think this is that bug in dwc2 that it reports a disconnect instead of a reset.

Please try this patch:

diff --git a/proxy.cpp b/proxy.cpp
index 28d288e..ec09b82 100644
--- a/proxy.cpp
+++ b/proxy.cpp
@@ -354,7 +354,7 @@ void ep0_loop(int fd) {
                        return;
                }
 
-               if (event.inner.type == USB_RAW_EVENT_RESET) {
+               if (event.inner.type == USB_RAW_EVENT_RESET || event.inner.type == USB_RAW_EVENT_DISCONNECT) {
                        printf("Resetting device\n");
                        // Normally, we would need to stop endpoint threads first and only then
                        // reset the device. However, libusb does not allow interrupting queued

If it still fails, please upload the full log somewhere and share.

@xairy
Copy link
Contributor

xairy commented Oct 27, 2023

Checked with my R Pi 4, we also need something like this:

diff --git a/proxy.cpp b/proxy.cpp
index 2ae255a..6474545 100644
--- a/proxy.cpp
+++ b/proxy.cpp
@@ -129,7 +129,7 @@ void *ep_loop_write(void *arg) {
 
                if (ep.bEndpointAddress & USB_DIR_IN) {
                        int rv = usb_raw_ep_write(fd, (struct usb_raw_ep_io *)&io);
-                       if (rv < 0 && please_stop_eps) {
+                       if (rv < 0) {
                                printf("EP%x(%s_%s): thread interrupted\n", ep.bEndpointAddress,
                                        transfer_type.c_str(), dir.c_str());
                                break;
@@ -218,7 +218,7 @@ void *ep_loop_read(void *arg) {
                        io.inner.length = sizeof(io.data);
 
                        int rv = usb_raw_ep_read(fd, (struct usb_raw_ep_io *)&io);
-                       if (rv < 0 && please_stop_eps) {
+                       if (rv < 0) {
                                printf("EP%x(%s_%s): thread interrupted\n", ep.bEndpointAddress,
                                        transfer_type.c_str(), dir.c_str());
                                break;

And then it works.

I'll think about how to implement this more cleanly.

@xairy
Copy link
Contributor

xairy commented Oct 28, 2023

Raw Gadget patches are now in usb-next, so I includes the changes into the master branch of Raw Gadget.

Waiting for #10 to be reviewed, and then will send another PR with reset handling that fixes this issue (https://github.com/xairy/usb-proxy/commits/reset).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants