Skip to contentSkip to conversation

: Fix deadlock in USBIP driver (staging), linux-2.6.34-rc5

PATCH In review Eric Lescouet 2 participants 4 messages Apr 22, 2010 Staging

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 : Fix deadlock in USBIP driver (staging), linux-2.6.34-rc5

Hi,
When detaching a port from the client side (usbip --detach 0), the event thread, on the server side, is going to deadlock. The "eh" server thread is getting USBIP_EH_RESET event and calls:
-> stub_device_reset() -> usb_reset_device()
the USB framework is then calling back _in the same "eh" thread_ :
-> stub_disconnect() -> usbip_stop_eh() -> wait_for_completion()
the "eh" thread is being asleep forever, waiting for its own completion. The patch checks if "eh" is the current thread, in usbip_stop_eh().

Please Cc me in reply, I'm not in the list.

b.r.

------------------

drivers/staging/usbip/usbip_event.c 2010-04-22 17:07:36.249588273 +0200 renamed from drivers/staging/usbip/usbip_event.c 2010-04-20 01:29:56.000000000 +0200 +7 -0
diff -Nur linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c linux-2.6.34-rc5.new/drivers/staging/usbip/usbip_event.c
@@ -116,6 +116,13 @@
 void usbip_stop_eh(struct usbip_device *ud)
 {
     struct usbip_task *eh = &ud->eh;
+    int                i_am_eh;
+
+    lock_kernel();
+    i_am_eh = (eh->thread == current);
+    unlock_kernel();
+    if (i_am_eh)
+        return; /* do not wait for myself */
 
     wait_for_completion(&eh->thread_done);
     usbip_dbg_eh("usbip_eh has finished\n");
#1 lore raw
Quoted 9 lines · Eric Lescouet

Hi,
When detaching a port from the client side (usbip --detach 0), the event thread, on the server side, is going to deadlock. The "eh" server thread is getting USBIP_EH_RESET event and calls:
-> stub_device_reset() -> usb_reset_device()
the USB framework is then calling back _in the same "eh" thread_ :
-> stub_disconnect() -> usbip_stop_eh() -> wait_for_completion()
the "eh" thread is being asleep forever, waiting for its own completion. The patch checks if "eh" is the current thread, in usbip_stop_eh().

Ok, but does the lock_kernel() call you just made actually prevent this from happening? Isn't there some other lock you can use instead?

Quoted 1 line

Please Cc me in reply, I'm not in the list.

Also, to have patches that we can apply, we need a "Signed-off-by:" line. See the file, Documentation/SubmittingPatches for what this means.

Quoted 9 lines

b.r.

------------------

diff -Nur linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c linux-2.6.34-rc5.new/drivers/staging/usbip/usbip_event.c --- linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c

2010-04-20 01:29:56.000000000 +0200

Your email client wrapped the patch and made it unappliable :(

thanks,

greg k-h

#2 lore raw

Greg KH wrote:
[...]

Quoted 3 lines

Ok, but does the lock_kernel() call you just made actually prevent this from happening? Isn't there some other lock you can use instead?

Right. lock_kernel() is not needed (was copy/paste). The deadlock is avoided by not calling wait_for_completion() when the code is executed by the "eh" thread itself.

Quoted 3 lines

Also, to have patches that we can apply, we need a "Signed-off-by:" line. See the file, Documentation/SubmittingPatches for what this means.

Understood.

Quoted 5 lines

Your email client wrapped the patch and made it unappliable :(

thanks,

greg k-h

Sorry about that. Please find below an hopefully better one. Thanks,
Eric.

-------------

diff -Nur linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c linux-2.6.34-rc5.new/drivers/staging/usbip/usbip_event.c --- linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c 2010-04-20 01:29:56.000000000 +0200 +++ linux-2.6.34-rc5.new/drivers/staging/usbip/usbip_event.c 2010-04-22 19:19:52.997889126 +0200 @@ -117,6 +117,9 @@

{                                                                                                                                        
       struct usbip_task *eh = &ud->eh;                                                                                                  

+ if (eh->thread == current) + return; /* do not wait for myself */ +

       wait_for_completion(&eh->thread_done);                                                                                            
       usbip_dbg_eh("usbip_eh has finished\n");                                                                                          
}                                                                                                                                        
#3 lore raw
Quoted 37 lines · Eric Lescouet

Greg KH wrote:
[...]

Quoted 3 lines

Ok, but does the lock_kernel() call you just made actually prevent this from happening? Isn't there some other lock you can use instead?

Right. lock_kernel() is not needed (was copy/paste). The deadlock is avoided by not calling wait_for_completion() when the code is executed by the "eh" thread itself.

Quoted 3 lines

Also, to have patches that we can apply, we need a "Signed-off-by:" line. See the file, Documentation/SubmittingPatches for what this means.

Understood.

Quoted 5 lines

Your email client wrapped the patch and made it unappliable :(

thanks,

greg k-h

Sorry about that. Please find below an hopefully better one. Thanks,
Eric.

-------------

diff -Nur linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c linux-2.6.34-rc5.new/drivers/staging/usbip/usbip_event.c --- linux-2.6.34-rc5/drivers/staging/usbip/usbip_event.c

2010-04-20 01:29:56.000000000 +0200
+++ linux-2.6.34-rc5.new/drivers/staging/usbip/usbip_event.c
2010-04-22 19:19:52.997889126 +0200

drivers/staging/usbip/usbip_event.c 2010-04-22 17:07:36.249588273 +0200 line 123

Quoted 7 lines
drivers/staging/usbip/usbip_event.c 2010-04-22 17:07:36.249588273 +0200 renamed from drivers/staging/usbip/usbip_event.c 2010-04-20 01:29:56.000000000 +0200 +3 -0
@@ -117,6 +117,9 @@
 {
 struct usbip_task *eh = &ud->eh;
+       if (eh->thread == current)
+               return; /* do not wait for myself */
+
 wait_for_completion(&eh->thread_done);
 usbip_dbg_eh("usbip_eh has finished\n");

Odd, the patch has all leading tabs and spaces stripped out of it, is linewrapped, and the tabs are converted to spaces :(

Care to try again, third time's a charm...

thanks,

greg k-h