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

Groups > linux.kernel > #1290821 > unrolled thread

[PATCH v2] usb: gadget: forbid queuing request to a disabled ep

Started bychangbin.du@intel.com
First post2015-12-14 05:00 +0100
Last post2015-12-18 08:50 +0100
Articles 8 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2] usb: gadget: forbid queuing request to a disabled ep changbin.du@intel.com - 2015-12-14 05:00 +0100
    RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep "Du, Changbin" <changbin.du@intel.com> - 2015-12-14 11:30 +0100
      RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep Felipe Balbi <balbi@ti.com> - 2015-12-16 18:00 +0100
        RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep "Du, Changbin" <changbin.du@intel.com> - 2015-12-17 10:40 +0100
        [PATCH v3] usb: gadget: forbid queuing request to a disabled ep changbin.du@intel.com - 2015-12-17 11:10 +0100
          Re: [PATCH v3] usb: gadget: forbid queuing request to a disabled ep Felipe Balbi <balbi@ti.com> - 2015-12-17 16:30 +0100
            RE: [PATCH v3] usb: gadget: forbid queuing request to a disabled ep "Du, Changbin" <changbin.du@intel.com> - 2015-12-18 08:40 +0100
            [PATCH v4] usb: gadget: forbid queuing request to a disabled ep changbin.du@intel.com - 2015-12-18 08:50 +0100

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

Fromchangbin.du@intel.com
Date2015-12-14 05:00 +0100
Subject[PATCH v2] usb: gadget: forbid queuing request to a disabled ep
Message-ID<qFssO-HZ-7@gated-at.bofh.it>
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

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

#1291057

From"Du, Changbin" <changbin.du@intel.com>
Date2015-12-14 11:30 +0100
Message-ID<qFyye-4PE-7@gated-at.bofh.it>
In reply to#1290821
> 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.

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

#1293116

FromFelipe Balbi <balbi@ti.com>
Date2015-12-16 18:00 +0100
Message-ID<qGnAN-4iQ-85@gated-at.bofh.it>
In reply to#1291057

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

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

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

#1293704

From"Du, Changbin" <changbin.du@intel.com>
Date2015-12-17 10:40 +0100
Message-ID<qGDcu-5Vw-5@gated-at.bofh.it>
In reply to#1293116
> >> 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.
--
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]

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

Fromchangbin.du@intel.com
Date2015-12-17 11:10 +0100
Subject[PATCH v3] usb: gadget: forbid queuing request to a disabled ep
Message-ID<qGDFv-6lt-5@gated-at.bofh.it>
In reply to#1293116
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

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

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

FromFelipe Balbi <balbi@ti.com>
Date2015-12-17 16:30 +0100
SubjectRe: [PATCH v3] usb: gadget: forbid queuing request to a disabled ep
Message-ID<qGIFc-1cH-5@gated-at.bofh.it>
In reply to#1293725

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

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

#1294470 — RE: [PATCH v3] usb: gadget: forbid queuing request to a disabled ep

From"Du, Changbin" <changbin.du@intel.com>
Date2015-12-18 08:40 +0100
SubjectRE: [PATCH v3] usb: gadget: forbid queuing request to a disabled ep
Message-ID<qGXNT-2Cn-3@gated-at.bofh.it>
In reply to#1293977
> >
> > 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.
--
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]

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

Fromchangbin.du@intel.com
Date2015-12-18 08:50 +0100
Subject[PATCH v4] usb: gadget: forbid queuing request to a disabled ep
Message-ID<qGXXz-2FR-17@gated-at.bofh.it>
In reply to#1293977
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

--
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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web