Re: [PATCH] usb: use mutex_lock in iowarrior_read()

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Sep 16, 2024 at 01:06:29PM +0900, Jeongjun Park wrote:
> Currently, iowarrior_read() does not provide any protection for the
> iowarrior structure, so the iowarrior structure is vulnerable to data-race.
> 
> Therefore, I think it is appropriate to protect the structure using
> mutex_lock in iowarrior_read().
> 
> Fixes: 946b960d13c1 ("USB: add driver for iowarrior devices.")
> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> ---
>  drivers/usb/misc/iowarrior.c | 42 +++++++++++++++++++++++++++---------
>  1 file changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
> index 6d28467ce352..7f3d37b395c3 100644
> --- a/drivers/usb/misc/iowarrior.c
> +++ b/drivers/usb/misc/iowarrior.c
> @@ -277,28 +277,41 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer,
>  	struct iowarrior *dev;
>  	int read_idx;
>  	int offset;
> +	int retval = 0;
>  
>  	dev = file->private_data;
>  
> +	if (!dev) {

How can this happen?  How was this tested?

And you didn't mention this in your changelog, why?

> +		retval = -ENODEV;
> +		goto exit;
> +	}

What prevents dev from becoming invalid after it is checked here?

> +
> +	mutex_lock(&dev->mutex);

Please use the guard() form here, it makes the change much simpler and
easier to review and maintain.

thanks,

greg k-h