Opened 13 years ago
Closed 13 years ago
#379 closed defect (fixed)
Strange usbhub crash in async_get_call_timeout()
Reported by: | Jakub Jermář | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | 0.5.0 |
Component: | helenos/drv/usbhub | Version: | mainline |
Keywords: | Cc: | ||
Blocker for: | Depends on: | ||
See also: |
Description
As of default ia32 mainline,1259 UP, but not before mainline,1250, usbhub hits a strange assertion in the async framework:
ipc_callid_t async_get_call_timeout(ipc_call_t *call, suseconds_t usecs) { assert(call);
It is strange because call usually, and in this case specifically, is an address on the stack, i.e. never NULL. amd64 in the same configuration does not reproduce the problem.
I am going to attach a few screenshots I took when the system crashed.
Attachments (5)
Change History (11)
by , 13 years ago
Attachment: | SAM_0340.JPG added |
---|
by , 13 years ago
Attachment: | SAM_0341.JPG added |
---|
by , 13 years ago
Attachment: | SAM_0342.JPG added |
---|
by , 13 years ago
Attachment: | SAM_0343.JPG added |
---|
by , 13 years ago
Attachment: | usbhub.disasm added |
---|
comment:2 by , 13 years ago
This is beginning to look a lot like corrupted register EDI. I have built uspace with -O[0-3] and in all cases except of -O0, I can see that at one point there was a non-zero / valid preserved register EDI and several instructions later, usbhub
crashed because of 0 in or propagated from EDI:
- with -O3, EDI is computed as EBP - 40 (see 379#comment:1), a guaranteed non-zero
- with -O1 and -O2, EDI holds a non-zero address of
dev
indriver_dev_add()
and we crash inlist_append()
when accessingdev->link
In the former case, there are no interleaved function calls, which makes one think that EDI is corrupted because of the kernel preemption somehow. I was not able to spot anything suspicious in the ia32 preemption code though.
Another mystery is how is this related to mainline,1250 and why we don't see other crashes happening also to other tasks besides usbhub
. Interestingly, mainline,1250, touched usbhub
code a lot.
comment:3 by , 13 years ago
There is a loop in driver_connection() that I originally missed. Considering the loop, we could assume the EDI register was clobbered somewhere in usb_hub_device_add()
called from driver_dev_add()
.
comment:4 by , 13 years ago
At last, I found it. In usb_hub_process_hub_specific_info()
there is:
usb_hub_descriptor_header_t descriptor; size_t received_size; int opResult = usb_request_get_descriptor(control_pipe, USB_REQUEST_TYPE_CLASS, USB_REQUEST_RECIPIENT_DEVICE, USB_DESCTYPE_HUB, 0, 0, &descriptor, sizeof(usb_hub_descriptor_t), &received_size);
descriptor
is a usb_hub_descriptor_header_t
, but we pass sizeof(sizeof(usb_hub_descriptor_t)
to usb_request_get_descriptor()
. These two arguments have the meaning of buffer and buffer size in the called function.
The following patch fixes the problem for me, but I am not sure whether we should not rather make descriptor
a usb_hub_descriptor_t
:
=== modified file 'uspace/drv/bus/usb/usbhub/usbhub.c' --- uspace/drv/bus/usb/usbhub/usbhub.c 2011-09-27 18:39:12 +0000 +++ uspace/drv/bus/usb/usbhub/usbhub.c 2011-10-12 11:52:48 +0000 @@ -241,7 +241,7 @@ int opResult = usb_request_get_descriptor(control_pipe, USB_REQUEST_TYPE_CLASS, USB_REQUEST_RECIPIENT_DEVICE, USB_DESCTYPE_HUB, 0, 0, &descriptor, - sizeof(usb_hub_descriptor_t), &received_size); + sizeof(usb_hub_descriptor_header_t), &received_size); if (opResult != EOK) { usb_log_error("Failed to receive hub descriptor: %s.\n", str_error(opResult));
comment:5 by , 13 years ago
On 10/12/2011 02:53 PM, Jan Vesely wrote: > only data from the first 7 bytes are used, so asking for > sizeof(usb_hub_descriptor_header_t) > is enough, this is a leftover from usbhub rewrite, sorry
From the stacktrace as displayed on one of the screenshots, we see that the place from async_get_call_timeout() is called is this:
And here is the beginning of async_get_call_timeout():