* Fwd: [DWC3][Gadget] Question regarding the unmapping of request as part of queue failure [not found] <0105a5cd-936e-fb08-77bf-c2f1dbf0aeed@codeaurora.org> @ 2020-03-26 6:10 ` Sriharsha Allenki 2020-03-28 8:34 ` Felipe Balbi 0 siblings, 1 reply; 4+ messages in thread From: Sriharsha Allenki @ 2020-03-26 6:10 UTC (permalink / raw) To: balbi; +Cc: linux-usb, gregkh, Manu Gautam, Jack Pham Hi Felipe, I was looking at the code flow for ep_queue and came across the following piece of code. __dwc3_gadget_kick_transfer { dwc3_prepare_trbs(dep); req = next_request(&dep->started_list); if (!req) { dep->flags |= DWC3_EP_PENDING_REQUEST; return 0; } } As part of dwc3_prepare_trbs(dep), we get a request from the pending_list and queue to the tail of the started_list. But here we get the head of the started_list, now if there is any failure in issuing UPDATE_TRANSFER to the core, we unmap this request using "dwc3_gadget_del_and_unmap_request". But if this kick_transfer was part of the ep_queue and we have failed to issue update transfer, instead of unmapping the request we are trying to queue, we will be unmapping a different request (first in the started_list) which the core could have already started processing. I believe we should unmap the request we are trying to queue but not any other. Please provide your comments on this. Thanks, Sriharsha ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Fwd: [DWC3][Gadget] Question regarding the unmapping of request as part of queue failure 2020-03-26 6:10 ` Fwd: [DWC3][Gadget] Question regarding the unmapping of request as part of queue failure Sriharsha Allenki @ 2020-03-28 8:34 ` Felipe Balbi 2020-04-01 13:26 ` Sriharsha Allenki 0 siblings, 1 reply; 4+ messages in thread From: Felipe Balbi @ 2020-03-28 8:34 UTC (permalink / raw) To: Sriharsha Allenki; +Cc: linux-usb, gregkh, Manu Gautam, Jack Pham [-- Attachment #1: Type: text/plain, Size: 1564 bytes --] Hi, Sriharsha Allenki <sallenki@codeaurora.org> writes: > I was looking at the code flow for ep_queue and came across the > following piece of code. > > __dwc3_gadget_kick_transfer { > > dwc3_prepare_trbs(dep); > req = next_request(&dep->started_list); > if (!req) { > dep->flags |= DWC3_EP_PENDING_REQUEST; > return 0; > } > } > > As part of dwc3_prepare_trbs(dep), we get a request from the pending_list > and queue to the tail of the started_list. But here we get the head of > the started_list, now if there is any failure in issuing UPDATE_TRANSFER > to the core, we unmap this request using "dwc3_gadget_del_and_unmap_request". > > But if this kick_transfer was part of the ep_queue and we have failed > to issue update transfer, instead of unmapping the request we are trying > to queue, we will be unmapping a different request (first in the started_list) > which the core could have already started processing. I believe we should unmap > the request we are trying to queue but not any other. no, we have to start requests in order and dequeue them in order as well. There's no way to verify that the request is already processed by the HW, other than checking HWO bit which is set during dwc3_prepare_trbs(). This is a HW-SW race condition that we can't really fix. It is minimized, however, by the fact that, at least for non-isoc endpoints, we use No Status Update Transfer commands, which means the command can't fail. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Fwd: [DWC3][Gadget] Question regarding the unmapping of request as part of queue failure 2020-03-28 8:34 ` Felipe Balbi @ 2020-04-01 13:26 ` Sriharsha Allenki 2020-04-02 9:53 ` Felipe Balbi 0 siblings, 1 reply; 4+ messages in thread From: Sriharsha Allenki @ 2020-04-01 13:26 UTC (permalink / raw) To: Felipe Balbi; +Cc: linux-usb, gregkh, Manu Gautam, Jack Pham Hi, On 3/28/2020 2:04 PM, Felipe Balbi wrote: > Hi, > > Sriharsha Allenki <sallenki@codeaurora.org> writes: >> I was looking at the code flow for ep_queue and came across the >> following piece of code. >> >> __dwc3_gadget_kick_transfer { >> >> dwc3_prepare_trbs(dep); >> req = next_request(&dep->started_list); >> if (!req) { >> dep->flags |= DWC3_EP_PENDING_REQUEST; >> return 0; >> } >> } >> >> As part of dwc3_prepare_trbs(dep), we get a request from the pending_list >> and queue to the tail of the started_list. But here we get the head of >> the started_list, now if there is any failure in issuing UPDATE_TRANSFER >> to the core, we unmap this request using "dwc3_gadget_del_and_unmap_request". >> >> But if this kick_transfer was part of the ep_queue and we have failed >> to issue update transfer, instead of unmapping the request we are trying >> to queue, we will be unmapping a different request (first in the started_list) >> which the core could have already started processing. I believe we should unmap >> the request we are trying to queue but not any other. > no, we have to start requests in order and dequeue them in order as > well. There's no way to verify that the request is already processed by > the HW, other than checking HWO bit which is set during > dwc3_prepare_trbs(). This is a HW-SW race condition that we can't really > fix. > > It is minimized, however, by the fact that, at least for non-isoc > endpoints, we use No Status Update Transfer commands, which means the > command can't fail. Thanks Felipe for the reply. I see that this is a trick race condition between HW-SW, I have seen one occurrence where ep_queue from f_fs has failed (at kick_transfer).And since Asynchronous IO has been enabled, the request was freed leading to thecorruption of started_list because the list_del and unmap was happened on the requestat the head, but the request freed by the f_fs is at the tail of the started_list. This caused a use after free issue. Please let me know your comments. Thanks and Regards, Sriharsha ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Fwd: [DWC3][Gadget] Question regarding the unmapping of request as part of queue failure 2020-04-01 13:26 ` Sriharsha Allenki @ 2020-04-02 9:53 ` Felipe Balbi 0 siblings, 0 replies; 4+ messages in thread From: Felipe Balbi @ 2020-04-02 9:53 UTC (permalink / raw) To: Sriharsha Allenki; +Cc: linux-usb, gregkh, Manu Gautam, Jack Pham [-- Attachment #1: Type: text/plain, Size: 2406 bytes --] Hi, Sriharsha Allenki <sallenki@codeaurora.org> writes: >> Sriharsha Allenki <sallenki@codeaurora.org> writes: >>> I was looking at the code flow for ep_queue and came across the >>> following piece of code. >>> >>> __dwc3_gadget_kick_transfer { >>> >>> dwc3_prepare_trbs(dep); >>> req = next_request(&dep->started_list); >>> if (!req) { >>> dep->flags |= DWC3_EP_PENDING_REQUEST; >>> return 0; >>> } >>> } >>> >>> As part of dwc3_prepare_trbs(dep), we get a request from the pending_list >>> and queue to the tail of the started_list. But here we get the head of >>> the started_list, now if there is any failure in issuing UPDATE_TRANSFER >>> to the core, we unmap this request using "dwc3_gadget_del_and_unmap_request". >>> >>> But if this kick_transfer was part of the ep_queue and we have failed >>> to issue update transfer, instead of unmapping the request we are trying >>> to queue, we will be unmapping a different request (first in the started_list) >>> which the core could have already started processing. I believe we should unmap >>> the request we are trying to queue but not any other. >> no, we have to start requests in order and dequeue them in order as >> well. There's no way to verify that the request is already processed by >> the HW, other than checking HWO bit which is set during >> dwc3_prepare_trbs(). This is a HW-SW race condition that we can't really >> fix. >> >> It is minimized, however, by the fact that, at least for non-isoc >> endpoints, we use No Status Update Transfer commands, which means the >> command can't fail. > Thanks Felipe for the reply. I see that this is a trick race condition > between HW-SW, I have seen one occurrence where ep_queue from f_fs has > failed (at kick_transfer).And since Asynchronous IO has been enabled, what the reason to failure? Capture the debug data and send as a reply to this message. Method for reporting bugs on dwc3 is documented. > the request was freed leading to thecorruption of started_list because > the list_del and unmap was happened on the requestat the head, but the > request freed by the f_fs is at the tail of the started_list. This > caused a use after free issue. > > Please let me know your comments. No comments, really. I need to see debug data from dwc3. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-02 9:54 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <0105a5cd-936e-fb08-77bf-c2f1dbf0aeed@codeaurora.org> 2020-03-26 6:10 ` Fwd: [DWC3][Gadget] Question regarding the unmapping of request as part of queue failure Sriharsha Allenki 2020-03-28 8:34 ` Felipe Balbi 2020-04-01 13:26 ` Sriharsha Allenki 2020-04-02 9:53 ` Felipe Balbi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).