diff mbox series

[v1,2/2] usb: dwc3: Prevent cleanup cancelled requests at the same time.

Message ID 1644831438-125403-3-git-send-email-dh10.jung@samsung.com (mailing list archive)
State Superseded
Headers show
Series Fix ep command fail issue in dequeue | expand

Commit Message

Jung Daehwan Feb. 14, 2022, 9:37 a.m. UTC 
We added cleanup cancelled requests when ep cmd timeout on ep dequeue
because there's no complete interrupt then. But, we find out new case
that complete interrupt comes up later. list_for_each_entry_safe is
used when cleanup cancelled requests and it has vulnerabilty on multi-core
environment. dwc3_gadget_giveback unlocks dwc->lock temporarily and other
core(ISR) can get lock and try to cleanup them again. It could cause
list_del corruption and we use DWC3_EP_END_TRANSFER_PENDING to prevent it.

1. MTP server cancels -> ep dequeue -> ep cmd timeout(END_TRANSFER)
   -> cleanup cancelled requests -> dwc3_gadget_giveback -> list_del -> release lock temporarily
2. Complete with END_TRANSFER -> ISR(dwc3_gadget_endpoint_command_complete) gets lock
   -> cleanup cancelled requests -> dwc3_gadget_giveback -> list_del
3. MTP server process gets lock again -> tries to access POISON list(list_del corruption)

[  205.014697] [2:      MtpServer: 5032] dwc3 10b00000.dwc3: request cancelled with wrong reason:5
[  205.014719] [2:      MtpServer: 5032] list_del corruption, ffffff88b6963968->next is LIST_POISON1 (dead000000000100)

Change-Id: I9df055c6c04855edd09e330300914454a6657a23
Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>

Change-Id: If87c88c3bb4c17ea1a5bde2bfec1382769f7ecab
Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
---
 drivers/usb/dwc3/gadget.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Greg Kroah-Hartman Feb. 14, 2022, 10:42 a.m. UTC | #1 
On Mon, Feb 14, 2022 at 06:37:18PM +0900, Daehwan Jung wrote:
> We added cleanup cancelled requests when ep cmd timeout on ep dequeue
> because there's no complete interrupt then. But, we find out new case
> that complete interrupt comes up later. list_for_each_entry_safe is
> used when cleanup cancelled requests and it has vulnerabilty on multi-core
> environment. dwc3_gadget_giveback unlocks dwc->lock temporarily and other
> core(ISR) can get lock and try to cleanup them again. It could cause
> list_del corruption and we use DWC3_EP_END_TRANSFER_PENDING to prevent it.
> 
> 1. MTP server cancels -> ep dequeue -> ep cmd timeout(END_TRANSFER)
>    -> cleanup cancelled requests -> dwc3_gadget_giveback -> list_del -> release lock temporarily
> 2. Complete with END_TRANSFER -> ISR(dwc3_gadget_endpoint_command_complete) gets lock
>    -> cleanup cancelled requests -> dwc3_gadget_giveback -> list_del
> 3. MTP server process gets lock again -> tries to access POISON list(list_del corruption)
> 
> [  205.014697] [2:      MtpServer: 5032] dwc3 10b00000.dwc3: request cancelled with wrong reason:5
> [  205.014719] [2:      MtpServer: 5032] list_del corruption, ffffff88b6963968->next is LIST_POISON1 (dead000000000100)
> 
> Change-Id: I9df055c6c04855edd09e330300914454a6657a23
> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> 
> Change-Id: If87c88c3bb4c17ea1a5bde2bfec1382769f7ecab
> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>

Why did you sign off on this twice?

And did you run it through checkpatch.pl?  It would have reminded you
that Change-Id: should not be on patches :(

Same for patch 1/1.

Please fix.

thanks,

greg k-h
Jung Daehwan Feb. 14, 2022, 10:46 a.m. UTC | #2 
On Mon, Feb 14, 2022 at 11:42:03AM +0100, Greg Kroah-Hartman wrote:
> On Mon, Feb 14, 2022 at 06:37:18PM +0900, Daehwan Jung wrote:
> > We added cleanup cancelled requests when ep cmd timeout on ep dequeue
> > because there's no complete interrupt then. But, we find out new case
> > that complete interrupt comes up later. list_for_each_entry_safe is
> > used when cleanup cancelled requests and it has vulnerabilty on multi-core
> > environment. dwc3_gadget_giveback unlocks dwc->lock temporarily and other
> > core(ISR) can get lock and try to cleanup them again. It could cause
> > list_del corruption and we use DWC3_EP_END_TRANSFER_PENDING to prevent it.
> > 
> > 1. MTP server cancels -> ep dequeue -> ep cmd timeout(END_TRANSFER)
> >    -> cleanup cancelled requests -> dwc3_gadget_giveback -> list_del -> release lock temporarily
> > 2. Complete with END_TRANSFER -> ISR(dwc3_gadget_endpoint_command_complete) gets lock
> >    -> cleanup cancelled requests -> dwc3_gadget_giveback -> list_del
> > 3. MTP server process gets lock again -> tries to access POISON list(list_del corruption)
> > 
> > [  205.014697] [2:      MtpServer: 5032] dwc3 10b00000.dwc3: request cancelled with wrong reason:5
> > [  205.014719] [2:      MtpServer: 5032] list_del corruption, ffffff88b6963968->next is LIST_POISON1 (dead000000000100)
> > 
> > Change-Id: I9df055c6c04855edd09e330300914454a6657a23
> > Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> > 
> > Change-Id: If87c88c3bb4c17ea1a5bde2bfec1382769f7ecab
> > Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> 
> Why did you sign off on this twice?
> 
> And did you run it through checkpatch.pl?  It would have reminded you
> that Change-Id: should not be on patches :(
> 
> Same for patch 1/1.
> 
> Please fix.
> 
> thanks,
> 
> greg k-h
> 

Dear greg,

I'm so sorry. It's my fault when getting patches from our system.
I'm going to fix and re-send it.

Best Regards,
Jung Daehwan
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 3ad3bc5813ca..2e0183512d5b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2046,8 +2046,11 @@  static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 
 			/* If ep cmd fails, then force to giveback cancelled requests here */
 			if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING)) {
-				dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
+				dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
 				dwc3_gadget_ep_cleanup_cancelled_requests(dep);
+
+				dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
+				dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
 			}
 
 			dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
@@ -3426,9 +3429,12 @@  static void dwc3_gadget_endpoint_command_complete(struct dwc3_ep *dep,
 	if (dep->stream_capable)
 		dep->flags |= DWC3_EP_IGNORE_NEXT_NOSTREAM;
 
+	if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING)) {
+		dwc3_gadget_ep_cleanup_cancelled_requests(dep);
+	}
+
 	dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
 	dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
-	dwc3_gadget_ep_cleanup_cancelled_requests(dep);
 
 	if (dep->flags & DWC3_EP_PENDING_CLEAR_STALL) {
 		struct dwc3 *dwc = dep->dwc;