Re: possible deadlock in display_open

[Sean Young <sean@mess.org>]

Hello,

On Mon, Apr 25, 2022 at 02:29:26PM +0900, Tetsuo Handa wrote:
> 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.
> 
> But actually do we need to hold these locks?
> Could you explain possible race scenario if we do below?

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.

If the imon_probe is running for interface 1, and the probe for interface 0
has not completed yet, then the driver may erronously think the probe for
interface 0 failed at `if (!first_if_ctx) {` on line 2442.

Of course, this depends on probe/disconnect functions being allowed to run
concurrently on different interfaces of the same usb device.

This code is rather tricky, and I'm sure there must be a better way of
dealing with multiple interfaces for a usb driver than what imon.c does.

Thanks
Sean

> 
>  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
> 
> On 2019/08/09 22:18, syzbot wrote:
> > Hello,
> > 
> > syzbot found the following crash on:
> > 
> > HEAD commit:    e96407b4 usb-fuzzer: main usb gadget fuzzer driver
> > git tree:       https://github.com/google/kasan.git usb-fuzzer
> > console output: https://syzkaller.appspot.com/x/log.txt?x=13b29b26600000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e
> > dashboard link: https://syzkaller.appspot.com/bug?extid=c558267ad910fc494497
> > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=15427002600000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=111cb61c600000
> > 
> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > Reported-by: syzbot+c558267ad910fc494497@syzkaller.appspotmail.com