Re: possible deadlock in open_rio

Re: possible deadlock in open_rio

[Alan Stern]

On Wed, 7 Aug 2019, Oliver Neukum wrote:

> Am Mittwoch, den 07.08.2019, 10:07 -0400 schrieb Alan Stern:
> > On Wed, 7 Aug 2019, Oliver Neukum wrote:

> > > technically yes. However in practical terms the straight revert I sent
> > > out yesterday should fix it.
> > 
> > I didn't see the revert, and it doesn't appear to have reached the 
> > mailing list archive.  Can you post it again?
> 
> As soon as our VPN server is back up again.

The revert may not be necessay; a little fix should get rid of the
locking violation.  The key is to avoid calling the registration or
deregistration routines while holding the rio500_mutex, and to
recognize that the probe and disconnect routines are both protected by
the device mutex.

How does this patch look?

Alan Stern


#syz test: https://github.com/google/kasan.git 7f7867ff

Index: usb-devel/drivers/usb/misc/rio500.c
===================================================================
--- usb-devel.orig/drivers/usb/misc/rio500.c
+++ usb-devel/drivers/usb/misc/rio500.c
@@ -454,52 +454,54 @@ static int probe_rio(struct usb_interfac
 {
 	struct usb_device *dev = interface_to_usbdev(intf);
 	struct rio_usb_data *rio = &rio_instance;
-	int retval = 0;
+	int retval;
+	char *ibuf, *obuf;
 
-	mutex_lock(&rio500_mutex);
 	if (rio->present) {
 		dev_info(&intf->dev, "Second USB Rio at address %d refused\n", dev->devnum);
-		retval = -EBUSY;
-		goto bail_out;
-	} else {
-		dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
+		return -EBUSY;
 	}
+	dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
 
 	retval = usb_register_dev(intf, &usb_rio_class);
 	if (retval) {
 		dev_err(&dev->dev,
 			"Not able to get a minor for this device.\n");
-		retval = -ENOMEM;
-		goto bail_out;
+		goto err_register;
 	}
 
-	rio->rio_dev = dev;
-
-	if (!(rio->obuf = kmalloc(OBUF_SIZE, GFP_KERNEL))) {
+	obuf = kmalloc(OBUF_SIZE, GFP_KERNEL);
+	if (!obuf) {
 		dev_err(&dev->dev,
 			"probe_rio: Not enough memory for the output buffer\n");
-		usb_deregister_dev(intf, &usb_rio_class);
-		retval = -ENOMEM;
-		goto bail_out;
+		goto err_obuf;
 	}
-	dev_dbg(&intf->dev, "obuf address:%p\n", rio->obuf);
+	dev_dbg(&intf->dev, "obuf address: %p\n", obuf);
 
-	if (!(rio->ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL))) {
+	ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL);
+	if (!ibuf) {
 		dev_err(&dev->dev,
 			"probe_rio: Not enough memory for the input buffer\n");
-		usb_deregister_dev(intf, &usb_rio_class);
-		kfree(rio->obuf);
-		retval = -ENOMEM;
-		goto bail_out;
+		goto err_ibuf;
 	}
-	dev_dbg(&intf->dev, "ibuf address:%p\n", rio->ibuf);
+	dev_dbg(&intf->dev, "ibuf address: %p\n", ibuf);
 
+	mutex_lock(&rio500_mutex);
+	rio->rio_dev = dev;
+	rio->ibuf = ibuf;
+	rio->obuf = obuf;
 	usb_set_intfdata (intf, rio);
 	rio->present = 1;
-bail_out:
 	mutex_unlock(&rio500_mutex);
 
 	return retval;
+
+ err_ibuf:
+	kfree(obuf);
+ err_obuf:
+	usb_deregister_dev(intf, &usb_rio_class);
+ err_register:
+	return -ENOMEM;
 }
 
 static void disconnect_rio(struct usb_interface *intf)
@@ -507,10 +509,10 @@ static void disconnect_rio(struct usb_in
 	struct rio_usb_data *rio = usb_get_intfdata (intf);
 
 	usb_set_intfdata (intf, NULL);
-	mutex_lock(&rio500_mutex);
 	if (rio) {
 		usb_deregister_dev(intf, &usb_rio_class);
 
+		mutex_lock(&rio500_mutex);
 		if (rio->isopen) {
 			rio->isopen = 0;
 			/* better let it finish - the release will do whats needed */
@@ -524,8 +526,8 @@ static void disconnect_rio(struct usb_in
 		dev_info(&intf->dev, "USB Rio disconnected.\n");
 
 		rio->present = 0;
+		mutex_unlock(&rio500_mutex);
 	}
-	mutex_unlock(&rio500_mutex);
 }
 
 static const struct usb_device_id rio_table[] = {

Re: Re: possible deadlock in open_rio

[syzbot]

> On Wed, 7 Aug 2019, Oliver Neukum wrote:

>> Am Mittwoch, den 07.08.2019, 10:07 -0400 schrieb Alan Stern:
>> > On Wed, 7 Aug 2019, Oliver Neukum wrote:

>> > > technically yes. However in practical terms the straight revert I  
>> sent
>> > > out yesterday should fix it.
>> >
>> > I didn't see the revert, and it doesn't appear to have reached the
>> > mailing list archive.  Can you post it again?

>> As soon as our VPN server is back up again.

> The revert may not be necessay; a little fix should get rid of the
> locking violation.  The key is to avoid calling the registration or
> deregistration routines while holding the rio500_mutex, and to
> recognize that the probe and disconnect routines are both protected by
> the device mutex.

> How does this patch look?

> Alan Stern


> #syz test: https://github.com/google/kasan.git 7f7867ff

This crash does not have a reproducer. I cannot test it.


> Index: usb-devel/drivers/usb/misc/rio500.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/misc/rio500.c
> +++ usb-devel/drivers/usb/misc/rio500.c
> @@ -454,52 +454,54 @@ static int probe_rio(struct usb_interfac
>   {
>   	struct usb_device *dev = interface_to_usbdev(intf);
>   	struct rio_usb_data *rio = &rio_instance;
> -	int retval = 0;
> +	int retval;
> +	char *ibuf, *obuf;

> -	mutex_lock(&rio500_mutex);
>   	if (rio->present) {
>   		dev_info(&intf->dev, "Second USB Rio at address %d refused\n",  
> dev->devnum);
> -		retval = -EBUSY;
> -		goto bail_out;
> -	} else {
> -		dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
> +		return -EBUSY;
>   	}
> +	dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);

>   	retval = usb_register_dev(intf, &usb_rio_class);
>   	if (retval) {
>   		dev_err(&dev->dev,
>   			"Not able to get a minor for this device.\n");
> -		retval = -ENOMEM;
> -		goto bail_out;
> +		goto err_register;
>   	}

> -	rio->rio_dev = dev;
> -
> -	if (!(rio->obuf = kmalloc(OBUF_SIZE, GFP_KERNEL))) {
> +	obuf = kmalloc(OBUF_SIZE, GFP_KERNEL);
> +	if (!obuf) {
>   		dev_err(&dev->dev,
>   			"probe_rio: Not enough memory for the output buffer\n");
> -		usb_deregister_dev(intf, &usb_rio_class);
> -		retval = -ENOMEM;
> -		goto bail_out;
> +		goto err_obuf;
>   	}
> -	dev_dbg(&intf->dev, "obuf address:%p\n", rio->obuf);
> +	dev_dbg(&intf->dev, "obuf address: %p\n", obuf);

> -	if (!(rio->ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL))) {
> +	ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL);
> +	if (!ibuf) {
>   		dev_err(&dev->dev,
>   			"probe_rio: Not enough memory for the input buffer\n");
> -		usb_deregister_dev(intf, &usb_rio_class);
> -		kfree(rio->obuf);
> -		retval = -ENOMEM;
> -		goto bail_out;
> +		goto err_ibuf;
>   	}
> -	dev_dbg(&intf->dev, "ibuf address:%p\n", rio->ibuf);
> +	dev_dbg(&intf->dev, "ibuf address: %p\n", ibuf);

> +	mutex_lock(&rio500_mutex);
> +	rio->rio_dev = dev;
> +	rio->ibuf = ibuf;
> +	rio->obuf = obuf;
>   	usb_set_intfdata (intf, rio);
>   	rio->present = 1;
> -bail_out:
>   	mutex_unlock(&rio500_mutex);

>   	return retval;
> +
> + err_ibuf:
> +	kfree(obuf);
> + err_obuf:
> +	usb_deregister_dev(intf, &usb_rio_class);
> + err_register:
> +	return -ENOMEM;
>   }

>   static void disconnect_rio(struct usb_interface *intf)
> @@ -507,10 +509,10 @@ static void disconnect_rio(struct usb_in
>   	struct rio_usb_data *rio = usb_get_intfdata (intf);

>   	usb_set_intfdata (intf, NULL);
> -	mutex_lock(&rio500_mutex);
>   	if (rio) {
>   		usb_deregister_dev(intf, &usb_rio_class);

> +		mutex_lock(&rio500_mutex);
>   		if (rio->isopen) {
>   			rio->isopen = 0;
>   			/* better let it finish - the release will do whats needed */
> @@ -524,8 +526,8 @@ static void disconnect_rio(struct usb_in
>   		dev_info(&intf->dev, "USB Rio disconnected.\n");

>   		rio->present = 0;
> +		mutex_unlock(&rio500_mutex);
>   	}
> -	mutex_unlock(&rio500_mutex);
>   }

>   static const struct usb_device_id rio_table[] = {

Re: Re: possible deadlock in open_rio

[syzbot]

> On Wed, 7 Aug 2019, Oliver Neukum wrote:

>> Am Mittwoch, den 07.08.2019, 10:07 -0400 schrieb Alan Stern:
>> > On Wed, 7 Aug 2019, Oliver Neukum wrote:

>> > > technically yes. However in practical terms the straight revert I  
>> sent
>> > > out yesterday should fix it.
>> >
>> > I didn't see the revert, and it doesn't appear to have reached the
>> > mailing list archive.  Can you post it again?

>> As soon as our VPN server is back up again.

> The revert may not be necessay; a little fix should get rid of the
> locking violation.  The key is to avoid calling the registration or
> deregistration routines while holding the rio500_mutex, and to
> recognize that the probe and disconnect routines are both protected by
> the device mutex.

> How does this patch look?

> Alan Stern


> #syz test: https://github.com/google/kasan.git 7f7867ff

This crash does not have a reproducer. I cannot test it.


> Index: usb-devel/drivers/usb/misc/rio500.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/misc/rio500.c
> +++ usb-devel/drivers/usb/misc/rio500.c
> @@ -454,52 +454,54 @@ static int probe_rio(struct usb_interfac
>   {
>   	struct usb_device *dev = interface_to_usbdev(intf);
>   	struct rio_usb_data *rio = &rio_instance;
> -	int retval = 0;
> +	int retval;
> +	char *ibuf, *obuf;

> -	mutex_lock(&rio500_mutex);
>   	if (rio->present) {
>   		dev_info(&intf->dev, "Second USB Rio at address %d refused\n",  
> dev->devnum);
> -		retval = -EBUSY;
> -		goto bail_out;
> -	} else {
> -		dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
> +		return -EBUSY;
>   	}
> +	dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);

>   	retval = usb_register_dev(intf, &usb_rio_class);
>   	if (retval) {
>   		dev_err(&dev->dev,
>   			"Not able to get a minor for this device.\n");
> -		retval = -ENOMEM;
> -		goto bail_out;
> +		goto err_register;
>   	}

> -	rio->rio_dev = dev;
> -
> -	if (!(rio->obuf = kmalloc(OBUF_SIZE, GFP_KERNEL))) {
> +	obuf = kmalloc(OBUF_SIZE, GFP_KERNEL);
> +	if (!obuf) {
>   		dev_err(&dev->dev,
>   			"probe_rio: Not enough memory for the output buffer\n");
> -		usb_deregister_dev(intf, &usb_rio_class);
> -		retval = -ENOMEM;
> -		goto bail_out;
> +		goto err_obuf;
>   	}
> -	dev_dbg(&intf->dev, "obuf address:%p\n", rio->obuf);
> +	dev_dbg(&intf->dev, "obuf address: %p\n", obuf);

> -	if (!(rio->ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL))) {
> +	ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL);
> +	if (!ibuf) {
>   		dev_err(&dev->dev,
>   			"probe_rio: Not enough memory for the input buffer\n");
> -		usb_deregister_dev(intf, &usb_rio_class);
> -		kfree(rio->obuf);
> -		retval = -ENOMEM;
> -		goto bail_out;
> +		goto err_ibuf;
>   	}
> -	dev_dbg(&intf->dev, "ibuf address:%p\n", rio->ibuf);
> +	dev_dbg(&intf->dev, "ibuf address: %p\n", ibuf);

> +	mutex_lock(&rio500_mutex);
> +	rio->rio_dev = dev;
> +	rio->ibuf = ibuf;
> +	rio->obuf = obuf;
>   	usb_set_intfdata (intf, rio);
>   	rio->present = 1;
> -bail_out:
>   	mutex_unlock(&rio500_mutex);

>   	return retval;
> +
> + err_ibuf:
> +	kfree(obuf);
> + err_obuf:
> +	usb_deregister_dev(intf, &usb_rio_class);
> + err_register:
> +	return -ENOMEM;
>   }

>   static void disconnect_rio(struct usb_interface *intf)
> @@ -507,10 +509,10 @@ static void disconnect_rio(struct usb_in
>   	struct rio_usb_data *rio = usb_get_intfdata (intf);

>   	usb_set_intfdata (intf, NULL);
> -	mutex_lock(&rio500_mutex);
>   	if (rio) {
>   		usb_deregister_dev(intf, &usb_rio_class);

> +		mutex_lock(&rio500_mutex);
>   		if (rio->isopen) {
>   			rio->isopen = 0;
>   			/* better let it finish - the release will do whats needed */
> @@ -524,8 +526,8 @@ static void disconnect_rio(struct usb_in
>   		dev_info(&intf->dev, "USB Rio disconnected.\n");

>   		rio->present = 0;
> +		mutex_unlock(&rio500_mutex);
>   	}
> -	mutex_unlock(&rio500_mutex);
>   }

>   static const struct usb_device_id rio_table[] = {

Re: possible deadlock in open_rio

[Andrey Konovalov]

On Thu, Aug 8, 2019 at 4:33 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, 7 Aug 2019, Oliver Neukum wrote:
>
> > Am Mittwoch, den 07.08.2019, 10:07 -0400 schrieb Alan Stern:
> > > On Wed, 7 Aug 2019, Oliver Neukum wrote:
>
> > > > technically yes. However in practical terms the straight revert I sent
> > > > out yesterday should fix it.
> > >
> > > I didn't see the revert, and it doesn't appear to have reached the
> > > mailing list archive.  Can you post it again?
> >
> > As soon as our VPN server is back up again.
>
> The revert may not be necessay; a little fix should get rid of the
> locking violation.  The key is to avoid calling the registration or
> deregistration routines while holding the rio500_mutex, and to
> recognize that the probe and disconnect routines are both protected by
> the device mutex.
>
> How does this patch look?
>
> Alan Stern
>
>
> #syz test: https://github.com/google/kasan.git 7f7867ff

There's no reproducer yet (it should appear at some point, I've
enabled fuzzing of USB char devices). I've tested your patch manually
and the deadlock report is gone. Thanks!

Tested-by: Andrey Konovalov <andreyknvl@google.com>

>
> Index: usb-devel/drivers/usb/misc/rio500.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/misc/rio500.c
> +++ usb-devel/drivers/usb/misc/rio500.c
> @@ -454,52 +454,54 @@ static int probe_rio(struct usb_interfac
>  {
>         struct usb_device *dev = interface_to_usbdev(intf);
>         struct rio_usb_data *rio = &rio_instance;
> -       int retval = 0;
> +       int retval;
> +       char *ibuf, *obuf;
>
> -       mutex_lock(&rio500_mutex);
>         if (rio->present) {
>                 dev_info(&intf->dev, "Second USB Rio at address %d refused\n", dev->devnum);
> -               retval = -EBUSY;
> -               goto bail_out;
> -       } else {
> -               dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
> +               return -EBUSY;
>         }
> +       dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
>
>         retval = usb_register_dev(intf, &usb_rio_class);
>         if (retval) {
>                 dev_err(&dev->dev,
>                         "Not able to get a minor for this device.\n");
> -               retval = -ENOMEM;
> -               goto bail_out;
> +               goto err_register;
>         }
>
> -       rio->rio_dev = dev;
> -
> -       if (!(rio->obuf = kmalloc(OBUF_SIZE, GFP_KERNEL))) {
> +       obuf = kmalloc(OBUF_SIZE, GFP_KERNEL);
> +       if (!obuf) {
>                 dev_err(&dev->dev,
>                         "probe_rio: Not enough memory for the output buffer\n");
> -               usb_deregister_dev(intf, &usb_rio_class);
> -               retval = -ENOMEM;
> -               goto bail_out;
> +               goto err_obuf;
>         }
> -       dev_dbg(&intf->dev, "obuf address:%p\n", rio->obuf);
> +       dev_dbg(&intf->dev, "obuf address: %p\n", obuf);
>
> -       if (!(rio->ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL))) {
> +       ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL);
> +       if (!ibuf) {
>                 dev_err(&dev->dev,
>                         "probe_rio: Not enough memory for the input buffer\n");
> -               usb_deregister_dev(intf, &usb_rio_class);
> -               kfree(rio->obuf);
> -               retval = -ENOMEM;
> -               goto bail_out;
> +               goto err_ibuf;
>         }
> -       dev_dbg(&intf->dev, "ibuf address:%p\n", rio->ibuf);
> +       dev_dbg(&intf->dev, "ibuf address: %p\n", ibuf);
>
> +       mutex_lock(&rio500_mutex);
> +       rio->rio_dev = dev;
> +       rio->ibuf = ibuf;
> +       rio->obuf = obuf;
>         usb_set_intfdata (intf, rio);
>         rio->present = 1;
> -bail_out:
>         mutex_unlock(&rio500_mutex);
>
>         return retval;
> +
> + err_ibuf:
> +       kfree(obuf);
> + err_obuf:
> +       usb_deregister_dev(intf, &usb_rio_class);
> + err_register:
> +       return -ENOMEM;
>  }
>
>  static void disconnect_rio(struct usb_interface *intf)
> @@ -507,10 +509,10 @@ static void disconnect_rio(struct usb_in
>         struct rio_usb_data *rio = usb_get_intfdata (intf);
>
>         usb_set_intfdata (intf, NULL);
> -       mutex_lock(&rio500_mutex);
>         if (rio) {
>                 usb_deregister_dev(intf, &usb_rio_class);
>
> +               mutex_lock(&rio500_mutex);
>                 if (rio->isopen) {
>                         rio->isopen = 0;
>                         /* better let it finish - the release will do whats needed */
> @@ -524,8 +526,8 @@ static void disconnect_rio(struct usb_in
>                 dev_info(&intf->dev, "USB Rio disconnected.\n");
>
>                 rio->present = 0;
> +               mutex_unlock(&rio500_mutex);
>         }
> -       mutex_unlock(&rio500_mutex);
>  }
>
>  static const struct usb_device_id rio_table[] = {
>

[PATCH] USB: rio500: Fix lockdep violation

[Alan Stern]

The syzbot fuzzer found a lockdep violation in the rio500 driver:

	======================================================
	WARNING: possible circular locking dependency detected
	5.3.0-rc2+ #23 Not tainted
	------------------------------------------------------
	syz-executor.2/20386 is trying to acquire lock:
	00000000772249c6 (rio500_mutex){+.+.}, at: open_rio+0x16/0xc0  
	drivers/usb/misc/rio500.c:64

	but task is already holding lock:
	00000000d3e8f4b9 (minor_rwsem){++++}, at: usb_open+0x23/0x270  
	drivers/usb/core/file.c:39

	which lock already depends on the new lock.

The problem is that the driver's open_rio() routine is called while
the usbcore's minor_rwsem is locked for reading, and it acquires the
rio500_mutex; whereas conversely, probe_rio() and disconnect_rio()
first acquire the rio500_mutex and then call usb_register_dev() or
usb_deregister_dev(), which lock minor_rwsem for writing.

The correct ordering of acquisition should be: minor_rwsem first, then
rio500_mutex (since the locking in open_rio() cannot be changed).
Thus, the probe and disconnect routines should avoid holding
rio500_mutex while doing their registration and deregistration.

This patch adjusts the code in those two routines to do just that.  It
also relies on the fact that the probe and disconnect routines are
protected by the device mutex, so the initial test of rio->present
needs no extra locking.

Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Fixes: d710734b0677 ("USB: rio500: simplify locking")
CC: Oliver Neukum <oneukum@suse.com>
CC: <stable@vger.kernel.org>

---

This patch is different from the one I posted earlier.  I realized that 
we don't want to register the device's char file until after the 
buffers have been allocated.


[as1906]


 drivers/usb/misc/rio500.c |   66 ++++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 31 deletions(-)

Index: usb-devel/drivers/usb/misc/rio500.c
===================================================================
--- usb-devel.orig/drivers/usb/misc/rio500.c
+++ usb-devel/drivers/usb/misc/rio500.c
@@ -454,51 +454,55 @@ static int probe_rio(struct usb_interfac
 {
 	struct usb_device *dev = interface_to_usbdev(intf);
 	struct rio_usb_data *rio = &rio_instance;
-	int retval = 0;
+	int retval = -ENOMEM;
+	char *ibuf, *obuf;
 
-	mutex_lock(&rio500_mutex);
 	if (rio->present) {
 		dev_info(&intf->dev, "Second USB Rio at address %d refused\n", dev->devnum);
-		retval = -EBUSY;
-		goto bail_out;
-	} else {
-		dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
+		return -EBUSY;
 	}
+	dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
 
-	retval = usb_register_dev(intf, &usb_rio_class);
-	if (retval) {
-		dev_err(&dev->dev,
-			"Not able to get a minor for this device.\n");
-		retval = -ENOMEM;
-		goto bail_out;
-	}
-
-	rio->rio_dev = dev;
-
-	if (!(rio->obuf = kmalloc(OBUF_SIZE, GFP_KERNEL))) {
+	obuf = kmalloc(OBUF_SIZE, GFP_KERNEL);
+	if (!obuf) {
 		dev_err(&dev->dev,
 			"probe_rio: Not enough memory for the output buffer\n");
-		usb_deregister_dev(intf, &usb_rio_class);
-		retval = -ENOMEM;
-		goto bail_out;
+		goto err_obuf;
 	}
-	dev_dbg(&intf->dev, "obuf address:%p\n", rio->obuf);
+	dev_dbg(&intf->dev, "obuf address: %p\n", obuf);
 
-	if (!(rio->ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL))) {
+	ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL);
+	if (!ibuf) {
 		dev_err(&dev->dev,
 			"probe_rio: Not enough memory for the input buffer\n");
-		usb_deregister_dev(intf, &usb_rio_class);
-		kfree(rio->obuf);
-		retval = -ENOMEM;
-		goto bail_out;
+		goto err_ibuf;
 	}
-	dev_dbg(&intf->dev, "ibuf address:%p\n", rio->ibuf);
+	dev_dbg(&intf->dev, "ibuf address: %p\n", ibuf);
 
-	usb_set_intfdata (intf, rio);
+	mutex_lock(&rio500_mutex);
+	rio->rio_dev = dev;
+	rio->ibuf = ibuf;
+	rio->obuf = obuf;
 	rio->present = 1;
-bail_out:
 	mutex_unlock(&rio500_mutex);
 
+	retval = usb_register_dev(intf, &usb_rio_class);
+	if (retval) {
+		dev_err(&dev->dev,
+			"Not able to get a minor for this device.\n");
+		goto err_register;
+	}
+
+	usb_set_intfdata(intf, rio);
+	return retval;
+
+ err_register:
+	mutex_lock(&rio500_mutex);
+	rio->present = 0;
+	mutex_unlock(&rio500_mutex);
+ err_ibuf:
+	kfree(obuf);
+ err_obuf:
 	return retval;
 }
 
@@ -507,10 +511,10 @@ static void disconnect_rio(struct usb_in
 	struct rio_usb_data *rio = usb_get_intfdata (intf);
 
 	usb_set_intfdata (intf, NULL);
-	mutex_lock(&rio500_mutex);
 	if (rio) {
 		usb_deregister_dev(intf, &usb_rio_class);
 
+		mutex_lock(&rio500_mutex);
 		if (rio->isopen) {
 			rio->isopen = 0;
 			/* better let it finish - the release will do whats needed */
@@ -524,8 +528,8 @@ static void disconnect_rio(struct usb_in
 		dev_info(&intf->dev, "USB Rio disconnected.\n");
 
 		rio->present = 0;
+		mutex_unlock(&rio500_mutex);
 	}
-	mutex_unlock(&rio500_mutex);
 }
 
 static const struct usb_device_id rio_table[] = {

Re: possible deadlock in iowarrior

[Alan Stern]

On Wed, 7 Aug 2019, Oliver Neukum wrote:

> > PS: syzbot reported a similar lock inversion problem (involving two
> > mutexes rather than just one) in drivers/usb/misc/iowarrior.c.  
> > Probably the two drivers need similar fixes.
> 
> No, but I got a fix.
> 
> 	Regards
> 		Oliver
> 
> From 973e82b3f583113e4d7fe5cd2f918a16022c4e38 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@suse.com>
> Date: Tue, 6 Aug 2019 16:17:54 +0200
> Subject: [PATCH] usb: iowarrior: fix deadlock on disconnect
> 
> We have to drop the mutex before we close() upon disconnect()
> as close() needs the lock. This is safe to do by dropping the
> mutex as intfdata is already set to NULL, so open() will fail.
> 
> Fixes: 03f36e885fc26 ("USB: open disconnect race in iowarrior")
> Reported-by: syzbot+a64a382964bf6c71a9c0@syzkaller.appspotmail.com
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/usb/misc/iowarrior.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
> index ba05dd80a020..f5bed9f29e56 100644
> --- a/drivers/usb/misc/iowarrior.c
> +++ b/drivers/usb/misc/iowarrior.c
> @@ -866,19 +866,20 @@ static void iowarrior_disconnect(struct usb_interface *interface)
>  	dev = usb_get_intfdata(interface);
>  	mutex_lock(&iowarrior_open_disc_lock);
>  	usb_set_intfdata(interface, NULL);
> +	/* prevent device read, write and ioctl */
> +	dev->present = 0;
>  
>  	minor = dev->minor;
> +	mutex_unlock(&iowarrior_open_disc_lock);
> +	/* give back our minor - this will call close() locks need to be dropped at this point*/

Ungrammatical and untrue: usb_deregister_dev() does not call close().

>  
> -	/* give back our minor */
>  	usb_deregister_dev(interface, &iowarrior_class);
>  
>  	mutex_lock(&dev->mutex);
>  
>  	/* prevent device read, write and ioctl */
> -	dev->present = 0;
>  
>  	mutex_unlock(&dev->mutex);
> -	mutex_unlock(&iowarrior_open_disc_lock);

That part looks weird.  The critical section is empty, except for a 
comment.  Maybe you meant to lock dev->mutex around the assignment to 
dev->present above?

In any case, this driver still seems to have at least one unnecessary 
mutex.  Look at the locking mess in iowarrior_open().

Alan Stern

Re: [PATCH] USB: rio500: Fix lockdep violation

[Greg KH]

On Thu, Aug 08, 2019 at 01:34:08PM -0400, Alan Stern wrote:
> The syzbot fuzzer found a lockdep violation in the rio500 driver:
> 
> 	======================================================
> 	WARNING: possible circular locking dependency detected
> 	5.3.0-rc2+ #23 Not tainted
> 	------------------------------------------------------
> 	syz-executor.2/20386 is trying to acquire lock:
> 	00000000772249c6 (rio500_mutex){+.+.}, at: open_rio+0x16/0xc0  
> 	drivers/usb/misc/rio500.c:64
> 
> 	but task is already holding lock:
> 	00000000d3e8f4b9 (minor_rwsem){++++}, at: usb_open+0x23/0x270  
> 	drivers/usb/core/file.c:39
> 
> 	which lock already depends on the new lock.
> 
> The problem is that the driver's open_rio() routine is called while
> the usbcore's minor_rwsem is locked for reading, and it acquires the
> rio500_mutex; whereas conversely, probe_rio() and disconnect_rio()
> first acquire the rio500_mutex and then call usb_register_dev() or
> usb_deregister_dev(), which lock minor_rwsem for writing.
> 
> The correct ordering of acquisition should be: minor_rwsem first, then
> rio500_mutex (since the locking in open_rio() cannot be changed).
> Thus, the probe and disconnect routines should avoid holding
> rio500_mutex while doing their registration and deregistration.
> 
> This patch adjusts the code in those two routines to do just that.  It
> also relies on the fact that the probe and disconnect routines are
> protected by the device mutex, so the initial test of rio->present
> needs no extra locking.
> 
> Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Fixes: d710734b0677 ("USB: rio500: simplify locking")
> CC: Oliver Neukum <oneukum@suse.com>
> CC: <stable@vger.kernel.org>
> 
> ---
> 
> This patch is different from the one I posted earlier.  I realized that 
> we don't want to register the device's char file until after the 
> buffers have been allocated.

Should I revert Oliver's patch?

confused,

greg k-h

Re: [PATCH] USB: rio500: Fix lockdep violation

[Alan Stern]

On Thu, 8 Aug 2019, Greg KH wrote:

> On Thu, Aug 08, 2019 at 01:34:08PM -0400, Alan Stern wrote:
> > The syzbot fuzzer found a lockdep violation in the rio500 driver:
> > 
> > 	======================================================
> > 	WARNING: possible circular locking dependency detected
> > 	5.3.0-rc2+ #23 Not tainted
> > 	------------------------------------------------------
> > 	syz-executor.2/20386 is trying to acquire lock:
> > 	00000000772249c6 (rio500_mutex){+.+.}, at: open_rio+0x16/0xc0  
> > 	drivers/usb/misc/rio500.c:64
> > 
> > 	but task is already holding lock:
> > 	00000000d3e8f4b9 (minor_rwsem){++++}, at: usb_open+0x23/0x270  
> > 	drivers/usb/core/file.c:39
> > 
> > 	which lock already depends on the new lock.
> > 
> > The problem is that the driver's open_rio() routine is called while
> > the usbcore's minor_rwsem is locked for reading, and it acquires the
> > rio500_mutex; whereas conversely, probe_rio() and disconnect_rio()
> > first acquire the rio500_mutex and then call usb_register_dev() or
> > usb_deregister_dev(), which lock minor_rwsem for writing.
> > 
> > The correct ordering of acquisition should be: minor_rwsem first, then
> > rio500_mutex (since the locking in open_rio() cannot be changed).
> > Thus, the probe and disconnect routines should avoid holding
> > rio500_mutex while doing their registration and deregistration.
> > 
> > This patch adjusts the code in those two routines to do just that.  It
> > also relies on the fact that the probe and disconnect routines are
> > protected by the device mutex, so the initial test of rio->present
> > needs no extra locking.
> > 
> > Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > Fixes: d710734b0677 ("USB: rio500: simplify locking")
> > CC: Oliver Neukum <oneukum@suse.com>
> > CC: <stable@vger.kernel.org>
> > 
> > ---
> > 
> > This patch is different from the one I posted earlier.  I realized that 
> > we don't want to register the device's char file until after the 
> > buffers have been allocated.
> 
> Should I revert Oliver's patch?

Sorry, I should have explained more clearly: This goes on top of 
Oliver's patch.  In fact, Oliver's patch is the one listed in the 
Fixes: tag.

You do not need to apply Oliver's reversion.  Assuming he agrees that 
this patch is correct, of course.

Alan Stern

Re: [PATCH] USB: rio500: Fix lockdep violation

[Greg KH]

On Thu, Aug 08, 2019 at 02:23:00PM -0400, Alan Stern wrote:
> On Thu, 8 Aug 2019, Greg KH wrote:
> 
> > On Thu, Aug 08, 2019 at 01:34:08PM -0400, Alan Stern wrote:
> > > The syzbot fuzzer found a lockdep violation in the rio500 driver:
> > > 
> > > 	======================================================
> > > 	WARNING: possible circular locking dependency detected
> > > 	5.3.0-rc2+ #23 Not tainted
> > > 	------------------------------------------------------
> > > 	syz-executor.2/20386 is trying to acquire lock:
> > > 	00000000772249c6 (rio500_mutex){+.+.}, at: open_rio+0x16/0xc0  
> > > 	drivers/usb/misc/rio500.c:64
> > > 
> > > 	but task is already holding lock:
> > > 	00000000d3e8f4b9 (minor_rwsem){++++}, at: usb_open+0x23/0x270  
> > > 	drivers/usb/core/file.c:39
> > > 
> > > 	which lock already depends on the new lock.
> > > 
> > > The problem is that the driver's open_rio() routine is called while
> > > the usbcore's minor_rwsem is locked for reading, and it acquires the
> > > rio500_mutex; whereas conversely, probe_rio() and disconnect_rio()
> > > first acquire the rio500_mutex and then call usb_register_dev() or
> > > usb_deregister_dev(), which lock minor_rwsem for writing.
> > > 
> > > The correct ordering of acquisition should be: minor_rwsem first, then
> > > rio500_mutex (since the locking in open_rio() cannot be changed).
> > > Thus, the probe and disconnect routines should avoid holding
> > > rio500_mutex while doing their registration and deregistration.
> > > 
> > > This patch adjusts the code in those two routines to do just that.  It
> > > also relies on the fact that the probe and disconnect routines are
> > > protected by the device mutex, so the initial test of rio->present
> > > needs no extra locking.
> > > 
> > > Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com
> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > > Fixes: d710734b0677 ("USB: rio500: simplify locking")
> > > CC: Oliver Neukum <oneukum@suse.com>
> > > CC: <stable@vger.kernel.org>
> > > 
> > > ---
> > > 
> > > This patch is different from the one I posted earlier.  I realized that 
> > > we don't want to register the device's char file until after the 
> > > buffers have been allocated.
> > 
> > Should I revert Oliver's patch?
> 
> Sorry, I should have explained more clearly: This goes on top of 
> Oliver's patch.  In fact, Oliver's patch is the one listed in the 
> Fixes: tag.
> 
> You do not need to apply Oliver's reversion.  Assuming he agrees that 
> this patch is correct, of course.

Ok, I applied the revert, and that's in 5.3-rc4.  So of course this does
not apply :)

Shoudl I revert the revert and then apply this?  I will if I can get an
ack from Oliver...

thanks,

greg k-h

Re: [PATCH] USB: rio500: Fix lockdep violation

[Alan Stern]

On Thu, 15 Aug 2019, Greg KH wrote:

> On Thu, Aug 08, 2019 at 02:23:00PM -0400, Alan Stern wrote:
> > On Thu, 8 Aug 2019, Greg KH wrote:
> > 
> > > On Thu, Aug 08, 2019 at 01:34:08PM -0400, Alan Stern wrote:
> > > > The syzbot fuzzer found a lockdep violation in the rio500 driver:
> > > > 
> > > > 	======================================================
> > > > 	WARNING: possible circular locking dependency detected
> > > > 	5.3.0-rc2+ #23 Not tainted
> > > > 	------------------------------------------------------
> > > > 	syz-executor.2/20386 is trying to acquire lock:
> > > > 	00000000772249c6 (rio500_mutex){+.+.}, at: open_rio+0x16/0xc0  
> > > > 	drivers/usb/misc/rio500.c:64
> > > > 
> > > > 	but task is already holding lock:
> > > > 	00000000d3e8f4b9 (minor_rwsem){++++}, at: usb_open+0x23/0x270  
> > > > 	drivers/usb/core/file.c:39
> > > > 
> > > > 	which lock already depends on the new lock.
> > > > 
> > > > The problem is that the driver's open_rio() routine is called while
> > > > the usbcore's minor_rwsem is locked for reading, and it acquires the
> > > > rio500_mutex; whereas conversely, probe_rio() and disconnect_rio()
> > > > first acquire the rio500_mutex and then call usb_register_dev() or
> > > > usb_deregister_dev(), which lock minor_rwsem for writing.
> > > > 
> > > > The correct ordering of acquisition should be: minor_rwsem first, then
> > > > rio500_mutex (since the locking in open_rio() cannot be changed).
> > > > Thus, the probe and disconnect routines should avoid holding
> > > > rio500_mutex while doing their registration and deregistration.
> > > > 
> > > > This patch adjusts the code in those two routines to do just that.  It
> > > > also relies on the fact that the probe and disconnect routines are
> > > > protected by the device mutex, so the initial test of rio->present
> > > > needs no extra locking.
> > > > 
> > > > Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com
> > > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > > > Fixes: d710734b0677 ("USB: rio500: simplify locking")
> > > > CC: Oliver Neukum <oneukum@suse.com>
> > > > CC: <stable@vger.kernel.org>
> > > > 
> > > > ---
> > > > 
> > > > This patch is different from the one I posted earlier.  I realized that 
> > > > we don't want to register the device's char file until after the 
> > > > buffers have been allocated.
> > > 
> > > Should I revert Oliver's patch?
> > 
> > Sorry, I should have explained more clearly: This goes on top of 
> > Oliver's patch.  In fact, Oliver's patch is the one listed in the 
> > Fixes: tag.
> > 
> > You do not need to apply Oliver's reversion.  Assuming he agrees that 
> > this patch is correct, of course.
> 
> Ok, I applied the revert, and that's in 5.3-rc4.  So of course this does
> not apply :)
> 
> Shoudl I revert the revert and then apply this?  I will if I can get an
> ack from Oliver...

Either that or else Oliver and I can squash the two patches into one.

Alan Stern

Re: [PATCH] USB: rio500: Fix lockdep violation

[Oliver Neukum]

Am Donnerstag, den 15.08.2019, 14:48 +0200 schrieb Greg KH:
> On Thu, Aug 08, 2019 at 02:23:00PM -0400, Alan Stern wrote:
> > On Thu, 8 Aug 2019, Greg KH wrote:
> > 
> > > On Thu, Aug 08, 2019 at 01:34:08PM -0400, Alan Stern wrote:
> > > > The syzbot fuzzer found a lockdep violation in the rio500 driver:
> > > > 
> > > > 	======================================================
> > > > 	WARNING: possible circular locking dependency detected
> > > > 	5.3.0-rc2+ #23 Not tainted
> > > > 	------------------------------------------------------
> > > > 	syz-executor.2/20386 is trying to acquire lock:
> > > > 	00000000772249c6 (rio500_mutex){+.+.}, at: open_rio+0x16/0xc0  
> > > > 	drivers/usb/misc/rio500.c:64
> > > > 
> > > > 	but task is already holding lock:
> > > > 	00000000d3e8f4b9 (minor_rwsem){++++}, at: usb_open+0x23/0x270  
> > > > 	drivers/usb/core/file.c:39
> > > > 
> > > > 	which lock already depends on the new lock.
> > > > 
> > > > The problem is that the driver's open_rio() routine is called while
> > > > the usbcore's minor_rwsem is locked for reading, and it acquires the
> > > > rio500_mutex; whereas conversely, probe_rio() and disconnect_rio()
> > > > first acquire the rio500_mutex and then call usb_register_dev() or
> > > > usb_deregister_dev(), which lock minor_rwsem for writing.
> > > > 
> > > > The correct ordering of acquisition should be: minor_rwsem first, then
> > > > rio500_mutex (since the locking in open_rio() cannot be changed).
> > > > Thus, the probe and disconnect routines should avoid holding
> > > > rio500_mutex while doing their registration and deregistration.
> > > > 
> > > > This patch adjusts the code in those two routines to do just that.  It
> > > > also relies on the fact that the probe and disconnect routines are
> > > > protected by the device mutex, so the initial test of rio->present
> > > > needs no extra locking.
> > > > 
> > > > Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com
> > > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > > > Fixes: d710734b0677 ("USB: rio500: simplify locking")
> > > > CC: Oliver Neukum <oneukum@suse.com>
> > > > CC: <stable@vger.kernel.org>
> > > > 
> > > > ---
> > > > 
> > > > This patch is different from the one I posted earlier.  I realized that 
> > > > we don't want to register the device's char file until after the 
> > > > buffers have been allocated.
> > > 
> > > Should I revert Oliver's patch?
> > 
> > Sorry, I should have explained more clearly: This goes on top of 
> > Oliver's patch.  In fact, Oliver's patch is the one listed in the 
> > Fixes: tag.
> > 
> > You do not need to apply Oliver's reversion.  Assuming he agrees that 
> > this patch is correct, of course.
> 
> Ok, I applied the revert, and that's in 5.3-rc4.  So of course this does
> not apply :)
> 
> Shoudl I revert the revert and then apply this?  I will if I can get an
> ack from Oliver...

Acked-by: Oliver Neukum <oneukum@suse.com>

Re: [PATCH] USB: rio500: Fix lockdep violation

[Greg KH]

On Thu, Aug 15, 2019 at 10:47:45AM -0400, Alan Stern wrote:
> On Thu, 15 Aug 2019, Greg KH wrote:
> 
> > On Thu, Aug 08, 2019 at 02:23:00PM -0400, Alan Stern wrote:
> > > On Thu, 8 Aug 2019, Greg KH wrote:
> > > 
> > > > On Thu, Aug 08, 2019 at 01:34:08PM -0400, Alan Stern wrote:
> > > > > The syzbot fuzzer found a lockdep violation in the rio500 driver:
> > > > > 
> > > > > 	======================================================
> > > > > 	WARNING: possible circular locking dependency detected
> > > > > 	5.3.0-rc2+ #23 Not tainted
> > > > > 	------------------------------------------------------
> > > > > 	syz-executor.2/20386 is trying to acquire lock:
> > > > > 	00000000772249c6 (rio500_mutex){+.+.}, at: open_rio+0x16/0xc0  
> > > > > 	drivers/usb/misc/rio500.c:64
> > > > > 
> > > > > 	but task is already holding lock:
> > > > > 	00000000d3e8f4b9 (minor_rwsem){++++}, at: usb_open+0x23/0x270  
> > > > > 	drivers/usb/core/file.c:39
> > > > > 
> > > > > 	which lock already depends on the new lock.
> > > > > 
> > > > > The problem is that the driver's open_rio() routine is called while
> > > > > the usbcore's minor_rwsem is locked for reading, and it acquires the
> > > > > rio500_mutex; whereas conversely, probe_rio() and disconnect_rio()
> > > > > first acquire the rio500_mutex and then call usb_register_dev() or
> > > > > usb_deregister_dev(), which lock minor_rwsem for writing.
> > > > > 
> > > > > The correct ordering of acquisition should be: minor_rwsem first, then
> > > > > rio500_mutex (since the locking in open_rio() cannot be changed).
> > > > > Thus, the probe and disconnect routines should avoid holding
> > > > > rio500_mutex while doing their registration and deregistration.
> > > > > 
> > > > > This patch adjusts the code in those two routines to do just that.  It
> > > > > also relies on the fact that the probe and disconnect routines are
> > > > > protected by the device mutex, so the initial test of rio->present
> > > > > needs no extra locking.
> > > > > 
> > > > > Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com
> > > > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > > > > Fixes: d710734b0677 ("USB: rio500: simplify locking")
> > > > > CC: Oliver Neukum <oneukum@suse.com>
> > > > > CC: <stable@vger.kernel.org>
> > > > > 
> > > > > ---
> > > > > 
> > > > > This patch is different from the one I posted earlier.  I realized that 
> > > > > we don't want to register the device's char file until after the 
> > > > > buffers have been allocated.
> > > > 
> > > > Should I revert Oliver's patch?
> > > 
> > > Sorry, I should have explained more clearly: This goes on top of 
> > > Oliver's patch.  In fact, Oliver's patch is the one listed in the 
> > > Fixes: tag.
> > > 
> > > You do not need to apply Oliver's reversion.  Assuming he agrees that 
> > > this patch is correct, of course.
> > 
> > Ok, I applied the revert, and that's in 5.3-rc4.  So of course this does
> > not apply :)
> > 
> > Shoudl I revert the revert and then apply this?  I will if I can get an
> > ack from Oliver...
> 
> Either that or else Oliver and I can squash the two patches into one.

I've now merged both, thanks.

greg k-h