Re: possible deadlock in open_rio

[Andrey Konovalov <andreyknvl@google.com>]

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