FAILED: patch "[PATCH] usb: xhci: Handle error condition in xhci_stop_device()" failed to apply to 4.9-stable tree

PATCH In review [email protected] 2 participants 5 messages Oct 23, 2017 – Oct 30, 2017

Subsystems (3)

Stable branch USB USB xHCI driver

lore

Share & tools

lore

[email protected]

author maintainer

2017-10-23 12:52 UTC #0 lore raw

The patch below does not apply to the 4.9-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

Quoted 1 line

From b3207c65dfafae27e7c492cb9188c0dc0eeaf3fd Mon Sep 17 00:00:00 2001

From: Mayank Rana <[email protected]>
Date: Fri, 6 Oct 2017 17:45:30 +0300
Subject: [PATCH] usb: xhci: Handle error condition in xhci_stop_device()

xhci_stop_device() calls xhci_queue_stop_endpoint() multiple times without checking the return value. xhci_queue_stop_endpoint() can return error if the HC is already halted or unable to queue commands. This can cause a deadlock condition as xhci_stop_device() would end up waiting indefinitely for a completion for the command that didn't get queued. Fix this by checking the return value and bailing out of xhci_stop_device() in case of error. This patch happens to fix potential memory leaks of the allocated command structures as well.

Fixes c311e391a7ef ("xhci: rework command timeout and cancellation,")

Cc <[email protected]>

Signed-off-by Mayank Rana <[email protected]>

Signed-off-by Jack Pham <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

drivers/usb/host/xhci-hub.c +18 -5

index da9158f171cb..a2336deb5e36 100644
@@ -420,14 +420,25 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
 						     GFP_NOWAIT);
 			if (!command) {
 				spin_unlock_irqrestore(&xhci->lock, flags);
-				xhci_free_command(xhci, cmd);
-				return -ENOMEM;
+				ret = -ENOMEM;
+				goto cmd_cleanup;
+			}
+
+			ret = xhci_queue_stop_endpoint(xhci, command, slot_id,
+						       i, suspend);
+			if (ret) {
+				spin_unlock_irqrestore(&xhci->lock, flags);
+				xhci_free_command(xhci, command);
+				goto cmd_cleanup;
 			}
-			xhci_queue_stop_endpoint(xhci, command, slot_id, i,
-						 suspend);
 		}
 	}
-	xhci_queue_stop_endpoint(xhci, cmd, slot_id, 0, suspend);
+	ret = xhci_queue_stop_endpoint(xhci, cmd, slot_id, 0, suspend);
+	if (ret) {
+		spin_unlock_irqrestore(&xhci->lock, flags);
+		goto cmd_cleanup;
+	}
+
 	xhci_ring_cmd_db(xhci);
 	spin_unlock_irqrestore(&xhci->lock, flags);

@@ -439,6 +450,8 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
 		xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n");
 		ret = -ETIME;
 	}
+
+cmd_cleanup:
 	xhci_free_command(xhci, cmd);
 	return ret;
 }

Jack Pham

2017-10-24 01:13 UTC #1 lore raw

Hi Greg,

Quoted 5 lines · On Mon, Oct 23, 2017 at 02:52:50PM +0200, [email protected] wrote:

The patch below does not apply to the 4.9-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

It appears to be a trivial conflict. See below.

Quoted 28 lines

------------------ original commit in Linus's tree ------------------
From b3207c65dfafae27e7c492cb9188c0dc0eeaf3fd Mon Sep 17 00:00:00 2001
From: Mayank Rana <[email protected]>
Date: Fri, 6 Oct 2017 17:45:30 +0300
Subject: [PATCH] usb: xhci: Handle error condition in xhci_stop_device()
xhci_stop_device() calls xhci_queue_stop_endpoint() multiple times without checking the return value. xhci_queue_stop_endpoint() can return error if the HC is already halted or unable to queue commands. This can cause a deadlock condition as xhci_stop_device() would end up waiting indefinitely for a completion for the command that didn't get queued. Fix this by checking the return value and bailing out of xhci_stop_device() in case of error. This patch happens to fix potential memory leaks of the allocated command structures as well.
Fixes c311e391a7ef ("xhci: rework command timeout and cancellation,")
Cc <[email protected]>
Signed-off-by Mayank Rana <[email protected]>
Signed-off-by Jack Pham <[email protected]>
Signed-off-by Mathias Nyman <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
drivers/usb/host/xhci-hub.c +0 -0
index da9158f171cb..a2336deb5e36 100644

drivers/usb/host/xhci-hub.c static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend) · line 424

Quoted 5 lines

drivers/usb/host/xhci-hub.c +0 -2

@@ -420,14 +420,25 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
 						     GFP_NOWAIT);
 			if (!command) {
 				spin_unlock_irqrestore(&xhci->lock, flags);
-				xhci_free_command(xhci, cmd);
-				return -ENOMEM;

The conflict appears to be here. There was an extra blank line after this that had been removed by another patch in 4.13. So when applying to the older stable trees which still has the blank line I guess git couldn't resolve it.

Is this enough info for you to fix up or would you need me to send a revised backport patch? If I send, would I need to add my S-o-b again after the other tags?

Jack

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Greg KH

author maintainer

2017-10-24 10:22 UTC #2 lore raw

Quoted 50 lines · Jack Pham

Hi Greg,
Quoted 5 lines · On Mon, Oct 23, 2017 at 02:52:50PM +0200, [email protected] wrote:
The patch below does not apply to the 4.9-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
It appears to be a trivial conflict. See below.
Quoted 28 lines
------------------ original commit in Linus's tree ------------------
From b3207c65dfafae27e7c492cb9188c0dc0eeaf3fd Mon Sep 17 00:00:00 2001
From: Mayank Rana <[email protected]>
Date: Fri, 6 Oct 2017 17:45:30 +0300
Subject: [PATCH] usb: xhci: Handle error condition in xhci_stop_device()
xhci_stop_device() calls xhci_queue_stop_endpoint() multiple times without checking the return value. xhci_queue_stop_endpoint() can return error if the HC is already halted or unable to queue commands. This can cause a deadlock condition as xhci_stop_device() would end up waiting indefinitely for a completion for the command that didn't get queued. Fix this by checking the return value and bailing out of xhci_stop_device() in case of error. This patch happens to fix potential memory leaks of the allocated command structures as well.
Fixes c311e391a7ef ("xhci: rework command timeout and cancellation,")
Cc <[email protected]>
Signed-off-by Mayank Rana <[email protected]>
Signed-off-by Jack Pham <[email protected]>
Signed-off-by Mathias Nyman <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
drivers/usb/host/xhci-hub.c +0 -0
index da9158f171cb..a2336deb5e36 100644
drivers/usb/host/xhci-hub.c static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend) · line 424
Quoted 5 lines
drivers/usb/host/xhci-hub.c +0 -2
@@ -420,14 +420,25 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
 						     GFP_NOWAIT);
 			if (!command) {
 				spin_unlock_irqrestore(&xhci->lock, flags);
-				xhci_free_command(xhci, cmd);
-				return -ENOMEM;
The conflict appears to be here. There was an extra blank line after this that had been removed by another patch in 4.13. So when applying to the older stable trees which still has the blank line I guess git couldn't resolve it.

Quoted 3 lines

Is this enough info for you to fix up or would you need me to send a revised backport patch? If I send, would I need to add my S-o-b again after the other tags?

I need a new patch please, that makes it the easiest for me to ensure I got it right :)

And if you want to add a new s-o-b, that's fine as well, but not needed.

thanks,

greg k-h

Jack Pham

2017-10-24 16:16 UTC #3 lore raw

PATCH 1/1 usb: xhci: Handle error condition in xhci_stop_device()

From: Mayank Rana <[email protected]>

commit b3207c65dfafae27e7c492cb9188c0dc0eeaf3fd upstream.

Fixes c311e391a7ef ("xhci: rework command timeout and cancellation,")

Signed-off-by Mayank Rana <[email protected]>

Signed-off-by Jack Pham <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
Hi Greg,

I tested that this applies cleanly to 3.18.y, 4.4.y and 4.9.y.

Thanks,
jack

 drivers/usb/host/xhci-hub.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

drivers/usb/host/xhci-hub.c +17 -5

index 4a02c5c..0722f75 100644
@@ -412,15 +412,25 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
 						     GFP_NOWAIT);
 			if (!command) {
 				spin_unlock_irqrestore(&xhci->lock, flags);
-				xhci_free_command(xhci, cmd);
-				return -ENOMEM;
+				ret = -ENOMEM;
+				goto cmd_cleanup;
+			}
 
+			ret = xhci_queue_stop_endpoint(xhci, command, slot_id,
+						       i, suspend);
+			if (ret) {
+				spin_unlock_irqrestore(&xhci->lock, flags);
+				xhci_free_command(xhci, command);
+				goto cmd_cleanup;
 			}
-			xhci_queue_stop_endpoint(xhci, command, slot_id, i,
-						 suspend);
 		}
 	}
-	xhci_queue_stop_endpoint(xhci, cmd, slot_id, 0, suspend);
+	ret = xhci_queue_stop_endpoint(xhci, cmd, slot_id, 0, suspend);
+	if (ret) {
+		spin_unlock_irqrestore(&xhci->lock, flags);
+		goto cmd_cleanup;
+	}
+
 	xhci_ring_cmd_db(xhci);
 	spin_unlock_irqrestore(&xhci->lock, flags);

@@ -431,6 +441,8 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
 		xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n");
 		ret = -ETIME;
 	}
+
+cmd_cleanup:
 	xhci_free_command(xhci, cmd);
 	return ret;
 }

Signature · 1 line

2.9.1.200.gb1ec08f

Greg KH

author maintainer

2017-10-30 09:02 UTC #4 lore raw

Re: [PATCH] usb: xhci: Handle error condition in xhci_stop_device()

Quoted 18 lines · Jack Pham

From: Mayank Rana <[email protected]>
commit b3207c65dfafae27e7c492cb9188c0dc0eeaf3fd upstream.
xhci_stop_device() calls xhci_queue_stop_endpoint() multiple times without checking the return value. xhci_queue_stop_endpoint() can return error if the HC is already halted or unable to queue commands. This can cause a deadlock condition as xhci_stop_device() would end up waiting indefinitely for a completion for the command that didn't get queued. Fix this by checking the return value and bailing out of xhci_stop_device() in case of error. This patch happens to fix potential memory leaks of the allocated command structures as well.
Fixes c311e391a7ef ("xhci: rework command timeout and cancellation,")
Signed-off-by Mayank Rana <[email protected]>
Signed-off-by Jack Pham <[email protected]>
Signed-off-by Mathias Nyman <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>

Quoted 4 lines

---
Hi Greg,
I tested that this applies cleanly to 3.18.y, 4.4.y and 4.9.y.

Thanks for the patch, now queued up.

greg k-h