Since usb_register_dev() from imon_init_display() from imon_probe() holds minor_rwsem while display_open() which holds driver_lock and ictx->lock is called with minor_rwsem held from usb_open(), holding driver_lock or ictx->lock when calling usb_register_dev() causes circular locking dependency problem. Since usb_deregister_dev() from imon_disconnect() holds minor_rwsem while display_open() which holds driver_lock is called with minor_rwsem held, holding driver_lock when calling usb_deregister_dev() also causes circular locking dependency problem. Sean Young explained that the problem is there are imon devices which have two usb interfaces, even though it is one device. The probe and disconnect function of both usb interfaces can run concurrently. Alan Stern responded that the driver and USB cores guarantee that when an interface is probed, both the interface and its USB device are locked. Ditto for when the disconnect callback gets run. So concurrent probing/ disconnection of multiple interfaces on the same device is not possible. Therefore, simply remove redundant serialization. Link: https://syzkaller.appspot.com/bug?extid=c558267ad910fc494497 Reported-by: syzbot <syzbot+c558267ad910fc494497@xxxxxxxxxxxxxxxxxxxxxxxxx> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> Cc: Sean Young <sean@xxxxxxxx> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> --- This patch might be OK for solving circular locking dependency problem. But https://syzkaller.appspot.com/text?tag=CrashReport&x=1632f89f700000 says free_imon_context() from imon_disconnect() can call kfree(ictx) before display_close() is called, resulting in use-after-free problem. I guess that we need to defer free_imon_context() using refcount till display_close() is called. Fix as a separate patch or include into this patch? drivers/media/rc/imon.c | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c index 54da6f60079b..e0e893d96cf3 100644 --- a/drivers/media/rc/imon.c +++ b/drivers/media/rc/imon.c @@ -439,9 +439,6 @@ static struct usb_driver imon_driver = { .id_table = imon_usb_id_table, }; -/* to prevent races between open() and disconnect(), probing, etc */ -static DEFINE_MUTEX(driver_lock); - /* Module bookkeeping bits */ MODULE_AUTHOR(MOD_AUTHOR); MODULE_DESCRIPTION(MOD_DESC); @@ -499,9 +496,6 @@ static int display_open(struct inode *inode, struct file *file) int subminor; int retval = 0; - /* prevent races with disconnect */ - mutex_lock(&driver_lock); - subminor = iminor(inode); interface = usb_find_interface(&imon_driver, subminor); if (!interface) { @@ -534,7 +528,6 @@ static int display_open(struct inode *inode, struct file *file) mutex_unlock(&ictx->lock); exit: - mutex_unlock(&driver_lock); return retval; } @@ -2416,9 +2409,6 @@ static int imon_probe(struct usb_interface *interface, dev_dbg(dev, "%s: found iMON device (%04x:%04x, intf%d)\n", __func__, vendor, product, ifnum); - /* prevent races probing devices w/multiple interfaces */ - mutex_lock(&driver_lock); - first_if = usb_ifnum_to_if(usbdev, 0); if (!first_if) { ret = -ENODEV; @@ -2456,8 +2446,6 @@ static int imon_probe(struct usb_interface *interface, usb_set_intfdata(interface, ictx); if (ifnum == 0) { - mutex_lock(&ictx->lock); - if (product == 0xffdc && ictx->rf_device) { sysfs_err = sysfs_create_group(&interface->dev.kobj, &imon_rf_attr_group); @@ -2468,21 +2456,17 @@ static int imon_probe(struct usb_interface *interface, if (ictx->display_supported) imon_init_display(ictx, interface); - - mutex_unlock(&ictx->lock); } dev_info(dev, "iMON device (%04x:%04x, intf%d) on usb<%d:%d> initialized\n", vendor, product, ifnum, usbdev->bus->busnum, usbdev->devnum); - mutex_unlock(&driver_lock); usb_put_dev(usbdev); return 0; fail: - mutex_unlock(&driver_lock); usb_put_dev(usbdev); dev_err(dev, "unable to register, err %d\n", ret); @@ -2498,9 +2482,6 @@ static void imon_disconnect(struct usb_interface *interface) struct device *dev; int ifnum; - /* prevent races with multi-interface device probing and display_open */ - mutex_lock(&driver_lock); - ictx = usb_get_intfdata(interface); dev = ictx->dev; ifnum = interface->cur_altsetting->desc.bInterfaceNumber; @@ -2545,8 +2526,6 @@ static void imon_disconnect(struct usb_interface *interface) if (!ictx->dev_present_intf0 && !ictx->dev_present_intf1) free_imon_context(ictx); - mutex_unlock(&driver_lock); - dev_dbg(dev, "%s: iMON device (intf%d) disconnected\n", __func__, ifnum); } -- 2.34.1
