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

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 2829 bytes --]

On Mon, Oct 17, 2022 at 07:12:36PM +0300, Mathias Nyman wrote:
> 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?

Yes, this helps!

xhci_drop_ep_from_interval_table() does few more things, but I assume
this all doesn't matter at the xhci_free_virt_device() time, right?

> 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

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]