| Message ID | 349f3e34-41ed-f832-3b22-ae10c50e3868@I-love.SAKURA.ne.jp |
|---|---|
| State | New |
| Headers | show |
| Series | media: imon: remove redundant serialization | expand |
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
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); }
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()).
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
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...
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
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() ?
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
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;
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); }
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(-)