From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mathias Nyman Subject: Re: usb HC busted? Date: Fri, 20 Jul 2018 14:10:58 +0300 Message-ID: References: <20180717114104.irgdb5rmg2qxclgp@debian> <20180717144022.4wabhdhysori3mvg@debian> <20180717144918.7ymzbhijxfcc3fhb@debian> <20180717151017.yk5d4glzwqcrxpqc@debian> <20180719113455.urktqxijzoyhfoou@debian> <20180719173256.hc5aprma4biuqd4p@debian> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------9B3FAA6CDCAE530347B9BBF6" Return-path: In-Reply-To: <20180719173256.hc5aprma4biuqd4p@debian> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Sudip Mukherjee Cc: Mathias Nyman , Greg KH , linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, Christoph Hellwig , Andy Shevchenko , Alan Stern , Andy Shevchenko , lukaszx.szulc-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org List-Id: iommu@lists.linux-foundation.org This is a multi-part message in MIME format. --------------9B3FAA6CDCAE530347B9BBF6 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On 19.07.2018 20:32, Sudip Mukherjee wrote: > Hi Mathias, > > On Thu, Jul 19, 2018 at 06:42:19PM +0300, Mathias Nyman wrote: >>>> As first aid I could try to implement checks that make sure the flushed URBs >>>> trb pointers really are on the current endpoint ring, and also add some warning >>>> if we are we are dropping endpoints with URBs still queued. >>> >>> Yes, please. I think your first-aid will be a much better option than >>> the hacky patch I am using atm. >>> >> >> Attached a patch that checks canceled URB td/trb pointers. >> I haven't tested it at all (well compiles and boots, but new code never exercised) >> >> Does it work for you? > > No, not exactly. :( > > I can see your message getting printed. > [ 249.518394] xhci_hcd 0000:00:14.0: Canceled URB td not found on endpoint ring > [ 249.518431] xhci_hcd 0000:00:14.0: Canceled URB td not found on endpoint ring > > But I can see the message from slub debug again: > > [ 348.279986] ============================================================================= > [ 348.279993] BUG kmalloc-96 (Tainted: G U O ): Poison overwritten > [ 348.279995] ----------------------------------------------------------------------------- > > [ 348.279997] Disabling lock debugging due to kernel taint > [ 348.280000] INFO: 0xe5acda60-0xe5acda67. First byte 0x60 instead of 0x6b > [ 348.280012] INFO: Allocated in xhci_ring_alloc.constprop.14+0x31/0x125 [xhci_hcd] age=129264 cpu=0 pid=33 ... > [ 348.280095] INFO: Freed in xhci_ring_free+0xa7/0xc6 [xhci_hcd] age=98722 cpu=0 pid=33 ... > [ 348.280158] INFO: Slab 0xf46e0fe0 objects=29 used=29 fp=0x (null) flags=0x40008100 > [ 348.280160] INFO: Object 0xe5acda48 @offset=6728 fp=0xe5acd700 > > [ 348.280164] Redzone e5acda40: bb bb bb bb bb bb bb bb ........ > [ 348.280167] Object e5acda48: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk > [ 348.280169] Object e5acda58: 6b 6b 6b 6b 6b 6b 6b 6b 60 da ac e5 60 da ac e5 kkkkkkkk`...`... So poison is overwritten at e5acda58 with almost its own address, (reading backwards) e5 ac da 60, twice. looks like something (32bit?)is pointing to itself twice, maybe a linked list node next and prev pointer being set to point to itself as last item was removed from list. The cancelled_td_list is part of struct xhci_virt_ep, so that should be fine. But td_list is part of struct xhci_ring, which was freed. and we removed the URBs tds from the td_list when flushing the ring after ring was freed I changed the patch (attached) to make sure it doesn't touch the td_list when canceling a URB after ring is freed. How about this one, any improvements? -Mathias --------------9B3FAA6CDCAE530347B9BBF6 Content-Type: text/x-patch; name="0001-xhci-when-dequeing-a-URB-make-sure-it-exists-on-the-.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0001-xhci-when-dequeing-a-URB-make-sure-it-exists-on-the-.pa"; filename*1="tch" >>From ee48d9f9c2d82058489dcdc38faa34a3cbdb08d1 Mon Sep 17 00:00:00 2001 From: Mathias Nyman Date: Thu, 19 Jul 2018 18:06:18 +0300 Subject: [PATCH v2] xhci: when dequeing a URB make sure it exists on the current endpoint ring. If the endpoint ring has been reallocated since the URB was enqueued, then URB may contain TD and TRB pointers to a already freed ring. If this the case then manuallt return the URB without touching any of the freed ring structure data. Don't try to stop the ring. It would be useless. This can happened if endpoint is not flushed before it is dropped and re-added, which is the case in usb_set_interface() as xhci does things in an odd order. Signed-off-by: Mathias Nyman --- drivers/usb/host/xhci.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 711da33..7093341 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -37,6 +37,21 @@ static unsigned int quirks; module_param(quirks, uint, S_IRUGO); MODULE_PARM_DESC(quirks, "Bit flags for quirks to be enabled as default"); +static bool td_on_ring(struct xhci_td *td, struct xhci_ring *ring) +{ + struct xhci_segment *seg = ring->first_seg; + + if (!td || !td->start_seg) + return false; + do { + if (seg == td->start_seg) + return true; + seg = seg->next; + } while (seg && seg != ring->first_seg); + + return false; +} + /* TODO: copied from ehci-hcd.c - can this be refactored? */ /* * xhci_handshake - spin reading hc until handshake completes or fails @@ -1467,6 +1482,21 @@ static int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status) goto done; } + /* + * check ring is not re-allocated since URB was enqueued. If it is, then + * make sure none of the ring related pointers in this URB private data + * are touched, such as td_list, otherwise we overwrite freed data + */ + if (!td_on_ring(&urb_priv->td[0], ep_ring)) { + xhci_err(xhci, "Canceled URB td not found on endpoint ring"); + for (i = urb_priv->num_tds_done; i < urb_priv->num_tds; i++) { + td = &urb_priv->td[i]; + if (!list_empty(&td->cancelled_td_list)) + list_del_init(&td->cancelled_td_list); + } + goto err_giveback; + } + if (xhci->xhc_state & XHCI_STATE_HALTED) { xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb, "HC halted, freeing TD manually."); -- 2.7.4 --------------9B3FAA6CDCAE530347B9BBF6 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline --------------9B3FAA6CDCAE530347B9BBF6-- From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: usb HC busted? From: Mathias Nyman Message-Id: Date: Fri, 20 Jul 2018 14:10:58 +0300 To: Sudip Mukherjee Cc: Alan Stern , Greg KH , Andy Shevchenko , Andy Shevchenko , Mathias Nyman , linux-usb@vger.kernel.org, lukaszx.szulc@intel.com, Christoph Hellwig , Marek Szyprowski , iommu@lists.linux-foundation.org List-ID: T24gMTkuMDcuMjAxOCAyMDozMiwgU3VkaXAgTXVraGVyamVlIHdyb3RlOgo+IEhpIE1hdGhpYXMs Cj4gCj4gT24gVGh1LCBKdWwgMTksIDIwMTggYXQgMDY6NDI6MTlQTSArMDMwMCwgTWF0aGlhcyBO eW1hbiB3cm90ZToKPj4+PiBBcyBmaXJzdCBhaWQgSSBjb3VsZCB0cnkgdG8gaW1wbGVtZW50IGNo ZWNrcyB0aGF0IG1ha2Ugc3VyZSB0aGUgZmx1c2hlZCBVUkJzCj4+Pj4gdHJiIHBvaW50ZXJzIHJl YWxseSBhcmUgb24gdGhlIGN1cnJlbnQgZW5kcG9pbnQgcmluZywgYW5kIGFsc28gYWRkIHNvbWUg d2FybmluZwo+Pj4+IGlmIHdlIGFyZSB3ZSBhcmUgZHJvcHBpbmcgZW5kcG9pbnRzIHdpdGggVVJC cyBzdGlsbCBxdWV1ZWQuCj4+Pgo+Pj4gWWVzLCBwbGVhc2UuIEkgdGhpbmsgeW91ciBmaXJzdC1h aWQgd2lsbCBiZSBhIG11Y2ggYmV0dGVyIG9wdGlvbiB0aGFuCj4+PiB0aGUgaGFja3kgcGF0Y2gg SSBhbSB1c2luZyBhdG0uCj4+Pgo+Pgo+PiBBdHRhY2hlZCBhIHBhdGNoIHRoYXQgY2hlY2tzIGNh bmNlbGVkIFVSQiB0ZC90cmIgcG9pbnRlcnMuCj4+IEkgaGF2ZW4ndCB0ZXN0ZWQgaXQgYXQgYWxs ICh3ZWxsIGNvbXBpbGVzIGFuZCBib290cywgYnV0IG5ldyBjb2RlIG5ldmVyIGV4ZXJjaXNlZCkK Pj4KPj4gRG9lcyBpdCB3b3JrIGZvciB5b3U/Cj4gCj4gTm8sIG5vdCBleGFjdGx5LiA6KAo+IAo+ IEkgY2FuIHNlZSB5b3VyIG1lc3NhZ2UgZ2V0dGluZyBwcmludGVkLgo+IFsgIDI0OS41MTgzOTRd IHhoY2lfaGNkIDAwMDA6MDA6MTQuMDogQ2FuY2VsZWQgVVJCIHRkIG5vdCBmb3VuZCBvbiBlbmRw b2ludCByaW5nCj4gWyAgMjQ5LjUxODQzMV0geGhjaV9oY2QgMDAwMDowMDoxNC4wOiBDYW5jZWxl ZCBVUkIgdGQgbm90IGZvdW5kIG9uIGVuZHBvaW50IHJpbmcKPiAKPiBCdXQgSSBjYW4gc2VlIHRo ZSBtZXNzYWdlIGZyb20gc2x1YiBkZWJ1ZyBhZ2FpbjoKPiAKPiBbICAzNDguMjc5OTg2XSA9PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQo+IFsgIDM0OC4yNzk5OTNdIEJVRyBrbWFsbG9jLTk2IChUYWludGVk OiBHICAgICBVICAgICBPICAgKTogUG9pc29uIG92ZXJ3cml0dGVuCj4gWyAgMzQ4LjI3OTk5NV0g LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0KPiAKPiBbICAzNDguMjc5OTk3XSBEaXNhYmxpbmcgbG9jayBk ZWJ1Z2dpbmcgZHVlIHRvIGtlcm5lbCB0YWludAo+IFsgIDM0OC4yODAwMDBdIElORk86IDB4ZTVh Y2RhNjAtMHhlNWFjZGE2Ny4gRmlyc3QgYnl0ZSAweDYwIGluc3RlYWQgb2YgMHg2Ygo+IFsgIDM0 OC4yODAwMTJdIElORk86IEFsbG9jYXRlZCBpbiB4aGNpX3JpbmdfYWxsb2MuY29uc3Rwcm9wLjE0 KzB4MzEvMHgxMjUgW3hoY2lfaGNkXSBhZ2U9MTI5MjY0IGNwdT0wIHBpZD0zMwouLi4KPiBbICAz NDguMjgwMDk1XSBJTkZPOiBGcmVlZCBpbiB4aGNpX3JpbmdfZnJlZSsweGE3LzB4YzYgW3hoY2lf aGNkXSBhZ2U9OTg3MjIgY3B1PTAgcGlkPTMzCi4uLgo+IFsgIDM0OC4yODAxNThdIElORk86IFNs YWIgMHhmNDZlMGZlMCBvYmplY3RzPTI5IHVzZWQ9MjkgZnA9MHggIChudWxsKSBmbGFncz0weDQw MDA4MTAwCj4gWyAgMzQ4LjI4MDE2MF0gSU5GTzogT2JqZWN0IDB4ZTVhY2RhNDggQG9mZnNldD02 NzI4IGZwPTB4ZTVhY2Q3MDAKPiAKPiBbICAzNDguMjgwMTY0XSBSZWR6b25lIGU1YWNkYTQwOiBi YiBiYiBiYiBiYiBiYiBiYiBiYiBiYiAgICAgICAgICAgICAgICAgICAgICAgICAgLi4uLi4uLi4K PiBbICAzNDguMjgwMTY3XSBPYmplY3QgZTVhY2RhNDg6IDZiIDZiIDZiIDZiIDZiIDZiIDZiIDZi IDZiIDZiIDZiIDZiIDZiIDZiIDZiIDZiICBra2tra2tra2tra2tra2trCj4gWyAgMzQ4LjI4MDE2 OV0gT2JqZWN0IGU1YWNkYTU4OiA2YiA2YiA2YiA2YiA2YiA2YiA2YiA2YiA2MCBkYSBhYyBlNSA2 MCBkYSBhYyBlNSAga2tra2tra2tgLi4uYC4uLgoKU28gcG9pc29uIGlzIG92ZXJ3cml0dGVuIGF0 IGU1YWNkYTU4IHdpdGggYWxtb3N0IGl0cyBvd24gYWRkcmVzcywgKHJlYWRpbmcgYmFja3dhcmRz KSBlNSBhYyBkYSA2MCwgdHdpY2UuCmxvb2tzIGxpa2Ugc29tZXRoaW5nICgzMmJpdD8paXMgcG9p bnRpbmcgdG8gaXRzZWxmIHR3aWNlLCBtYXliZSBhIGxpbmtlZCBsaXN0IG5vZGUgbmV4dCBhbmQg cHJldiBwb2ludGVyCmJlaW5nIHNldCB0byBwb2ludCB0byBpdHNlbGYgYXMgbGFzdCBpdGVtIHdh cyByZW1vdmVkIGZyb20gbGlzdC4KClRoZSBjYW5jZWxsZWRfdGRfbGlzdCBpcyBwYXJ0IG9mIHN0 cnVjdCB4aGNpX3ZpcnRfZXAsIHNvIHRoYXQgc2hvdWxkIGJlIGZpbmUuCkJ1dCB0ZF9saXN0IGlz IHBhcnQgb2Ygc3RydWN0IHhoY2lfcmluZywgd2hpY2ggd2FzIGZyZWVkLiBhbmQgd2UgcmVtb3Zl ZCB0aGUgVVJCcyB0ZHMgZnJvbSB0aGUgdGRfbGlzdCB3aGVuCmZsdXNoaW5nIHRoZSByaW5nIGFm dGVyIHJpbmcgd2FzIGZyZWVkCgpJIGNoYW5nZWQgdGhlIHBhdGNoIChhdHRhY2hlZCkgdG8gbWFr ZSBzdXJlIGl0IGRvZXNuJ3QgdG91Y2ggdGhlIHRkX2xpc3Qgd2hlbiBjYW5jZWxpbmcgYSBVUkIg YWZ0ZXIKcmluZyBpcyBmcmVlZC4KCkhvdyBhYm91dCB0aGlzIG9uZSwgYW55IGltcHJvdmVtZW50 cz8KCi1NYXRoaWFzCgpGcm9tIGVlNDhkOWY5YzJkODIwNTg0ODlkY2RjMzhmYWEzNGEzY2JkYjA4 ZDEgTW9uIFNlcCAxNyAwMDowMDowMCAyMDAxCkZyb206IE1hdGhpYXMgTnltYW4gPG1hdGhpYXMu bnltYW5AbGludXguaW50ZWwuY29tPgpEYXRlOiBUaHUsIDE5IEp1bCAyMDE4IDE4OjA2OjE4ICsw MzAwClN1YmplY3Q6IFtQQVRDSCB2Ml0geGhjaTogd2hlbiBkZXF1ZWluZyBhIFVSQiBtYWtlIHN1 cmUgaXQgZXhpc3RzIG9uIHRoZQogY3VycmVudCBlbmRwb2ludCByaW5nLgoKSWYgdGhlIGVuZHBv aW50IHJpbmcgaGFzIGJlZW4gcmVhbGxvY2F0ZWQgc2luY2UgdGhlIFVSQiB3YXMgZW5xdWV1ZWQs CnRoZW4gVVJCIG1heSBjb250YWluIFREIGFuZCBUUkIgcG9pbnRlcnMgdG8gYSBhbHJlYWR5IGZy ZWVkIHJpbmcuCklmIHRoaXMgdGhlIGNhc2UgdGhlbiBtYW51YWxsdCByZXR1cm4gdGhlIFVSQiB3 aXRob3V0IHRvdWNoaW5nIGFueSBvZiB0aGUKZnJlZWQgcmluZyBzdHJ1Y3R1cmUgZGF0YS4KCkRv bid0IHRyeSB0byBzdG9wIHRoZSByaW5nLiBJdCB3b3VsZCBiZSB1c2VsZXNzLgoKVGhpcyBjYW4g aGFwcGVuZWQgaWYgZW5kcG9pbnQgaXMgbm90IGZsdXNoZWQgYmVmb3JlIGl0IGlzIGRyb3BwZWQg YW5kCnJlLWFkZGVkLCB3aGljaCBpcyB0aGUgY2FzZSBpbiB1c2Jfc2V0X2ludGVyZmFjZSgpIGFz IHhoY2kgZG9lcwp0aGluZ3MgaW4gYW4gb2RkIG9yZGVyLgoKU2lnbmVkLW9mZi1ieTogTWF0aGlh cyBOeW1hbiA8bWF0aGlhcy5ueW1hbkBsaW51eC5pbnRlbC5jb20+Ci0tLQogZHJpdmVycy91c2Iv aG9zdC94aGNpLmMgfCAzMCArKysrKysrKysrKysrKysrKysrKysrKysrKysrKysKIDEgZmlsZSBj aGFuZ2VkLCAzMCBpbnNlcnRpb25zKCspCgpkaWZmIC0tZ2l0IGEvZHJpdmVycy91c2IvaG9zdC94 aGNpLmMgYi9kcml2ZXJzL3VzYi9ob3N0L3hoY2kuYwppbmRleCA3MTFkYTMzLi43MDkzMzQxIDEw MDY0NAotLS0gYS9kcml2ZXJzL3VzYi9ob3N0L3hoY2kuYworKysgYi9kcml2ZXJzL3VzYi9ob3N0 L3hoY2kuYwpAQCAtMzcsNiArMzcsMjEgQEAgc3RhdGljIHVuc2lnbmVkIGludCBxdWlya3M7CiBt b2R1bGVfcGFyYW0ocXVpcmtzLCB1aW50LCBTX0lSVUdPKTsKIE1PRFVMRV9QQVJNX0RFU0MocXVp cmtzLCAiQml0IGZsYWdzIGZvciBxdWlya3MgdG8gYmUgZW5hYmxlZCBhcyBkZWZhdWx0Iik7CiAK K3N0YXRpYyBib29sIHRkX29uX3Jpbmcoc3RydWN0IHhoY2lfdGQgKnRkLCBzdHJ1Y3QgeGhjaV9y aW5nICpyaW5nKQoreworCXN0cnVjdCB4aGNpX3NlZ21lbnQgKnNlZyA9IHJpbmctPmZpcnN0X3Nl ZzsKKworCWlmICghdGQgfHwgIXRkLT5zdGFydF9zZWcpCisJCXJldHVybiBmYWxzZTsKKwlkbyB7 CisJCWlmIChzZWcgPT0gdGQtPnN0YXJ0X3NlZykKKwkJCXJldHVybiB0cnVlOworCQlzZWcgPSBz ZWctPm5leHQ7CisJfSB3aGlsZSAoc2VnICYmIHNlZyAhPSByaW5nLT5maXJzdF9zZWcpOworCisJ cmV0dXJuIGZhbHNlOworfQorCiAvKiBUT0RPOiBjb3BpZWQgZnJvbSBlaGNpLWhjZC5jIC0gY2Fu IHRoaXMgYmUgcmVmYWN0b3JlZD8gKi8KIC8qCiAgKiB4aGNpX2hhbmRzaGFrZSAtIHNwaW4gcmVh ZGluZyBoYyB1bnRpbCBoYW5kc2hha2UgY29tcGxldGVzIG9yIGZhaWxzCkBAIC0xNDY3LDYgKzE0 ODIsMjEgQEAgc3RhdGljIGludCB4aGNpX3VyYl9kZXF1ZXVlKHN0cnVjdCB1c2JfaGNkICpoY2Qs IHN0cnVjdCB1cmIgKnVyYiwgaW50IHN0YXR1cykKIAkJZ290byBkb25lOwogCX0KIAorCS8qCisJ ICogY2hlY2sgcmluZyBpcyBub3QgcmUtYWxsb2NhdGVkIHNpbmNlIFVSQiB3YXMgZW5xdWV1ZWQu IElmIGl0IGlzLCB0aGVuCisJICogbWFrZSBzdXJlIG5vbmUgb2YgdGhlIHJpbmcgcmVsYXRlZCBw b2ludGVycyBpbiB0aGlzIFVSQiBwcml2YXRlIGRhdGEKKwkgKiBhcmUgdG91Y2hlZCwgc3VjaCBh cyB0ZF9saXN0LCBvdGhlcndpc2Ugd2Ugb3ZlcndyaXRlIGZyZWVkIGRhdGEKKwkgKi8KKwlpZiAo IXRkX29uX3JpbmcoJnVyYl9wcml2LT50ZFswXSwgZXBfcmluZykpIHsKKwkJeGhjaV9lcnIoeGhj aSwgIkNhbmNlbGVkIFVSQiB0ZCBub3QgZm91bmQgb24gZW5kcG9pbnQgcmluZyIpOworCQlmb3Ig KGkgPSB1cmJfcHJpdi0+bnVtX3Rkc19kb25lOyBpIDwgdXJiX3ByaXYtPm51bV90ZHM7IGkrKykg eworCQkJdGQgPSAmdXJiX3ByaXYtPnRkW2ldOworCQkJaWYgKCFsaXN0X2VtcHR5KCZ0ZC0+Y2Fu Y2VsbGVkX3RkX2xpc3QpKQorCQkJCWxpc3RfZGVsX2luaXQoJnRkLT5jYW5jZWxsZWRfdGRfbGlz dCk7CisJCX0KKwkJZ290byBlcnJfZ2l2ZWJhY2s7CisJfQorCiAJaWYgKHhoY2ktPnhoY19zdGF0 ZSAmIFhIQ0lfU1RBVEVfSEFMVEVEKSB7CiAJCXhoY2lfZGJnX3RyYWNlKHhoY2ksIHRyYWNlX3ho Y2lfZGJnX2NhbmNlbF91cmIsCiAJCQkJIkhDIGhhbHRlZCwgZnJlZWluZyBURCBtYW51YWxseS4i KTsKLS0gCjIuNy40Cgo=