On 12/23/2015 08:40 AM, Joe Lawrence wrote:
> On 12/21/2015 10:07 AM, Mathias Nyman wrote:
>> Hi
>>
>> On 18.12.2015 18:48, Joe Lawrence wrote:
>>> Hello Roger and Mathias,
>>>
>>> Running with slub_debug=FZPU and removing an XHCI host controller via
>>> sysfs, I've hit a use-after-free that I've bisected to:
>>>
>>> 8c24d6d7b09deee3036ddc4f2b81b53b28c8f877 is the first bad commit
>>> commit 8c24d6d7b09deee3036ddc4f2b81b53b28c8f877
>> ...
>>>
>>> If I revert 8c24d6d7b09d "usb: xhci: stop everything on the first call
>>> to xhci_stop", the warning goes away.
>>>
>>> Let me know if any additional instrumentation or information would help
>>> track down this issue.
>>>
>>
>> Thanks for the in-depth analysis.
>>
>> Seems that the problem is that xhci driver frees data for all devices,
>> both
>> usb2 and and usb3 the first time usb_remove_hcd() is called.
>>
>> All data includes the all devices all endpoints and endpoint rings,
>> including the
>> the td_list in the xhci_ring struct.
>>
>> When we call usb_remove_hcd() a second time for the second xhci bus, as
>> part of
>> usb_hcd_pci_remove() in xhci_pci_remove() it will try to dequeue all
>> pending urbs,
>> but td_list is already freed for that endpoint ring.
>>
>> before commit 8c24d6d7b09d we only halted xhci when first hcd was
>> removed,
>> devices were freed only after second hcd removal.
>>
>> we will also free xhci->devs[slot_id] and set it to null after freeing
>> the endpoint rings,
>> so something like this should work in this case (copypaste of git diff):
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 3f91270..dfc11d0 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -1549,7 +1549,9 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct
>> urb *urb, int status)
>> xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
>> "HW died, freeing TD.");
>> urb_priv = urb->hcpriv;
>> - for (i = urb_priv->td_cnt; i < urb_priv->length; i++) {
>> + for (i = urb_priv->td_cnt;
>> + i < urb_priv->length &&
>> xhci->devs[urb->dev->slot_id];
>> + i++) {
>> td = urb_priv->td[i];
>> if (!list_empty(&td->td_list))
>> list_del_init(&td->td_list);
>>
>> There might be other similar cases caused by commit 8c24d6d7b09d.
>> This whole issue probably needs more attention.
>
> Hi Mathias,
>
> Thanks for taking a look. I applied these changes on top of 4.3 and
> didn't see any further slub_debug complaints on sysfs or hotplug removal
> during overnight testing. :) Feel free to add my Reported-by or
> Tested-by tag.
Gentle ping -- is there anything else I should need to test for this patch?
Regards,
-- Joe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
