possible deadlock in display_open

possible deadlock in display_open

[syzbot]

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

======================================================
WARNING: possible circular locking dependency detected
5.3.0-rc2+ #25 Not tainted
------------------------------------------------------
syz-executor626/1723 is trying to acquire lock:
000000001a0d74d7 (driver_lock#2){+.+.}, at: display_open+0x1f/0x1d0  
drivers/media/rc/imon.c:475

but task is already holding lock:
00000000076a0058 (minor_rwsem){++++}, at: usb_open+0x23/0x270  
drivers/usb/core/file.c:39

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #2 (minor_rwsem){++++}:
        down_write+0x92/0x150 kernel/locking/rwsem.c:1500
        usb_register_dev drivers/usb/core/file.c:187 [inline]
        usb_register_dev+0x131/0x6a0 drivers/usb/core/file.c:156
        imon_init_display drivers/media/rc/imon.c:2343 [inline]
        imon_probe+0x244d/0x2af0 drivers/media/rc/imon.c:2426
        usb_probe_interface+0x305/0x7a0 drivers/usb/core/driver.c:361
        really_probe+0x281/0x650 drivers/base/dd.c:548
        driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709
        __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816
        bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
        __device_attach+0x217/0x360 drivers/base/dd.c:882
        bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514
        device_add+0xae6/0x16f0 drivers/base/core.c:2114
        usb_set_configuration+0xdf6/0x1670 drivers/usb/core/message.c:2023
        generic_probe+0x9d/0xd5 drivers/usb/core/generic.c:210
        usb_probe_device+0x99/0x100 drivers/usb/core/driver.c:266
        really_probe+0x281/0x650 drivers/base/dd.c:548
        driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709
        __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816
        bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
        __device_attach+0x217/0x360 drivers/base/dd.c:882
        bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514
        device_add+0xae6/0x16f0 drivers/base/core.c:2114
        usb_new_device.cold+0x6a4/0xe79 drivers/usb/core/hub.c:2536
        hub_port_connect drivers/usb/core/hub.c:5098 [inline]
        hub_port_connect_change drivers/usb/core/hub.c:5213 [inline]
        port_event drivers/usb/core/hub.c:5359 [inline]
        hub_event+0x1b5c/0x3640 drivers/usb/core/hub.c:5441
        process_one_work+0x92b/0x1530 kernel/workqueue.c:2269
        worker_thread+0x96/0xe20 kernel/workqueue.c:2415
        kthread+0x318/0x420 kernel/kthread.c:255
        ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352

-> #1 (&ictx->lock){+.+.}:
        __mutex_lock_common kernel/locking/mutex.c:930 [inline]
        __mutex_lock+0x158/0x1360 kernel/locking/mutex.c:1077
        imon_init_intf0 drivers/media/rc/imon.c:2188 [inline]
        imon_probe+0xf0c/0x2af0 drivers/media/rc/imon.c:2387
        usb_probe_interface+0x305/0x7a0 drivers/usb/core/driver.c:361
        really_probe+0x281/0x650 drivers/base/dd.c:548
        driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709
        __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816
        bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
        __device_attach+0x217/0x360 drivers/base/dd.c:882
        bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514
        device_add+0xae6/0x16f0 drivers/base/core.c:2114
        usb_set_configuration+0xdf6/0x1670 drivers/usb/core/message.c:2023
        generic_probe+0x9d/0xd5 drivers/usb/core/generic.c:210
        usb_probe_device+0x99/0x100 drivers/usb/core/driver.c:266
        really_probe+0x281/0x650 drivers/base/dd.c:548
        driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709
        __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816
        bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
        __device_attach+0x217/0x360 drivers/base/dd.c:882
        bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514
        device_add+0xae6/0x16f0 drivers/base/core.c:2114
        usb_new_device.cold+0x6a4/0xe79 drivers/usb/core/hub.c:2536
        hub_port_connect drivers/usb/core/hub.c:5098 [inline]
        hub_port_connect_change drivers/usb/core/hub.c:5213 [inline]
        port_event drivers/usb/core/hub.c:5359 [inline]
        hub_event+0x1b5c/0x3640 drivers/usb/core/hub.c:5441
        process_one_work+0x92b/0x1530 kernel/workqueue.c:2269
        worker_thread+0x96/0xe20 kernel/workqueue.c:2415
        kthread+0x318/0x420 kernel/kthread.c:255
        ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352

-> #0 (driver_lock#2){+.+.}:
        check_prev_add kernel/locking/lockdep.c:2405 [inline]
        check_prevs_add kernel/locking/lockdep.c:2507 [inline]
        validate_chain kernel/locking/lockdep.c:2897 [inline]
        __lock_acquire+0x1f7c/0x3b50 kernel/locking/lockdep.c:3880
        lock_acquire+0x127/0x320 kernel/locking/lockdep.c:4412
        __mutex_lock_common kernel/locking/mutex.c:930 [inline]
        __mutex_lock+0x158/0x1360 kernel/locking/mutex.c:1077
        display_open+0x1f/0x1d0 drivers/media/rc/imon.c:475
        usb_open+0x1df/0x270 drivers/usb/core/file.c:48
        chrdev_open+0x219/0x5c0 fs/char_dev.c:414
        do_dentry_open+0x494/0x1120 fs/open.c:797
        do_last fs/namei.c:3416 [inline]
        path_openat+0x1430/0x3f50 fs/namei.c:3533
        do_filp_open+0x1a1/0x280 fs/namei.c:3563
        do_sys_open+0x3c0/0x580 fs/open.c:1089
        do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
        entry_SYSCALL_64_after_hwframe+0x49/0xbe

other info that might help us debug this:

Chain exists of:
   driver_lock#2 --> &ictx->lock --> minor_rwsem

  Possible unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(minor_rwsem);
                                lock(&ictx->lock);
                                lock(minor_rwsem);
   lock(driver_lock#2);

  *** DEADLOCK ***

1 lock held by syz-executor626/1723:
  #0: 00000000076a0058 (minor_rwsem){++++}, at: usb_open+0x23/0x270  
drivers/usb/core/file.c:39

stack backtrace:
CPU: 1 PID: 1723 Comm: syz-executor626 Not tainted 5.3.0-rc2+ #25
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0xca/0x13e lib/dump_stack.c:113
  check_noncircular+0x345/0x3e0 kernel/locking/lockdep.c:1741
  check_prev_add kernel/locking/lockdep.c:2405 [inline]
  check_prevs_add kernel/locking/lockdep.c:2507 [inline]
  validate_chain kernel/locking/lockdep.c:2897 [inline]
  __lock_acquire+0x1f7c/0x3b50 kernel/locking/lockdep.c:3880
  lock_acquire+0x127/0x320 kernel/locking/lockdep.c:4412
  __mutex_lock_common kernel/locking/mutex.c:930 [inline]
  __mutex_lock+0x158/0x1360 kernel/locking/mutex.c:1077
  display_open+0x1f/0x1d0 drivers/media/rc/imon.c:475
  usb_open+0x1df/0x270 drivers/usb/core/file.c:48
  chrdev_open+0x219/0x5c0 fs/char_dev.c:414
  do_dentry_open+0x494/0x1120 fs/open.c:797
  do_last fs/namei.c:3416 [inline]
  path_openat+0x1430/0x3f50 fs/namei.c:3533
  do_filp_open+0x1a1/0x280 fs/namei.c:3563
  do_sys_open+0x3c0/0x580 fs/open.c:1089
  do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x401300
Code: 01 f0 ff ff 0f 83 00 0b 00 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f  
44 00 00 83 3d 8d 0a 2d 00 00 75 14 b8 02 00 00 00 0f 05 <48> 3d 01 f0 ff  
ff 0f 83 d4 0a 00 00 c3 48 83 ec 08 e8 3a 00 00 00
RSP: 002b:00007ffc59dcdec8 EFLAGS: 00000246 ORIG_RAX: 0000000000000002
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000401300
RDX: 0000000000000000 RSI: 0000000000000002 RDI: 00007ffc59dcdee0
RBP: 00000


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

Re: possible deadlock in display_open

[Tetsuo Handa]

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?

 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

Re: possible deadlock in display_open

[Sean Young]

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

Re: possible deadlock in display_open

[Tetsuo Handa]

On 2022/04/25 18:20, Sean Young wrote:
> 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.
> 
> Of course, this depends on probe/disconnect functions being allowed to run
> concurrently on different interfaces of the same usb device.

I don't have real hardware to confirm. If you have an imon device which has
two usb interfaces, please try below debug printk() patch in order to see
whether the caller is already holding locks for serialization.

------------------------------------------------------------
 drivers/media/rc/imon.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index 54da6f60079b..a309c28e63d8 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -30,6 +30,7 @@
 #include <media/rc-core.h>
 
 #include <linux/timer.h>
+#include <linux/sched/debug.h>
 
 #define MOD_AUTHOR	"Jarod Wilson <jarod@wilsonet.com>"
 #define MOD_DESC	"Driver for SoundGraph iMON MultiMedia IR/Display"
@@ -439,9 +440,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 +497,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 +529,6 @@ static int display_open(struct inode *inode, struct file *file)
 	mutex_unlock(&ictx->lock);
 
 exit:
-	mutex_unlock(&driver_lock);
 	return retval;
 }
 
@@ -2407,6 +2401,8 @@ static int imon_probe(struct usb_interface *interface,
 	struct imon_context *first_if_ctx = NULL;
 	u16 vendor, product;
 
+	dump_stack();
+	debug_show_held_locks(current);
 	usbdev     = usb_get_dev(interface_to_usbdev(interface));
 	iface_desc = interface->cur_altsetting;
 	ifnum      = iface_desc->desc.bInterfaceNumber;
@@ -2416,9 +2412,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 +2449,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 +2459,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,8 +2485,8 @@ 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);
+	dump_stack();
+	debug_show_held_locks(current);
 
 	ictx = usb_get_intfdata(interface);
 	dev = ictx->dev;
@@ -2545,8 +2532,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);
 }
------------------------------------------------------------

Below is a result with syzkaller-generated reproducer. If an imon device
which has two usb interfaces shares locks held, "prevent races with
multi-interface device probing" part can be removed.

------------------------------------------------------------
[  193.854446][ T4177] usb 2-1: new high-speed USB device number 2 using dummy_hcd
[  194.381949][ T4177] usb 2-1: config 0 interface 0 altsetting 0 endpoint 0x84 has an invalid bInterval 0, changing to 7
[  194.389192][ T4177] usb 2-1: New USB device found, idVendor=15c2, idProduct=0039, bcdDevice=d2.65
[  194.394772][ T4177] usb 2-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
[  194.415288][ T4177] usb 2-1: config 0 descriptor??
[  194.458149][ T4177] CPU: 0 PID: 4177 Comm: kworker/0:3 Not tainted 5.18.0-rc4-dirty #772
[  194.460825][ T4177] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
[  194.463534][ T4177] Workqueue: usb_hub_wq hub_event
[  194.465133][ T4177] Call Trace:
[  194.466158][ T4177]  <TASK>
[  194.467087][ T4177]  dump_stack_lvl+0xcd/0x134
[  194.468655][ T4177]  imon_probe+0x26/0x10da
[  194.470109][ T4177]  ? _raw_spin_unlock_irqrestore+0x3d/0x70
[  194.471973][ T4177]  ? __pm_runtime_set_status+0x29c/0x5d0
[  194.473543][ T4177]  ? _raw_spin_unlock_irqrestore+0x3d/0x70
[  194.475219][ T4177]  usb_probe_interface+0x19b/0x3e0
[  194.476806][ T4177]  ? usb_match_dynamic_id+0xe0/0xe0
[  194.478282][ T4177]  really_probe+0x138/0x4c0
[  194.479511][ T4177]  __driver_probe_device+0x191/0x220
[  194.481069][ T4177]  driver_probe_device+0x2a/0x120
[  194.482622][ T4177]  __device_attach_driver+0x105/0x1a0
[  194.484609][ T4177]  ? driver_allows_async_probing+0x90/0x90
[  194.486344][ T4177]  bus_for_each_drv+0xba/0x100
[  194.487985][ T4177]  __device_attach+0x130/0x290
[  194.489305][ T4177]  bus_probe_device+0xdb/0xf0
[  194.490604][ T4177]  device_add+0x635/0xdf0
[  194.492076][ T4177]  ? __mutex_unlock_slowpath+0x37/0x280
[  194.493649][ T4177]  usb_set_configuration+0x936/0xc20
[  194.495389][ T4177]  usb_generic_driver_probe+0x8c/0xc0
[  194.496883][ T4177]  usb_probe_device+0x6c/0x180
[  194.498207][ T4177]  ? usb_driver_release_interface+0xc0/0xc0
[  194.500193][ T4177]  really_probe+0x138/0x4c0
[  194.501561][ T4177]  __driver_probe_device+0x191/0x220
[  194.503154][ T4177]  driver_probe_device+0x2a/0x120
[  194.504596][ T4177]  __device_attach_driver+0x105/0x1a0
[  194.506200][ T4177]  ? driver_allows_async_probing+0x90/0x90
[  194.507925][ T4177]  bus_for_each_drv+0xba/0x100
[  194.509331][ T4177]  __device_attach+0x130/0x290
[  194.510671][ T4177]  bus_probe_device+0xdb/0xf0
[  194.512115][ T4177]  device_add+0x635/0xdf0
[  194.513392][ T4177]  usb_new_device.cold+0x10f/0x58e
[  194.514905][ T4177]  hub_event+0x15f9/0x2650
[  194.516333][ T4177]  process_one_work+0x381/0x940
[  194.517826][ T4177]  worker_thread+0x5b/0x5d0
[  194.519247][ T4177]  ? process_one_work+0x940/0x940
[  194.527532][ T4177]  kthread+0x135/0x170
[  194.535432][ T4177]  ? kthread_complete_and_exit+0x30/0x30
[  194.543576][ T4177]  ret_from_fork+0x1f/0x30
[  194.551127][ T4177]  </TASK>
[  194.558820][ T4177] 5 locks held by kworker/0:3/4177:
[  194.566387][ T4177]  #0: ffff88800a0fed38 ((wq_completion)usb_hub_wq){+.+.}-{0:0}, at: process_one_work+0x2b5/0x940
[  194.575897][ T4177]  #1: ffffc90002fe3e70 ((work_completion)(&hub->events)){+.+.}-{0:0}, at: process_one_work+0x2b5/0x940
[  194.585166][ T4177]  #2: ffff888105129220 (&dev->mutex){....}-{3:3}, at: hub_event+0xc4/0x2650
[  194.593581][ T4177]  #3: ffff88810cc40220 (&dev->mutex){....}-{3:3}, at: __device_attach+0x41/0x290
[  194.603139][ T4177]  #4: ffff88810cc409a8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x41/0x290
[  194.618658][ T4177] input: iMON Panel, Knob and Mouse(15c2:0039) as /devices/platform/dummy_hcd.0/usb2/2-1/2-1:0.0/input/input6
[  194.781071][ T4177] rc_core: IR keymap rc-imon-pad not found
[  194.789123][ T4177] Registered IR keymap rc-empty
[  194.796624][ T4177] imon 2-1:0.0: Looks like you're trying to use an IR protocol this device does not support
[  194.806014][ T4177] imon 2-1:0.0: Unsupported IR protocol specified, overriding to iMON IR protocol
[  194.965830][ T4177] rc rc0: iMON Remote (15c2:0039) as /devices/platform/dummy_hcd.0/usb2/2-1/2-1:0.0/rc/rc0
[  194.998797][ T4177] input: iMON Remote (15c2:0039) as /devices/platform/dummy_hcd.0/usb2/2-1/2-1:0.0/rc/rc0/input7
[  195.033215][ T4177] imon 2-1:0.0: iMON device (15c2:0039, intf0) on usb<2:2> initialized
[  195.119182][ T3553] usb 2-1: USB disconnect, device number 2
[  195.131624][    C0] imon 2-1:0.0: imon usb_rx_callback_intf0: status(-71): ignored
[  195.157933][ T3553] CPU: 2 PID: 3553 Comm: kworker/2:3 Not tainted 5.18.0-rc4-dirty #772
[  195.171455][ T3553] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
[  195.184454][ T3553] Workqueue: usb_hub_wq hub_event
[  195.198577][ T3553] Call Trace:
[  195.208174][ T3553]  <TASK>
[  195.216716][ T3553]  dump_stack_lvl+0xcd/0x134
[  195.225712][ T3553]  imon_disconnect+0x1c/0x271
[  195.234653][ T3553]  usb_unbind_interface+0xe2/0x3e0
[  195.243813][ T3553]  ? usb_unbind_device+0xc0/0xc0
[  195.253236][ T3553]  device_remove+0x6d/0x80
[  195.262130][ T3553]  device_release_driver_internal+0x275/0x300
[  195.271507][ T3553]  bus_remove_device+0x12f/0x1d0
[  195.281785][ T3553]  device_del+0x1e5/0x4e0
[  195.290748][ T3553]  usb_disable_device+0x15f/0x310
[  195.300276][ T3553]  usb_disconnect.cold+0xd8/0x2fd
[  195.309417][ T3553]  ? __mutex_unlock_slowpath+0x37/0x280
[  195.318605][ T3553]  hub_event+0x190b/0x2650
[  195.327623][ T3553]  process_one_work+0x381/0x940
[  195.336256][ T3553]  worker_thread+0x5b/0x5d0
[  195.345289][ T3553]  ? process_one_work+0x940/0x940
[  195.354733][ T3553]  kthread+0x135/0x170
[  195.363529][ T3553]  ? kthread_complete_and_exit+0x30/0x30
[  195.373531][ T3553]  ret_from_fork+0x1f/0x30
[  195.382434][ T3553]  </TASK>
[  195.397291][ T3553] 5 locks held by kworker/2:3/3553:
[  195.408408][ T3553]  #0: ffff88800a0fed38 ((wq_completion)usb_hub_wq){+.+.}-{0:0}, at: process_one_work+0x2b5/0x940
[  195.417251][ T3553]  #1: ffffc90002963e70 ((work_completion)(&hub->events)){+.+.}-{0:0}, at: process_one_work+0x2b5/0x940
[  195.429307][ T3553]  #2: ffff888105129220 (&dev->mutex){....}-{3:3}, at: hub_event+0xc4/0x2650
[  195.439282][ T3553]  #3: ffff88810cc40220 (&dev->mutex){....}-{3:3}, at: usb_disconnect+0x8e/0xa0
[  195.450701][ T3553]  #4: ffff88810cc409a8 (&dev->mutex){....}-{3:3}, at: device_release_driver_internal+0x55/0x300
------------------------------------------------------------

Re: possible deadlock in display_open

[Sean Young]

On Mon, Apr 25, 2022 at 08:14:12PM +0900, Tetsuo Handa wrote:
> On 2022/04/25 18:20, Sean Young wrote:
> > 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.
> > 
> > Of course, this depends on probe/disconnect functions being allowed to run
> > concurrently on different interfaces of the same usb device.
> 
> I don't have real hardware to confirm. If you have an imon device which has
> two usb interfaces, please try below debug printk() patch in order to see
> whether the caller is already holding locks for serialization.

I am afraid calling debug_show_held_locks() is not really going to tell us
this information. This should be figured out from understanding the usb
stack, not from seeing if the probe happens to be concurrent.

Just because the locks were not held, does not mean that the usb interface
initialization is not concurrent.

Thanks
Sean


> 
> ------------------------------------------------------------
>  drivers/media/rc/imon.c | 25 +++++--------------------
>  1 file changed, 5 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
> index 54da6f60079b..a309c28e63d8 100644
> --- a/drivers/media/rc/imon.c
> +++ b/drivers/media/rc/imon.c
> @@ -30,6 +30,7 @@
>  #include <media/rc-core.h>
>  
>  #include <linux/timer.h>
> +#include <linux/sched/debug.h>
>  
>  #define MOD_AUTHOR	"Jarod Wilson <jarod@wilsonet.com>"
>  #define MOD_DESC	"Driver for SoundGraph iMON MultiMedia IR/Display"
> @@ -439,9 +440,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 +497,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 +529,6 @@ static int display_open(struct inode *inode, struct file *file)
>  	mutex_unlock(&ictx->lock);
>  
>  exit:
> -	mutex_unlock(&driver_lock);
>  	return retval;
>  }
>  
> @@ -2407,6 +2401,8 @@ static int imon_probe(struct usb_interface *interface,
>  	struct imon_context *first_if_ctx = NULL;
>  	u16 vendor, product;
>  
> +	dump_stack();
> +	debug_show_held_locks(current);
>  	usbdev     = usb_get_dev(interface_to_usbdev(interface));
>  	iface_desc = interface->cur_altsetting;
>  	ifnum      = iface_desc->desc.bInterfaceNumber;
> @@ -2416,9 +2412,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 +2449,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 +2459,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,8 +2485,8 @@ 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);
> +	dump_stack();
> +	debug_show_held_locks(current);
>  
>  	ictx = usb_get_intfdata(interface);
>  	dev = ictx->dev;
> @@ -2545,8 +2532,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);
>  }
> ------------------------------------------------------------
> 
> Below is a result with syzkaller-generated reproducer. If an imon device
> which has two usb interfaces shares locks held, "prevent races with
> multi-interface device probing" part can be removed.
> 
> ------------------------------------------------------------
> [  193.854446][ T4177] usb 2-1: new high-speed USB device number 2 using dummy_hcd
> [  194.381949][ T4177] usb 2-1: config 0 interface 0 altsetting 0 endpoint 0x84 has an invalid bInterval 0, changing to 7
> [  194.389192][ T4177] usb 2-1: New USB device found, idVendor=15c2, idProduct=0039, bcdDevice=d2.65
> [  194.394772][ T4177] usb 2-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
> [  194.415288][ T4177] usb 2-1: config 0 descriptor??
> [  194.458149][ T4177] CPU: 0 PID: 4177 Comm: kworker/0:3 Not tainted 5.18.0-rc4-dirty #772
> [  194.460825][ T4177] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
> [  194.463534][ T4177] Workqueue: usb_hub_wq hub_event
> [  194.465133][ T4177] Call Trace:
> [  194.466158][ T4177]  <TASK>
> [  194.467087][ T4177]  dump_stack_lvl+0xcd/0x134
> [  194.468655][ T4177]  imon_probe+0x26/0x10da
> [  194.470109][ T4177]  ? _raw_spin_unlock_irqrestore+0x3d/0x70
> [  194.471973][ T4177]  ? __pm_runtime_set_status+0x29c/0x5d0
> [  194.473543][ T4177]  ? _raw_spin_unlock_irqrestore+0x3d/0x70
> [  194.475219][ T4177]  usb_probe_interface+0x19b/0x3e0
> [  194.476806][ T4177]  ? usb_match_dynamic_id+0xe0/0xe0
> [  194.478282][ T4177]  really_probe+0x138/0x4c0
> [  194.479511][ T4177]  __driver_probe_device+0x191/0x220
> [  194.481069][ T4177]  driver_probe_device+0x2a/0x120
> [  194.482622][ T4177]  __device_attach_driver+0x105/0x1a0
> [  194.484609][ T4177]  ? driver_allows_async_probing+0x90/0x90
> [  194.486344][ T4177]  bus_for_each_drv+0xba/0x100
> [  194.487985][ T4177]  __device_attach+0x130/0x290
> [  194.489305][ T4177]  bus_probe_device+0xdb/0xf0
> [  194.490604][ T4177]  device_add+0x635/0xdf0
> [  194.492076][ T4177]  ? __mutex_unlock_slowpath+0x37/0x280
> [  194.493649][ T4177]  usb_set_configuration+0x936/0xc20
> [  194.495389][ T4177]  usb_generic_driver_probe+0x8c/0xc0
> [  194.496883][ T4177]  usb_probe_device+0x6c/0x180
> [  194.498207][ T4177]  ? usb_driver_release_interface+0xc0/0xc0
> [  194.500193][ T4177]  really_probe+0x138/0x4c0
> [  194.501561][ T4177]  __driver_probe_device+0x191/0x220
> [  194.503154][ T4177]  driver_probe_device+0x2a/0x120
> [  194.504596][ T4177]  __device_attach_driver+0x105/0x1a0
> [  194.506200][ T4177]  ? driver_allows_async_probing+0x90/0x90
> [  194.507925][ T4177]  bus_for_each_drv+0xba/0x100
> [  194.509331][ T4177]  __device_attach+0x130/0x290
> [  194.510671][ T4177]  bus_probe_device+0xdb/0xf0
> [  194.512115][ T4177]  device_add+0x635/0xdf0
> [  194.513392][ T4177]  usb_new_device.cold+0x10f/0x58e
> [  194.514905][ T4177]  hub_event+0x15f9/0x2650
> [  194.516333][ T4177]  process_one_work+0x381/0x940
> [  194.517826][ T4177]  worker_thread+0x5b/0x5d0
> [  194.519247][ T4177]  ? process_one_work+0x940/0x940
> [  194.527532][ T4177]  kthread+0x135/0x170
> [  194.535432][ T4177]  ? kthread_complete_and_exit+0x30/0x30
> [  194.543576][ T4177]  ret_from_fork+0x1f/0x30
> [  194.551127][ T4177]  </TASK>
> [  194.558820][ T4177] 5 locks held by kworker/0:3/4177:
> [  194.566387][ T4177]  #0: ffff88800a0fed38 ((wq_completion)usb_hub_wq){+.+.}-{0:0}, at: process_one_work+0x2b5/0x940
> [  194.575897][ T4177]  #1: ffffc90002fe3e70 ((work_completion)(&hub->events)){+.+.}-{0:0}, at: process_one_work+0x2b5/0x940
> [  194.585166][ T4177]  #2: ffff888105129220 (&dev->mutex){....}-{3:3}, at: hub_event+0xc4/0x2650
> [  194.593581][ T4177]  #3: ffff88810cc40220 (&dev->mutex){....}-{3:3}, at: __device_attach+0x41/0x290
> [  194.603139][ T4177]  #4: ffff88810cc409a8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x41/0x290
> [  194.618658][ T4177] input: iMON Panel, Knob and Mouse(15c2:0039) as /devices/platform/dummy_hcd.0/usb2/2-1/2-1:0.0/input/input6
> [  194.781071][ T4177] rc_core: IR keymap rc-imon-pad not found
> [  194.789123][ T4177] Registered IR keymap rc-empty
> [  194.796624][ T4177] imon 2-1:0.0: Looks like you're trying to use an IR protocol this device does not support
> [  194.806014][ T4177] imon 2-1:0.0: Unsupported IR protocol specified, overriding to iMON IR protocol
> [  194.965830][ T4177] rc rc0: iMON Remote (15c2:0039) as /devices/platform/dummy_hcd.0/usb2/2-1/2-1:0.0/rc/rc0
> [  194.998797][ T4177] input: iMON Remote (15c2:0039) as /devices/platform/dummy_hcd.0/usb2/2-1/2-1:0.0/rc/rc0/input7
> [  195.033215][ T4177] imon 2-1:0.0: iMON device (15c2:0039, intf0) on usb<2:2> initialized
> [  195.119182][ T3553] usb 2-1: USB disconnect, device number 2
> [  195.131624][    C0] imon 2-1:0.0: imon usb_rx_callback_intf0: status(-71): ignored
> [  195.157933][ T3553] CPU: 2 PID: 3553 Comm: kworker/2:3 Not tainted 5.18.0-rc4-dirty #772
> [  195.171455][ T3553] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
> [  195.184454][ T3553] Workqueue: usb_hub_wq hub_event
> [  195.198577][ T3553] Call Trace:
> [  195.208174][ T3553]  <TASK>
> [  195.216716][ T3553]  dump_stack_lvl+0xcd/0x134
> [  195.225712][ T3553]  imon_disconnect+0x1c/0x271
> [  195.234653][ T3553]  usb_unbind_interface+0xe2/0x3e0
> [  195.243813][ T3553]  ? usb_unbind_device+0xc0/0xc0
> [  195.253236][ T3553]  device_remove+0x6d/0x80
> [  195.262130][ T3553]  device_release_driver_internal+0x275/0x300
> [  195.271507][ T3553]  bus_remove_device+0x12f/0x1d0
> [  195.281785][ T3553]  device_del+0x1e5/0x4e0
> [  195.290748][ T3553]  usb_disable_device+0x15f/0x310
> [  195.300276][ T3553]  usb_disconnect.cold+0xd8/0x2fd
> [  195.309417][ T3553]  ? __mutex_unlock_slowpath+0x37/0x280
> [  195.318605][ T3553]  hub_event+0x190b/0x2650
> [  195.327623][ T3553]  process_one_work+0x381/0x940
> [  195.336256][ T3553]  worker_thread+0x5b/0x5d0
> [  195.345289][ T3553]  ? process_one_work+0x940/0x940
> [  195.354733][ T3553]  kthread+0x135/0x170
> [  195.363529][ T3553]  ? kthread_complete_and_exit+0x30/0x30
> [  195.373531][ T3553]  ret_from_fork+0x1f/0x30
> [  195.382434][ T3553]  </TASK>
> [  195.397291][ T3553] 5 locks held by kworker/2:3/3553:
> [  195.408408][ T3553]  #0: ffff88800a0fed38 ((wq_completion)usb_hub_wq){+.+.}-{0:0}, at: process_one_work+0x2b5/0x940
> [  195.417251][ T3553]  #1: ffffc90002963e70 ((work_completion)(&hub->events)){+.+.}-{0:0}, at: process_one_work+0x2b5/0x940
> [  195.429307][ T3553]  #2: ffff888105129220 (&dev->mutex){....}-{3:3}, at: hub_event+0xc4/0x2650
> [  195.439282][ T3553]  #3: ffff88810cc40220 (&dev->mutex){....}-{3:3}, at: usb_disconnect+0x8e/0xa0
> [  195.450701][ T3553]  #4: ffff88810cc409a8 (&dev->mutex){....}-{3:3}, at: device_release_driver_internal+0x55/0x300
> ------------------------------------------------------------

Re: possible deadlock in display_open

[Alan Stern]

On Mon, Apr 25, 2022 at 12:56:19PM +0100, Sean Young wrote:
> On Mon, Apr 25, 2022 at 08:14:12PM +0900, Tetsuo Handa wrote:
> > On 2022/04/25 18:20, Sean Young wrote:
> > > 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.
> > > 
> > > Of course, this depends on probe/disconnect functions being allowed to run
> > > concurrently on different interfaces of the same usb device.
> > 
> > I don't have real hardware to confirm. If you have an imon device which has
> > two usb interfaces, please try below debug printk() patch in order to see
> > whether the caller is already holding locks for serialization.
> 
> I am afraid calling debug_show_held_locks() is not really going to tell us
> this information. This should be figured out from understanding the usb
> stack, not from seeing if the probe happens to be concurrent.
> 
> Just because the locks were not held, does not mean that the usb interface
> initialization is not concurrent.

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.

Alan Stern

Re: possible deadlock in display_open

[Sean Young]

Hi Alan, Tetsuo,

On Mon, Apr 25, 2022 at 12:01:23PM -0400, Alan Stern wrote:
> On Mon, Apr 25, 2022 at 12:56:19PM +0100, Sean Young wrote:
> > On Mon, Apr 25, 2022 at 08:14:12PM +0900, Tetsuo Handa wrote:
> > > On 2022/04/25 18:20, Sean Young wrote:
> > > > 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.
> > > > 
> > > > Of course, this depends on probe/disconnect functions being allowed to run
> > > > concurrently on different interfaces of the same usb device.
> > > 
> > > I don't have real hardware to confirm. If you have an imon device which has
> > > two usb interfaces, please try below debug printk() patch in order to see
> > > whether the caller is already holding locks for serialization.
> > 
> > I am afraid calling debug_show_held_locks() is not really going to tell us
> > this information. This should be figured out from understanding the usb
> > stack, not from seeing if the probe happens to be concurrent.
> > 
> > Just because the locks were not held, does not mean that the usb interface
> > initialization is not concurrent.
> 
> 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.

That is very helpful, thank you Alan. I think the original patch as proposed
by Tetsuo Handa is correct.

However, the commit message does need to say that the lock is not needed
because usb core already guarantees this. Please submit a v2 for this.

Thank you both,

Sean

[PATCH] media: imon: remove redundant serialization

[Tetsuo Handa]

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@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Sean Young <sean@mess.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
---
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

Re: [PATCH] media: imon: remove redundant serialization

[Sean Young]

On Tue, Apr 26, 2022 at 09:54:18AM +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.
> 
> 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@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Sean Young <sean@mess.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>

Thanks, looks good!

> ---
> 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?

That is a separate issue, so that needs a separate patch.

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
> 

[PATCH v2] media: imon: reorganize serialization

[Tetsuo Handa]

On 2022/04/26 16:57, Sean Young wrote:
>> 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?
> 
> That is a separate issue, so that needs a separate patch.
> 

I think this should be addressed in this fix, for this patch needs to keep
"prevent races between open() and disconnect()" role of driver_lock.

It seems to me that "struct imon_context" is shared between intf0 and intf1.
Is imon_disconnect() called on the same "struct imon_context" for two times
if ictx->dev_present_intf0 = true and ictx->dev_present_intf1 = true ? Below
patch assumes that imon_disconnect() is called on the same "struct imon_context"
for two times if ictx->dev_present_intf0 = true and ictx->dev_present_intf1 = true.
Is this assumption correct?

From 73df086af9b4786cfcee81a7e82ee8e71be82290 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Tue, 26 Apr 2022 10:15:24 +0000
Subject: [PATCH v2] media: imon: reorganize serialization

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, we don't need locks for handling race between imon_probe() and
imon_disconnect(). But we still need to handle race between display_open()
/vfd_write()/lcd_write()/display_close() and imon_disconnect(), for
disconnect event can happen while file descriptors are in use.

Since "struct file"->private_data is set by display_open(), vfd_write()/
lcd_write()/display_close() can assume that "struct file"->private_data
is not NULL even after usb_set_intfdata(interface, NULL) was called.

Replace insufficiently held driver_lock with refcount_t based management.
Add a boolean flag for recording whether imon_disconnect() was already
called. Use RCU for accessing this boolean flag and refcount_t.

Link: https://syzkaller.appspot.com/bug?extid=c558267ad910fc494497
Reported-by: syzbot <syzbot+c558267ad910fc494497@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Sean Young <sean@mess.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
---
 drivers/media/rc/imon.c | 99 +++++++++++++++++++----------------------
 1 file changed, 47 insertions(+), 52 deletions(-)

diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index 54da6f60079b..9a4f24e294bc 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -153,6 +153,24 @@ struct imon_context {
 	const struct imon_usb_dev_descr *dev_descr;
 					/* device description with key */
 					/* table for front panels */
+	/*
+	 * Fields for deferring free_imon_context().
+	 *
+	 * Since reference to "struct imon_context" is stored into
+	 * "struct file_operations"->private_data, we need to remember
+	 * how many file descriptors might access this "struct imon_context".
+	 */
+	refcount_t users;
+	/*
+	 * Use a flag for telling display_open()/vfd_write()/lcd_write() that
+	 * imon_disconnect() was already called.
+	 */
+	bool disconnected;
+	/*
+	 * We need to wait for RCU grace period in order to allow
+	 * display_open() to safely check ->disconnected and increment ->users.
+	 */
+	struct rcu_head rcu;
 };
 
 #define TOUCH_TIMEOUT	(HZ/30)
@@ -160,18 +178,18 @@ struct imon_context {
 /* vfd character device file operations */
 static const struct file_operations vfd_fops = {
 	.owner		= THIS_MODULE,
-	.open		= &display_open,
-	.write		= &vfd_write,
-	.release	= &display_close,
+	.open		= display_open,
+	.write		= vfd_write,
+	.release	= display_close,
 	.llseek		= noop_llseek,
 };
 
 /* lcd character device file operations */
 static const struct file_operations lcd_fops = {
 	.owner		= THIS_MODULE,
-	.open		= &display_open,
-	.write		= &lcd_write,
-	.release	= &display_close,
+	.open		= display_open,
+	.write		= lcd_write,
+	.release	= display_close,
 	.llseek		= noop_llseek,
 };
 
@@ -439,9 +457,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);
@@ -481,9 +496,11 @@ static void free_imon_context(struct imon_context *ictx)
 	struct device *dev = ictx->dev;
 
 	usb_free_urb(ictx->tx_urb);
+	WARN_ON(ictx->dev_present_intf0);
 	usb_free_urb(ictx->rx_urb_intf0);
+	WARN_ON(ictx->dev_present_intf1);
 	usb_free_urb(ictx->rx_urb_intf1);
-	kfree(ictx);
+	kfree_rcu(ictx, rcu);
 
 	dev_dbg(dev, "%s: iMON context freed\n", __func__);
 }
@@ -499,9 +516,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) {
@@ -509,13 +523,16 @@ static int display_open(struct inode *inode, struct file *file)
 		retval = -ENODEV;
 		goto exit;
 	}
-	ictx = usb_get_intfdata(interface);
 
-	if (!ictx) {
+	rcu_read_lock();
+	ictx = usb_get_intfdata(interface);
+	if (!ictx || ictx->disconnected || !refcount_inc_not_zero(&ictx->users)) {
+		rcu_read_unlock();
 		pr_err("no context found for minor %d\n", subminor);
 		retval = -ENODEV;
 		goto exit;
 	}
+	rcu_read_unlock();
 
 	mutex_lock(&ictx->lock);
 
@@ -533,8 +550,10 @@ static int display_open(struct inode *inode, struct file *file)
 
 	mutex_unlock(&ictx->lock);
 
+	if (retval && refcount_dec_and_test(&ictx->users))
+		free_imon_context(ictx);
+
 exit:
-	mutex_unlock(&driver_lock);
 	return retval;
 }
 
@@ -544,16 +563,9 @@ static int display_open(struct inode *inode, struct file *file)
  */
 static int display_close(struct inode *inode, struct file *file)
 {
-	struct imon_context *ictx = NULL;
+	struct imon_context *ictx = file->private_data;
 	int retval = 0;
 
-	ictx = file->private_data;
-
-	if (!ictx) {
-		pr_err("no context for device\n");
-		return -ENODEV;
-	}
-
 	mutex_lock(&ictx->lock);
 
 	if (!ictx->display_supported) {
@@ -568,6 +580,8 @@ static int display_close(struct inode *inode, struct file *file)
 	}
 
 	mutex_unlock(&ictx->lock);
+	if (refcount_dec_and_test(&ictx->users))
+		free_imon_context(ictx);
 	return retval;
 }
 
@@ -934,15 +948,12 @@ static ssize_t vfd_write(struct file *file, const char __user *buf,
 	int offset;
 	int seq;
 	int retval = 0;
-	struct imon_context *ictx;
+	struct imon_context *ictx = file->private_data;
 	static const unsigned char vfd_packet6[] = {
 		0x01, 0x00, 0x00, 0x00, 0x00, 0xFF, 0xFF };
 
-	ictx = file->private_data;
-	if (!ictx) {
-		pr_err_ratelimited("no context for device\n");
+	if (ictx->disconnected)
 		return -ENODEV;
-	}
 
 	mutex_lock(&ictx->lock);
 
@@ -1018,13 +1029,10 @@ static ssize_t lcd_write(struct file *file, const char __user *buf,
 			 size_t n_bytes, loff_t *pos)
 {
 	int retval = 0;
-	struct imon_context *ictx;
+	struct imon_context *ictx = file->private_data;
 
-	ictx = file->private_data;
-	if (!ictx) {
-		pr_err_ratelimited("no context for device\n");
+	if (ictx->disconnected)
 		return -ENODEV;
-	}
 
 	mutex_lock(&ictx->lock);
 
@@ -2404,7 +2412,6 @@ static int imon_probe(struct usb_interface *interface,
 	int ifnum, sysfs_err;
 	int ret = 0;
 	struct imon_context *ictx = NULL;
-	struct imon_context *first_if_ctx = NULL;
 	u16 vendor, product;
 
 	usbdev     = usb_get_dev(interface_to_usbdev(interface));
@@ -2416,17 +2423,12 @@ 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;
 		goto fail;
 	}
 
-	first_if_ctx = usb_get_intfdata(first_if);
-
 	if (ifnum == 0) {
 		ictx = imon_init_intf0(interface, id);
 		if (!ictx) {
@@ -2434,9 +2436,11 @@ static int imon_probe(struct usb_interface *interface,
 			ret = -ENODEV;
 			goto fail;
 		}
+		refcount_set(&ictx->users, 1);
 
 	} else {
 		/* this is the secondary interface on the device */
+		struct imon_context *first_if_ctx = usb_get_intfdata(first_if);
 
 		/* fail early if first intf failed to register */
 		if (!first_if_ctx) {
@@ -2450,14 +2454,13 @@ static int imon_probe(struct usb_interface *interface,
 			ret = -ENODEV;
 			goto fail;
 		}
+		refcount_inc(&ictx->users);
 
 	}
 
 	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 +2471,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,10 +2497,8 @@ 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);
+	ictx->disconnected = true;
 	dev = ictx->dev;
 	ifnum = interface->cur_altsetting->desc.bInterfaceNumber;
 
@@ -2542,11 +2539,9 @@ static void imon_disconnect(struct usb_interface *interface)
 		}
 	}
 
-	if (!ictx->dev_present_intf0 && !ictx->dev_present_intf1)
+	if (refcount_dec_and_test(&ictx->users))
 		free_imon_context(ictx);
 
-	mutex_unlock(&driver_lock);
-
 	dev_dbg(dev, "%s: iMON device (intf%d) disconnected\n",
 		__func__, ifnum);
 }
-- 
2.34.1

[PATCH v2 (resend)] media: imon: reorganize serialization

[Tetsuo Handa]

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, we don't need locks for handling race between imon_probe() and
imon_disconnect(). But we still need to handle race between display_open()
/vfd_write()/lcd_write()/display_close() and imon_disconnect(), for
disconnect event can happen while file descriptors are in use.

Since "struct file"->private_data is set by display_open(), vfd_write()/
lcd_write()/display_close() can assume that "struct file"->private_data
is not NULL even after usb_set_intfdata(interface, NULL) was called.

Replace insufficiently held driver_lock with refcount_t based management.
Add a boolean flag for recording whether imon_disconnect() was already
called. Use RCU for accessing this boolean flag and refcount_t.

Since the boolean flag for imon_disconnect() is shared, disconnect event
on either intf0 or intf1 affects both interfaces. But I assume that this
change does not matter, for usually disconnect event would not happen
while interfaces are in use.

Link: https://syzkaller.appspot.com/bug?extid=c558267ad910fc494497
Reported-by: syzbot <syzbot+c558267ad910fc494497@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: syzbot <syzbot+c558267ad910fc494497@syzkaller.appspotmail.com>
Cc: Sean Young <sean@mess.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
---
Changes in v2:
  Defer free_imon_context() using refcount till display_close() is called.

 drivers/media/rc/imon.c | 99 +++++++++++++++++++----------------------
 1 file changed, 47 insertions(+), 52 deletions(-)

diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index 54da6f60079b..9a4f24e294bc 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -153,6 +153,24 @@ struct imon_context {
 	const struct imon_usb_dev_descr *dev_descr;
 					/* device description with key */
 					/* table for front panels */
+	/*
+	 * Fields for deferring free_imon_context().
+	 *
+	 * Since reference to "struct imon_context" is stored into
+	 * "struct file_operations"->private_data, we need to remember
+	 * how many file descriptors might access this "struct imon_context".
+	 */
+	refcount_t users;
+	/*
+	 * Use a flag for telling display_open()/vfd_write()/lcd_write() that
+	 * imon_disconnect() was already called.
+	 */
+	bool disconnected;
+	/*
+	 * We need to wait for RCU grace period in order to allow
+	 * display_open() to safely check ->disconnected and increment ->users.
+	 */
+	struct rcu_head rcu;
 };
 
 #define TOUCH_TIMEOUT	(HZ/30)
@@ -160,18 +178,18 @@ struct imon_context {
 /* vfd character device file operations */
 static const struct file_operations vfd_fops = {
 	.owner		= THIS_MODULE,
-	.open		= &display_open,
-	.write		= &vfd_write,
-	.release	= &display_close,
+	.open		= display_open,
+	.write		= vfd_write,
+	.release	= display_close,
 	.llseek		= noop_llseek,
 };
 
 /* lcd character device file operations */
 static const struct file_operations lcd_fops = {
 	.owner		= THIS_MODULE,
-	.open		= &display_open,
-	.write		= &lcd_write,
-	.release	= &display_close,
+	.open		= display_open,
+	.write		= lcd_write,
+	.release	= display_close,
 	.llseek		= noop_llseek,
 };
 
@@ -439,9 +457,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);
@@ -481,9 +496,11 @@ static void free_imon_context(struct imon_context *ictx)
 	struct device *dev = ictx->dev;
 
 	usb_free_urb(ictx->tx_urb);
+	WARN_ON(ictx->dev_present_intf0);
 	usb_free_urb(ictx->rx_urb_intf0);
+	WARN_ON(ictx->dev_present_intf1);
 	usb_free_urb(ictx->rx_urb_intf1);
-	kfree(ictx);
+	kfree_rcu(ictx, rcu);
 
 	dev_dbg(dev, "%s: iMON context freed\n", __func__);
 }
@@ -499,9 +516,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) {
@@ -509,13 +523,16 @@ static int display_open(struct inode *inode, struct file *file)
 		retval = -ENODEV;
 		goto exit;
 	}
-	ictx = usb_get_intfdata(interface);
 
-	if (!ictx) {
+	rcu_read_lock();
+	ictx = usb_get_intfdata(interface);
+	if (!ictx || ictx->disconnected || !refcount_inc_not_zero(&ictx->users)) {
+		rcu_read_unlock();
 		pr_err("no context found for minor %d\n", subminor);
 		retval = -ENODEV;
 		goto exit;
 	}
+	rcu_read_unlock();
 
 	mutex_lock(&ictx->lock);
 
@@ -533,8 +550,10 @@ static int display_open(struct inode *inode, struct file *file)
 
 	mutex_unlock(&ictx->lock);
 
+	if (retval && refcount_dec_and_test(&ictx->users))
+		free_imon_context(ictx);
+
 exit:
-	mutex_unlock(&driver_lock);
 	return retval;
 }
 
@@ -544,16 +563,9 @@ static int display_open(struct inode *inode, struct file *file)
  */
 static int display_close(struct inode *inode, struct file *file)
 {
-	struct imon_context *ictx = NULL;
+	struct imon_context *ictx = file->private_data;
 	int retval = 0;
 
-	ictx = file->private_data;
-
-	if (!ictx) {
-		pr_err("no context for device\n");
-		return -ENODEV;
-	}
-
 	mutex_lock(&ictx->lock);
 
 	if (!ictx->display_supported) {
@@ -568,6 +580,8 @@ static int display_close(struct inode *inode, struct file *file)
 	}
 
 	mutex_unlock(&ictx->lock);
+	if (refcount_dec_and_test(&ictx->users))
+		free_imon_context(ictx);
 	return retval;
 }
 
@@ -934,15 +948,12 @@ static ssize_t vfd_write(struct file *file, const char __user *buf,
 	int offset;
 	int seq;
 	int retval = 0;
-	struct imon_context *ictx;
+	struct imon_context *ictx = file->private_data;
 	static const unsigned char vfd_packet6[] = {
 		0x01, 0x00, 0x00, 0x00, 0x00, 0xFF, 0xFF };
 
-	ictx = file->private_data;
-	if (!ictx) {
-		pr_err_ratelimited("no context for device\n");
+	if (ictx->disconnected)
 		return -ENODEV;
-	}
 
 	mutex_lock(&ictx->lock);
 
@@ -1018,13 +1029,10 @@ static ssize_t lcd_write(struct file *file, const char __user *buf,
 			 size_t n_bytes, loff_t *pos)
 {
 	int retval = 0;
-	struct imon_context *ictx;
+	struct imon_context *ictx = file->private_data;
 
-	ictx = file->private_data;
-	if (!ictx) {
-		pr_err_ratelimited("no context for device\n");
+	if (ictx->disconnected)
 		return -ENODEV;
-	}
 
 	mutex_lock(&ictx->lock);
 
@@ -2404,7 +2412,6 @@ static int imon_probe(struct usb_interface *interface,
 	int ifnum, sysfs_err;
 	int ret = 0;
 	struct imon_context *ictx = NULL;
-	struct imon_context *first_if_ctx = NULL;
 	u16 vendor, product;
 
 	usbdev     = usb_get_dev(interface_to_usbdev(interface));
@@ -2416,17 +2423,12 @@ 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;
 		goto fail;
 	}
 
-	first_if_ctx = usb_get_intfdata(first_if);
-
 	if (ifnum == 0) {
 		ictx = imon_init_intf0(interface, id);
 		if (!ictx) {
@@ -2434,9 +2436,11 @@ static int imon_probe(struct usb_interface *interface,
 			ret = -ENODEV;
 			goto fail;
 		}
+		refcount_set(&ictx->users, 1);
 
 	} else {
 		/* this is the secondary interface on the device */
+		struct imon_context *first_if_ctx = usb_get_intfdata(first_if);
 
 		/* fail early if first intf failed to register */
 		if (!first_if_ctx) {
@@ -2450,14 +2454,13 @@ static int imon_probe(struct usb_interface *interface,
 			ret = -ENODEV;
 			goto fail;
 		}
+		refcount_inc(&ictx->users);
 
 	}
 
 	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 +2471,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,10 +2497,8 @@ 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);
+	ictx->disconnected = true;
 	dev = ictx->dev;
 	ifnum = interface->cur_altsetting->desc.bInterfaceNumber;
 
@@ -2542,11 +2539,9 @@ static void imon_disconnect(struct usb_interface *interface)
 		}
 	}
 
-	if (!ictx->dev_present_intf0 && !ictx->dev_present_intf1)
+	if (refcount_dec_and_test(&ictx->users))
 		free_imon_context(ictx);
 
-	mutex_unlock(&driver_lock);
-
 	dev_dbg(dev, "%s: iMON device (intf%d) disconnected\n",
 		__func__, ifnum);
 }
-- 
2.34.1

Re: [PATCH v2 (resend)] media: imon: reorganize serialization

[Sean Young]

Hello,

Sorry for not responding earlier.

On Mon, May 02, 2022 at 12:49:04PM +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.
> 
> 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.

So this part of the patch address the issue of driver_lock being
unnecessary. This should be in its own patch, so I am going to merge:

https://patchwork.linuxtv.org/project/linux-media/patch/349f3e34-41ed-f832-3b22-ae10c50e3868@I-love.SAKURA.ne.jp/

> Therefore, we don't need locks for handling race between imon_probe() and
> imon_disconnect(). But we still need to handle race between display_open()
> /vfd_write()/lcd_write()/display_close() and imon_disconnect(), for
> disconnect event can happen while file descriptors are in use.
> 
> Since "struct file"->private_data is set by display_open(), vfd_write()/
> lcd_write()/display_close() can assume that "struct file"->private_data
> is not NULL even after usb_set_intfdata(interface, NULL) was called.
> 
> Replace insufficiently held driver_lock with refcount_t based management.
> Add a boolean flag for recording whether imon_disconnect() was already
> called. Use RCU for accessing this boolean flag and refcount_t.
> 
> Since the boolean flag for imon_disconnect() is shared, disconnect event
> on either intf0 or intf1 affects both interfaces. But I assume that this
> change does not matter, for usually disconnect event would not happen
> while interfaces are in use.
> 
> Link: https://syzkaller.appspot.com/bug?extid=c558267ad910fc494497
> Reported-by: syzbot <syzbot+c558267ad910fc494497@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Tested-by: syzbot <syzbot+c558267ad910fc494497@syzkaller.appspotmail.com>
> Cc: Sean Young <sean@mess.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> ---
> Changes in v2:
>   Defer free_imon_context() using refcount till display_close() is called.
> 
>  drivers/media/rc/imon.c | 99 +++++++++++++++++++----------------------
>  1 file changed, 47 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
> index 54da6f60079b..9a4f24e294bc 100644
> --- a/drivers/media/rc/imon.c
> +++ b/drivers/media/rc/imon.c
> @@ -153,6 +153,24 @@ struct imon_context {
>  	const struct imon_usb_dev_descr *dev_descr;
>  					/* device description with key */
>  					/* table for front panels */
> +	/*
> +	 * Fields for deferring free_imon_context().
> +	 *
> +	 * Since reference to "struct imon_context" is stored into
> +	 * "struct file_operations"->private_data, we need to remember
> +	 * how many file descriptors might access this "struct imon_context".
> +	 */
> +	refcount_t users;
> +	/*
> +	 * Use a flag for telling display_open()/vfd_write()/lcd_write() that
> +	 * imon_disconnect() was already called.
> +	 */
> +	bool disconnected;
> +	/*
> +	 * We need to wait for RCU grace period in order to allow
> +	 * display_open() to safely check ->disconnected and increment ->users.
> +	 */
> +	struct rcu_head rcu;

Is it possible to modify the users/disconnected fields while holding the
lock mutex in imon_context? This would make it unnecessary to use rcu and
simplify the code.

Thanks

Sean

>  };
>  
>  #define TOUCH_TIMEOUT	(HZ/30)
> @@ -160,18 +178,18 @@ struct imon_context {
>  /* vfd character device file operations */
>  static const struct file_operations vfd_fops = {
>  	.owner		= THIS_MODULE,
> -	.open		= &display_open,
> -	.write		= &vfd_write,
> -	.release	= &display_close,
> +	.open		= display_open,
> +	.write		= vfd_write,
> +	.release	= display_close,
>  	.llseek		= noop_llseek,
>  };
>  
>  /* lcd character device file operations */
>  static const struct file_operations lcd_fops = {
>  	.owner		= THIS_MODULE,
> -	.open		= &display_open,
> -	.write		= &lcd_write,
> -	.release	= &display_close,
> +	.open		= display_open,
> +	.write		= lcd_write,
> +	.release	= display_close,
>  	.llseek		= noop_llseek,
>  };
>  
> @@ -439,9 +457,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);
> @@ -481,9 +496,11 @@ static void free_imon_context(struct imon_context *ictx)
>  	struct device *dev = ictx->dev;
>  
>  	usb_free_urb(ictx->tx_urb);
> +	WARN_ON(ictx->dev_present_intf0);
>  	usb_free_urb(ictx->rx_urb_intf0);
> +	WARN_ON(ictx->dev_present_intf1);
>  	usb_free_urb(ictx->rx_urb_intf1);
> -	kfree(ictx);
> +	kfree_rcu(ictx, rcu);
>  
>  	dev_dbg(dev, "%s: iMON context freed\n", __func__);
>  }
> @@ -499,9 +516,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) {
> @@ -509,13 +523,16 @@ static int display_open(struct inode *inode, struct file *file)
>  		retval = -ENODEV;
>  		goto exit;
>  	}
> -	ictx = usb_get_intfdata(interface);
>  
> -	if (!ictx) {
> +	rcu_read_lock();
> +	ictx = usb_get_intfdata(interface);
> +	if (!ictx || ictx->disconnected || !refcount_inc_not_zero(&ictx->users)) {
> +		rcu_read_unlock();
>  		pr_err("no context found for minor %d\n", subminor);
>  		retval = -ENODEV;
>  		goto exit;
>  	}
> +	rcu_read_unlock();
>  
>  	mutex_lock(&ictx->lock);
>  
> @@ -533,8 +550,10 @@ static int display_open(struct inode *inode, struct file *file)
>  
>  	mutex_unlock(&ictx->lock);
>  
> +	if (retval && refcount_dec_and_test(&ictx->users))
> +		free_imon_context(ictx);
> +
>  exit:
> -	mutex_unlock(&driver_lock);
>  	return retval;
>  }
>  
> @@ -544,16 +563,9 @@ static int display_open(struct inode *inode, struct file *file)
>   */
>  static int display_close(struct inode *inode, struct file *file)
>  {
> -	struct imon_context *ictx = NULL;
> +	struct imon_context *ictx = file->private_data;
>  	int retval = 0;
>  
> -	ictx = file->private_data;
> -
> -	if (!ictx) {
> -		pr_err("no context for device\n");
> -		return -ENODEV;
> -	}
> -
>  	mutex_lock(&ictx->lock);
>  
>  	if (!ictx->display_supported) {
> @@ -568,6 +580,8 @@ static int display_close(struct inode *inode, struct file *file)
>  	}
>  
>  	mutex_unlock(&ictx->lock);
> +	if (refcount_dec_and_test(&ictx->users))
> +		free_imon_context(ictx);
>  	return retval;
>  }
>  
> @@ -934,15 +948,12 @@ static ssize_t vfd_write(struct file *file, const char __user *buf,
>  	int offset;
>  	int seq;
>  	int retval = 0;
> -	struct imon_context *ictx;
> +	struct imon_context *ictx = file->private_data;
>  	static const unsigned char vfd_packet6[] = {
>  		0x01, 0x00, 0x00, 0x00, 0x00, 0xFF, 0xFF };
>  
> -	ictx = file->private_data;
> -	if (!ictx) {
> -		pr_err_ratelimited("no context for device\n");
> +	if (ictx->disconnected)
>  		return -ENODEV;
> -	}
>  
>  	mutex_lock(&ictx->lock);
>  
> @@ -1018,13 +1029,10 @@ static ssize_t lcd_write(struct file *file, const char __user *buf,
>  			 size_t n_bytes, loff_t *pos)
>  {
>  	int retval = 0;
> -	struct imon_context *ictx;
> +	struct imon_context *ictx = file->private_data;
>  
> -	ictx = file->private_data;
> -	if (!ictx) {
> -		pr_err_ratelimited("no context for device\n");
> +	if (ictx->disconnected)
>  		return -ENODEV;
> -	}
>  
>  	mutex_lock(&ictx->lock);
>  
> @@ -2404,7 +2412,6 @@ static int imon_probe(struct usb_interface *interface,
>  	int ifnum, sysfs_err;
>  	int ret = 0;
>  	struct imon_context *ictx = NULL;
> -	struct imon_context *first_if_ctx = NULL;
>  	u16 vendor, product;
>  
>  	usbdev     = usb_get_dev(interface_to_usbdev(interface));
> @@ -2416,17 +2423,12 @@ 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;
>  		goto fail;
>  	}
>  
> -	first_if_ctx = usb_get_intfdata(first_if);
> -
>  	if (ifnum == 0) {
>  		ictx = imon_init_intf0(interface, id);
>  		if (!ictx) {
> @@ -2434,9 +2436,11 @@ static int imon_probe(struct usb_interface *interface,
>  			ret = -ENODEV;
>  			goto fail;
>  		}
> +		refcount_set(&ictx->users, 1);
>  
>  	} else {
>  		/* this is the secondary interface on the device */
> +		struct imon_context *first_if_ctx = usb_get_intfdata(first_if);
>  
>  		/* fail early if first intf failed to register */
>  		if (!first_if_ctx) {
> @@ -2450,14 +2454,13 @@ static int imon_probe(struct usb_interface *interface,
>  			ret = -ENODEV;
>  			goto fail;
>  		}
> +		refcount_inc(&ictx->users);
>  
>  	}
>  
>  	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 +2471,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,10 +2497,8 @@ 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);
> +	ictx->disconnected = true;
>  	dev = ictx->dev;
>  	ifnum = interface->cur_altsetting->desc.bInterfaceNumber;
>  
> @@ -2542,11 +2539,9 @@ static void imon_disconnect(struct usb_interface *interface)
>  		}
>  	}
>  
> -	if (!ictx->dev_present_intf0 && !ictx->dev_present_intf1)
> +	if (refcount_dec_and_test(&ictx->users))
>  		free_imon_context(ictx);
>  
> -	mutex_unlock(&driver_lock);
> -
>  	dev_dbg(dev, "%s: iMON device (intf%d) disconnected\n",
>  		__func__, ifnum);
>  }
> -- 
> 2.34.1

Re: [PATCH v2 (resend)] media: imon: reorganize serialization

[Tetsuo Handa]

On 2022/05/02 18:39, Sean Young wrote:
> So this part of the patch address the issue of driver_lock being
> unnecessary. This should be in its own patch, so I am going to merge:
> 
> https://patchwork.linuxtv.org/project/linux-media/patch/349f3e34-41ed-f832-3b22-ae10c50e3868@I-love.SAKURA.ne.jp/

driver_lock is not unnecessary, unless combined with kfree_rcu() approach.

>> +	/*
>> +	 * We need to wait for RCU grace period in order to allow
>> +	 * display_open() to safely check ->disconnected and increment ->users.
>> +	 */
>> +	struct rcu_head rcu;
> 
> Is it possible to modify the users/disconnected fields while holding the
> lock mutex in imon_context? This would make it unnecessary to use rcu and
> simplify the code.

I don't think it is possible, and I don't think it simplifies the code.

Unless we revive global driver_lock lock (untested delta diff is shown below),
nothing prevents from calling kfree(ictx) before holding ictx->lock or
checking ->disconnected or incrementing ->users.

As a whole, I think kfree_rcu() is simpler while removing global lock.

diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index 9a4f24e294bc..469a2f869572 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -166,11 +166,6 @@ struct imon_context {
 	 * imon_disconnect() was already called.
 	 */
 	bool disconnected;
-	/*
-	 * We need to wait for RCU grace period in order to allow
-	 * display_open() to safely check ->disconnected and increment ->users.
-	 */
-	struct rcu_head rcu;
 };
 
 #define TOUCH_TIMEOUT	(HZ/30)
@@ -457,6 +452,9 @@ static struct usb_driver imon_driver = {
 	.id_table	= imon_usb_id_table,
 };
 
+/* to prevent races between open() and kfree() */
+static DEFINE_MUTEX(driver_lock);
+
 /* Module bookkeeping bits */
 MODULE_AUTHOR(MOD_AUTHOR);
 MODULE_DESCRIPTION(MOD_DESC);
@@ -516,6 +514,8 @@ static int display_open(struct inode *inode, struct file *file)
 	int subminor;
 	int retval = 0;
 
+	mutex_lock(&driver_lock);
+
 	subminor = iminor(inode);
 	interface = usb_find_interface(&imon_driver, subminor);
 	if (!interface) {
@@ -524,15 +524,13 @@ static int display_open(struct inode *inode, struct file *file)
 		goto exit;
 	}
 
-	rcu_read_lock();
 	ictx = usb_get_intfdata(interface);
 	if (!ictx || ictx->disconnected || !refcount_inc_not_zero(&ictx->users)) {
-		rcu_read_unlock();
 		pr_err("no context found for minor %d\n", subminor);
 		retval = -ENODEV;
 		goto exit;
 	}
-	rcu_read_unlock();
+	mutex_unlock(&driver_lock);
 
 	mutex_lock(&ictx->lock);
 
@@ -550,10 +548,15 @@ static int display_open(struct inode *inode, struct file *file)
 
 	mutex_unlock(&ictx->lock);
 
-	if (retval && refcount_dec_and_test(&ictx->users))
-		free_imon_context(ictx);
-
+	if (retval) {
+		mutex_unlock(&driver_lock);
+		if (refcount_dec_and_test(&ictx->users))
+			free_imon_context(ictx);
+		mutex_unlock(&driver_lock);
+	}
+	return retval;
 exit:
+	mutex_unlock(&driver_lock);
 	return retval;
 }
 
@@ -2497,6 +2500,8 @@ static void imon_disconnect(struct usb_interface *interface)
 	struct device *dev;
 	int ifnum;
 
+	mutex_lock(&driver_lock);
+
 	ictx = usb_get_intfdata(interface);
 	ictx->disconnected = true;
 	dev = ictx->dev;
@@ -2542,6 +2547,8 @@ static void imon_disconnect(struct usb_interface *interface)
 	if (refcount_dec_and_test(&ictx->users))
 		free_imon_context(ictx);
 
+	mutex_unlock(&driver_lock);
+
 	dev_dbg(dev, "%s: iMON device (intf%d) disconnected\n",
 		__func__, ifnum);
 }

Re: [PATCH v2 (resend)] media: imon: reorganize serialization

[Tetsuo Handa]

On 2022/05/02 19:26, Tetsuo Handa wrote:
> @@ -550,10 +548,15 @@ static int display_open(struct inode *inode, struct file *file)
>  
>  	mutex_unlock(&ictx->lock);
>  
> -	if (retval && refcount_dec_and_test(&ictx->users))
> -		free_imon_context(ictx);
> -
> +	if (retval) {
> +		mutex_unlock(&driver_lock);

Oops. This is mutex_lock(&driver_lock);.

> +		if (refcount_dec_and_test(&ictx->users))
> +			free_imon_context(ictx);
> +		mutex_unlock(&driver_lock);
> +	}
> +	return retval;
>  exit:
> +	mutex_unlock(&driver_lock);
>  	return retval;
>  }
>  

But if you merely want to avoid adding "struct rcu_head rcu;" to "struct imon_context",
we can use synchronize_rcu(); kfree(ictx); sequence (at the cost of waiting for RCU
grace period inside free_imon_context()).

Re: [PATCH v2 (resend)] media: imon: reorganize serialization

[Oliver Neukum]

On 02.05.22 05:49, Tetsuo Handa wrote:
Hi,

there is one open question with this patch I am afraid.
>  
> @@ -533,8 +550,10 @@ static int display_open(struct inode *inode, struct file *file)
>  
>  	mutex_unlock(&ictx->lock);
>  
> +	if (retval && refcount_dec_and_test(&ictx->users))
> +		free_imon_context(ictx);
> +
>

When could this ever happen? Either the device is disconnected, then
you'll go to 'exit' or the refcount will go back to something >0, won't it?

    Regards
        Oliver

Re: [PATCH v2 (resend)] media: imon: reorganize serialization

[Tetsuo Handa]

On 2022/05/02 19:46, Oliver Neukum wrote:
> 
> 
> On 02.05.22 05:49, Tetsuo Handa wrote:
> Hi,
> 
> there is one open question with this patch I am afraid.
>>  
>> @@ -533,8 +550,10 @@ static int display_open(struct inode *inode, struct file *file)
>>  
>>  	mutex_unlock(&ictx->lock);
>>  
>> +	if (retval && refcount_dec_and_test(&ictx->users))
>> +		free_imon_context(ictx);
>> +
>>
> 
> When could this ever happen? Either the device is disconnected, then
> you'll go to 'exit' or the refcount will go back to something >0, won't it?
> 

(Step 0) Say, ictx->users is initially 1.

(Step 1) display_open() increments via refcount_inc_not_zero(), now is 2.

(Step 2) imon_disconnect() decrements via refcount_dec_and_test(), now is 1.

(Step 3) if retval != 0, display_open() needs to undo (Step 1) via
         refcount_dec_and_test(), now is 0.

because imon_disconnect() can be called while display_open() is in progress...

Re: [PATCH v2 (resend)] media: imon: reorganize serialization

[Greg KH]

On Mon, May 02, 2022 at 12:49:04PM +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.
> 
> 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, we don't need locks for handling race between imon_probe() and
> imon_disconnect(). But we still need to handle race between display_open()
> /vfd_write()/lcd_write()/display_close() and imon_disconnect(), for
> disconnect event can happen while file descriptors are in use.
> 
> Since "struct file"->private_data is set by display_open(), vfd_write()/
> lcd_write()/display_close() can assume that "struct file"->private_data
> is not NULL even after usb_set_intfdata(interface, NULL) was called.
> 
> Replace insufficiently held driver_lock with refcount_t based management.
> Add a boolean flag for recording whether imon_disconnect() was already
> called. Use RCU for accessing this boolean flag and refcount_t.
> 
> Since the boolean flag for imon_disconnect() is shared, disconnect event
> on either intf0 or intf1 affects both interfaces. But I assume that this
> change does not matter, for usually disconnect event would not happen
> while interfaces are in use.
> 
> Link: https://syzkaller.appspot.com/bug?extid=c558267ad910fc494497
> Reported-by: syzbot <syzbot+c558267ad910fc494497@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Tested-by: syzbot <syzbot+c558267ad910fc494497@syzkaller.appspotmail.com>
> Cc: Sean Young <sean@mess.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> ---
> Changes in v2:
>   Defer free_imon_context() using refcount till display_close() is called.
> 
>  drivers/media/rc/imon.c | 99 +++++++++++++++++++----------------------
>  1 file changed, 47 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
> index 54da6f60079b..9a4f24e294bc 100644
> --- a/drivers/media/rc/imon.c
> +++ b/drivers/media/rc/imon.c
> @@ -153,6 +153,24 @@ struct imon_context {
>  	const struct imon_usb_dev_descr *dev_descr;
>  					/* device description with key */
>  					/* table for front panels */
> +	/*
> +	 * Fields for deferring free_imon_context().
> +	 *
> +	 * Since reference to "struct imon_context" is stored into
> +	 * "struct file_operations"->private_data, we need to remember
> +	 * how many file descriptors might access this "struct imon_context".
> +	 */
> +	refcount_t users;

Are you sure this is going to work properly?

How do you handle userspace passing around file descriptors to other
processes?

You really should not ever have to count this.

> +	/*
> +	 * Use a flag for telling display_open()/vfd_write()/lcd_write() that
> +	 * imon_disconnect() was already called.
> +	 */
> +	bool disconnected;
> +	/*
> +	 * We need to wait for RCU grace period in order to allow
> +	 * display_open() to safely check ->disconnected and increment ->users.
> +	 */
> +	struct rcu_head rcu;
>  };
>  
>  #define TOUCH_TIMEOUT	(HZ/30)
> @@ -160,18 +178,18 @@ struct imon_context {
>  /* vfd character device file operations */
>  static const struct file_operations vfd_fops = {
>  	.owner		= THIS_MODULE,
> -	.open		= &display_open,
> -	.write		= &vfd_write,
> -	.release	= &display_close,
> +	.open		= display_open,
> +	.write		= vfd_write,
> +	.release	= display_close,
>  	.llseek		= noop_llseek,
>  };
>  
>  /* lcd character device file operations */
>  static const struct file_operations lcd_fops = {
>  	.owner		= THIS_MODULE,
> -	.open		= &display_open,
> -	.write		= &lcd_write,
> -	.release	= &display_close,
> +	.open		= display_open,
> +	.write		= lcd_write,
> +	.release	= display_close,
>  	.llseek		= noop_llseek,
>  };
>  
> @@ -439,9 +457,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);
> @@ -481,9 +496,11 @@ static void free_imon_context(struct imon_context *ictx)
>  	struct device *dev = ictx->dev;
>  
>  	usb_free_urb(ictx->tx_urb);
> +	WARN_ON(ictx->dev_present_intf0);
>  	usb_free_urb(ictx->rx_urb_intf0);
> +	WARN_ON(ictx->dev_present_intf1);
>  	usb_free_urb(ictx->rx_urb_intf1);
> -	kfree(ictx);
> +	kfree_rcu(ictx, rcu);
>  
>  	dev_dbg(dev, "%s: iMON context freed\n", __func__);
>  }
> @@ -499,9 +516,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) {
> @@ -509,13 +523,16 @@ static int display_open(struct inode *inode, struct file *file)
>  		retval = -ENODEV;
>  		goto exit;
>  	}
> -	ictx = usb_get_intfdata(interface);
>  
> -	if (!ictx) {
> +	rcu_read_lock();
> +	ictx = usb_get_intfdata(interface);
> +	if (!ictx || ictx->disconnected || !refcount_inc_not_zero(&ictx->users)) {
> +		rcu_read_unlock();
>  		pr_err("no context found for minor %d\n", subminor);
>  		retval = -ENODEV;
>  		goto exit;
>  	}
> +	rcu_read_unlock();
>  
>  	mutex_lock(&ictx->lock);
>  
> @@ -533,8 +550,10 @@ static int display_open(struct inode *inode, struct file *file)
>  
>  	mutex_unlock(&ictx->lock);
>  
> +	if (retval && refcount_dec_and_test(&ictx->users))
> +		free_imon_context(ictx);
> +
>  exit:
> -	mutex_unlock(&driver_lock);
>  	return retval;
>  }
>  
> @@ -544,16 +563,9 @@ static int display_open(struct inode *inode, struct file *file)
>   */
>  static int display_close(struct inode *inode, struct file *file)
>  {
> -	struct imon_context *ictx = NULL;
> +	struct imon_context *ictx = file->private_data;
>  	int retval = 0;
>  
> -	ictx = file->private_data;
> -
> -	if (!ictx) {
> -		pr_err("no context for device\n");
> -		return -ENODEV;
> -	}
> -
>  	mutex_lock(&ictx->lock);
>  
>  	if (!ictx->display_supported) {
> @@ -568,6 +580,8 @@ static int display_close(struct inode *inode, struct file *file)
>  	}
>  
>  	mutex_unlock(&ictx->lock);
> +	if (refcount_dec_and_test(&ictx->users))
> +		free_imon_context(ictx);

Why not just put a kref into your larger structure?

I think trying to count users of open/close is never going to work, just
allow the normal open/close logic to work properly and track your data
structure based on reference counts like it should be doing already.

thanks,

greg k-h

Re: [PATCH v2 (resend)] media: imon: reorganize serialization

[Tetsuo Handa]

On 2022/05/02 21:53, Greg KH wrote:
>> @@ -153,6 +153,24 @@ struct imon_context {
>>  	const struct imon_usb_dev_descr *dev_descr;
>>  					/* device description with key */
>>  					/* table for front panels */
>> +	/*
>> +	 * Fields for deferring free_imon_context().
>> +	 *
>> +	 * Since reference to "struct imon_context" is stored into
>> +	 * "struct file_operations"->private_data, we need to remember
>> +	 * how many file descriptors might access this "struct imon_context".
>> +	 */
>> +	refcount_t users;
> 
> Are you sure this is going to work properly?
> 
> How do you handle userspace passing around file descriptors to other
> processes?
> 
> You really should not ever have to count this.

Passing around file descriptors results in nothing but delay of freeing memory.
Is this so fatal problem?

static void free_imon_context(struct imon_context *ictx)
{
	struct device *dev = ictx->dev;

	usb_free_urb(ictx->tx_urb);
	usb_free_urb(ictx->rx_urb_intf0);
	usb_free_urb(ictx->rx_urb_intf1);
	kfree(ictx);

	dev_dbg(dev, "%s: iMON context freed\n", __func__);
}

If this is so fatal, can we call usb_free_urb() upon imon_disconnect() ?

Re: [PATCH v2 (resend)] media: imon: reorganize serialization

[Greg KH]

On Mon, May 02, 2022 at 10:04:22PM +0900, Tetsuo Handa wrote:
> On 2022/05/02 21:53, Greg KH wrote:
> >> @@ -153,6 +153,24 @@ struct imon_context {
> >>  	const struct imon_usb_dev_descr *dev_descr;
> >>  					/* device description with key */
> >>  					/* table for front panels */
> >> +	/*
> >> +	 * Fields for deferring free_imon_context().
> >> +	 *
> >> +	 * Since reference to "struct imon_context" is stored into
> >> +	 * "struct file_operations"->private_data, we need to remember
> >> +	 * how many file descriptors might access this "struct imon_context".
> >> +	 */
> >> +	refcount_t users;
> > 
> > Are you sure this is going to work properly?
> > 
> > How do you handle userspace passing around file descriptors to other
> > processes?
> > 
> > You really should not ever have to count this.
> 
> Passing around file descriptors results in nothing but delay of freeing memory.
> Is this so fatal problem?

Not at all, it's just that any driver that tries to count open calls
usually is wrong as they forget about this type of thing, which is why I
asked.

thanks,

greg k-h

Re: [PATCH v2 (resend)] media: imon: reorganize serialization

[Tetsuo Handa]

On 2022/05/02 12:49, Tetsuo Handa wrote:
> @@ -153,6 +153,24 @@ struct imon_context {
>  	const struct imon_usb_dev_descr *dev_descr;
>  					/* device description with key */
>  					/* table for front panels */
> +	/*
> +	 * Fields for deferring free_imon_context().
> +	 *
> +	 * Since reference to "struct imon_context" is stored into
> +	 * "struct file_operations"->private_data, we need to remember

Oops. This is "struct file"->private_data. Please correct when applying. ;-)

> +	 * how many file descriptors might access this "struct imon_context".
> +	 */
> +	refcount_t users;

Re: [PATCH v2 (resend)] media: imon: reorganize serialization

[Oliver Neukum]

On 02.05.22 13:05, Tetsuo Handa wrote:
> On 2022/05/02 19:46, Oliver Neukum wrote:
>>
>> When could this ever happen? Either the device is disconnected, then
>> you'll go to 'exit' or the refcount will go back to something >0, won't it?
>>
> (Step 0) Say, ictx->users is initially 1.
>
> (Step 1) display_open() increments via refcount_inc_not_zero(), now is 2.
>
> (Step 2) imon_disconnect() decrements via refcount_dec_and_test(), now is 1.
>
> (Step 3) if retval != 0, display_open() needs to undo (Step 1) via
>          refcount_dec_and_test(), now is 0.
>
> because imon_disconnect() can be called while display_open() is in progress...
>
Hi,

thank you. I did not think of error handling. You are correct.

    Regards
        Oliver