* Re: possible deadlock in open_rio
[not found] <1565187142.15973.3.camel@neukum.org>
@ 2019-08-08 14:33 ` Alan Stern
2019-08-08 14:33 ` syzbot
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Alan Stern @ 2019-08-08 14:33 UTC (permalink / raw)
To: Oliver Neukum
Cc: andreyknvl, syzkaller-bugs, gregkh, syzbot,
Kernel development list, USB list
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[] = {
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Re: possible deadlock in open_rio
2019-08-08 14:33 ` possible deadlock in open_rio Alan Stern
@ 2019-08-08 14:33 ` syzbot
2019-08-08 14:33 ` syzbot
2019-08-08 14:44 ` Andrey Konovalov
2 siblings, 0 replies; 4+ messages in thread
From: syzbot @ 2019-08-08 14:33 UTC (permalink / raw)
To: Alan Stern
Cc: andreyknvl, gregkh, linux-kernel, linux-usb, oliver, stern,
syzkaller-bugs
> 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[] = {
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Re: possible deadlock in open_rio
2019-08-08 14:33 ` possible deadlock in open_rio Alan Stern
2019-08-08 14:33 ` syzbot
@ 2019-08-08 14:33 ` syzbot
2019-08-08 14:44 ` Andrey Konovalov
2 siblings, 0 replies; 4+ messages in thread
From: syzbot @ 2019-08-08 14:33 UTC (permalink / raw)
To: Alan Stern
Cc: andreyknvl, gregkh, linux-kernel, linux-usb, oliver, stern,
syzkaller-bugs
> 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[] = {
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: possible deadlock in open_rio
2019-08-08 14:33 ` possible deadlock in open_rio Alan Stern
2019-08-08 14:33 ` syzbot
2019-08-08 14:33 ` syzbot
@ 2019-08-08 14:44 ` Andrey Konovalov
2 siblings, 0 replies; 4+ messages in thread
From: Andrey Konovalov @ 2019-08-08 14:44 UTC (permalink / raw)
To: Alan Stern
Cc: Oliver Neukum, syzkaller-bugs, Greg Kroah-Hartman, syzbot,
Kernel development list, USB list
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[] = {
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-08-08 14:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1565187142.15973.3.camel@neukum.org>
2019-08-08 14:33 ` possible deadlock in open_rio Alan Stern
2019-08-08 14:33 ` syzbot
2019-08-08 14:33 ` syzbot
2019-08-08 14:44 ` Andrey Konovalov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).