[PATCH v3] usb: dwc3: gadget: Move null pinter check after window closed

[PATCH v3] usb: dwc3: gadget: Move null pinter check after window closed

[Albert Wang]

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

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))
-- 
2.36.0.550.gb090851708-goog

Re: [PATCH v3] usb: dwc3: gadget: Move null pinter check after window closed

[Greg KH]

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

Re: [PATCH v3] usb: dwc3: gadget: Move null pinter check after window closed

[Jack Pham]

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