| Message ID | 20240916040629.28750-1-aha310510@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: use mutex_lock in iowarrior_read() | expand |
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
Greg KH <gregkh@linuxfoundation.org> wrote: > > 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? There is no separate reproduction code or bug report. However, all other functions in iowarrior use mutex_lock to protect the iowarrior structure. Only iowarrior_read does not use mutex_lock, which could potentially cause bugs. There is no reason why this function should not use mutex_lock, so I think adding a lock is appropriate. > > > + retval = -ENODEV; > > + goto exit; > > + } > > What prevents dev from becoming invalid after it is checked here? I'm not sure what this means. Can you explain it in more detail? > > > + > > + mutex_lock(&dev->mutex); > > Please use the guard() form here, it makes the change much simpler and > easier to review and maintain. I didn't know such a convenient function existed. It certainly seems like it would make maintenance easier, but it also seems like it would be a good idea to consistently replace all mutex_locks in iowarrior.c with guard(). What do you think? Regards, Jeongjun Park > > thanks, > > greg k-h
On Mon, Sep 16, 2024 at 01:43:22PM +0900, Jeongjun Park wrote: > Greg KH <gregkh@linuxfoundation.org> wrote: > > > > 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? > > There is no separate reproduction code or bug report. However, all other > functions in iowarrior use mutex_lock to protect the iowarrior structure. > Only iowarrior_read does not use mutex_lock, which could potentially cause > bugs. But if you don't have a report, and don't have this device, how can you test this to make sure? > There is no reason why this function should not use mutex_lock, > so I think adding a lock is appropriate. Fair enough, but do it properly please. > > > + retval = -ENODEV; > > > + goto exit; > > > + } > > > > What prevents dev from becoming invalid after it is checked here? > > I'm not sure what this means. Can you explain it in more detail? What happens if the private_data pointer becomes "stale" right after checking it is not NULL? You need to explain how it is safe, if it is, to do this. Actually, what ever sets this to NULL? I think this check isn't needed at all from looking at the code (hint, think about the lifetime of the file pointer...) > > > + mutex_lock(&dev->mutex); > > > > Please use the guard() form here, it makes the change much simpler and > > easier to review and maintain. > > I didn't know such a convenient function existed. It certainly seems like > it would make maintenance easier, but it also seems like it would be a > good idea to consistently replace all mutex_locks in iowarrior.c with guard(). > > What do you think? Unless you have the hardware to test this, I would not worry about doing conversions like this. I think I have this device somewhere around in my "big box of USB devices", but testing any driver changes for it will take a while before I can find it. Actually, in looking at the code further, I think the lock is not taken on purpose, so if you want to change this, you will have to document why it is now really needed and what will happen if it is not. thanks, greg k-h
On 16.09.24 06:15, Greg KH wrote: > 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? It cannot happen. [..] >> + mutex_lock(&dev->mutex); > > 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: From 1627bc3a8e9aae60bdfc85430db2a44283e71a68 Mon Sep 17 00:00:00 2001 From: Oliver Neukum <oneukum@suse.com> Date: Thu, 12 Sep 2024 12:47:33 +0200 Subject: [PATCH] iowarrior: fix read racing against itself case In a multithreaded application iowarrior_read() can race against itself. It needs to take the mutex. Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/usb/misc/iowarrior.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c index 6d28467ce352..3b49d6c7b569 100644 --- a/drivers/usb/misc/iowarrior.c +++ b/drivers/usb/misc/iowarrior.c @@ -277,6 +277,8 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer, struct iowarrior *dev; int read_idx; int offset; + int result; + bool nonblock = file->f_flags & O_NONBLOCK; dev = file->private_data; @@ -292,14 +294,25 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer, && (count != (dev->report_size + 1))) return -EINVAL; + if (nonblock) { + result = mutex_trylock(&dev->mutex); + if (!result) + return -EAGAIN; + result = 0; + } else { + result = mutex_lock_interruptible(&dev->mutex); + if (result < 0) + return -EINTR; + } /* 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; - else { + if (nonblock) { + result = -EAGAIN; + goto out; + } else { //next line will return when there is either new data, or the device is unplugged int r = wait_event_interruptible(dev->read_wait, (!dev->present @@ -309,14 +322,17 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer, -1)); if (r) { //we were interrupted by a signal - return -ERESTART; + result = -ERESTART; + goto out; } if (!dev->present) { //The device was unplugged - return -ENODEV; + result = -ENODEV; + goto out; } if (read_idx == -1) { // Can this happen ??? + mutex_unlock(&dev->mutex); return 0; } } @@ -324,13 +340,16 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer, offset = read_idx * (dev->report_size + 1); if (copy_to_user(buffer, dev->read_queue + offset, count)) { - return -EFAULT; + result = -EFAULT; + goto out; } } while (atomic_read(&dev->overflow_flag)); read_idx = ++read_idx == MAX_INTERRUPT_BUFFER ? 0 : read_idx; atomic_set(&dev->read_idx, read_idx); - return count; +out: + mutex_unlock(&dev->mutex); + return result < 0 ? result : count; } /*
Oliver Neukum <oneukum@suse.com> wrote: > > > > On 16.09.24 06:15, Greg KH wrote: > > 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? > > It cannot happen. > > [..] > >> + mutex_lock(&dev->mutex); > > > > 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. > > From 1627bc3a8e9aae60bdfc85430db2a44283e71a68 Mon Sep 17 00:00:00 2001 > From: Oliver Neukum <oneukum@suse.com> > Date: Thu, 12 Sep 2024 12:47:33 +0200 > Subject: [PATCH] iowarrior: fix read racing against itself case > > In a multithreaded application iowarrior_read() can race against itself. > It needs to take the mutex. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > --- > drivers/usb/misc/iowarrior.c | 33 ++++++++++++++++++++++++++------- > 1 file changed, 26 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c > index 6d28467ce352..3b49d6c7b569 100644 > --- a/drivers/usb/misc/iowarrior.c > +++ b/drivers/usb/misc/iowarrior.c > @@ -277,6 +277,8 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer, > struct iowarrior *dev; > int read_idx; > int offset; > + int result; > + bool nonblock = file->f_flags & O_NONBLOCK; > > dev = file->private_data; > > @@ -292,14 +294,25 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer, > && (count != (dev->report_size + 1))) > return -EINVAL; > > + if (nonblock) { > + result = mutex_trylock(&dev->mutex); > + if (!result) > + return -EAGAIN; > + result = 0; > + } else { > + result = mutex_lock_interruptible(&dev->mutex); > + if (result < 0) > + return -EINTR; > + } > /* 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; > - else { > + if (nonblock) { > + result = -EAGAIN; > + goto out; > + } else { > //next line will return when there is either new data, or the device is unplugged > int r = wait_event_interruptible(dev->read_wait, > (!dev->present > @@ -309,14 +322,17 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer, > -1)); > if (r) { > //we were interrupted by a signal > - return -ERESTART; > + result = -ERESTART; > + goto out; > } > if (!dev->present) { > //The device was unplugged > - return -ENODEV; > + result = -ENODEV; > + goto out; > } > if (read_idx == -1) { > // Can this happen ??? > + mutex_unlock(&dev->mutex); > return 0; > } > } > @@ -324,13 +340,16 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer, > > offset = read_idx * (dev->report_size + 1); > if (copy_to_user(buffer, dev->read_queue + offset, count)) { > - return -EFAULT; > + result = -EFAULT; > + goto out; > } > } while (atomic_read(&dev->overflow_flag)); > > read_idx = ++read_idx == MAX_INTERRUPT_BUFFER ? 0 : read_idx; > atomic_set(&dev->read_idx, read_idx); > - return count; > +out: > + mutex_unlock(&dev->mutex); > + return result < 0 ? result : count; > } > > /* > -- > 2.45.2 > >
Hi, On 16.09.24 14:44, Jeongjun Park wrote: > Oliver Neukum <oneukum@suse.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. Regards Oliver
Oliver Neukum <oneukum@suse.com> wrote: > > Hi, > > On 16.09.24 14:44, Jeongjun Park wrote: > > Oliver Neukum <oneukum@suse.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
On 17.09.24 08:23, Jeongjun Park wrote: > Oliver Neukum <oneukum@suse.com> wrote: > 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. It will still block. > 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. You are right. dev->present should be checked under the lock only. > 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)) { This blocks. To quote the man page: O_NONBLOCK or O_NDELAY When possible, the file is opened in nonblocking mode. Neither the open() nor any subsequent I/O operations on the file descriptor which is returned will cause the calling process to wait. [..] > +unlock_exit: > + mutex_unlock(&dev->mutex); > +exit: > + return retval; The rest looks good to me. Regards Oliver
Oliver Neukum <oneukum@suse.com> wrote: > > On 17.09.24 08:23, Jeongjun Park wrote: > > Oliver Neukum <oneukum@suse.com> wrote: > > > 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. > > It will still block. > > > 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. > > You are right. dev->present should be checked under the lock only. > > > 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)) { > > This blocks. To quote the man page: > > O_NONBLOCK or O_NDELAY > When possible, the file is opened in nonblocking mode. > Neither the open() nor any subsequent I/O operations on the file descriptor which is > returned will cause the calling process to wait. > > Okay, I understand. Then I think it would be appropriate to do the patch below to prevent blocking, but I have one question. Currently, many misc usb drivers do not seem to handle the O_NONBLOCK flag when using mutex_lock. If this is really necessary code, I think it would require code modifications to other functions inside iowarrior and many misc usb drivers. What do you think about this? Regards, Jeongjun Park --- 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..dbf0ed04f7c3 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 = 0; 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; } /* -- > > [..] > > +unlock_exit: > > + mutex_unlock(&dev->mutex); > > +exit: > > + return retval; > > The rest looks good to me. > > Regards > Oliver >
On 17.09.24 12:01, Jeongjun Park wrote: > Okay, I understand. Then I think it would be appropriate to do > the patch below to prevent blocking, but I have one question. > > Currently, many misc usb drivers do not seem to handle the > O_NONBLOCK flag when using mutex_lock. If this is really Yes. The quality of many drivers could be improved. Feel free to make patches. However, the lack of quality elsewhere does not justify a regression. Hence code fixing drivers already correctly supporting O_NONBLOCK must be correct in that regard. > necessary code, I think it would require code modifications to > other functions inside iowarrior and many misc usb drivers. > > What do you think about this? The formatting seems to be broken. In terms of content it is good. Regards Oliver
Oliver Neukum <oneukum@suse.com> wrote: > > On 17.09.24 12:01, Jeongjun Park wrote: > > > Okay, I understand. Then I think it would be appropriate to do > > the patch below to prevent blocking, but I have one question. > > > > Currently, many misc usb drivers do not seem to handle the > > O_NONBLOCK flag when using mutex_lock. If this is really > > Yes. The quality of many drivers could be improved. > Feel free to make patches. However, the lack of quality elsewhere > does not justify a regression. Hence code fixing drivers already > correctly supporting O_NONBLOCK must be correct in that regard. > > > necessary code, I think it would require code modifications to > > other functions inside iowarrior and many misc usb drivers. > > > > What do you think about this? > > The formatting seems to be broken. In terms of content it is good. Oh, this format was sent broken. I'm sending you the patch below again. I'll send you a new patch with this patch right away. And I'll try to write a patch for other functions and misc usbs that don't support O_NONBLOCK properly soon. Regards, Jeongjun Park --- 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..dbf0ed04f7c3 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 = 0; 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; } /* -- > > Regards > Oliver
On 17.09.24 17:41, Jeongjun Park wrote: Hi, comments in line. > --- > 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..dbf0ed04f7c3 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 = 0; Initialization is useless. The rest is fine. Regards Oliver
Oliver Neukum <oneukum@suse.com> wrote: > > On 17.09.24 17:41, Jeongjun Park wrote: > > Hi, > > comments in line. > > > --- > > 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..dbf0ed04f7c3 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 = 0; > > Initialization is useless. > > The rest is fine. If so, I will modify only that part and send it to you as a patch. Regards, Jeongjun Park > > Regards > Oliver >
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) { + retval = -ENODEV; + goto exit; + } + + mutex_lock(&dev->mutex); /* 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 +322,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; } /*
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(-) --