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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxxx/

> 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@xxxxxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> Tested-by: syzbot <syzbot+c558267ad910fc494497@xxxxxxxxxxxxxxxxxxxxxxxxx>
> Cc: Sean Young <sean@xxxxxxxx>
> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> ---
> 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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux