| Message ID | 20220513065709.1606634-1-albertccwang@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] usb: dwc3: gadget: Move null pinter check after window closed | expand |
On Fri, May 13, 2022 at 02:57:09PM +0800, Albert Wang wrote: > After inspecting further, we do see the locking is implicit, with the > main gotcha being the unlock/re-lock. This sentance makes no sense at all. Who is "we"? What is the gotcha? What is the subject of the sentance? What is going on? > That creates a window for a race to happen. What is "that"? > This change moves the NULL check to be adjacent to where > to it's used and after the window is "closed". What is "this"? Please read Documentation/process/submitting-patches.rst for how to properly write a changelog text so that others can understand what is going on. thanks, greg k-h > > Fixes: 26288448120b ("usb: dwc3: gadget: Fix null pointer exception") > Signed-off-by: Albert Wang <albertccwang@google.com> > --- > v3: Add change log to be compliant with the canonical patch format > v2: Remove redundant 'else' and add additional comments and more > descriptive commit text > > drivers/usb/dwc3/gadget.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 19477f4bbf54..fda58951cf27 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -3366,14 +3366,19 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, > struct dwc3 *dwc = dep->dwc; > bool no_started_trb = true; > > - if (!dep->endpoint.desc) > - return no_started_trb; > - > + /* > + * This function eventually leads to dwc3_giveback() which unlocks > + * the dwc->lock and relocks afterwards. This actually creates a > + * a window for a race to happen. What race? Why mention it here? Why not fix it instead of documenting it? this comment does not make sense, sorry. thanks, greg k-h
On Fri, May 13, 2022 at 09:12:17AM +0200, Greg KH wrote: > On Fri, May 13, 2022 at 02:57:09PM +0800, Albert Wang wrote: > > After inspecting further, we do see the locking is implicit, with the > > main gotcha being the unlock/re-lock. > > This sentance makes no sense at all. > > Who is "we"? What is the gotcha? What is the subject of the sentance? > What is going on? > > > That creates a window for a race to happen. > > What is "that"? > > > This change moves the NULL check to be adjacent to where > > to it's used and after the window is "closed". > > What is "this"? > > Please read Documentation/process/submitting-patches.rst for how to > properly write a changelog text so that others can understand what is > going on. Albert, it looks like you took my reply comments verbatim. These were in context to questions Greg asked about locking or apparent lack thereof. But on their own they really don't make sense as Greg poitns out. Could you please write up the text (in your own words) in a way that would be clear to a person seeing this patch for the first time? Jack > > Fixes: 26288448120b ("usb: dwc3: gadget: Fix null pointer exception") > > Signed-off-by: Albert Wang <albertccwang@google.com> > > --- > > v3: Add change log to be compliant with the canonical patch format > > v2: Remove redundant 'else' and add additional comments and more > > descriptive commit text > > > > drivers/usb/dwc3/gadget.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > index 19477f4bbf54..fda58951cf27 100644 > > --- a/drivers/usb/dwc3/gadget.c > > +++ b/drivers/usb/dwc3/gadget.c > > @@ -3366,14 +3366,19 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, > > struct dwc3 *dwc = dep->dwc; > > bool no_started_trb = true; > > > > - if (!dep->endpoint.desc) > > - return no_started_trb; > > - > > + /* > > + * This function eventually leads to dwc3_giveback() which unlocks > > + * the dwc->lock and relocks afterwards. This actually creates a > > + * a window for a race to happen. > > What race? Why mention it here? Why not fix it instead of documenting > it? > > this comment does not make sense, sorry. > > thanks, > > greg k-h
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 19477f4bbf54..fda58951cf27 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -3366,14 +3366,19 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, struct dwc3 *dwc = dep->dwc; bool no_started_trb = true; - if (!dep->endpoint.desc) - return no_started_trb; - + /* + * This function eventually leads to dwc3_giveback() which unlocks + * the dwc->lock and relocks afterwards. This actually creates a + * a window for a race to happen. + */ dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) goto out; + if (!dep->endpoint.desc) + return no_started_trb; + if (usb_endpoint_xfer_isoc(dep->endpoint.desc) && list_empty(&dep->started_list) && (list_empty(&dep->pending_list) || status == -EXDEV))
After inspecting further, we do see the locking is implicit, with the main gotcha being the unlock/re-lock. That creates a window for a race to happen. This change moves the NULL check to be adjacent to where to it's used and after the window is "closed". Fixes: 26288448120b ("usb: dwc3: gadget: Fix null pointer exception") Signed-off-by: Albert Wang <albertccwang@google.com> --- v3: Add change log to be compliant with the canonical patch format v2: Remove redundant 'else' and add additional comments and more descriptive commit text drivers/usb/dwc3/gadget.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)