* [PATCH 0/2] Two fix for dwc2 gadget driver
@ 2015-11-30 5:21 changbin.du
2015-11-30 5:21 ` [PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable changbin.du
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: changbin.du @ 2015-11-30 5:21 UTC (permalink / raw)
To: johnyoun; +Cc: gregkh, linux-usb, linux-kernel, Du, Changbin
From: "Du, Changbin" <changbin.du@intel.com>
With the first patch, enable a enabled ep will return -EBUSY.
The second patch forbid queuing on disabled ep to avoid panic.
Du, Changbin (2):
usb: dwc2: add ep enabled flag to avoid double enable/disable
usb: dwc2: forbid queuing request to a disabled ep
drivers/usb/dwc2/core.h | 1 +
drivers/usb/dwc2/gadget.c | 26 +++++++++++++++++++++++++-
2 files changed, 26 insertions(+), 1 deletion(-)
--
2.5.0
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable 2015-11-30 5:21 [PATCH 0/2] Two fix for dwc2 gadget driver changbin.du @ 2015-11-30 5:21 ` changbin.du 2015-12-10 17:26 ` Felipe Balbi 2015-11-30 5:21 ` [PATCH 2/2] usb: dwc2: forbid queuing request to a disabled ep changbin.du 2015-12-03 1:20 ` [PATCH 0/2] Two fix for dwc2 gadget driver John Youn 2 siblings, 1 reply; 18+ messages in thread From: changbin.du @ 2015-11-30 5:21 UTC (permalink / raw) To: johnyoun; +Cc: gregkh, linux-usb, linux-kernel, Du, Changbin From: "Du, Changbin" <changbin.du@intel.com> Enabling a already enabled ep is illegal, because the ep may has trbs running. Reprogram the ep may break running transfer. So udc driver must avoid this happening by return an error -EBUSY. Gadget function driver also should avoid such things, but that is out of udc driver. Similarly, disable a disabled ep makes no sense, but no need return an error here. Signed-off-by: Du, Changbin <changbin.du@intel.com> --- drivers/usb/dwc2/core.h | 1 + drivers/usb/dwc2/gadget.c | 20 +++++++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index a66d3cb..cf7eccd 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -162,6 +162,7 @@ struct dwc2_hsotg_ep { unsigned char mc; unsigned char interval; + unsigned int enabled:1; unsigned int halted:1; unsigned int periodic:1; unsigned int isochronous:1; diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 0abf73c..586bbcd 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -2423,6 +2423,7 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg, /* enable, but don't activate EP0in */ dwc2_writel(dwc2_hsotg_ep0_mps(hsotg->eps_out[0]->ep.maxpacket) | DXEPCTL_USBACTEP, hsotg->regs + DIEPCTL0); + hsotg->eps_out[0]->enabled = 1; dwc2_hsotg_enqueue_setup(hsotg); @@ -2680,6 +2681,14 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep, return -EINVAL; } + spin_lock_irqsave(&hsotg->lock, flags); + if (hs_ep->enabled) { + dev_warn(hsotg->dev, "%s: ep %s already enabled\n", + __func__, hs_ep->name); + ret = -EBUSY; + goto error; + } + mps = usb_endpoint_maxp(desc); /* note, we handle this here instead of dwc2_hsotg_set_ep_maxpacket */ @@ -2690,7 +2699,6 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep, dev_dbg(hsotg->dev, "%s: read DxEPCTL=0x%08x from 0x%08x\n", __func__, epctrl, epctrl_reg); - spin_lock_irqsave(&hsotg->lock, flags); epctrl &= ~(DXEPCTL_EPTYPE_MASK | DXEPCTL_MPS_MASK); epctrl |= DXEPCTL_MPS(mps); @@ -2806,6 +2814,8 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep, /* enable the endpoint interrupt */ dwc2_hsotg_ctrl_epint(hsotg, index, dir_in, 1); + hs_ep->enabled = 1; + error: spin_unlock_irqrestore(&hsotg->lock, flags); return ret; @@ -2835,6 +2845,11 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep) epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index); spin_lock_irqsave(&hsotg->lock, flags); + if (!hs_ep->enabled) { + dev_warn(hsotg->dev, "%s: ep %s already disabled\n", + __func__, hs_ep->name); + goto out; + } hsotg->fifo_map &= ~(1<<hs_ep->fifo_index); hs_ep->fifo_index = 0; @@ -2854,6 +2869,9 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep) /* terminate all requests with shutdown */ kill_all_requests(hsotg, hs_ep, -ESHUTDOWN); + hs_ep->enabled = 0; + +out: spin_unlock_irqrestore(&hsotg->lock, flags); return 0; } -- 2.5.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable 2015-11-30 5:21 ` [PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable changbin.du @ 2015-12-10 17:26 ` Felipe Balbi 2015-12-14 3:23 ` Du, Changbin 0 siblings, 1 reply; 18+ messages in thread From: Felipe Balbi @ 2015-12-10 17:26 UTC (permalink / raw) To: changbin.du, johnyoun; +Cc: gregkh, linux-usb, linux-kernel, Du, Changbin [-- Attachment #1: Type: text/plain, Size: 2108 bytes --] Hi, changbin.du@intel.com writes: > From: "Du, Changbin" <changbin.du@intel.com> > > Enabling a already enabled ep is illegal, because the ep may has trbs > running. Reprogram the ep may break running transfer. So udc driver > must avoid this happening by return an error -EBUSY. Gadget function > driver also should avoid such things, but that is out of udc driver. > > Similarly, disable a disabled ep makes no sense, but no need return > an error here. > > Signed-off-by: Du, Changbin <changbin.du@intel.com> > --- > drivers/usb/dwc2/core.h | 1 + > drivers/usb/dwc2/gadget.c | 20 +++++++++++++++++++- > 2 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h > index a66d3cb..cf7eccd 100644 > --- a/drivers/usb/dwc2/core.h > +++ b/drivers/usb/dwc2/core.h > @@ -162,6 +162,7 @@ struct dwc2_hsotg_ep { > unsigned char mc; > unsigned char interval; > > + unsigned int enabled:1; > unsigned int halted:1; > unsigned int periodic:1; > unsigned int isochronous:1; > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index 0abf73c..586bbcd 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -2423,6 +2423,7 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg, > /* enable, but don't activate EP0in */ > dwc2_writel(dwc2_hsotg_ep0_mps(hsotg->eps_out[0]->ep.maxpacket) | > DXEPCTL_USBACTEP, hsotg->regs + DIEPCTL0); > + hsotg->eps_out[0]->enabled = 1; > > dwc2_hsotg_enqueue_setup(hsotg); > > @@ -2680,6 +2681,14 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep, > return -EINVAL; > } > > + spin_lock_irqsave(&hsotg->lock, flags); > + if (hs_ep->enabled) { > + dev_warn(hsotg->dev, "%s: ep %s already enabled\n", > + __func__, hs_ep->name); this is a rather serious condition. I'd rather use dev_WARN_ONCE(): if (dev_WARN_ONCE(hsotg->dev, hs_ep->enabled, "ep %s already enabled\n", hs_ep->name)) { -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable 2015-12-10 17:26 ` Felipe Balbi @ 2015-12-14 3:23 ` Du, Changbin 0 siblings, 0 replies; 18+ messages in thread From: Du, Changbin @ 2015-12-14 3:23 UTC (permalink / raw) To: Felipe Balbi, johnyoun; +Cc: gregkh, linux-usb, linux-kernel Hi, Balbi, Please ignore this patch set. Because we have a fix in gadget API layer. [PATCH] usb: gadget: forbid queuing request to a disabled ep > > Enabling an already enabled ep is illegal, because the ep may has trbs > > running. Reprogram the ep may break running transfer. So udc driver > > must avoid this happening by return an error -EBUSY. Gadget function > > driver also should avoid such things, but that is out of udc driver. > > > > Similarly, disable a disabled ep makes no sense, but no need return > > an error here. > > > > Signed-off-by: Du, Changbin <changbin.du@intel.com> > > --- > > drivers/usb/dwc2/core.h | 1 + > > drivers/usb/dwc2/gadget.c | 20 +++++++++++++++++++- > > 2 files changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h > > index a66d3cb..cf7eccd 100644 > > --- a/drivers/usb/dwc2/core.h > > +++ b/drivers/usb/dwc2/core.h > > @@ -162,6 +162,7 @@ struct dwc2_hsotg_ep { > > unsigned char mc; > > unsigned char interval; > > > > + unsigned int enabled:1; > > unsigned int halted:1; > > unsigned int periodic:1; > > unsigned int isochronous:1; > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > > index 0abf73c..586bbcd 100644 > > --- a/drivers/usb/dwc2/gadget.c > > +++ b/drivers/usb/dwc2/gadget.c > > @@ -2423,6 +2423,7 @@ void dwc2_hsotg_core_init_disconnected(struct > dwc2_hsotg *hsotg, > > /* enable, but don't activate EP0in */ > > dwc2_writel(dwc2_hsotg_ep0_mps(hsotg->eps_out[0]- > >ep.maxpacket) | > > DXEPCTL_USBACTEP, hsotg->regs + DIEPCTL0); > > + hsotg->eps_out[0]->enabled = 1; > > > > dwc2_hsotg_enqueue_setup(hsotg); > > > > @@ -2680,6 +2681,14 @@ static int dwc2_hsotg_ep_enable(struct usb_ep > *ep, > > return -EINVAL; > > } > > > > + spin_lock_irqsave(&hsotg->lock, flags); > > + if (hs_ep->enabled) { > > + dev_warn(hsotg->dev, "%s: ep %s already enabled\n", > > + __func__, hs_ep->name); > > this is a rather serious condition. I'd rather use dev_WARN_ONCE(): > > if (dev_WARN_ONCE(hsotg->dev, hs_ep->enabled, > "ep %s already enabled\n", hs_ep->name)) { > > -- > balbi ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] usb: dwc2: forbid queuing request to a disabled ep 2015-11-30 5:21 [PATCH 0/2] Two fix for dwc2 gadget driver changbin.du 2015-11-30 5:21 ` [PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable changbin.du @ 2015-11-30 5:21 ` changbin.du 2015-12-10 17:27 ` Felipe Balbi 2015-12-03 1:20 ` [PATCH 0/2] Two fix for dwc2 gadget driver John Youn 2 siblings, 1 reply; 18+ messages in thread From: changbin.du @ 2015-11-30 5:21 UTC (permalink / raw) To: johnyoun; +Cc: gregkh, linux-usb, linux-kernel, Du, Changbin From: "Du, Changbin" <changbin.du@intel.com> Queue a request to disabled ep doesn't make sense, and induce caller make mistakes. Here is a example for the android mtp gadget function driver. A mem corruption can happen on below senario. 1) On disconnect, mtp driver disable its EPs, 2) During send_file_work and receive_file_work, mtp queues a request to ep. (The mtp driver need improve its synchronization logic!) 3) mtp_function_unbind is invoked and all mtp requests are freed. 4) when dwc2 process the request queued on step 2, will cause kernel NULL pointer dereference exception. Signed-off-by: Du, Changbin <changbin.du@intel.com> --- drivers/usb/dwc2/gadget.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 586bbcd..4d637ab 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -786,6 +786,12 @@ static int dwc2_hsotg_ep_queue(struct usb_ep *ep, struct usb_request *req, ep->name, req, req->length, req->buf, req->no_interrupt, req->zero, req->short_not_ok); + if (!hs_ep->enabled) { + dev_warn(hs->dev, "%s: cannot queue to disabled ep\n", + __func__); + return -ESHUTDOWN; + } + /* Prevent new request submission when controller is suspended */ if (hs->lx_state == DWC2_L2) { dev_dbg(hs->dev, "%s: don't submit request while suspended\n", -- 2.5.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] usb: dwc2: forbid queuing request to a disabled ep 2015-11-30 5:21 ` [PATCH 2/2] usb: dwc2: forbid queuing request to a disabled ep changbin.du @ 2015-12-10 17:27 ` Felipe Balbi 0 siblings, 0 replies; 18+ messages in thread From: Felipe Balbi @ 2015-12-10 17:27 UTC (permalink / raw) To: changbin.du, johnyoun; +Cc: gregkh, linux-usb, linux-kernel, Du, Changbin [-- Attachment #1: Type: text/plain, Size: 1957 bytes --] Hi, changbin.du@intel.com writes: > From: "Du, Changbin" <changbin.du@intel.com> > > Queue a request to disabled ep doesn't make sense, and induce caller > make mistakes. > > Here is a example for the android mtp gadget function driver. A mem > corruption can happen on below senario. > 1) On disconnect, mtp driver disable its EPs, > 2) During send_file_work and receive_file_work, mtp queues a request > to ep. (The mtp driver need improve its synchronization logic!) > 3) mtp_function_unbind is invoked and all mtp requests are freed. > 4) when dwc2 process the request queued on step 2, will cause kernel > NULL pointer dereference exception. > > Signed-off-by: Du, Changbin <changbin.du@intel.com> > --- > drivers/usb/dwc2/gadget.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index 586bbcd..4d637ab 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -786,6 +786,12 @@ static int dwc2_hsotg_ep_queue(struct usb_ep *ep, struct usb_request *req, > ep->name, req, req->length, req->buf, req->no_interrupt, > req->zero, req->short_not_ok); > > + if (!hs_ep->enabled) { > + dev_warn(hs->dev, "%s: cannot queue to disabled ep\n", > + __func__); similar comment to previous patch: if (dev_WARN_ONCE(hs->dev, !hs_ep->enabled, "cannot queue to disabled ep %s\n", hs_ep->name)) > + return -ESHUTDOWN; > + } > + > /* Prevent new request submission when controller is suspended */ > if (hs->lx_state == DWC2_L2) { > dev_dbg(hs->dev, "%s: don't submit request while suspended\n", > -- > 2.5.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] Two fix for dwc2 gadget driver 2015-11-30 5:21 [PATCH 0/2] Two fix for dwc2 gadget driver changbin.du 2015-11-30 5:21 ` [PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable changbin.du 2015-11-30 5:21 ` [PATCH 2/2] usb: dwc2: forbid queuing request to a disabled ep changbin.du @ 2015-12-03 1:20 ` John Youn 2015-12-03 4:23 ` Du, Changbin 2 siblings, 1 reply; 18+ messages in thread From: John Youn @ 2015-12-03 1:20 UTC (permalink / raw) To: changbin.du, John.Youn; +Cc: gregkh, linux-usb, linux-kernel, balbi On 11/29/2015 9:29 PM, changbin.du@intel.com wrote: > From: "Du, Changbin" <changbin.du@intel.com> > > With the first patch, enable a enabled ep will return -EBUSY. > The second patch forbid queuing on disabled ep to avoid panic. The usb_ep->enabled flag was added in 4.4. It looks like these same checks are also added at the API level in the usb_ep_enable() and usb_ep_disable(). In case this is bypassed we should probably add them in the gadget anyways but using the existing flag. Regards, John > > Du, Changbin (2): > usb: dwc2: add ep enabled flag to avoid double enable/disable > usb: dwc2: forbid queuing request to a disabled ep > > drivers/usb/dwc2/core.h | 1 + > drivers/usb/dwc2/gadget.c | 26 +++++++++++++++++++++++++- > 2 files changed, 26 insertions(+), 1 deletion(-) > ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 0/2] Two fix for dwc2 gadget driver 2015-12-03 1:20 ` [PATCH 0/2] Two fix for dwc2 gadget driver John Youn @ 2015-12-03 4:23 ` Du, Changbin 2015-12-04 7:21 ` [PATCH] usb: gadget: forbid queuing request to a disabled ep changbin.du 0 siblings, 1 reply; 18+ messages in thread From: Du, Changbin @ 2015-12-03 4:23 UTC (permalink / raw) To: John Youn; +Cc: gregkh, linux-usb, linux-kernel, balbi > On 11/29/2015 9:29 PM, changbin.du@intel.com wrote: > > From: "Du, Changbin" <changbin.du@intel.com> > > > > With the first patch, enable a enabled ep will return -EBUSY. > > The second patch forbid queuing on disabled ep to avoid panic. > > > The usb_ep->enabled flag was added in 4.4. > > It looks like these same checks are also added at the API level in the > usb_ep_enable() and usb_ep_disable(). > > In case this is bypassed we should probably add them in the gadget > anyways but using the existing flag. > > Regards, > John > Hmm, just learnt the flag on gadget API layer. And I just see usb_ep_enable return success if it is already enabled. But I think it should return an error to inform the caller. Because the ep configuration may probably be changed. In this case, usb_ep_enable will do different behavior. Hmm, the usb_ep_queue doesn't check the enabled flag. Should be added. Let me have a try. Best Regards, Changbin ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] usb: gadget: forbid queuing request to a disabled ep 2015-12-03 4:23 ` Du, Changbin @ 2015-12-04 7:21 ` changbin.du 2015-12-10 17:28 ` Felipe Balbi 0 siblings, 1 reply; 18+ messages in thread From: changbin.du @ 2015-12-04 7:21 UTC (permalink / raw) To: balbi; +Cc: gregkh, John.Youn, linux-usb, linux-kernel, Du, Changbin From: "Du, Changbin" <changbin.du@intel.com> Queue a request to disabled ep doesn't make sense, and induce caller make mistakes. Here is a example for the android mtp gadget function driver. A mem corruption can happen on below senario. 1) On disconnect, mtp driver disable its EPs, 2) During send_file_work and receive_file_work, mtp queues a request to ep. (The mtp driver need improve its synchronization logic!) 3) mtp_function_unbind is invoked and all mtp requests are freed. 4) when udc process the request queued on step 2, will cause kernel NULL pointer dereference exception. Signed-off-by: Du, Changbin <changbin.du@intel.com> --- This patch is seprated from below patches because gadget layer has added the 'enabled' flag in v4.4. so abandon it and submit new one. [PATCH 0/2] Two fix for dwc2 gadget driver usb: dwc2: add ep enabled flag to avoid double enable/disable usb: dwc2: forbid queuing request to a disabled ep --- include/linux/usb/gadget.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index 3d583a1..d813bd2 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep *ep, static inline int usb_ep_queue(struct usb_ep *ep, struct usb_request *req, gfp_t gfp_flags) { + if (!ep->enabled) + return -ESHUTDOWN; + return ep->ops->queue(ep, req, gfp_flags); } -- 2.5.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] usb: gadget: forbid queuing request to a disabled ep 2015-12-04 7:21 ` [PATCH] usb: gadget: forbid queuing request to a disabled ep changbin.du @ 2015-12-10 17:28 ` Felipe Balbi 2015-12-14 3:48 ` [PATCH v2] " changbin.du 0 siblings, 1 reply; 18+ messages in thread From: Felipe Balbi @ 2015-12-10 17:28 UTC (permalink / raw) To: changbin.du; +Cc: gregkh, John.Youn, linux-usb, linux-kernel, Du, Changbin [-- Attachment #1: Type: text/plain, Size: 1740 bytes --] Hi, changbin.du@intel.com writes: > From: "Du, Changbin" <changbin.du@intel.com> > > Queue a request to disabled ep doesn't make sense, and induce caller > make mistakes. > > Here is a example for the android mtp gadget function driver. A mem > corruption can happen on below senario. > 1) On disconnect, mtp driver disable its EPs, > 2) During send_file_work and receive_file_work, mtp queues a request > to ep. (The mtp driver need improve its synchronization logic!) > 3) mtp_function_unbind is invoked and all mtp requests are freed. > 4) when udc process the request queued on step 2, will cause kernel > NULL pointer dereference exception. > > Signed-off-by: Du, Changbin <changbin.du@intel.com> > --- > This patch is seprated from below patches because gadget layer has > added the 'enabled' flag in v4.4. so abandon it and submit new one. > [PATCH 0/2] Two fix for dwc2 gadget driver > usb: dwc2: add ep enabled flag to avoid double enable/disable > usb: dwc2: forbid queuing request to a disabled ep > > --- > include/linux/usb/gadget.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > index 3d583a1..d813bd2 100644 > --- a/include/linux/usb/gadget.h > +++ b/include/linux/usb/gadget.h > @@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep *ep, > static inline int usb_ep_queue(struct usb_ep *ep, > struct usb_request *req, gfp_t gfp_flags) > { > + if (!ep->enabled) > + return -ESHUTDOWN; same warn here: if (WARN_ON_ONCE(!ep->enabled)) return -ESHUTDOWN; > + > return ep->ops->queue(ep, req, gfp_flags); > } > > -- > 2.5.0 > -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2] usb: gadget: forbid queuing request to a disabled ep 2015-12-10 17:28 ` Felipe Balbi @ 2015-12-14 3:48 ` changbin.du 2015-12-14 10:20 ` Du, Changbin 0 siblings, 1 reply; 18+ messages in thread From: changbin.du @ 2015-12-14 3:48 UTC (permalink / raw) To: balbi; +Cc: gregkh, John.Youn, linux-usb, linux-kernel, Du, Changbin From: "Du, Changbin" <changbin.du@intel.com> Queue a request to disabled ep doesn't make sense, and induce caller make mistakes. Here is a example for the android mtp gadget function driver. A mem corruption can happen on below senario. 1) On disconnect, mtp driver disable its EPs, 2) During send_file_work and receive_file_work, mtp queues a request to ep. (The mtp driver need improve its synchronization logic!) 3) mtp_function_unbind is invoked and all mtp requests are freed. 4) when udc process the request queued on step 2, will cause kernel NULL pointer dereference exception. Signed-off-by: Du, Changbin <changbin.du@intel.com> --- change from v1: add WARN_ON_ONCE message. --- include/linux/usb/gadget.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index 3d583a1..b566a4b 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep *ep, static inline int usb_ep_queue(struct usb_ep *ep, struct usb_request *req, gfp_t gfp_flags) { + if (WARN_ON_ONCE(!ep->enabled)) + return -ESHUTDOWN; + return ep->ops->queue(ep, req, gfp_flags); } -- 2.5.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep 2015-12-14 3:48 ` [PATCH v2] " changbin.du @ 2015-12-14 10:20 ` Du, Changbin 2015-12-16 16:52 ` Felipe Balbi 0 siblings, 1 reply; 18+ messages in thread From: Du, Changbin @ 2015-12-14 10:20 UTC (permalink / raw) To: balbi; +Cc: gregkh, John.Youn, linux-usb, linux-kernel, lkp, Wu, Fengguang > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > index 3d583a1..b566a4b 100644 > --- a/include/linux/usb/gadget.h > +++ b/include/linux/usb/gadget.h > @@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep > *ep, > static inline int usb_ep_queue(struct usb_ep *ep, > struct usb_request *req, gfp_t gfp_flags) > { > + if (WARN_ON_ONCE(!ep->enabled)) > + return -ESHUTDOWN; > + > return ep->ops->queue(ep, req, gfp_flags); > } > > -- > 2.5.0 With this patch, ep0 transfer breaks. it because the 'enabled' of ep0 is not set. Ep0 is not enabled by usb_ep_enable, but in UDC driver. So there need another patch to set ep0's flag also. ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep 2015-12-14 10:20 ` Du, Changbin @ 2015-12-16 16:52 ` Felipe Balbi 2015-12-17 9:35 ` Du, Changbin 2015-12-17 10:00 ` [PATCH v3] " changbin.du 0 siblings, 2 replies; 18+ messages in thread From: Felipe Balbi @ 2015-12-16 16:52 UTC (permalink / raw) To: Du, Changbin Cc: gregkh, John.Youn, linux-usb, linux-kernel, lkp, Wu, Fengguang [-- Attachment #1: Type: text/plain, Size: 909 bytes --] Hi, "Du, Changbin" <changbin.du@intel.com> writes: >> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h >> index 3d583a1..b566a4b 100644 >> --- a/include/linux/usb/gadget.h >> +++ b/include/linux/usb/gadget.h >> @@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep >> *ep, >> static inline int usb_ep_queue(struct usb_ep *ep, >> struct usb_request *req, gfp_t gfp_flags) >> { >> + if (WARN_ON_ONCE(!ep->enabled)) >> + return -ESHUTDOWN; >> + >> return ep->ops->queue(ep, req, gfp_flags); >> } >> >> -- >> 2.5.0 > > With this patch, ep0 transfer breaks. it because the 'enabled' of ep0 > is not set. Ep0 is not enabled by usb_ep_enable, but in UDC driver. So > there need another patch to set ep0's flag also. yeah, we don't like regressions :-) So the fix should come before $subject to avoid a regression. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep 2015-12-16 16:52 ` Felipe Balbi @ 2015-12-17 9:35 ` Du, Changbin 2015-12-17 10:00 ` [PATCH v3] " changbin.du 1 sibling, 0 replies; 18+ messages in thread From: Du, Changbin @ 2015-12-17 9:35 UTC (permalink / raw) To: Felipe Balbi Cc: gregkh, John.Youn, linux-usb, linux-kernel, lkp, Wu, Fengguang > >> 2.5.0 > > > > With this patch, ep0 transfer breaks. it because the 'enabled' of ep0 > > is not set. Ep0 is not enabled by usb_ep_enable, but in UDC driver. So > > there need another patch to set ep0's flag also. > > yeah, we don't like regressions :-) So the fix should come before > $subject to avoid a regression. > > -- > balbi It is hard to determine if ep0 is enabled or not in gadget API layer. Because it is controlled by udc driver, it may enable it at pullup, vbussession... But here, we can ignore for control-ep, considering it always enabled during usb session. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3] usb: gadget: forbid queuing request to a disabled ep 2015-12-16 16:52 ` Felipe Balbi 2015-12-17 9:35 ` Du, Changbin @ 2015-12-17 10:00 ` changbin.du 2015-12-17 15:26 ` Felipe Balbi 1 sibling, 1 reply; 18+ messages in thread From: changbin.du @ 2015-12-17 10:00 UTC (permalink / raw) To: balbi Cc: gregkh, John.Youn, linux-usb, linux-kernel, lkp, fengguang.wu, Du, Changbin From: "Du, Changbin" <changbin.du@intel.com> Queue a request to disabled ep doesn't make sense, and induce caller make mistakes. Here is a example for the android mtp gadget function driver. A mem corruption can happen on below senario. 1) On disconnect, mtp driver disable its EPs, 2) During send_file_work and receive_file_work, mtp queues a request to ep. (The mtp driver need improve its synchronization logic!) 3) mtp_function_unbind is invoked and all mtp requests are freed. 4) when udc process the request queued on step 2, will cause kernel NULL pointer dereference exception. Signed-off-by: Du, Changbin <changbin.du@intel.com> --- change from v2: igonre ep0 as it always enabled during usb session. change from v1: add WARN_ON_ONCE message. --- include/linux/usb/gadget.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index 3d583a1..0c5d9ea 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep *ep, static inline int usb_ep_queue(struct usb_ep *ep, struct usb_request *req, gfp_t gfp_flags) { + if (WARN_ON_ONCE(!ep->enabled && !ep->address)) + return -ESHUTDOWN; + return ep->ops->queue(ep, req, gfp_flags); } -- 2.5.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3] usb: gadget: forbid queuing request to a disabled ep 2015-12-17 10:00 ` [PATCH v3] " changbin.du @ 2015-12-17 15:26 ` Felipe Balbi 2015-12-18 7:34 ` Du, Changbin 2015-12-18 7:36 ` [PATCH v4] " changbin.du 0 siblings, 2 replies; 18+ messages in thread From: Felipe Balbi @ 2015-12-17 15:26 UTC (permalink / raw) To: changbin.du Cc: gregkh, John.Youn, linux-usb, linux-kernel, lkp, fengguang.wu, Du, Changbin [-- Attachment #1: Type: text/plain, Size: 1471 bytes --] Hi, changbin.du@intel.com writes: > From: "Du, Changbin" <changbin.du@intel.com> > > Queue a request to disabled ep doesn't make sense, and induce caller > make mistakes. > > Here is a example for the android mtp gadget function driver. A mem > corruption can happen on below senario. > 1) On disconnect, mtp driver disable its EPs, > 2) During send_file_work and receive_file_work, mtp queues a request > to ep. (The mtp driver need improve its synchronization logic!) > 3) mtp_function_unbind is invoked and all mtp requests are freed. > 4) when udc process the request queued on step 2, will cause kernel > NULL pointer dereference exception. > > Signed-off-by: Du, Changbin <changbin.du@intel.com> > --- > change from v2: igonre ep0 as it always enabled during usb session. > change from v1: add WARN_ON_ONCE message. > --- > include/linux/usb/gadget.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > index 3d583a1..0c5d9ea 100644 > --- a/include/linux/usb/gadget.h > +++ b/include/linux/usb/gadget.h > @@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep *ep, > static inline int usb_ep_queue(struct usb_ep *ep, > struct usb_request *req, gfp_t gfp_flags) > { > + if (WARN_ON_ONCE(!ep->enabled && !ep->address)) this will only trigger for a disabled ep0. Are you testing any of your patches at all ? -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v3] usb: gadget: forbid queuing request to a disabled ep 2015-12-17 15:26 ` Felipe Balbi @ 2015-12-18 7:34 ` Du, Changbin 2015-12-18 7:36 ` [PATCH v4] " changbin.du 1 sibling, 0 replies; 18+ messages in thread From: Du, Changbin @ 2015-12-18 7:34 UTC (permalink / raw) To: Felipe Balbi Cc: gregkh, John.Youn, linux-usb, linux-kernel, lkp, Wu, Fengguang > > > > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > > index 3d583a1..0c5d9ea 100644 > > --- a/include/linux/usb/gadget.h > > +++ b/include/linux/usb/gadget.h > > @@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct > usb_ep *ep, > > static inline int usb_ep_queue(struct usb_ep *ep, > > struct usb_request *req, gfp_t gfp_flags) > > { > > + if (WARN_ON_ONCE(!ep->enabled && !ep->address)) > > this will only trigger for a disabled ep0. Are you testing any of your > patches at all ? > > -- > balbi Oops, I sent a wrong patch. I will send right patch again as v4, very sorry for this. The right patch has been verified on 3.14 by back-porting related 1 patch. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4] usb: gadget: forbid queuing request to a disabled ep 2015-12-17 15:26 ` Felipe Balbi 2015-12-18 7:34 ` Du, Changbin @ 2015-12-18 7:36 ` changbin.du 1 sibling, 0 replies; 18+ messages in thread From: changbin.du @ 2015-12-18 7:36 UTC (permalink / raw) To: balbi Cc: gregkh, John.Youn, linux-usb, linux-kernel, lkp, fengguang.wu, Du, Changbin From: "Du, Changbin" <changbin.du@intel.com> Queue a request to disabled ep doesn't make sense, and induce caller make mistakes. Here is a example for the android mtp gadget function driver. A mem corruption can happen on below senario. 1) On disconnect, mtp driver disable its EPs, 2) During send_file_work and receive_file_work, mtp queues a request to ep. (The mtp driver need improve its synchronization logic!) 3) mtp_function_unbind is invoked and all mtp requests are freed. 4) when udc process the request queued on step 2, will cause kernel NULL pointer dereference exception. Signed-off-by: Du, Changbin <changbin.du@intel.com> --- change from v3: fix v3's error. change from v2: igonre ep0 as it always enabled during usb session. change from v1: add WARN_ON_ONCE message. --- include/linux/usb/gadget.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index 3d583a1..fc78687 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep *ep, static inline int usb_ep_queue(struct usb_ep *ep, struct usb_request *req, gfp_t gfp_flags) { + if (WARN_ON_ONCE(!ep->enabled && ep->address)) + return -ESHUTDOWN; + return ep->ops->queue(ep, req, gfp_flags); } -- 2.5.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
end of thread, other threads:[~2015-12-18 7:45 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-30 5:21 [PATCH 0/2] Two fix for dwc2 gadget driver changbin.du 2015-11-30 5:21 ` [PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable changbin.du 2015-12-10 17:26 ` Felipe Balbi 2015-12-14 3:23 ` Du, Changbin 2015-11-30 5:21 ` [PATCH 2/2] usb: dwc2: forbid queuing request to a disabled ep changbin.du 2015-12-10 17:27 ` Felipe Balbi 2015-12-03 1:20 ` [PATCH 0/2] Two fix for dwc2 gadget driver John Youn 2015-12-03 4:23 ` Du, Changbin 2015-12-04 7:21 ` [PATCH] usb: gadget: forbid queuing request to a disabled ep changbin.du 2015-12-10 17:28 ` Felipe Balbi 2015-12-14 3:48 ` [PATCH v2] " changbin.du 2015-12-14 10:20 ` Du, Changbin 2015-12-16 16:52 ` Felipe Balbi 2015-12-17 9:35 ` Du, Changbin 2015-12-17 10:00 ` [PATCH v3] " changbin.du 2015-12-17 15:26 ` Felipe Balbi 2015-12-18 7:34 ` Du, Changbin 2015-12-18 7:36 ` [PATCH v4] " changbin.du
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).