This commit fixes the following oops:
[10238.622067] scsi host3: uas_eh_bus_reset_handler start
[10240.766164] usb 3-4: reset SuperSpeed USB device number 3 using xhci_hcd
[10245.779365] usb 3-4: device descriptor read/8, error -110
[10245.883331] usb 3-4: reset SuperSpeed USB device number 3 using xhci_hcd
[10250.897603] usb 3-4: device descriptor read/8, error -110
[10251.058200] BUG: unable to handle kernel NULL pointer dereference at
0000000000000040
[10251.058244] IP: [<ffffffff815ac6e1>] xhci_check_streams_endpoint+0x91/0x140
<snip>
[10251.059473] Call Trace:
[10251.059487] [<ffffffff815aca6c>]
xhci_calculate_streams_and_bitmask+0xbc/0x130
[10251.059520] [<ffffffff815aeb5f>] xhci_alloc_streams+0x10f/0x5a0
[10251.059548] [<ffffffff810a4685>] ? check_preempt_curr+0x75/0xa0
[10251.059575] [<ffffffff810a46dc>] ? ttwu_do_wakeup+0x2c/0x100
[10251.059601] [<ffffffff810a49e6>] ? ttwu_do_activate.constprop.111+0x66/0x70
[10251.059635] [<ffffffff815779ab>] usb_alloc_streams+0xab/0xf0
[10251.059662] [<ffffffffc0616b48>] uas_configure_endpoints+0x128/0x150 [uas]
[10251.059694] [<ffffffffc0616bac>] uas_post_reset+0x3c/0xb0 [uas]
[10251.059722] [<ffffffff815727d9>] usb_reset_device+0x1b9/0x2a0
[10251.059749] [<ffffffffc0616f42>] uas_eh_bus_reset_handler+0xb2/0x190 [uas]
[10251.059781] [<ffffffff81514293>] scsi_try_bus_reset+0x53/0x110
[10251.059808] [<ffffffff815163b7>] scsi_eh_bus_reset+0xf7/0x270
<snip>
The problem is the following call sequence (simplified):
1) usb_reset_device
2) usb_reset_and_verify_device
2) hub_port_init
3) hub_port_finish_reset
3) xhci_discover_or_reset_device
This frees xhci->devs[slot_id]->eps[ep_index].ring for all eps but 0
4) usb_get_device_descriptor
This fails
5) hub_port_init fails
6) usb_reset_and_verify_device fails, does not restore device config
7) uas_post_reset
8) xhci_alloc_streams
NULL deref on the free-ed ring
The real problem here is that xhci_discover_or_reset_device frees the rings
during a reset, and if usb_reset_and_verify_device then fails to restore the
device configuration (and thus re-alloc the rings), that we're then left with
a device which from the usb_device_driver's pov is still functional (its
disconnect method will get called when khub gets around to it), but in
reality is not.
This commit adds a check for this condition to xhci_check_args(), which should
cover all public entry points into the xhci driver.
Cc: [email protected]
Reported-by: Claudio Bizzarri <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
---
drivers/usb/host/xhci.c | 37 ++++++++++++++++++++++++++-----------
1 file changed, 26 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 642791c..64160ff 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1146,10 +1146,12 @@ unsigned int xhci_last_valid_endpoint(u32 added_ctxs)
* returns 0 this is a root hub; returns -EINVAL for NULL pointers.
*/
static int xhci_check_args(struct usb_hcd *hcd, struct usb_device *udev,
- struct usb_host_endpoint *ep, int check_ep, bool check_virt_dev,
- const char *func) {
+ struct usb_host_endpoint *ep, bool check_ep,
+ bool check_ep_ring, bool check_virt_dev, const char *func)
+{
struct xhci_hcd *xhci;
- struct xhci_virt_device *virt_dev;
+ struct xhci_virt_device *virt_dev = NULL;
+ unsigned int ep_index;
if (!hcd || (check_ep && !ep) || !udev) {
pr_debug("xHCI %s called with invalid args\n", func);
@@ -1176,6 +1178,18 @@ static int xhci_check_args(struct usb_hcd *hcd, struct
usb_device *udev,
}
}
+ /*
+ * If we get called after a reset, then usb_reset_device ->
+ * xhci_discover_or_reset_device may have free-ed eps[ep_index].ring,
+ * without it being setup again because of the usb_reset_device
+ * failing to re-configure the device.
+ */
+ if (check_ep_ring) {
+ ep_index = xhci_get_endpoint_index(&ep->desc);
+ if (virt_dev->eps[ep_index].ring == NULL)
+ return -ENODEV;
+ }
+
if (xhci->xhc_state & XHCI_STATE_HALTED)
return -ENODEV;
@@ -1281,7 +1295,7 @@ int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb
*urb, gfp_t mem_flags)
int size, i;
if (!urb || xhci_check_args(hcd, urb->dev, urb->ep,
- true, true, __func__) <= 0)
+ true, true, true, __func__) <= 0)
return -EINVAL;
slot_id = urb->dev->slot_id;
@@ -1596,7 +1610,7 @@ int xhci_drop_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
u32 new_add_flags, new_drop_flags;
int ret;
- ret = xhci_check_args(hcd, udev, ep, 1, true, __func__);
+ ret = xhci_check_args(hcd, udev, ep, true, false, true, __func__);
if (ret <= 0)
return ret;
xhci = hcd_to_xhci(hcd);
@@ -1675,7 +1689,7 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
struct xhci_virt_device *virt_dev;
int ret = 0;
- ret = xhci_check_args(hcd, udev, ep, 1, true, __func__);
+ ret = xhci_check_args(hcd, udev, ep, true, false, true, __func__);
if (ret <= 0) {
/* So we won't queue a reset ep command for a root hub */
ep->hcpriv = NULL;
@@ -2694,7 +2708,7 @@ int xhci_check_bandwidth(struct usb_hcd *hcd, struct
usb_device *udev)
struct xhci_slot_ctx *slot_ctx;
struct xhci_command *command;
- ret = xhci_check_args(hcd, udev, NULL, 0, true, __func__);
+ ret = xhci_check_args(hcd, udev, NULL, false, false, true, __func__);
if (ret <= 0)
return ret;
xhci = hcd_to_xhci(hcd);
@@ -2793,7 +2807,7 @@ void xhci_reset_bandwidth(struct usb_hcd *hcd, struct
usb_device *udev)
struct xhci_virt_device *virt_dev;
int i, ret;
- ret = xhci_check_args(hcd, udev, NULL, 0, true, __func__);
+ ret = xhci_check_args(hcd, udev, NULL, false, false, true, __func__);
if (ret <= 0)
return;
xhci = hcd_to_xhci(hcd);
@@ -2979,7 +2993,8 @@ static int xhci_check_streams_endpoint(struct xhci_hcd
*xhci,
if (!ep)
return -EINVAL;
- ret = xhci_check_args(xhci_to_hcd(xhci), udev, ep, 1, true, __func__);
+ ret = xhci_check_args(xhci_to_hcd(xhci), udev, ep,
+ true, true, true, __func__);
if (ret <= 0)
return -EINVAL;
if (usb_ss_max_streams(&ep->ss_ep_comp) == 0) {
@@ -3429,7 +3444,7 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd,
struct usb_device *udev)
struct xhci_slot_ctx *slot_ctx;
int old_active_eps = 0;
- ret = xhci_check_args(hcd, udev, NULL, 0, false, __func__);
+ ret = xhci_check_args(hcd, udev, NULL, false, false, false, __func__);
if (ret <= 0)
return ret;
xhci = hcd_to_xhci(hcd);
@@ -3600,7 +3615,7 @@ void xhci_free_dev(struct usb_hcd *hcd, struct usb_device
*udev)
pm_runtime_put_noidle(hcd->self.controller);
#endif
- ret = xhci_check_args(hcd, udev, NULL, 0, true, __func__);
+ ret = xhci_check_args(hcd, udev, NULL, false, false, true, __func__);
/* If the host is halted due to driver unload, we still need to free the
* device.
*/
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
