Re: [PATCH v2] usb: using mutex lock and supporting O_NONBLOCK flag in iowarrior_read()

Jeongjun Park aha310510@gmail.com wrote:

iowarrior_read() uses the iowarrior dev structure, but does not use any lock on the structure. This can cause various bugs including data-races, so it is more appropriate to use a mutex lock to safely protect the iowarrior dev structure. When using a mutex lock, you should split the branch to prevent blocking when the O_NONBLOCK flag is set.

In addition, it is unnecessary to check for NULL on the iowarrior dev structure obtained by reading file->private_data. Therefore, it is better to remove the check.

Cc: stable@vger.kernel.org Fixes: 946b960d13c1 ("USB: add driver for iowarrior devices.") Signed-off-by: Jeongjun Park aha310510@gmail.com

I think this patch should be moved to the usb-linus tree to be applied in the next rc version. iowarrior_read() is very vulnerable to a data-race because it reads a struct iowarrior without a mutex_lock. I think this almost certainly leads to a data-race, so I think this function should be moved to the usb-linus tree to be fixed as soon as possible.

I would appreciate it if you could review this.

Regards,

Jeongjun Park

---

v1 -> v2: Added cc tag and change log

drivers/usb/misc/iowarrior.c | 46 ++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c index 6d28467ce352..a513766b4985 100644 --- a/drivers/usb/misc/iowarrior.c +++ b/drivers/usb/misc/iowarrior.c @@ -277,28 +277,45 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer, struct iowarrior *dev; int read_idx; int offset;

•   int retval;
  
    dev = file->private_data;
  
  

•   if (file->f_flags & O_NONBLOCK) {
  

•           retval = mutex_trylock(&dev->mutex);
  

•           if (!retval)
  

•                   return -EAGAIN;
  

•   } else {
  

•           retval = mutex_lock_interruptible(&dev->mutex);
  

•           if (retval)
  

•                   return -ERESTARTSYS;
  

•   }
  

•   /* verify that the device wasn't unplugged */
  

•   if (!dev || !dev->present)
  

•           return -ENODEV;
  

•   if (!dev->present) {
  

•           retval = -ENODEV;
  

•           goto exit;
  

•   }
  
    dev_dbg(&dev->interface->dev, "minor %d, count = %zd\n",
            dev->minor, count);
  
    /* read count must be packet size (+ time stamp) */
    if ((count != dev->report_size)
  

•       && (count != (dev->report_size + 1)))
  

•           return -EINVAL;
  

•       && (count != (dev->report_size + 1))) {
  

•           retval = -EINVAL;
  

•           goto exit;
  

•   }
  
    /* repeat until no buffer overrun in callback handler occur */
    do {
            atomic_set(&dev->overflow_flag, 0);
            if ((read_idx = read_index(dev)) == -1) {
                    /* queue empty */
  

•                   if (file->f_flags & O_NONBLOCK)
  

•                           return -EAGAIN;
  

•                   if (file->f_flags & O_NONBLOCK) {
  

•                           retval = -EAGAIN;
  

•                           goto exit;
  

•                   }
                    else {
                            //next line will return when there is either new data, or the device is unplugged
                            int r = wait_event_interruptible(dev->read_wait,
  

@@ -309,28 +326,37 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer, -1)); if (r) { //we were interrupted by a signal

•                                   return -ERESTART;
  

•                                   retval = -ERESTART;
  

•                                   goto exit;
                            }
                            if (!dev->present) {
                                    //The device was unplugged
  

•                                   return -ENODEV;
  

•                                   retval = -ENODEV;
  

•                                   goto exit;
                            }
                            if (read_idx == -1) {
                                    // Can this happen ???
  

•                                   return 0;
  

•                                   retval = 0;
  

•                                   goto exit;
                            }
                    }
            }
  
            offset = read_idx * (dev->report_size + 1);
            if (copy_to_user(buffer, dev->read_queue + offset, count)) {
  

•                   return -EFAULT;
  

•                   retval = -EFAULT;
  

•                   goto exit;
            }
    } while (atomic_read(&dev->overflow_flag));
  
    read_idx = ++read_idx == MAX_INTERRUPT_BUFFER ? 0 : read_idx;
    atomic_set(&dev->read_idx, read_idx);
  

•   mutex_unlock(&dev->mutex);
    return count;
  

+exit:

•   mutex_unlock(&dev->mutex);
  

•   return retval;
  

}

/*