diff mbox series

usbip: usbip_host: fix stub_dev lock context imbalance regression

Message ID 20190529194615.18765-1-skhan@linuxfoundation.org (mailing list archive)
State Mainlined
Commit 3ea3091f1bd8586125848c62be295910e9802af0
Headers show
Series usbip: usbip_host: fix stub_dev lock context imbalance regression | expand

Commit Message

Shuah Khan May 29, 2019, 7:46 p.m. UTC
Fix the following sparse context imbalance regression introduced in
a patch that fixed sleeping function called from invalid context bug.

kbuild test robot reported on:

tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git  usb-linus

Regressions in current branch:

drivers/usb/usbip/stub_dev.c:399:9: sparse: sparse: context imbalance in 'stub_probe' - different lock contexts for basic block
drivers/usb/usbip/stub_dev.c:418:13: sparse: sparse: context imbalance in 'stub_disconnect' - different lock contexts for basic block
drivers/usb/usbip/stub_dev.c:464:1-10: second lock on line 476

Error ids grouped by kconfigs:

recent_errors
├── i386-allmodconfig
│   └── drivers-usb-usbip-stub_dev.c:second-lock-on-line
├── x86_64-allmodconfig
│   ├── drivers-usb-usbip-stub_dev.c:sparse:sparse:context-imbalance-in-stub_disconnect-different-lock-contexts-for-basic-block
│   └── drivers-usb-usbip-stub_dev.c:sparse:sparse:context-imbalance-in-stub_probe-different-lock-contexts-for-basic-block
└── x86_64-allyesconfig
    └── drivers-usb-usbip-stub_dev.c:second-lock-on-line

This is a real problem in an error leg where spin_lock() is called on an
already held lock.

Fix the imbalance in stub_probe() and stub_disconnect().

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/usb/usbip/stub_dev.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

Comments

Greg KH May 29, 2019, 8:25 p.m. UTC | #1
On Wed, May 29, 2019 at 01:46:15PM -0600, Shuah Khan wrote:
> Fix the following sparse context imbalance regression introduced in
> a patch that fixed sleeping function called from invalid context bug.
> 
> kbuild test robot reported on:
> 
> tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git  usb-linus
> 
> Regressions in current branch:
> 
> drivers/usb/usbip/stub_dev.c:399:9: sparse: sparse: context imbalance in 'stub_probe' - different lock contexts for basic block
> drivers/usb/usbip/stub_dev.c:418:13: sparse: sparse: context imbalance in 'stub_disconnect' - different lock contexts for basic block
> drivers/usb/usbip/stub_dev.c:464:1-10: second lock on line 476
> 
> Error ids grouped by kconfigs:
> 
> recent_errors
> ├── i386-allmodconfig
> │   └── drivers-usb-usbip-stub_dev.c:second-lock-on-line
> ├── x86_64-allmodconfig
> │   ├── drivers-usb-usbip-stub_dev.c:sparse:sparse:context-imbalance-in-stub_disconnect-different-lock-contexts-for-basic-block
> │   └── drivers-usb-usbip-stub_dev.c:sparse:sparse:context-imbalance-in-stub_probe-different-lock-contexts-for-basic-block
> └── x86_64-allyesconfig
>     └── drivers-usb-usbip-stub_dev.c:second-lock-on-line
> 
> This is a real problem in an error leg where spin_lock() is called on an
> already held lock.
> 
> Fix the imbalance in stub_probe() and stub_disconnect().
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

I'll go add:

Fixes: 0c9e8b3cad65 ("usbip: usbip_host: fix BUG: sleeping function called from invalid context")
Cc: stable <stable@vger.kernel.org>

as the patch this fixes was tagged for stable.

thanks,

greg k-h
Shuah Khan May 29, 2019, 11:19 p.m. UTC | #2
On 5/29/19 2:25 PM, Greg KH wrote:
> On Wed, May 29, 2019 at 01:46:15PM -0600, Shuah Khan wrote:
>> Fix the following sparse context imbalance regression introduced in
>> a patch that fixed sleeping function called from invalid context bug.
>>
>> kbuild test robot reported on:
>>
>> tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git  usb-linus
>>
>> Regressions in current branch:
>>
>> drivers/usb/usbip/stub_dev.c:399:9: sparse: sparse: context imbalance in 'stub_probe' - different lock contexts for basic block
>> drivers/usb/usbip/stub_dev.c:418:13: sparse: sparse: context imbalance in 'stub_disconnect' - different lock contexts for basic block
>> drivers/usb/usbip/stub_dev.c:464:1-10: second lock on line 476
>>
>> Error ids grouped by kconfigs:
>>
>> recent_errors
>> ├── i386-allmodconfig
>> │   └── drivers-usb-usbip-stub_dev.c:second-lock-on-line
>> ├── x86_64-allmodconfig
>> │   ├── drivers-usb-usbip-stub_dev.c:sparse:sparse:context-imbalance-in-stub_disconnect-different-lock-contexts-for-basic-block
>> │   └── drivers-usb-usbip-stub_dev.c:sparse:sparse:context-imbalance-in-stub_probe-different-lock-contexts-for-basic-block
>> └── x86_64-allyesconfig
>>      └── drivers-usb-usbip-stub_dev.c:second-lock-on-line
>>
>> This is a real problem in an error leg where spin_lock() is called on an
>> already held lock.
>>
>> Fix the imbalance in stub_probe() and stub_disconnect().
>>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> 
> I'll go add:
> 
> Fixes: 0c9e8b3cad65 ("usbip: usbip_host: fix BUG: sleeping function called from invalid context")
> Cc: stable <stable@vger.kernel.org>
> 
> as the patch this fixes was tagged for stable.
> 

Sounds good. Thanks.

-- Shuah
diff mbox series

Patch

diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index d094c96643d2..7931e6cecc70 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -326,14 +326,17 @@  static int stub_probe(struct usb_device *udev)
 		 * See driver_probe_device() in driver/base/dd.c
 		 */
 		rc = -ENODEV;
-		goto sdev_free;
+		if (!busid_priv)
+			goto sdev_free;
+
+		goto call_put_busid_priv;
 	}
 
 	if (udev->descriptor.bDeviceClass == USB_CLASS_HUB) {
 		dev_dbg(&udev->dev, "%s is a usb hub device... skip!\n",
 			 udev_busid);
 		rc = -ENODEV;
-		goto sdev_free;
+		goto call_put_busid_priv;
 	}
 
 	if (!strcmp(udev->bus->bus_name, "vhci_hcd")) {
@@ -342,7 +345,7 @@  static int stub_probe(struct usb_device *udev)
 			udev_busid);
 
 		rc = -ENODEV;
-		goto sdev_free;
+		goto call_put_busid_priv;
 	}
 
 
@@ -361,6 +364,9 @@  static int stub_probe(struct usb_device *udev)
 	save_status = busid_priv->status;
 	busid_priv->status = STUB_BUSID_ALLOC;
 
+	/* release the busid_lock */
+	put_busid_priv(busid_priv);
+
 	/*
 	 * Claim this hub port.
 	 * It doesn't matter what value we pass as owner
@@ -373,9 +379,6 @@  static int stub_probe(struct usb_device *udev)
 		goto err_port;
 	}
 
-	/* release the busid_lock */
-	put_busid_priv(busid_priv);
-
 	rc = stub_add_files(&udev->dev);
 	if (rc) {
 		dev_err(&udev->dev, "stub_add_files for %s\n", udev_busid);
@@ -395,11 +398,17 @@  static int stub_probe(struct usb_device *udev)
 	spin_lock(&busid_priv->busid_lock);
 	busid_priv->sdev = NULL;
 	busid_priv->status = save_status;
-sdev_free:
-	stub_device_free(sdev);
+	spin_unlock(&busid_priv->busid_lock);
+	/* lock is released - go to free */
+	goto sdev_free;
+
+call_put_busid_priv:
 	/* release the busid_lock */
 	put_busid_priv(busid_priv);
 
+sdev_free:
+	stub_device_free(sdev);
+
 	return rc;
 }
 
@@ -435,7 +444,9 @@  static void stub_disconnect(struct usb_device *udev)
 	/* get stub_device */
 	if (!sdev) {
 		dev_err(&udev->dev, "could not get device");
-		goto call_put_busid_priv;
+		/* release busid_lock */
+		put_busid_priv(busid_priv);
+		return;
 	}
 
 	dev_set_drvdata(&udev->dev, NULL);
@@ -465,7 +476,7 @@  static void stub_disconnect(struct usb_device *udev)
 	if (!busid_priv->shutdown_busid)
 		busid_priv->shutdown_busid = 1;
 	/* release busid_lock */
-	put_busid_priv(busid_priv);
+	spin_unlock(&busid_priv->busid_lock);
 
 	/* shutdown the current connection */
 	shutdown_busid(busid_priv);
@@ -480,10 +491,9 @@  static void stub_disconnect(struct usb_device *udev)
 
 	if (busid_priv->status == STUB_BUSID_ALLOC)
 		busid_priv->status = STUB_BUSID_ADDED;
-
-call_put_busid_priv:
 	/* release busid_lock */
-	put_busid_priv(busid_priv);
+	spin_unlock(&busid_priv->busid_lock);
+	return;
 }
 
 #ifdef CONFIG_PM