From: "Du, Changbin" <[email protected]>
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
From: "Du, Changbin" <[email protected]>
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 <[email protected]>
---
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
From: "Du, Changbin" <[email protected]>
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 <[email protected]>
---
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
On 11/29/2015 9:29 PM, [email protected] wrote:
> From: "Du, Changbin" <[email protected]>
>
> 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(-)
>
> On 11/29/2015 9:29 PM, [email protected] wrote:
> > From: "Du, Changbin" <[email protected]>
> >
> > 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
From: "Du, Changbin" <[email protected]>
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 <[email protected]>
---
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
Hi,
[email protected] writes:
> From: "Du, Changbin" <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
Hi,
[email protected] writes:
> From: "Du, Changbin" <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
balbi
Hi,
[email protected] writes:
> From: "Du, Changbin" <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
From: "Du, Changbin" <[email protected]>
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 <[email protected]>
---
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
> 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.
Hi,
"Du, Changbin" <[email protected]> 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
> >> 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.
From: "Du, Changbin" <[email protected]>
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 <[email protected]>
---
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
Hi,
[email protected] writes:
> From: "Du, Changbin" <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
> >
> > 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.
From: "Du, Changbin" <[email protected]>
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 <[email protected]>
---
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