From mboxrd@z Thu Jan 1 00:00:00 1970 From: Felipe Balbi Subject: Re: [PATCH] usb: dwc3: gadget: Fail request submission if it was already queued Date: Fri, 11 Jan 2019 11:21:09 +0200 Message-ID: <87k1jbo8hm.fsf@linux.intel.com> References: <20190111060212.7390-1-mgautam@codeaurora.org> <87pnt3od02.fsf@linux.intel.com> Mime-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Manu Gautam Cc: linux-arm-msm@vger.kernel.org, Greg Kroah-Hartman , "open list:DESIGNWARE USB3 DRD IP DRIVER" , open list List-Id: linux-arm-msm@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Manu Gautam writes: >> Manu Gautam writes: >>> If a function driver tries to re-submit an already queued request, >>> it can results in corruption of pending/started request lists. >>> Catch such conditions and fail the request submission to DCD. >>> >>> Signed-off-by: Manu Gautam >>> --- >>> drivers/usb/dwc3/gadget.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index 679c12e14522..51716c6b286a 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -1290,6 +1290,12 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep= *dep, struct dwc3_request *req) >>> &req->request, req->dep->name)) >>> return -EINVAL; >>>=20=20 >>> + if (req->request.status =3D=3D -EINPROGRESS) { >> this test is really not enough. What if gadget driver set status to >> EINPROGRESS before submission? A better check would involve making sure >> req isn't part of dep->pending_list or dep->started_list or >> dep->cancelled_list. It's clear that this won't work very well as the >> amount of requests grow. > > Thanks for quick review. > 'request.status' check can be replaced: > +if (!list_empty(&req->list) { > > And replace list_del with list_del_init from dwc3_gadget_giveback() I would rather avoid this. We could start tracking our own internal dwc3_request status. Something along the lines of: diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index df876418cb78..5c3ee741541f 100644 =2D-- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -863,6 +863,7 @@ struct dwc3_hwparams { * @num_pending_sgs: counter to pending sgs * @num_queued_sgs: counter to the number of sgs which already got queued * @remaining: amount of data remaining + * @status: internal dwc3 request status tracking * @epnum: endpoint number to which this request refers * @trb: pointer to struct dwc3_trb * @trb_dma: DMA address of @trb @@ -883,6 +884,14 @@ struct dwc3_request { unsigned num_pending_sgs; unsigned int num_queued_sgs; unsigned remaining; + + unsigned int status; +#define DWC3_REQUEST_STATUS_QUEUED 0 +#define DWC3_REQUEST_STATUS_STARTED 1 +#define DWC3_REQUEST_STATUS_CANCELLED 2 +#define DWC3_REQUEST_STATUS_COMPLETED 3 +#define DWC3_REQUEST_STATUS_UNKNOWN -1 + u8 epnum; struct dwc3_trb *trb; dma_addr_t trb_dma; diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 07bd31bb2f8a..74db274786bc 100644 =2D-- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -208,6 +208,7 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct d= wc3_request *req, struct dwc3 *dwc =3D dep->dwc; =20 dwc3_gadget_del_and_unmap_request(dep, req, status); + req->status =3D DWC3_REQUEST_STATUS_COMPLETED; =20 spin_unlock(&dwc->lock); usb_gadget_giveback_request(&dep->endpoint, &req->request); @@ -846,6 +847,7 @@ static struct usb_request *dwc3_gadget_ep_alloc_request= (struct usb_ep *ep, req->direction =3D dep->direction; req->epnum =3D dep->number; req->dep =3D dep; + req->status =3D DWC3_REQUEST_STATUS_UNKNOWN; =20 trace_dwc3_alloc_request(req); =20 @@ -1434,6 +1436,11 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *de= p, struct dwc3_request *req) &req->request, req->dep->name)) return -EINVAL; =20 + if (WARN(req->status < DWC3_REQUEST_STATUS_COMPLETED, + "%s: request %pK already in flight\n", + dep->name, &req->request)) + return -EINVAL; + pm_runtime_get(dwc->dev); =20 req->request.actual =3D 0; @@ -1442,6 +1449,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep= , struct dwc3_request *req) trace_dwc3_ep_queue(req); =20 list_add_tail(&req->list, &dep->pending_list); + req->status =3D DWC3_REQUEST_STATUS_QUEUED; =20 /* * NOTICE: Isochronous endpoints should NEVER be prestarted. We must diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h index 023a473648eb..6aebe8c0eae1 100644 =2D-- a/drivers/usb/dwc3/gadget.h +++ b/drivers/usb/dwc3/gadget.h @@ -76,6 +76,7 @@ static inline void dwc3_gadget_move_started_request(struc= t dwc3_request *req) struct dwc3_ep *dep =3D req->dep; =20 req->started =3D true; + req->status =3D DWC3_REQUEST_STATUS_STARTED; list_move_tail(&req->list, &dep->started_list); } =20 @@ -91,6 +92,7 @@ static inline void dwc3_gadget_move_cancelled_request(str= uct dwc3_request *req) struct dwc3_ep *dep =3D req->dep; =20 req->started =3D false; + req->status =3D DWC3_REQUEST_STATUS_CANCELLED; list_move_tail(&req->list, &dep->cancelled_list); } =20 With this, we can remove some of the other request flags, such as "started". >> Anyway, which gadget driver did this? Why is it only affecting dwc3? > > Function driver is not in upstream (f_mtp.c). So this could happen with any UDC, right? Why is f_mtp.c queueing the same request twice? =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAlw4YAUACgkQzL64meEa mQZOcg/+NfHKxPdOWjps2XB/tM0mdLAdmFQbnf9SSgiozoqV9GXiZTGmpUcNufoN Q9/fc7Aiad9pN63mMEzfyF+YWEIF/e37LZhhH0TaQl+VVDeAbMli+FoGOKv6Py1W x9sZX7yMrG2fqUiwRFiZ0XIBLxjFD5vIZiC6DYF/KaLUxIT6V+UXc8Z6Z3fP06ov ABmMvG2EHU+vXshFRVg3ROogu+/ZGxz3URHtvyf0xRghjICFz9gvXHM4+4k67742 uNR3Yi00qZ5rMGjORFySoD3kctTid6vnhXMwMs7ro4KuOALazvmqs3lKDGG3Prt/ /XEj76PvP/PNTPlLGjfZ2pfEuBs1Pi99UP9Ec2w4plepZjWapkZsEkx4DwjE6tpl VKKLCeeeAJVSWpKsSNgQPP/dRYduF5+LYlHXqrsViALgSTvB1/6RKod5qGwBRb9+ vWCjZlAEnPpiTJUzpevv4rBGIlMTNhYN9GefB+xjn7adildR+wPZvMn7j1RsGRqr btfqrAYcZPtouhSWZZRqWYNV8B7sz/taim36/vMNkoMMDNz63kQOuNDXA5JHjKKJ 7fXkY+6fiA0eH8hj3RXaNKrIL7OgDuOhLDpBjbJR6hB613KtsLHbpgW1ZGyyAxwI TTp6x/S9ganAXZnjq04cdSlP5Fq51hjVz2r4pIJycawwic4isK4= =v5Qv -----END PGP SIGNATURE----- --=-=-=-- 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: dwc3: gadget: Fail request submission if it was already queued From: Felipe Balbi Message-Id: <87k1jbo8hm.fsf@linux.intel.com> Date: Fri, 11 Jan 2019 11:21:09 +0200 To: Manu Gautam Cc: linux-arm-msm@vger.kernel.org, Greg Kroah-Hartman , "open list:DESIGNWARE USB3 DRD IP DRIVER" , open list List-ID: SGksCgpNYW51IEdhdXRhbSA8bWdhdXRhbUBjb2RlYXVyb3JhLm9yZz4gd3JpdGVzOgo+PiBNYW51 IEdhdXRhbSA8bWdhdXRhbUBjb2RlYXVyb3JhLm9yZz4gd3JpdGVzOgo+Pj4gSWYgYSBmdW5jdGlv biBkcml2ZXIgdHJpZXMgdG8gcmUtc3VibWl0IGFuIGFscmVhZHkgcXVldWVkIHJlcXVlc3QsCj4+ PiBpdCBjYW4gcmVzdWx0cyBpbiBjb3JydXB0aW9uIG9mIHBlbmRpbmcvc3RhcnRlZCByZXF1ZXN0 IGxpc3RzLgo+Pj4gQ2F0Y2ggc3VjaCBjb25kaXRpb25zIGFuZCBmYWlsIHRoZSByZXF1ZXN0IHN1 Ym1pc3Npb24gdG8gRENELgo+Pj4KPj4+IFNpZ25lZC1vZmYtYnk6IE1hbnUgR2F1dGFtIDxtZ2F1 dGFtQGNvZGVhdXJvcmEub3JnPgo+Pj4gLS0tCj4+PiAgZHJpdmVycy91c2IvZHdjMy9nYWRnZXQu YyB8IDYgKysrKysrCj4+PiAgMSBmaWxlIGNoYW5nZWQsIDYgaW5zZXJ0aW9ucygrKQo+Pj4KPj4+ IGRpZmYgLS1naXQgYS9kcml2ZXJzL3VzYi9kd2MzL2dhZGdldC5jIGIvZHJpdmVycy91c2IvZHdj My9nYWRnZXQuYwo+Pj4gaW5kZXggNjc5YzEyZTE0NTIyLi41MTcxNmM2YjI4NmEgMTAwNjQ0Cj4+ PiAtLS0gYS9kcml2ZXJzL3VzYi9kd2MzL2dhZGdldC5jCj4+PiArKysgYi9kcml2ZXJzL3VzYi9k d2MzL2dhZGdldC5jCj4+PiBAQCAtMTI5MCw2ICsxMjkwLDEyIEBAIHN0YXRpYyBpbnQgX19kd2Mz X2dhZGdldF9lcF9xdWV1ZShzdHJ1Y3QgZHdjM19lcCAqZGVwLCBzdHJ1Y3QgZHdjM19yZXF1ZXN0 ICpyZXEpCj4+PiAgCQkJCSZyZXEtPnJlcXVlc3QsIHJlcS0+ZGVwLT5uYW1lKSkKPj4+ICAJCXJl dHVybiAtRUlOVkFMOwo+Pj4gIAo+Pj4gKwlpZiAocmVxLT5yZXF1ZXN0LnN0YXR1cyA9PSAtRUlO UFJPR1JFU1MpIHsKPj4gdGhpcyB0ZXN0IGlzIHJlYWxseSBub3QgZW5vdWdoLiBXaGF0IGlmIGdh ZGdldCBkcml2ZXIgc2V0IHN0YXR1cyB0bwo+PiBFSU5QUk9HUkVTUyBiZWZvcmUgc3VibWlzc2lv bj8gQSBiZXR0ZXIgY2hlY2sgd291bGQgaW52b2x2ZSBtYWtpbmcgc3VyZQo+PiByZXEgaXNuJ3Qg cGFydCBvZiBkZXAtPnBlbmRpbmdfbGlzdCBvciBkZXAtPnN0YXJ0ZWRfbGlzdCBvcgo+PiBkZXAt PmNhbmNlbGxlZF9saXN0LiBJdCdzIGNsZWFyIHRoYXQgdGhpcyB3b24ndCB3b3JrIHZlcnkgd2Vs bCBhcyB0aGUKPj4gYW1vdW50IG9mIHJlcXVlc3RzIGdyb3cuCj4KPiBUaGFua3MgZm9yIHF1aWNr IHJldmlldy4KPiAncmVxdWVzdC5zdGF0dXMnIGNoZWNrIGNhbiBiZSByZXBsYWNlZDoKPiAraWYg KCFsaXN0X2VtcHR5KCZyZXEtPmxpc3QpIHsKPgo+IEFuZCByZXBsYWNlIGxpc3RfZGVsIHdpdGgg bGlzdF9kZWxfaW5pdCBmcm9tIGR3YzNfZ2FkZ2V0X2dpdmViYWNrKCkKCkkgd291bGQgcmF0aGVy IGF2b2lkIHRoaXMuIFdlIGNvdWxkIHN0YXJ0IHRyYWNraW5nIG91ciBvd24gaW50ZXJuYWwKZHdj M19yZXF1ZXN0IHN0YXR1cy4gU29tZXRoaW5nIGFsb25nIHRoZSBsaW5lcyBvZjoKCldpdGggdGhp cywgd2UgY2FuIHJlbW92ZSBzb21lIG9mIHRoZSBvdGhlciByZXF1ZXN0IGZsYWdzLCBzdWNoIGFz ICJzdGFydGVkIi4KCj4+IEFueXdheSwgd2hpY2ggZ2FkZ2V0IGRyaXZlciBkaWQgdGhpcz8gV2h5 IGlzIGl0IG9ubHkgYWZmZWN0aW5nIGR3YzM/Cj4KPiBGdW5jdGlvbiBkcml2ZXIgaXMgbm90IGlu IHVwc3RyZWFtIChmX210cC5jKS4KClNvIHRoaXMgY291bGQgaGFwcGVuIHdpdGggYW55IFVEQywg cmlnaHQ/IFdoeSBpcyBmX210cC5jIHF1ZXVlaW5nIHRoZQpzYW1lIHJlcXVlc3QgdHdpY2U/Cgpk aWZmIC0tZ2l0IGEvZHJpdmVycy91c2IvZHdjMy9jb3JlLmggYi9kcml2ZXJzL3VzYi9kd2MzL2Nv cmUuaAppbmRleCBkZjg3NjQxOGNiNzguLjVjM2VlNzQxNTQxZiAxMDA2NDQKLS0tIGEvZHJpdmVy cy91c2IvZHdjMy9jb3JlLmgKKysrIGIvZHJpdmVycy91c2IvZHdjMy9jb3JlLmgKQEAgLTg2Myw2 ICs4NjMsNyBAQCBzdHJ1Y3QgZHdjM19od3BhcmFtcyB7CiAgKiBAbnVtX3BlbmRpbmdfc2dzOiBj b3VudGVyIHRvIHBlbmRpbmcgc2dzCiAgKiBAbnVtX3F1ZXVlZF9zZ3M6IGNvdW50ZXIgdG8gdGhl IG51bWJlciBvZiBzZ3Mgd2hpY2ggYWxyZWFkeSBnb3QgcXVldWVkCiAgKiBAcmVtYWluaW5nOiBh bW91bnQgb2YgZGF0YSByZW1haW5pbmcKKyAqIEBzdGF0dXM6IGludGVybmFsIGR3YzMgcmVxdWVz dCBzdGF0dXMgdHJhY2tpbmcKICAqIEBlcG51bTogZW5kcG9pbnQgbnVtYmVyIHRvIHdoaWNoIHRo aXMgcmVxdWVzdCByZWZlcnMKICAqIEB0cmI6IHBvaW50ZXIgdG8gc3RydWN0IGR3YzNfdHJiCiAg KiBAdHJiX2RtYTogRE1BIGFkZHJlc3Mgb2YgQHRyYgpAQCAtODgzLDYgKzg4NCwxNCBAQCBzdHJ1 Y3QgZHdjM19yZXF1ZXN0IHsKIAl1bnNpZ25lZAkJbnVtX3BlbmRpbmdfc2dzOwogCXVuc2lnbmVk IGludAkJbnVtX3F1ZXVlZF9zZ3M7CiAJdW5zaWduZWQJCXJlbWFpbmluZzsKKworCXVuc2lnbmVk IGludAkJc3RhdHVzOworI2RlZmluZSBEV0MzX1JFUVVFU1RfU1RBVFVTX1FVRVVFRAkwCisjZGVm aW5lIERXQzNfUkVRVUVTVF9TVEFUVVNfU1RBUlRFRAkxCisjZGVmaW5lIERXQzNfUkVRVUVTVF9T VEFUVVNfQ0FOQ0VMTEVECTIKKyNkZWZpbmUgRFdDM19SRVFVRVNUX1NUQVRVU19DT01QTEVURUQJ MworI2RlZmluZSBEV0MzX1JFUVVFU1RfU1RBVFVTX1VOS05PV04JLTEKKwogCXU4CQkJZXBudW07 CiAJc3RydWN0IGR3YzNfdHJiCQkqdHJiOwogCWRtYV9hZGRyX3QJCXRyYl9kbWE7CmRpZmYgLS1n aXQgYS9kcml2ZXJzL3VzYi9kd2MzL2dhZGdldC5jIGIvZHJpdmVycy91c2IvZHdjMy9nYWRnZXQu YwppbmRleCAwN2JkMzFiYjJmOGEuLjc0ZGIyNzQ3ODZiYyAxMDA2NDQKLS0tIGEvZHJpdmVycy91 c2IvZHdjMy9nYWRnZXQuYworKysgYi9kcml2ZXJzL3VzYi9kd2MzL2dhZGdldC5jCkBAIC0yMDgs NiArMjA4LDcgQEAgdm9pZCBkd2MzX2dhZGdldF9naXZlYmFjayhzdHJ1Y3QgZHdjM19lcCAqZGVw LCBzdHJ1Y3QgZHdjM19yZXF1ZXN0ICpyZXEsCiAJc3RydWN0IGR3YzMJCQkqZHdjID0gZGVwLT5k d2M7CiAKIAlkd2MzX2dhZGdldF9kZWxfYW5kX3VubWFwX3JlcXVlc3QoZGVwLCByZXEsIHN0YXR1 cyk7CisJcmVxLT5zdGF0dXMgPSBEV0MzX1JFUVVFU1RfU1RBVFVTX0NPTVBMRVRFRDsKIAogCXNw aW5fdW5sb2NrKCZkd2MtPmxvY2spOwogCXVzYl9nYWRnZXRfZ2l2ZWJhY2tfcmVxdWVzdCgmZGVw LT5lbmRwb2ludCwgJnJlcS0+cmVxdWVzdCk7CkBAIC04NDYsNiArODQ3LDcgQEAgc3RhdGljIHN0 cnVjdCB1c2JfcmVxdWVzdCAqZHdjM19nYWRnZXRfZXBfYWxsb2NfcmVxdWVzdChzdHJ1Y3QgdXNi X2VwICplcCwKIAlyZXEtPmRpcmVjdGlvbgk9IGRlcC0+ZGlyZWN0aW9uOwogCXJlcS0+ZXBudW0J PSBkZXAtPm51bWJlcjsKIAlyZXEtPmRlcAk9IGRlcDsKKwlyZXEtPnN0YXR1cwk9IERXQzNfUkVR VUVTVF9TVEFUVVNfVU5LTk9XTjsKIAogCXRyYWNlX2R3YzNfYWxsb2NfcmVxdWVzdChyZXEpOwog CkBAIC0xNDM0LDYgKzE0MzYsMTEgQEAgc3RhdGljIGludCBfX2R3YzNfZ2FkZ2V0X2VwX3F1ZXVl KHN0cnVjdCBkd2MzX2VwICpkZXAsIHN0cnVjdCBkd2MzX3JlcXVlc3QgKnJlcSkKIAkJCQkmcmVx LT5yZXF1ZXN0LCByZXEtPmRlcC0+bmFtZSkpCiAJCXJldHVybiAtRUlOVkFMOwogCisJaWYgKFdB Uk4ocmVxLT5zdGF0dXMgPCBEV0MzX1JFUVVFU1RfU1RBVFVTX0NPTVBMRVRFRCwKKwkJCQkiJXM6 IHJlcXVlc3QgJXBLIGFscmVhZHkgaW4gZmxpZ2h0XG4iLAorCQkJCWRlcC0+bmFtZSwgJnJlcS0+ cmVxdWVzdCkpCisJCXJldHVybiAtRUlOVkFMOworCiAJcG1fcnVudGltZV9nZXQoZHdjLT5kZXYp OwogCiAJcmVxLT5yZXF1ZXN0LmFjdHVhbAk9IDA7CkBAIC0xNDQyLDYgKzE0NDksNyBAQCBzdGF0 aWMgaW50IF9fZHdjM19nYWRnZXRfZXBfcXVldWUoc3RydWN0IGR3YzNfZXAgKmRlcCwgc3RydWN0 IGR3YzNfcmVxdWVzdCAqcmVxKQogCXRyYWNlX2R3YzNfZXBfcXVldWUocmVxKTsKIAogCWxpc3Rf YWRkX3RhaWwoJnJlcS0+bGlzdCwgJmRlcC0+cGVuZGluZ19saXN0KTsKKwlyZXEtPnN0YXR1cyA9 IERXQzNfUkVRVUVTVF9TVEFUVVNfUVVFVUVEOwogCiAJLyoKIAkgKiBOT1RJQ0U6IElzb2Nocm9u b3VzIGVuZHBvaW50cyBzaG91bGQgTkVWRVIgYmUgcHJlc3RhcnRlZC4gV2UgbXVzdApkaWZmIC0t Z2l0IGEvZHJpdmVycy91c2IvZHdjMy9nYWRnZXQuaCBiL2RyaXZlcnMvdXNiL2R3YzMvZ2FkZ2V0 LmgKaW5kZXggMDIzYTQ3MzY0OGViLi42YWViZThjMGVhZTEgMTAwNjQ0Ci0tLSBhL2RyaXZlcnMv dXNiL2R3YzMvZ2FkZ2V0LmgKKysrIGIvZHJpdmVycy91c2IvZHdjMy9nYWRnZXQuaApAQCAtNzYs NiArNzYsNyBAQCBzdGF0aWMgaW5saW5lIHZvaWQgZHdjM19nYWRnZXRfbW92ZV9zdGFydGVkX3Jl cXVlc3Qoc3RydWN0IGR3YzNfcmVxdWVzdCAqcmVxKQogCXN0cnVjdCBkd2MzX2VwCQkqZGVwID0g cmVxLT5kZXA7CiAKIAlyZXEtPnN0YXJ0ZWQgPSB0cnVlOworCXJlcS0+c3RhdHVzID0gRFdDM19S RVFVRVNUX1NUQVRVU19TVEFSVEVEOwogCWxpc3RfbW92ZV90YWlsKCZyZXEtPmxpc3QsICZkZXAt PnN0YXJ0ZWRfbGlzdCk7CiB9CiAKQEAgLTkxLDYgKzkyLDcgQEAgc3RhdGljIGlubGluZSB2b2lk IGR3YzNfZ2FkZ2V0X21vdmVfY2FuY2VsbGVkX3JlcXVlc3Qoc3RydWN0IGR3YzNfcmVxdWVzdCAq cmVxKQogCXN0cnVjdCBkd2MzX2VwCQkqZGVwID0gcmVxLT5kZXA7CiAKIAlyZXEtPnN0YXJ0ZWQg PSBmYWxzZTsKKwlyZXEtPnN0YXR1cyA9IERXQzNfUkVRVUVTVF9TVEFUVVNfQ0FOQ0VMTEVEOwog CWxpc3RfbW92ZV90YWlsKCZyZXEtPmxpc3QsICZkZXAtPmNhbmNlbGxlZF9saXN0KTsKIH0KIAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.6 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 230D8C43387 for ; Fri, 11 Jan 2019 09:21:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id DBCAA20872 for ; Fri, 11 Jan 2019 09:21:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1547198478; bh=fVNOIb5a4GeBaDXRTWB/ohknU5FrRErhCpuT8IJxtCg=; h=From:To:Cc:Subject:In-Reply-To:References:Date:List-ID:From; b=PKZK9r5DPgSB5vT44xWKq/+OT/koKt/RylsmVKR8uavU8JiQlBIoPLB42zf5hXFtz iaFnW1V4HEZM6lmNFwANs2ehtxwGrdLLcTd9CMdXN89P1jhnnDrV+NE24ZMaxKkOvF eulQdUEZ20/IN60j4eSWHIekRkiNZWtey9eU8X0g= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731404AbfAKJVR (ORCPT ); Fri, 11 Jan 2019 04:21:17 -0500 Received: from mga07.intel.com ([134.134.136.100]:25562 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727653AbfAKJVQ (ORCPT ); Fri, 11 Jan 2019 04:21:16 -0500 X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Jan 2019 01:21:15 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,465,1539673200"; d="asc'?scan'208";a="115965735" Received: from pipin.fi.intel.com (HELO localhost) ([10.237.72.175]) by fmsmga008.fm.intel.com with ESMTP; 11 Jan 2019 01:21:13 -0800 From: Felipe Balbi To: Manu Gautam Cc: linux-arm-msm@vger.kernel.org, Greg Kroah-Hartman , "open list\:DESIGNWARE USB3 DRD IP DRIVER" , open list Subject: Re: [PATCH] usb: dwc3: gadget: Fail request submission if it was already queued In-Reply-To: References: <20190111060212.7390-1-mgautam@codeaurora.org> <87pnt3od02.fsf@linux.intel.com> Date: Fri, 11 Jan 2019 11:21:09 +0200 Message-ID: <87k1jbo8hm.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Manu Gautam writes: >> Manu Gautam writes: >>> If a function driver tries to re-submit an already queued request, >>> it can results in corruption of pending/started request lists. >>> Catch such conditions and fail the request submission to DCD. >>> >>> Signed-off-by: Manu Gautam >>> --- >>> drivers/usb/dwc3/gadget.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index 679c12e14522..51716c6b286a 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -1290,6 +1290,12 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep= *dep, struct dwc3_request *req) >>> &req->request, req->dep->name)) >>> return -EINVAL; >>>=20=20 >>> + if (req->request.status =3D=3D -EINPROGRESS) { >> this test is really not enough. What if gadget driver set status to >> EINPROGRESS before submission? A better check would involve making sure >> req isn't part of dep->pending_list or dep->started_list or >> dep->cancelled_list. It's clear that this won't work very well as the >> amount of requests grow. > > Thanks for quick review. > 'request.status' check can be replaced: > +if (!list_empty(&req->list) { > > And replace list_del with list_del_init from dwc3_gadget_giveback() I would rather avoid this. We could start tracking our own internal dwc3_request status. Something along the lines of: diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index df876418cb78..5c3ee741541f 100644 =2D-- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -863,6 +863,7 @@ struct dwc3_hwparams { * @num_pending_sgs: counter to pending sgs * @num_queued_sgs: counter to the number of sgs which already got queued * @remaining: amount of data remaining + * @status: internal dwc3 request status tracking * @epnum: endpoint number to which this request refers * @trb: pointer to struct dwc3_trb * @trb_dma: DMA address of @trb @@ -883,6 +884,14 @@ struct dwc3_request { unsigned num_pending_sgs; unsigned int num_queued_sgs; unsigned remaining; + + unsigned int status; +#define DWC3_REQUEST_STATUS_QUEUED 0 +#define DWC3_REQUEST_STATUS_STARTED 1 +#define DWC3_REQUEST_STATUS_CANCELLED 2 +#define DWC3_REQUEST_STATUS_COMPLETED 3 +#define DWC3_REQUEST_STATUS_UNKNOWN -1 + u8 epnum; struct dwc3_trb *trb; dma_addr_t trb_dma; diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 07bd31bb2f8a..74db274786bc 100644 =2D-- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -208,6 +208,7 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct d= wc3_request *req, struct dwc3 *dwc =3D dep->dwc; =20 dwc3_gadget_del_and_unmap_request(dep, req, status); + req->status =3D DWC3_REQUEST_STATUS_COMPLETED; =20 spin_unlock(&dwc->lock); usb_gadget_giveback_request(&dep->endpoint, &req->request); @@ -846,6 +847,7 @@ static struct usb_request *dwc3_gadget_ep_alloc_request= (struct usb_ep *ep, req->direction =3D dep->direction; req->epnum =3D dep->number; req->dep =3D dep; + req->status =3D DWC3_REQUEST_STATUS_UNKNOWN; =20 trace_dwc3_alloc_request(req); =20 @@ -1434,6 +1436,11 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *de= p, struct dwc3_request *req) &req->request, req->dep->name)) return -EINVAL; =20 + if (WARN(req->status < DWC3_REQUEST_STATUS_COMPLETED, + "%s: request %pK already in flight\n", + dep->name, &req->request)) + return -EINVAL; + pm_runtime_get(dwc->dev); =20 req->request.actual =3D 0; @@ -1442,6 +1449,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep= , struct dwc3_request *req) trace_dwc3_ep_queue(req); =20 list_add_tail(&req->list, &dep->pending_list); + req->status =3D DWC3_REQUEST_STATUS_QUEUED; =20 /* * NOTICE: Isochronous endpoints should NEVER be prestarted. We must diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h index 023a473648eb..6aebe8c0eae1 100644 =2D-- a/drivers/usb/dwc3/gadget.h +++ b/drivers/usb/dwc3/gadget.h @@ -76,6 +76,7 @@ static inline void dwc3_gadget_move_started_request(struc= t dwc3_request *req) struct dwc3_ep *dep =3D req->dep; =20 req->started =3D true; + req->status =3D DWC3_REQUEST_STATUS_STARTED; list_move_tail(&req->list, &dep->started_list); } =20 @@ -91,6 +92,7 @@ static inline void dwc3_gadget_move_cancelled_request(str= uct dwc3_request *req) struct dwc3_ep *dep =3D req->dep; =20 req->started =3D false; + req->status =3D DWC3_REQUEST_STATUS_CANCELLED; list_move_tail(&req->list, &dep->cancelled_list); } =20 With this, we can remove some of the other request flags, such as "started". >> Anyway, which gadget driver did this? Why is it only affecting dwc3? > > Function driver is not in upstream (f_mtp.c). So this could happen with any UDC, right? Why is f_mtp.c queueing the same request twice? =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAlw4YAUACgkQzL64meEa mQZOcg/+NfHKxPdOWjps2XB/tM0mdLAdmFQbnf9SSgiozoqV9GXiZTGmpUcNufoN Q9/fc7Aiad9pN63mMEzfyF+YWEIF/e37LZhhH0TaQl+VVDeAbMli+FoGOKv6Py1W x9sZX7yMrG2fqUiwRFiZ0XIBLxjFD5vIZiC6DYF/KaLUxIT6V+UXc8Z6Z3fP06ov ABmMvG2EHU+vXshFRVg3ROogu+/ZGxz3URHtvyf0xRghjICFz9gvXHM4+4k67742 uNR3Yi00qZ5rMGjORFySoD3kctTid6vnhXMwMs7ro4KuOALazvmqs3lKDGG3Prt/ /XEj76PvP/PNTPlLGjfZ2pfEuBs1Pi99UP9Ec2w4plepZjWapkZsEkx4DwjE6tpl VKKLCeeeAJVSWpKsSNgQPP/dRYduF5+LYlHXqrsViALgSTvB1/6RKod5qGwBRb9+ vWCjZlAEnPpiTJUzpevv4rBGIlMTNhYN9GefB+xjn7adildR+wPZvMn7j1RsGRqr btfqrAYcZPtouhSWZZRqWYNV8B7sz/taim36/vMNkoMMDNz63kQOuNDXA5JHjKKJ 7fXkY+6fiA0eH8hj3RXaNKrIL7OgDuOhLDpBjbJR6hB613KtsLHbpgW1ZGyyAxwI TTp6x/S9ganAXZnjq04cdSlP5Fq51hjVz2r4pIJycawwic4isK4= =v5Qv -----END PGP SIGNATURE----- --=-=-=--