Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1279570 > unrolled thread

[PATCH 0/2] Two fix for dwc2 gadget driver

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] Two fix for dwc2 gadget driver changbin.du@intel.com - 2015-11-30 06:30 +0100
    [PATCH 2/2] usb: dwc2: forbid queuing request to a disabled ep changbin.du@intel.com - 2015-11-30 06:40 +0100
      Re: [PATCH 2/2] usb: dwc2: forbid queuing request to a disabled ep Felipe Balbi <balbi@ti.com> - 2015-12-10 18:30 +0100
    [PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable changbin.du@intel.com - 2015-11-30 06:40 +0100
      Re: [PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable Felipe Balbi <balbi@ti.com> - 2015-12-10 18:30 +0100
    Re: [PATCH 0/2] Two fix for dwc2 gadget driver John Youn <John.Youn@synopsys.com> - 2015-12-03 02:30 +0100
      RE: [PATCH 0/2] Two fix for dwc2 gadget driver "Du, Changbin" <changbin.du@intel.com> - 2015-12-03 05:30 +0100
        [PATCH] usb: gadget: forbid queuing request to a disabled ep changbin.du@intel.com - 2015-12-04 08:30 +0100
          Re: [PATCH] usb: gadget: forbid queuing request to a disabled ep Felipe Balbi <balbi@ti.com> - 2015-12-10 18:30 +0100

#1279570 — [PATCH 0/2] Two fix for dwc2 gadget driver

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

--
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/

[toc] | [next] | [standalone]

#1279571 — [PATCH 2/2] usb: dwc2: forbid queuing request to a disabled ep

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

--
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/

[toc] | [prev] | [next] | [standalone]

#1288707 — Re: [PATCH 2/2] usb: dwc2: forbid queuing request to a disabled ep

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1279573 — [PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable

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

--
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/

[toc] | [prev] | [next] | [standalone]

#1288706 — Re: [PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1282634

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(-)
> 

--
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/

[toc] | [prev] | [next] | [standalone]

#1282694

> 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
--
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/

[toc] | [prev] | [next] | [standalone]

#1283605 — [PATCH] usb: gadget: forbid queuing request to a disabled ep

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

--
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/

[toc] | [prev] | [next] | [standalone]

#1288700 — Re: [PATCH] usb: gadget: forbid queuing request to a disabled ep

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web