From 35773dac5f862cb1c82ea151eba3e2f6de51ec3e Mon Sep 17 00:00:00 2001 From: David Laight Date: Mon, 11 Nov 2013 12:26:54 +0000 Subject: usb: xhci: Link TRB must not occur within a USB payload burst Section 4.11.7.1 of rev 1.0 of the xhci specification states that a link TRB can only occur at a boundary between underlying USB frames (512 bytes for high speed devices). If this isn't done the USB frames aren't formatted correctly and, for example, the USB3 ethernet ax88179_178a card will stop sending (while still receiving) when running a netperf tcp transmit test with (say) and 8k buffer. This should be a candidate for stable, the ax88179_178a driver defaults to gso and tso enabled so it passes a lot of fragmented skb to the USB stack. Notes from Sarah: Discussion: http://marc.info/?l=linux-usb&m=138384509604981&w=2 This patch fixes a long-standing xHCI driver bug that was revealed by a change in 3.12 in the usb-net driver. Commit 638c5115a794981441246fa8fa5d95c1875af5ba "USBNET: support DMA SG" added support to use bulk endpoint scatter-gather (urb->sg). Only the USB ethernet drivers trigger this bug, because the mass storage driver sends sg list entries in page-sized chunks. This patch only fixes the issue for bulk endpoint scatter-gather. The problem will still occur for periodic endpoints, because hosts will interpret no-op transfers as a request to skip a service interval, which is not what we want. Luckily, the USB core isn't set up for scatter-gather on isochronous endpoints, and no USB drivers use scatter-gather for interrupt endpoints. Document this known limitation so that developers won't try to use urb->sg for interrupt endpoints until this issue is fixed. The more comprehensive fix would be to allow link TRBs in the middle of the endpoint ring and revert this patch, but that fix would touch too much code to be allowed in for stable. This patch should be backported to kernels as old as 3.12, that contain the commit 638c5115a794981441246fa8fa5d95c1875af5ba "USBNET: support DMA SG". Without this patch, the USB network device gets wedged, and stops sending packets. Mark Lord confirms this patch fixes the regression: http://marc.info/?l=linux-netdev&m=138487107625966&w=2 Signed-off-by: David Laight Signed-off-by: Sarah Sharp Tested-by: Mark Lord Cc: stable@vger.kernel.org --- include/linux/usb.h | 2 ++ 1 file changed, 2 insertions(+) (limited to 'include') diff --git a/include/linux/usb.h b/include/linux/usb.h index 7454865ad148..512ab162832c 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -1264,6 +1264,8 @@ typedef void (*usb_complete_t)(struct urb *); * @sg: scatter gather buffer list, the buffer size of each element in * the list (except the last) must be divisible by the endpoint's * max packet size if no_sg_constraint isn't set in 'struct usb_bus' + * (FIXME: scatter-gather under xHCI is broken for periodic transfers. + * Do not use urb->sg for interrupt endpoints for now, only bulk.) * @num_mapped_sgs: (internal) number of mapped sg entries * @num_sgs: number of entries in the sg list * @transfer_buffer_length: How big is transfer_buffer. The transfer may -- cgit v1.2.3-70-g09d2 From 471e42ad148c05d091219096726d751684ebf918 Mon Sep 17 00:00:00 2001 From: Thomas Pugliese Date: Mon, 2 Dec 2013 15:39:45 -0600 Subject: usb: wusbcore: fix deadlock in wusbhc_gtk_rekey When multiple wireless USB devices are connected and one of the devices disconnects, the host will distribute a new group key to the remaining devicese using wusbhc_gtk_rekey. wusbhc_gtk_rekey takes the wusbhc->mutex and holds it while it submits a URB to set the new key. This causes a deadlock in wa_urb_enqueue when it calls a device lookup helper function that takes the same lock. This patch changes wusbhc_gtk_rekey to submit a work item to set the GTK so that the URB is submitted without holding wusbhc->mutex. Signed-off-by: Thomas Pugliese Signed-off-by: Greg Kroah-Hartman --- drivers/usb/wusbcore/devconnect.c | 24 +--------- drivers/usb/wusbcore/security.c | 98 +++++++++++++++++++++++---------------- drivers/usb/wusbcore/wusbhc.h | 6 +-- include/linux/usb/wusb.h | 2 + 4 files changed, 62 insertions(+), 68 deletions(-) (limited to 'include') diff --git a/drivers/usb/wusbcore/devconnect.c b/drivers/usb/wusbcore/devconnect.c index 5107ca999d4c..f14e7929ba22 100644 --- a/drivers/usb/wusbcore/devconnect.c +++ b/drivers/usb/wusbcore/devconnect.c @@ -97,18 +97,12 @@ static void wusbhc_devconnect_acked_work(struct work_struct *work); static void wusb_dev_free(struct wusb_dev *wusb_dev) { - if (wusb_dev) { - kfree(wusb_dev->set_gtk_req); - usb_free_urb(wusb_dev->set_gtk_urb); - kfree(wusb_dev); - } + kfree(wusb_dev); } static struct wusb_dev *wusb_dev_alloc(struct wusbhc *wusbhc) { struct wusb_dev *wusb_dev; - struct urb *urb; - struct usb_ctrlrequest *req; wusb_dev = kzalloc(sizeof(*wusb_dev), GFP_KERNEL); if (wusb_dev == NULL) @@ -118,22 +112,6 @@ static struct wusb_dev *wusb_dev_alloc(struct wusbhc *wusbhc) INIT_WORK(&wusb_dev->devconnect_acked_work, wusbhc_devconnect_acked_work); - urb = usb_alloc_urb(0, GFP_KERNEL); - if (urb == NULL) - goto err; - wusb_dev->set_gtk_urb = urb; - - req = kmalloc(sizeof(*req), GFP_KERNEL); - if (req == NULL) - goto err; - wusb_dev->set_gtk_req = req; - - req->bRequestType = USB_DIR_OUT | USB_TYPE_STANDARD | USB_RECIP_DEVICE; - req->bRequest = USB_REQ_SET_DESCRIPTOR; - req->wValue = cpu_to_le16(USB_DT_KEY << 8 | wusbhc->gtk_index); - req->wIndex = 0; - req->wLength = cpu_to_le16(wusbhc->gtk.descr.bLength); - return wusb_dev; err: wusb_dev_free(wusb_dev); diff --git a/drivers/usb/wusbcore/security.c b/drivers/usb/wusbcore/security.c index dd88441c8f78..4c40d0dbf53d 100644 --- a/drivers/usb/wusbcore/security.c +++ b/drivers/usb/wusbcore/security.c @@ -29,19 +29,16 @@ #include #include "wusbhc.h" -static void wusbhc_set_gtk_callback(struct urb *urb); -static void wusbhc_gtk_rekey_done_work(struct work_struct *work); +static void wusbhc_gtk_rekey_work(struct work_struct *work); int wusbhc_sec_create(struct wusbhc *wusbhc) { wusbhc->gtk.descr.bLength = sizeof(wusbhc->gtk.descr) + sizeof(wusbhc->gtk.data); wusbhc->gtk.descr.bDescriptorType = USB_DT_KEY; wusbhc->gtk.descr.bReserved = 0; + wusbhc->gtk_index = 0; - wusbhc->gtk_index = wusb_key_index(0, WUSB_KEY_INDEX_TYPE_GTK, - WUSB_KEY_INDEX_ORIGINATOR_HOST); - - INIT_WORK(&wusbhc->gtk_rekey_done_work, wusbhc_gtk_rekey_done_work); + INIT_WORK(&wusbhc->gtk_rekey_work, wusbhc_gtk_rekey_work); return 0; } @@ -113,7 +110,7 @@ int wusbhc_sec_start(struct wusbhc *wusbhc) wusbhc_generate_gtk(wusbhc); result = wusbhc->set_gtk(wusbhc, wusbhc->gtk_tkid, - &wusbhc->gtk.descr.bKeyData, key_size); + &wusbhc->gtk.descr.bKeyData, key_size); if (result < 0) dev_err(wusbhc->dev, "cannot set GTK for the host: %d\n", result); @@ -129,7 +126,7 @@ int wusbhc_sec_start(struct wusbhc *wusbhc) */ void wusbhc_sec_stop(struct wusbhc *wusbhc) { - cancel_work_sync(&wusbhc->gtk_rekey_done_work); + cancel_work_sync(&wusbhc->gtk_rekey_work); } @@ -185,12 +182,14 @@ static int wusb_dev_set_encryption(struct usb_device *usb_dev, int value) static int wusb_dev_set_gtk(struct wusbhc *wusbhc, struct wusb_dev *wusb_dev) { struct usb_device *usb_dev = wusb_dev->usb_dev; + u8 key_index = wusb_key_index(wusbhc->gtk_index, + WUSB_KEY_INDEX_TYPE_GTK, WUSB_KEY_INDEX_ORIGINATOR_HOST); return usb_control_msg( usb_dev, usb_sndctrlpipe(usb_dev, 0), USB_REQ_SET_DESCRIPTOR, USB_DIR_OUT | USB_TYPE_STANDARD | USB_RECIP_DEVICE, - USB_DT_KEY << 8 | wusbhc->gtk_index, 0, + USB_DT_KEY << 8 | key_index, 0, &wusbhc->gtk.descr, wusbhc->gtk.descr.bLength, 1000); } @@ -520,24 +519,55 @@ error_kzalloc: * Once all connected and authenticated devices have received the new * GTK, switch the host to using it. */ -static void wusbhc_gtk_rekey_done_work(struct work_struct *work) +static void wusbhc_gtk_rekey_work(struct work_struct *work) { - struct wusbhc *wusbhc = container_of(work, struct wusbhc, gtk_rekey_done_work); + struct wusbhc *wusbhc = container_of(work, + struct wusbhc, gtk_rekey_work); size_t key_size = sizeof(wusbhc->gtk.data); + int port_idx; + struct wusb_dev *wusb_dev, *wusb_dev_next; + LIST_HEAD(rekey_list); mutex_lock(&wusbhc->mutex); + /* generate the new key */ + wusbhc_generate_gtk(wusbhc); + /* roll the gtk index. */ + wusbhc->gtk_index = (wusbhc->gtk_index + 1) % (WUSB_KEY_INDEX_MAX + 1); + /* + * Save all connected devices on a list while holding wusbhc->mutex and + * take a reference to each one. Then submit the set key request to + * them after releasing the lock in order to avoid a deadlock. + */ + for (port_idx = 0; port_idx < wusbhc->ports_max; port_idx++) { + wusb_dev = wusbhc->port[port_idx].wusb_dev; + if (!wusb_dev || !wusb_dev->usb_dev + || !wusb_dev->usb_dev->authenticated) + continue; - if (--wusbhc->pending_set_gtks == 0) - wusbhc->set_gtk(wusbhc, wusbhc->gtk_tkid, &wusbhc->gtk.descr.bKeyData, key_size); - + wusb_dev_get(wusb_dev); + list_add_tail(&wusb_dev->rekey_node, &rekey_list); + } mutex_unlock(&wusbhc->mutex); -} -static void wusbhc_set_gtk_callback(struct urb *urb) -{ - struct wusbhc *wusbhc = urb->context; + /* Submit the rekey requests without holding wusbhc->mutex. */ + list_for_each_entry_safe(wusb_dev, wusb_dev_next, &rekey_list, + rekey_node) { + list_del_init(&wusb_dev->rekey_node); + dev_dbg(&wusb_dev->usb_dev->dev, "%s: rekey device at port %d\n", + __func__, wusb_dev->port_idx); + + if (wusb_dev_set_gtk(wusbhc, wusb_dev) < 0) { + dev_err(&wusb_dev->usb_dev->dev, "%s: rekey device at port %d failed\n", + __func__, wusb_dev->port_idx); + } + wusb_dev_put(wusb_dev); + } - queue_work(wusbd, &wusbhc->gtk_rekey_done_work); + /* Switch the host controller to use the new GTK. */ + mutex_lock(&wusbhc->mutex); + wusbhc->set_gtk(wusbhc, wusbhc->gtk_tkid, + &wusbhc->gtk.descr.bKeyData, key_size); + mutex_unlock(&wusbhc->mutex); } /** @@ -553,26 +583,12 @@ static void wusbhc_set_gtk_callback(struct urb *urb) */ void wusbhc_gtk_rekey(struct wusbhc *wusbhc) { - static const size_t key_size = sizeof(wusbhc->gtk.data); - int p; - - wusbhc_generate_gtk(wusbhc); - - for (p = 0; p < wusbhc->ports_max; p++) { - struct wusb_dev *wusb_dev; - - wusb_dev = wusbhc->port[p].wusb_dev; - if (!wusb_dev || !wusb_dev->usb_dev || !wusb_dev->usb_dev->authenticated) - continue; - - usb_fill_control_urb(wusb_dev->set_gtk_urb, wusb_dev->usb_dev, - usb_sndctrlpipe(wusb_dev->usb_dev, 0), - (void *)wusb_dev->set_gtk_req, - &wusbhc->gtk.descr, wusbhc->gtk.descr.bLength, - wusbhc_set_gtk_callback, wusbhc); - if (usb_submit_urb(wusb_dev->set_gtk_urb, GFP_KERNEL) == 0) - wusbhc->pending_set_gtks++; - } - if (wusbhc->pending_set_gtks == 0) - wusbhc->set_gtk(wusbhc, wusbhc->gtk_tkid, &wusbhc->gtk.descr.bKeyData, key_size); + /* + * We need to submit a URB to the downstream WUSB devices in order to + * change the group key. This can't be done while holding the + * wusbhc->mutex since that is also taken in the urb_enqueue routine + * and will cause a deadlock. Instead, queue a work item to do + * it when the lock is not held + */ + queue_work(wusbd, &wusbhc->gtk_rekey_work); } diff --git a/drivers/usb/wusbcore/wusbhc.h b/drivers/usb/wusbcore/wusbhc.h index 711b1952b114..6bd3b819a6b5 100644 --- a/drivers/usb/wusbcore/wusbhc.h +++ b/drivers/usb/wusbcore/wusbhc.h @@ -97,6 +97,7 @@ struct wusb_dev { struct kref refcnt; struct wusbhc *wusbhc; struct list_head cack_node; /* Connect-Ack list */ + struct list_head rekey_node; /* GTK rekey list */ u8 port_idx; u8 addr; u8 beacon_type:4; @@ -107,8 +108,6 @@ struct wusb_dev { struct usb_wireless_cap_descriptor *wusb_cap_descr; struct uwb_mas_bm availability; struct work_struct devconnect_acked_work; - struct urb *set_gtk_urb; - struct usb_ctrlrequest *set_gtk_req; struct usb_device *usb_dev; }; @@ -296,8 +295,7 @@ struct wusbhc { } __attribute__((packed)) gtk; u8 gtk_index; u32 gtk_tkid; - struct work_struct gtk_rekey_done_work; - int pending_set_gtks; + struct work_struct gtk_rekey_work; struct usb_encryption_descriptor *ccm1_etd; }; diff --git a/include/linux/usb/wusb.h b/include/linux/usb/wusb.h index 0c4d4ca370ec..eeb28329fa3c 100644 --- a/include/linux/usb/wusb.h +++ b/include/linux/usb/wusb.h @@ -271,6 +271,8 @@ static inline u8 wusb_key_index(int index, int type, int originator) #define WUSB_KEY_INDEX_TYPE_GTK 2 #define WUSB_KEY_INDEX_ORIGINATOR_HOST 0 #define WUSB_KEY_INDEX_ORIGINATOR_DEVICE 1 +/* bits 0-3 used for the key index. */ +#define WUSB_KEY_INDEX_MAX 15 /* A CCM Nonce, defined in WUSB1.0[6.4.1] */ struct aes_ccm_nonce { -- cgit v1.2.3-70-g09d2