[RFC PATCH] xhci: Fix memory use after free in xhci_free_virt_device

[RFC PATCH] xhci: Fix memory use after free in xhci_free_virt_device

[Guenter Roeck]

The following use-after-free reports were seen on resume with a specific
USB hub.

BUG: KASAN: use-after-free in xhci_free_virt_device+0x8c/0x21c
	at addr ffffffc0cc1a2eb0
BUG: KASAN: use-after-free in xhci_update_tt_active_eps+0x9c/0xdc
	at addr ffffffc0cc1a2eb0

Relevant traceback for the first case is:

xhci_free_virt_device+0x8c/0x21c
xhci_mem_cleanup+0x294/0x81c
xhci_resume+0x410/0x618
xhci_plat_resume+0x54/0x74
platform_pm_resume+0x74/0x84

which points to the following code in xhci_free_virt_device().

	if (dev->tt_info)
                old_active_eps = dev->tt_info->active_eps;

Problem with this code is that xhci_mem_cleanup() cleans up devices
starting with slot 1, and dev->tt_info for a device with higher slot
number can point back to the tt_info associated with device 1.
In lsusb, this looks as follows.

/:  Bus 05.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M
    |__ Port 1: Dev 4, If 0, Class=Hub, Driver=hub/4p, 480M
        |__ Port 3: Dev 7, If 0, Class=Vendor Specific Class, Driver=, 12M

When the higher-numbered device is cleared, it tries to access the already
released tt_info from slot 1 to get the value of old_active_eps.

The problem is not seen with all USB hubs since not all USB hubs require
the cleanup handling in xhci_resume().

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
Marked as RFC because I don't really like this fix at all and would prefer
a different solution.

It might be possible, for example, to change the code in xhci_mem_cleanup()
from
	for (i = 1; i < MAX_HC_SLOTS; ++i)
		xhci_free_virt_device(xhci, i);
to
	for (i = MAX_HC_SLOTS - 1; i >= 1; i--)
		xhci_free_virt_device(xhci, i);
if it is guaranteed that tt_info never points to a device with higher slot
number.

 drivers/usb/host/xhci-mem.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 6afe32381209..97a61c57a2ed 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -873,8 +873,14 @@ static void xhci_free_tt_info(struct xhci_hcd *xhci,
 	list_for_each_entry_safe(tt_info, next, tt_list_head, tt_list) {
 		/* Multi-TT hubs will have more than one entry */
 		if (tt_info->slot_id == slot_id) {
+			int i;
+
 			slot_found = true;
 			list_del(&tt_info->tt_list);
+			for (i = 0; i < MAX_HC_SLOTS; i++)
+				if (xhci->devs[i] &&
+				    xhci->devs[i]->tt_info == tt_info)
+					xhci->devs[i]->tt_info = NULL;
 			kfree(tt_info);
 		} else if (slot_found) {
 			break;
-- 
2.5.0

Re: [RFC PATCH] xhci: Fix memory use after free in xhci_free_virt_device

[Mathias Nyman]

On 15.11.2016 22:36, Guenter Roeck wrote:
> The following use-after-free reports were seen on resume with a specific
> USB hub.
>
> BUG: KASAN: use-after-free in xhci_free_virt_device+0x8c/0x21c
> 	at addr ffffffc0cc1a2eb0
> BUG: KASAN: use-after-free in xhci_update_tt_active_eps+0x9c/0xdc
> 	at addr ffffffc0cc1a2eb0
>
> Relevant traceback for the first case is:
>
> xhci_free_virt_device+0x8c/0x21c
> xhci_mem_cleanup+0x294/0x81c
> xhci_resume+0x410/0x618
> xhci_plat_resume+0x54/0x74
> platform_pm_resume+0x74/0x84
>
> which points to the following code in xhci_free_virt_device().
>
> 	if (dev->tt_info)
>                  old_active_eps = dev->tt_info->active_eps;
>
> Problem with this code is that xhci_mem_cleanup() cleans up devices
> starting with slot 1, and dev->tt_info for a device with higher slot
> number can point back to the tt_info associated with device 1.
> In lsusb, this looks as follows.
>
> /:  Bus 05.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M
>      |__ Port 1: Dev 4, If 0, Class=Hub, Driver=hub/4p, 480M
>          |__ Port 3: Dev 7, If 0, Class=Vendor Specific Class, Driver=, 12M
>
> When the higher-numbered device is cleared, it tries to access the already
> released tt_info from slot 1 to get the value of old_active_eps.
>
> The problem is not seen with all USB hubs since not all USB hubs require
> the cleanup handling in xhci_resume().
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---

Thanks, nice catch.

> Marked as RFC because I don't really like this fix at all and would prefer
> a different solution.
>

I just started digging into this and see if I can come up with some solution.
It's currently a bit messy the whole thing how the tt_info and bw_table are
allocated, freed, pointed to and added and removed to/from lists

-Mathias

[RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Mathias Nyman]

the tt_info provided by a HS hub might be in use to by a child device
Make sure we free the devices in the correct order.

This is needed in special cases such as when xhci controller is
reset when resuming from hibernate, and all virt_devices are freed.

Also free the virt_devices starting from max slot_id as children
more commonly have higher slot_id than parent.

CC: <stable@vger.kernel.org>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>

---

Guenter Roeck, does this work for you?

A rework of how tt_info is stored and used might be needed,
but that will take some time and won't go to stable as easily.
---
 drivers/usb/host/xhci-mem.c | 38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 6afe323..b3a5cd8 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -979,6 +979,40 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id)
 	xhci->devs[slot_id] = NULL;
 }
 
+/*
+ * Free a virt_device structure.
+ * If the virt_device added a tt_info (a hub) and has children pointing to
+ * that tt_info, then free the child first. Recursive.
+ * We can't rely on udev at this point to find child-parent relationships.
+ */
+void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id)
+{
+	struct xhci_virt_device *vdev;
+	struct list_head *tt_list_head;
+	struct xhci_tt_bw_info *tt_info, *next;
+	int i;
+
+	vdev = xhci->devs[slot_id];
+	if (!vdev)
+		return;
+
+	tt_list_head = &(xhci->rh_bw[vdev->real_port - 1].tts);
+	list_for_each_entry_safe(tt_info, next, tt_list_head, tt_list) {
+		/* is this a hub device that added a tt_info to the tts list */
+		if (tt_info->slot_id == slot_id) {
+			/* are any devices using this tt_info? */
+			for (i = 1; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
+				vdev = xhci->devs[i];
+				if (vdev && (vdev->tt_info == tt_info))
+					xhci_free_virt_devices_depth_first(
+						xhci, i);
+			}
+		}
+	}
+	/* we are now at a leaf device */
+	xhci_free_virt_device(xhci, slot_id);
+}
+
 int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
 		struct usb_device *udev, gfp_t flags)
 {
@@ -1829,8 +1863,8 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
 		}
 	}
 
-	for (i = 1; i < MAX_HC_SLOTS; ++i)
-		xhci_free_virt_device(xhci, i);
+	for (i = HCS_MAX_SLOTS(xhci->hcs_params1); i > 0; i--)
+		xhci_free_virt_devices_depth_first(xhci, i);
 
 	dma_pool_destroy(xhci->segment_pool);
 	xhci->segment_pool = NULL;
-- 
1.9.1

Re: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Guenter Roeck]

On 11/23/2016 04:24 AM, Mathias Nyman wrote:
> the tt_info provided by a HS hub might be in use to by a child device
> Make sure we free the devices in the correct order.
>
> This is needed in special cases such as when xhci controller is
> reset when resuming from hibernate, and all virt_devices are freed.
>
> Also free the virt_devices starting from max slot_id as children
> more commonly have higher slot_id than parent.
>
> CC: <stable@vger.kernel.org>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>
> ---
>
> Guenter Roeck, does this work for you?
>
> A rework of how tt_info is stored and used might be needed,
> but that will take some time and won't go to stable as easily.

I'll give it a try. One concern, though: xhci_free_virt_device() is called
from multiple places, and does not always remove all devices. Is it save
to assume that all other callers remove children first ?

Thanks,
Guenter

> ---
>  drivers/usb/host/xhci-mem.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 6afe323..b3a5cd8 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -979,6 +979,40 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id)
>  	xhci->devs[slot_id] = NULL;
>  }
>
> +/*
> + * Free a virt_device structure.
> + * If the virt_device added a tt_info (a hub) and has children pointing to
> + * that tt_info, then free the child first. Recursive.
> + * We can't rely on udev at this point to find child-parent relationships.
> + */
> +void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id)
> +{
> +	struct xhci_virt_device *vdev;
> +	struct list_head *tt_list_head;
> +	struct xhci_tt_bw_info *tt_info, *next;
> +	int i;
> +
> +	vdev = xhci->devs[slot_id];
> +	if (!vdev)
> +		return;
> +
> +	tt_list_head = &(xhci->rh_bw[vdev->real_port - 1].tts);
> +	list_for_each_entry_safe(tt_info, next, tt_list_head, tt_list) {
> +		/* is this a hub device that added a tt_info to the tts list */
> +		if (tt_info->slot_id == slot_id) {
> +			/* are any devices using this tt_info? */
> +			for (i = 1; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
> +				vdev = xhci->devs[i];
> +				if (vdev && (vdev->tt_info == tt_info))
> +					xhci_free_virt_devices_depth_first(
> +						xhci, i);
> +			}
> +		}
> +	}
> +	/* we are now at a leaf device */
> +	xhci_free_virt_device(xhci, slot_id);
> +}
> +
>  int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
>  		struct usb_device *udev, gfp_t flags)
>  {
> @@ -1829,8 +1863,8 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>  		}
>  	}
>
> -	for (i = 1; i < MAX_HC_SLOTS; ++i)
> -		xhci_free_virt_device(xhci, i);
> +	for (i = HCS_MAX_SLOTS(xhci->hcs_params1); i > 0; i--)
> +		xhci_free_virt_devices_depth_first(xhci, i);
>
>  	dma_pool_destroy(xhci->segment_pool);
>  	xhci->segment_pool = NULL;
>

Re: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Mathias Nyman]

On 23.11.2016 15:32, Guenter Roeck wrote:
> On 11/23/2016 04:24 AM, Mathias Nyman wrote:
>> the tt_info provided by a HS hub might be in use to by a child device
>> Make sure we free the devices in the correct order.
>>
>> This is needed in special cases such as when xhci controller is
>> reset when resuming from hibernate, and all virt_devices are freed.
>>
>> Also free the virt_devices starting from max slot_id as children
>> more commonly have higher slot_id than parent.
>>
>> CC: <stable@vger.kernel.org>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>>
>> ---
>>
>> Guenter Roeck, does this work for you?
>>
>> A rework of how tt_info is stored and used might be needed,
>> but that will take some time and won't go to stable as easily.
>
> I'll give it a try. One concern, though: xhci_free_virt_device() is called
> from multiple places, and does not always remove all devices. Is it save
> to assume that all other callers remove children first ?
>

This should be the only place that xhci does a massive xhci_free_virt_device(),

In the other places it's initiated per device by usb core which should handle
child-parent relationships, or then just error paths when failing to allocate
a device in the first place (no children yet)

-Mathias

Re: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Felipe Balbi]

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


Hi,

Mathias Nyman <mathias.nyman@linux.intel.com> writes:
> the tt_info provided by a HS hub might be in use to by a child device
> Make sure we free the devices in the correct order.
>
> This is needed in special cases such as when xhci controller is
> reset when resuming from hibernate, and all virt_devices are freed.
>
> Also free the virt_devices starting from max slot_id as children
> more commonly have higher slot_id than parent.
>
> CC: <stable@vger.kernel.org>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>
> ---
>
> Guenter Roeck, does this work for you?
>
> A rework of how tt_info is stored and used might be needed,
> but that will take some time and won't go to stable as easily.
> ---
>  drivers/usb/host/xhci-mem.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 6afe323..b3a5cd8 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -979,6 +979,40 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id)
>  	xhci->devs[slot_id] = NULL;
>  }
>  
> +/*
> + * Free a virt_device structure.
> + * If the virt_device added a tt_info (a hub) and has children pointing to
> + * that tt_info, then free the child first. Recursive.
> + * We can't rely on udev at this point to find child-parent relationships.
> + */
> +void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id)
> +{
> +	struct xhci_virt_device *vdev;
> +	struct list_head *tt_list_head;
> +	struct xhci_tt_bw_info *tt_info, *next;
> +	int i;
> +
> +	vdev = xhci->devs[slot_id];
> +	if (!vdev)
> +		return;
> +
> +	tt_list_head = &(xhci->rh_bw[vdev->real_port - 1].tts);
> +	list_for_each_entry_safe(tt_info, next, tt_list_head, tt_list) {
> +		/* is this a hub device that added a tt_info to the tts list */
> +		if (tt_info->slot_id == slot_id) {

		if (tt_info->slot_id != slot_id)
                	continue;

> +			/* are any devices using this tt_info? */
> +			for (i = 1; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) {

off-by-one here ? Why is i starting from 1?

> +				vdev = xhci->devs[i];
> +				if (vdev && (vdev->tt_info == tt_info))

			if (!vdev || vdev->tt_info != tt_info)
                        	continue;

> +					xhci_free_virt_devices_depth_first(
> +						xhci, i);
> +			}
> +		}
> +	}
> +	/* we are now at a leaf device */
> +	xhci_free_virt_device(xhci, slot_id);
> +}
> +
>  int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
>  		struct usb_device *udev, gfp_t flags)
>  {
> @@ -1829,8 +1863,8 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>  		}
>  	}
>  
> -	for (i = 1; i < MAX_HC_SLOTS; ++i)
> -		xhci_free_virt_device(xhci, i);
> +	for (i = HCS_MAX_SLOTS(xhci->hcs_params1); i > 0; i--)

converting MAX_HC_SLOTS to HCS_MAX_SLOTS(xhci->hcs_params1) seems
unrelated to $subject. Perhaps just mention on commit log?

-- 
balbi

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

Re: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Mathias Nyman]

On 24.11.2016 11:02, Felipe Balbi wrote:
>
> Hi,
>
> Mathias Nyman <mathias.nyman@linux.intel.com> writes:
>> the tt_info provided by a HS hub might be in use to by a child device
>> Make sure we free the devices in the correct order.
>>
>> This is needed in special cases such as when xhci controller is
>> reset when resuming from hibernate, and all virt_devices are freed.
>>
>> Also free the virt_devices starting from max slot_id as children
>> more commonly have higher slot_id than parent.
>>
>> CC: <stable@vger.kernel.org>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>>
>> ---
>>
>> Guenter Roeck, does this work for you?
>>
>> A rework of how tt_info is stored and used might be needed,
>> but that will take some time and won't go to stable as easily.
>> ---
>>   drivers/usb/host/xhci-mem.c | 38 ++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index 6afe323..b3a5cd8 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -979,6 +979,40 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id)
>>   	xhci->devs[slot_id] = NULL;
>>   }
>>
>> +/*
>> + * Free a virt_device structure.
>> + * If the virt_device added a tt_info (a hub) and has children pointing to
>> + * that tt_info, then free the child first. Recursive.
>> + * We can't rely on udev at this point to find child-parent relationships.
>> + */
>> +void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id)
>> +{
>> +	struct xhci_virt_device *vdev;
>> +	struct list_head *tt_list_head;
>> +	struct xhci_tt_bw_info *tt_info, *next;
>> +	int i;
>> +
>> +	vdev = xhci->devs[slot_id];
>> +	if (!vdev)
>> +		return;
>> +
>> +	tt_list_head = &(xhci->rh_bw[vdev->real_port - 1].tts);
>> +	list_for_each_entry_safe(tt_info, next, tt_list_head, tt_list) {
>> +		/* is this a hub device that added a tt_info to the tts list */
>> +		if (tt_info->slot_id == slot_id) {
>
> 		if (tt_info->slot_id != slot_id)
>                  	continue;
>
>> +			/* are any devices using this tt_info? */
>> +			for (i = 1; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
>
> off-by-one here ? Why is i starting from 1?
>
>> +				vdev = xhci->devs[i];

slit_id 0 is reserved and xhci->devs[0] is not used, so ne need to check it.

All other places that check xhci->devs[0] are avtually buggy

-Mathias  

Re: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Felipe Balbi]

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


Hi,

Mathias Nyman <mathias.nyman@linux.intel.com> writes:
>>> +			/* are any devices using this tt_info? */
>>> +			for (i = 1; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
>>
>> off-by-one here ? Why is i starting from 1?
>>
>>> +				vdev = xhci->devs[i];
>
> slit_id 0 is reserved and xhci->devs[0] is not used, so ne need to
> check it.

hmm... it's reserved for the HW, sure. Do you need to over allocate the
array by 1 just to keep this first member unused? Couldn't you handle
the +1/-1 (depending on the case) in xhci driver itself? Saves a bit of
memory there.

> All other places that check xhci->devs[0] are avtually buggy

fair enough, sounds like an accessor guaranteeing the 'rules of
engagement' for this would be useful.

-- 
balbi

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

Re: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Mathias Nyman]

On 24.11.2016 13:03, Felipe Balbi wrote:
>
> Hi,
>
> Mathias Nyman <mathias.nyman@linux.intel.com> writes:
>>>> +			/* are any devices using this tt_info? */
>>>> +			for (i = 1; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
>>>
>>> off-by-one here ? Why is i starting from 1?
>>>
>>>> +				vdev = xhci->devs[i];
>>
>> slit_id 0 is reserved and xhci->devs[0] is not used, so ne need to
>> check it.
>
> hmm... it's reserved for the HW, sure. Do you need to over allocate the
> array by 1 just to keep this first member unused? Couldn't you handle
> the +1/-1 (depending on the case) in xhci driver itself? Saves a bit of
> memory there.
>

There are many things that needs fixing in this area, but not in this patch

-Mathias

Re: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Guenter Roeck]

On 11/23/2016 04:24 AM, Mathias Nyman wrote:
> the tt_info provided by a HS hub might be in use to by a child device
> Make sure we free the devices in the correct order.
>
> This is needed in special cases such as when xhci controller is
> reset when resuming from hibernate, and all virt_devices are freed.
>
> Also free the virt_devices starting from max slot_id as children
> more commonly have higher slot_id than parent.
>
> CC: <stable@vger.kernel.org>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>
> ---
>
> Guenter Roeck, does this work for you?
>

Sorry, I didn't have time this week. I'll test it first thing next week.

Guenter

> A rework of how tt_info is stored and used might be needed,
> but that will take some time and won't go to stable as easily.
> ---
>  drivers/usb/host/xhci-mem.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 6afe323..b3a5cd8 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -979,6 +979,40 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id)
>  	xhci->devs[slot_id] = NULL;
>  }
>
> +/*
> + * Free a virt_device structure.
> + * If the virt_device added a tt_info (a hub) and has children pointing to
> + * that tt_info, then free the child first. Recursive.
> + * We can't rely on udev at this point to find child-parent relationships.
> + */
> +void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id)
> +{
> +	struct xhci_virt_device *vdev;
> +	struct list_head *tt_list_head;
> +	struct xhci_tt_bw_info *tt_info, *next;
> +	int i;
> +
> +	vdev = xhci->devs[slot_id];
> +	if (!vdev)
> +		return;
> +
> +	tt_list_head = &(xhci->rh_bw[vdev->real_port - 1].tts);
> +	list_for_each_entry_safe(tt_info, next, tt_list_head, tt_list) {
> +		/* is this a hub device that added a tt_info to the tts list */
> +		if (tt_info->slot_id == slot_id) {
> +			/* are any devices using this tt_info? */
> +			for (i = 1; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
> +				vdev = xhci->devs[i];
> +				if (vdev && (vdev->tt_info == tt_info))
> +					xhci_free_virt_devices_depth_first(
> +						xhci, i);
> +			}
> +		}
> +	}
> +	/* we are now at a leaf device */
> +	xhci_free_virt_device(xhci, slot_id);
> +}
> +
>  int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
>  		struct usb_device *udev, gfp_t flags)
>  {
> @@ -1829,8 +1863,8 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>  		}
>  	}
>
> -	for (i = 1; i < MAX_HC_SLOTS; ++i)
> -		xhci_free_virt_device(xhci, i);
> +	for (i = HCS_MAX_SLOTS(xhci->hcs_params1); i > 0; i--)
> +		xhci_free_virt_devices_depth_first(xhci, i);
>
>  	dma_pool_destroy(xhci->segment_pool);
>  	xhci->segment_pool = NULL;
>

Re: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Guenter Roeck]

On Wed, Nov 23, 2016 at 02:24:27PM +0200, Mathias Nyman wrote:
> the tt_info provided by a HS hub might be in use to by a child device
> Make sure we free the devices in the correct order.
> 
> This is needed in special cases such as when xhci controller is
> reset when resuming from hibernate, and all virt_devices are freed.
> 
> Also free the virt_devices starting from max slot_id as children
> more commonly have higher slot_id than parent.
> 
> CC: <stable@vger.kernel.org>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> 
> ---
> 
> Guenter Roeck, does this work for you?
> 
Yes, it does.

Tested-by: Guenter Roeck <groeck@chromium.org>

Thanks,
Guenter

> A rework of how tt_info is stored and used might be needed,
> but that will take some time and won't go to stable as easily.
> ---
>  drivers/usb/host/xhci-mem.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 6afe323..b3a5cd8 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -979,6 +979,40 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id)
>  	xhci->devs[slot_id] = NULL;
>  }
>  
> +/*
> + * Free a virt_device structure.
> + * If the virt_device added a tt_info (a hub) and has children pointing to
> + * that tt_info, then free the child first. Recursive.
> + * We can't rely on udev at this point to find child-parent relationships.
> + */
> +void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id)
> +{
> +	struct xhci_virt_device *vdev;
> +	struct list_head *tt_list_head;
> +	struct xhci_tt_bw_info *tt_info, *next;
> +	int i;
> +
> +	vdev = xhci->devs[slot_id];
> +	if (!vdev)
> +		return;
> +
> +	tt_list_head = &(xhci->rh_bw[vdev->real_port - 1].tts);
> +	list_for_each_entry_safe(tt_info, next, tt_list_head, tt_list) {
> +		/* is this a hub device that added a tt_info to the tts list */
> +		if (tt_info->slot_id == slot_id) {
> +			/* are any devices using this tt_info? */
> +			for (i = 1; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
> +				vdev = xhci->devs[i];
> +				if (vdev && (vdev->tt_info == tt_info))
> +					xhci_free_virt_devices_depth_first(
> +						xhci, i);
> +			}
> +		}
> +	}
> +	/* we are now at a leaf device */
> +	xhci_free_virt_device(xhci, slot_id);
> +}
> +
>  int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
>  		struct usb_device *udev, gfp_t flags)
>  {
> @@ -1829,8 +1863,8 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>  		}
>  	}
>  
> -	for (i = 1; i < MAX_HC_SLOTS; ++i)
> -		xhci_free_virt_device(xhci, i);
> +	for (i = HCS_MAX_SLOTS(xhci->hcs_params1); i > 0; i--)
> +		xhci_free_virt_devices_depth_first(xhci, i);
>  
>  	dma_pool_destroy(xhci->segment_pool);
>  	xhci->segment_pool = NULL;
> -- 
> 1.9.1
> 

Re: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Mathias Nyman]

On 28.11.2016 22:24, Guenter Roeck wrote:
> On Wed, Nov 23, 2016 at 02:24:27PM +0200, Mathias Nyman wrote:
>> the tt_info provided by a HS hub might be in use to by a child device
>> Make sure we free the devices in the correct order.
>>
>> This is needed in special cases such as when xhci controller is
>> reset when resuming from hibernate, and all virt_devices are freed.
>>
>> Also free the virt_devices starting from max slot_id as children
>> more commonly have higher slot_id than parent.
>>
>> CC: <stable@vger.kernel.org>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>>
>> ---
>>
>> Guenter Roeck, does this work for you?
>>
> Yes, it does.
>
> Tested-by: Guenter Roeck <groeck@chromium.org>
>
> Thanks,
> Guenter
>

Thanks, I'll send it forward with proper Reported-by and Tested-by tags
after 4.10-rc1

-Mathias

Re: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Guenter Roeck]

On Wed, Nov 30, 2016 at 01:41:24PM +0200, Mathias Nyman wrote:
> On 28.11.2016 22:24, Guenter Roeck wrote:
> >On Wed, Nov 23, 2016 at 02:24:27PM +0200, Mathias Nyman wrote:
> >>the tt_info provided by a HS hub might be in use to by a child device
> >>Make sure we free the devices in the correct order.
> >>
> >>This is needed in special cases such as when xhci controller is
> >>reset when resuming from hibernate, and all virt_devices are freed.
> >>
> >>Also free the virt_devices starting from max slot_id as children
> >>more commonly have higher slot_id than parent.
> >>
> >>CC: <stable@vger.kernel.org>
> >>Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> >>
> >>---
> >>
> >>Guenter Roeck, does this work for you?
> >>
> >Yes, it does.
> >
> >Tested-by: Guenter Roeck <groeck@chromium.org>
> >
> >Thanks,
> >Guenter
> >
> 
> Thanks, I'll send it forward with proper Reported-by and Tested-by tags
> after 4.10-rc1
> 
Do you have this patch sitting in some branch, by any chance ?
I would like to pick it up if possible. 

Thanks,
Guenter

Re: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Mathias Nyman]

On 09.12.2016 23:28, Guenter Roeck wrote:
> On Wed, Nov 30, 2016 at 01:41:24PM +0200, Mathias Nyman wrote:
>> On 28.11.2016 22:24, Guenter Roeck wrote:
>>> On Wed, Nov 23, 2016 at 02:24:27PM +0200, Mathias Nyman wrote:
>>>> the tt_info provided by a HS hub might be in use to by a child device
>>>> Make sure we free the devices in the correct order.
>>>>
>>>> This is needed in special cases such as when xhci controller is
>>>> reset when resuming from hibernate, and all virt_devices are freed.
>>>>
>>>> Also free the virt_devices starting from max slot_id as children
>>>> more commonly have higher slot_id than parent.
>>>>
>>>> CC: <stable@vger.kernel.org>
>>>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>>>>
>>>> ---
>>>>
>>>> Guenter Roeck, does this work for you?
>>>>
>>> Yes, it does.
>>>
>>> Tested-by: Guenter Roeck <groeck@chromium.org>
>>>
>>> Thanks,
>>> Guenter
>>>
>>
>> Thanks, I'll send it forward with proper Reported-by and Tested-by tags
>> after 4.10-rc1
>>
> Do you have this patch sitting in some branch, by any chance ?
> I would like to pick it up if possible.
>

Now in

git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git for-usb-linus

Branch is not very well tested yet

-Mathias