[2/3] usb: dwc2: host: do not delay retries after successful transfer

[2/3] usb: dwc2: host: do not delay retries after successful transfer

[Torokhov]

When handling split transactions we should not be delaying retrying
SSPLIT/CSPLIT after we successfully communicate with the device, so
let's reset dtd->num_naks counter when handling XFERCOMPL.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/usb/dwc2/hcd_intr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index d423b6a49f96c..b9879de6230d0 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -1092,6 +1092,7 @@ static void dwc2_hc_xfercomp_intr(struct dwc2_hsotg *hsotg,
 	}
 
 handle_xfercomp_done:
+	qtd->num_naks = 0;
 	disable_hc_int(hsotg, chnum, HCINTMSK_XFERCOMPL);
 }
 

[2/3] usb: dwc2: host: do not delay retries after successful transfer

[Doug Anderson]

Hi,

On Fri, Jun 15, 2018 at 3:01 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> When handling split transactions we should not be delaying retrying
> SSPLIT/CSPLIT after we successfully communicate with the device, so
> let's reset dtd->num_naks counter when handling XFERCOMPL.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/usb/dwc2/hcd_intr.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[2/3] usb: dwc2: host: do not delay retries after successful transfer

[Doug Anderson]

Hi,

On Fri, Jun 15, 2018 at 4:49 PM, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Fri, Jun 15, 2018 at 3:01 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> When handling split transactions we should not be delaying retrying
>> SSPLIT/CSPLIT after we successfully communicate with the device, so
>> let's reset dtd->num_naks counter when handling XFERCOMPL.
>>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> ---
>>  drivers/usb/dwc2/hcd_intr.c | 1 +
>>  1 file changed, 1 insertion(+)
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Though this looked good, I just tested this myself and found that it
corrupts memory.  Please do not merge it.

Specifically slub_debug complains of a use after free and sure enough
I can see that in one case we call dwc2_xfercomp_isoc_split_in() and
it can call dwc2_release_channel() and that can free the qtd.  Setting
the qtd->num_naks to 0 at the end of the function is a use after free.

I've just tested the same USB peripheral that Dmitry was testing with.
I found two things:

* This patch is important for making that peripheral work

* When I move the "qtd->num_naks" to the beginning it's all good.

Dmitry is OOTO at the moment, so I'll re-post this patch with that change.



-Doug
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html