Message-ID: <CAO9qdTHWfYv8u-gJqGkuG_OSdkU9c=qZSnEbE+zCYWG5bT6r+Q@mail.gmail.com>
Date: Tue, 17 Sep 2024 15:23:02 +0900
From: Jeongjun Park <aha310510@...il.com>
To: Oliver Neukum <oneukum@...e.com>
Cc: Greg KH <gregkh@...uxfoundation.org>, colin.i.king@...il.com, 
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: use mutex_lock in iowarrior_read()

Oliver Neukum <oneukum@...e.com> wrote:
>
> Hi,
>
> On 16.09.24 14:44, Jeongjun Park wrote:
> > Oliver Neukum <oneukum@...e.com> wrote:
> >>
> >>
> >>
> >> On 16.09.24 06:15, Greg KH wrote:
> >>> On Mon, Sep 16, 2024 at 01:06:29PM +0900, Jeongjun Park wrote:
>
> >>> Please use the guard() form here, it makes the change much simpler and
> >>> easier to review and maintain.
> >>
> >> That would break the O_NONBLOCK case.
> >>
> >> Looking at the code it indeed looks like iowarrior_read() can race
> >> with itself. Strictly speaking it always could happen if a task used
> >> fork() after open(). The driver tries to restrict its usage to one
> >> thread, but I doubt that the logic is functional.
> >>
> >> It seems to me the correct fix is something like this:
> >
> > Well, I don't know why it's necessary to modify it like this.
> > I think it would be more appropriate to patch it to make it
> > more maintainable by using guard() as Greg suggested.
>
> Allow me to explain detail.
>
> guard() internally uses mutex_lock(). That means that
>
> a) it will block
> b) having blocked it will sleep in the state TASK_UNINTERRUPTIBLE
>
> The driver itself uses TASK_INTERRUPTIBLE in iowarrior_read(),
> when it waits for IO. That is entirely correct, as it waits for
> an external device doing an operation that may never occur. You
> must use TASK_INTERRUPTIBLE.
>
> Now, if you use mutex_lock() to wait for a task waiting for IO
> to occur in the state TASK_INTERRUPTIBLE, you are indirectlywaiting for
> an event that you must wait for in TASK_INTERRUPTIBLE in the state
> TASK_UNINTERRUPTIBLE.
> That is a bug. You have created a task that cannot be killed (uid may not match),
> but may have to be killed. Furthermore you block even in case the
> device has been opened with O_NONBLOCK, which is a second bug.
>
> These limitations are inherent in guard(). Therefore you cannot use
> guard here.

Okay. But O_NONBLOCK flag check already exists, and I don't know
if we need to branch separately to mutex_trylock just because O_NONBLOCK
flag exists. I think mutex_lock_interruptible is enough.

And the point of locking is too late. I think it would be more appropriate to
read file->private_data and then lock it right away.

I think this patch is a more appropriate patch:

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

diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
index 6d28467ce352..6fb4ecebbc15 100644
--- a/drivers/usb/misc/iowarrior.c
+++ b/drivers/usb/misc/iowarrior.c
@@ -277,28 +277,40 @@ 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 (mutex_lock_interruptible(&dev->mutex)) {
+       retval = -EAGAIN;
+       goto exit;
+   }
+
    /* verify that the device wasn't unplugged */
-   if (!dev || !dev->present)
-       return -ENODEV;
+   if (!dev->present) {
+       retval = -ENODEV;
+       goto unlock_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 unlock_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 unlock_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 +321,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 unlock_exit;
                }
                if (!dev->present) {
                    //The device was unplugged
-                   return -ENODEV;
+                   retval = -ENODEV;
+                   goto unlock_exit;
                }
                if (read_idx == -1) {
                    // Can this happen ???
-                   return 0;
+                   goto unlock_exit;
                }
            }
        }

        offset = read_idx * (dev->report_size + 1);
        if (copy_to_user(buffer, dev->read_queue + offset, count)) {
-           return -EFAULT;
+           retval = -EFAULT;
+           goto unlock_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;
+
+unlock_exit:
+   mutex_unlock(&dev->mutex);
+exit:
+   return retval;
 }

 /*
--

>
>         Regards
>                 Oliver

Powered by blists - more mailing lists