Re: Re: possible deadlock in open_rio

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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[] = {




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux