Re: list_del corruption (NULL pointer dereference) on xhci-pci unbind

Mathias Nyman
2022-10-17T16:13:44Z

On 14.10.2022 23.29, Marek Marczykowski-Górecki wrote:
> On Fri, Oct 14, 2022 at 07:02:13PM +0300, Mathias Nyman wrote:
>> This whole software bandwidth issue should only be visible in Intel
>> Panther Point PCH xHC (Ivy bridge)
>
> It is indeed Ivy Bridge platform.
>
>> Endpoints should be deleted from bw_table list, and xhci_virt_devices
>> should be freed already before xhci_mem_cleanup() is called if all goes well.
>>
>> Normally endpoints are deleted from bw_table list during usb_disconnect()
>>
>> usb_disconnect()
>> ...
>> usb_hcd_alloc_bandwidth(dev, NULL, NULL, NULL);
>> hcd->driver->drop_endpoint() // flags endpoint to be dropped
>> hcd->driver->check_bandwidth()
>> ->xhci_check_bandwidth()
>> xhci_configure_endpoint()
>> xhci_reserve_bandwidth() // only for Panther Point
>> xhci_drop_ep_from_interval_table()
>>
>> But to avoid queuing new commands to a host in XHCI_STATE_DYING or
>> XHCI_STATE_REMOVING state we return early, not calling xhci_reserve_bandwidth().
>
> Indeed when I remove that early return in xhci_check_bandwidth(), the
> crash is gone. What's the proper solution?
>

We could probably just delete the endpoint from the bw list when freeing the device and
endpoints. Currently we just print that "endpoint x not removed from BW list!" message

does the below help?

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 9e56aa28efcd..2adc0c2b470c 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -894,10 +894,12 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id)
* We can't drop them anyway, because the udev might have gone
* away by this point, and we can't tell what speed it was.
*/
- if (!list_empty(&dev->eps[i].bw_endpoint_list))
+ if (!list_empty(&dev->eps[i].bw_endpoint_list)) {
+ list_del_init(&dev->eps[i].bw_endpoint_list);
xhci_warn(xhci, "Slot %u endpoint %u "
"not removed from BW list!\n",
slot_id, i);
+ }
}
/* If this is a hub, free the TT(s) from the TT list */
xhci_free_tt_info(xhci, dev, slot_id);

Thanks
-Mathias