Re: [PATCH v3 2/3] uio: change to use the mutex lock instead of the spin lock

[Hamish Martin]

Looks ok to me.

Reviewed-by: Hamish Martin <hamish.martin@xxxxxxxxxxxxxxxxxxx>

On 07/06/2018 02:57 PM, xiubli@xxxxxxxxxx wrote:
> From: Xiubo Li <xiubli@xxxxxxxxxx>
>
> We are hitting a regression with the following commit:
>
> commit a93e7b331568227500186a465fee3c2cb5dffd1f
> Author: Hamish Martin <hamish.martin@xxxxxxxxxxxxxxxxxxx>
> Date:   Mon May 14 13:32:23 2018 +1200
>
>      uio: Prevent device destruction while fds are open
>
> The problem is the addition of spin_lock_irqsave in uio_write. This
> leads to hitting  uio_write -> copy_from_user -> _copy_from_user ->
> might_fault and the logs filling up with sleeping warnings.
>
> I also noticed some uio drivers allocate memory, sleep, grab mutexes
> from callouts like open() and release and uio is now doing
> spin_lock_irqsave while calling them.
>
> Reported-by: Mike Christie <mchristi@xxxxxxxxxx>
> CC: Hamish Martin <hamish.martin@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> ---
>   drivers/uio/uio.c          | 32 +++++++++++++-------------------
>   include/linux/uio_driver.h |  2 +-
>   2 files changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index b4b2ae1..655ade4 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -433,7 +433,6 @@ static int uio_open(struct inode *inode, struct file *filep)
>   	struct uio_device *idev;
>   	struct uio_listener *listener;
>   	int ret = 0;
> -	unsigned long flags;
>   
>   	mutex_lock(&minor_lock);
>   	idev = idr_find(&uio_idr, iminor(inode));
> @@ -460,10 +459,10 @@ static int uio_open(struct inode *inode, struct file *filep)
>   	listener->event_count = atomic_read(&idev->event);
>   	filep->private_data = listener;
>   
> -	spin_lock_irqsave(&idev->info_lock, flags);
> +	mutex_lock(&idev->info_lock);
>   	if (idev->info && idev->info->open)
>   		ret = idev->info->open(idev->info, inode);
> -	spin_unlock_irqrestore(&idev->info_lock, flags);
> +	mutex_unlock(&idev->info_lock);
>   	if (ret)
>   		goto err_infoopen;
>   
> @@ -495,12 +494,11 @@ static int uio_release(struct inode *inode, struct file *filep)
>   	int ret = 0;
>   	struct uio_listener *listener = filep->private_data;
>   	struct uio_device *idev = listener->dev;
> -	unsigned long flags;
>   
> -	spin_lock_irqsave(&idev->info_lock, flags);
> +	mutex_lock(&idev->info_lock);
>   	if (idev->info && idev->info->release)
>   		ret = idev->info->release(idev->info, inode);
> -	spin_unlock_irqrestore(&idev->info_lock, flags);
> +	mutex_unlock(&idev->info_lock);
>   
>   	module_put(idev->owner);
>   	kfree(listener);
> @@ -513,12 +511,11 @@ static __poll_t uio_poll(struct file *filep, poll_table *wait)
>   	struct uio_listener *listener = filep->private_data;
>   	struct uio_device *idev = listener->dev;
>   	__poll_t ret = 0;
> -	unsigned long flags;
>   
> -	spin_lock_irqsave(&idev->info_lock, flags);
> +	mutex_lock(&idev->info_lock);
>   	if (!idev->info || !idev->info->irq)
>   		ret = -EIO;
> -	spin_unlock_irqrestore(&idev->info_lock, flags);
> +	mutex_unlock(&idev->info_lock);
>   
>   	if (ret)
>   		return ret;
> @@ -537,12 +534,11 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
>   	DECLARE_WAITQUEUE(wait, current);
>   	ssize_t retval = 0;
>   	s32 event_count;
> -	unsigned long flags;
>   
> -	spin_lock_irqsave(&idev->info_lock, flags);
> +	mutex_lock(&idev->info_lock);
>   	if (!idev->info || !idev->info->irq)
>   		retval = -EIO;
> -	spin_unlock_irqrestore(&idev->info_lock, flags);
> +	mutex_unlock(&idev->info_lock);
>   
>   	if (retval)
>   		return retval;
> @@ -592,9 +588,8 @@ static ssize_t uio_write(struct file *filep, const char __user *buf,
>   	struct uio_device *idev = listener->dev;
>   	ssize_t retval;
>   	s32 irq_on;
> -	unsigned long flags;
>   
> -	spin_lock_irqsave(&idev->info_lock, flags);
> +	mutex_lock(&idev->info_lock);
>   	if (!idev->info || !idev->info->irq) {
>   		retval = -EIO;
>   		goto out;
> @@ -618,7 +613,7 @@ static ssize_t uio_write(struct file *filep, const char __user *buf,
>   	retval = idev->info->irqcontrol(idev->info, irq_on);
>   
>   out:
> -	spin_unlock_irqrestore(&idev->info_lock, flags);
> +	mutex_unlock(&idev->info_lock);
>   	return retval ? retval : sizeof(s32);
>   }
>   
> @@ -865,7 +860,7 @@ int __uio_register_device(struct module *owner,
>   
>   	idev->owner = owner;
>   	idev->info = info;
> -	spin_lock_init(&idev->info_lock);
> +	mutex_init(&idev->info_lock);
>   	init_waitqueue_head(&idev->wait);
>   	atomic_set(&idev->event, 0);
>   
> @@ -929,7 +924,6 @@ int __uio_register_device(struct module *owner,
>   void uio_unregister_device(struct uio_info *info)
>   {
>   	struct uio_device *idev;
> -	unsigned long flags;
>   
>   	if (!info || !info->uio_dev)
>   		return;
> @@ -943,9 +937,9 @@ void uio_unregister_device(struct uio_info *info)
>   	if (info->irq && info->irq != UIO_IRQ_CUSTOM)
>   		free_irq(info->irq, idev);
>   
> -	spin_lock_irqsave(&idev->info_lock, flags);
> +	mutex_lock(&idev->info_lock);
>   	idev->info = NULL;
> -	spin_unlock_irqrestore(&idev->info_lock, flags);
> +	mutex_unlock(&idev->info_lock);
>   
>   	device_unregister(&idev->dev);
>   
> diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
> index 6c5f207..6f8b68c 100644
> --- a/include/linux/uio_driver.h
> +++ b/include/linux/uio_driver.h
> @@ -75,7 +75,7 @@ struct uio_device {
>           struct fasync_struct    *async_queue;
>           wait_queue_head_t       wait;
>           struct uio_info         *info;
> -	spinlock_t		info_lock;
> +	struct mutex		info_lock;
>           struct kobject          *map_dir;
>           struct kobject          *portio_dir;
>   };