Fwd: [DWC3][Gadget] Question regarding the unmapping of request as part of queue failure

Fwd: [DWC3][Gadget] Question regarding the unmapping of request as part of queue failure

[Sriharsha Allenki]

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

Re: Fwd: [DWC3][Gadget] Question regarding the unmapping of request as part of queue failure

[Felipe Balbi]

[-- 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 --]

Re: Fwd: [DWC3][Gadget] Question regarding the unmapping of request as part of queue failure

[Sriharsha Allenki]

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

Re: Fwd: [DWC3][Gadget] Question regarding the unmapping of request as part of queue failure

[Felipe Balbi]

[-- 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 --]