RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep
> >> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep
> >> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep
Hi, "Du, Changbin" 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 signature.asc Description: PGP signature
RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep
Hi, "Du, Changbin"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 signature.asc Description: PGP signature
RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep
> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep
> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] usb: gadget: forbid queuing request to a disabled ep
From: "Du, Changbin" 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 --- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] usb: gadget: forbid queuing request to a disabled ep
From: "Du, Changbin"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 --- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/