| Message ID | 20180615220131.65402-2-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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); }
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(+)