Tidy up the "write" file operation handler, `comedi_write()` a bit and
improve the error handling.
1) staging: comedi: rearrange comedi_write() code
2) staging: comedi: do extra checks for becoming non-busy for "write"
3) staging: comedi: make some variables unsigned in comedi_write()
4) staging: comedi: avoid bad truncation of a size_t in comedi_write()
5) staging: comedi: allow buffer wraparound in comedi_write()
6) staging: comedi: return error on "write" if no command set up
7) staging: comedi: simplify returned errors for comedi_write()
8) staging: comedi: check for more errors for zero-length write
drivers/staging/comedi/comedi_fops.c | 124 ++++++++++++++++-------------------
1 file changed, 56 insertions(+), 68 deletions(-)
Rearrange the code in `comedi_write()` to reduce the amount of
indentation. The code never reiterates the `while` loop once `count`
has become non-zero, so we can check that in the `while` condition to
save an indentation level. (Note that `nbytes` has been checked to be
non-zero before entering the loop, so we can remove that check.) Move
the code that makes the subdevice "become non-busy" outside the `while`
loop, using a new flag variable `become_nonbusy` to decide whether it
needs to be done. This simplifies the wait queue handling so there is a
single place where the task is removed from the wait queue, and we can
remove the `on_wait_queue` flag variable.
Signed-off-by: Ian Abbott <[email protected]>
---
drivers/staging/comedi/comedi_fops.c | 71 +++++++++++++++---------------------
1 file changed, 30 insertions(+), 41 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 7b4af51..c9da6f3 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2307,7 +2307,7 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
DECLARE_WAITQUEUE(wait, current);
struct comedi_file *cfp = file->private_data;
struct comedi_device *dev = cfp->dev;
- bool on_wait_queue = false;
+ bool become_nonbusy = false;
bool attach_locked;
unsigned int old_detach_count;
@@ -2342,48 +2342,16 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
}
add_wait_queue(&async->wait_head, &wait);
- on_wait_queue = true;
- while (nbytes > 0 && !retval) {
+ while (count == 0 && !retval) {
unsigned runflags;
set_current_state(TASK_INTERRUPTIBLE);
runflags = comedi_get_subdevice_runflags(s);
if (!comedi_is_runflags_running(runflags)) {
- if (count == 0) {
- struct comedi_subdevice *new_s;
-
- if (comedi_is_runflags_in_error(runflags))
- retval = -EPIPE;
- else
- retval = 0;
- /*
- * To avoid deadlock, cannot acquire dev->mutex
- * while dev->attach_lock is held. Need to
- * remove task from the async wait queue before
- * releasing dev->attach_lock, as it might not
- * be valid afterwards.
- */
- remove_wait_queue(&async->wait_head, &wait);
- on_wait_queue = false;
- up_read(&dev->attach_lock);
- attach_locked = false;
- mutex_lock(&dev->mutex);
- /*
- * Become non-busy unless things have changed
- * behind our back. Checking dev->detach_count
- * is unchanged ought to be sufficient (unless
- * there have been 2**32 detaches in the
- * meantime!), but check the subdevice pointer
- * as well just in case.
- */
- new_s = comedi_file_write_subdevice(file);
- if (dev->attached &&
- old_detach_count == dev->detach_count &&
- s == new_s && new_s->async == async)
- do_become_nonbusy(dev, s);
- mutex_unlock(&dev->mutex);
- }
+ if (comedi_is_runflags_in_error(runflags))
+ retval = -EPIPE;
+ become_nonbusy = true;
break;
}
@@ -2433,12 +2401,33 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
nbytes -= n;
buf += n;
- break; /* makes device work like a pipe */
}
-out:
- if (on_wait_queue)
- remove_wait_queue(&async->wait_head, &wait);
+ remove_wait_queue(&async->wait_head, &wait);
set_current_state(TASK_RUNNING);
+ if (become_nonbusy && count == 0) {
+ struct comedi_subdevice *new_s;
+
+ /*
+ * To avoid deadlock, cannot acquire dev->mutex
+ * while dev->attach_lock is held.
+ */
+ up_read(&dev->attach_lock);
+ attach_locked = false;
+ mutex_lock(&dev->mutex);
+ /*
+ * Check device hasn't become detached behind our back.
+ * Checking dev->detach_count is unchanged ought to be
+ * sufficient (unless there have been 2**32 detaches in the
+ * meantime!), but check the subdevice pointer as well just in
+ * case.
+ */
+ new_s = comedi_file_write_subdevice(file);
+ if (dev->attached && old_detach_count == dev->detach_count &&
+ s == new_s && new_s->async == async)
+ do_become_nonbusy(dev, s);
+ mutex_unlock(&dev->mutex);
+ }
+out:
if (attach_locked)
up_read(&dev->attach_lock);
--
2.6.2
`comedi_write()` is the handler for the "write" file operation for
COMEDI devices. It mostly runs without using the main mutex of the
COMEDI device, but uses the `attach_lock` rw_semaphore to protect
against the COMEDI device becoming "detached". A file object can write
data for a COMEDI asynchonous command if it initiated the command. The
COMEDI subdevice is marked as busy when the command is started. At some
point, the "write" handler detects that the command has terminated and
so marks the subdevice as non-busy.
In order to mark the subdevice as non-busy, the "write" handler needs to
release the `attach_lock` rw_semaphore and `acquire the main `mutex`.
There is a vulnerable point between the two, so it checks that the
device is still attached after acquiring the mutex. However, it does
not currently check that the conditions for becoming non-busy still
hold. Add some more checks that the subdevice is still busy with a
command initiated by the same file object, and that the command is in
the correct direction (in case the subdevice supports both "read" and
"write").
Signed-off-by: Ian Abbott <[email protected]>
---
drivers/staging/comedi/comedi_fops.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index c9da6f3..94c2348 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2420,10 +2420,15 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
* sufficient (unless there have been 2**32 detaches in the
* meantime!), but check the subdevice pointer as well just in
* case.
+ *
+ * Also check the subdevice is still in a suitable state to
+ * become non-busy in case it changed behind our back.
*/
new_s = comedi_file_write_subdevice(file);
if (dev->attached && old_detach_count == dev->detach_count &&
- s == new_s && new_s->async == async)
+ s == new_s && new_s->async == async && s->busy == file &&
+ (async->cmd.flags & CMDF_WRITE) &&
+ !comedi_is_subdevice_running(s))
do_become_nonbusy(dev, s);
mutex_unlock(&dev->mutex);
}
--
2.6.2
In `comedi_write()`, the `n` and `m` variables are of type `int`.
Change them to `unsigned int` as they are used to measure a positive
number of bytes. The `count` variable is also of type `int` and holds
the returned number of bytes written. Change it to type `ssize_t` to
match the function's return type.
Signed-off-by: Ian Abbott <[email protected]>
---
drivers/staging/comedi/comedi_fops.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 94c2348..188a12a 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2303,7 +2303,9 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
{
struct comedi_subdevice *s;
struct comedi_async *async;
- int n, m, count = 0, retval = 0;
+ unsigned int n, m;
+ ssize_t count = 0;
+ int retval = 0;
DECLARE_WAITQUEUE(wait, current);
struct comedi_file *cfp = file->private_data;
struct comedi_device *dev = cfp->dev;
--
2.6.2
At one point in `comedi_write()`, the variable `n` gets assigned to the
minimum of the parameter `nbytes` and the amount of writeable buffer
space. The way that is done currently is unsafe in the unlikely case
that `nbytes` exceeds `UINT_MAX`, so fix it.
Signed-off-by: Ian Abbott <[email protected]>
---
drivers/staging/comedi/comedi_fops.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 188a12a..8c784c4 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2357,16 +2357,13 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
break;
}
- n = nbytes;
-
- m = n;
+ /* Allocate all free buffer space. */
+ comedi_buf_write_alloc(s, async->prealloc_bufsz);
+ m = comedi_buf_write_n_allocated(s);
+ /* Avoid buffer wraparound. */
if (async->buf_write_ptr + m > async->prealloc_bufsz)
m = async->prealloc_bufsz - async->buf_write_ptr;
- comedi_buf_write_alloc(s, async->prealloc_bufsz);
- if (m > comedi_buf_write_n_allocated(s))
- m = comedi_buf_write_n_allocated(s);
- if (m < n)
- n = m;
+ n = min_t(size_t, m, nbytes);
if (n == 0) {
if (file->f_flags & O_NONBLOCK) {
--
2.6.2
`comedi_write()` copies data from the user buffer to the acquisition
data buffer, which is cyclic, using a single call to `copy_from_user()`.
It currently avoids having to deal with wraparound of the cyclic buffer
by limiting the amount it copies (and the amount returned to the user).
Change it to deal with the wraparound using two calls to
`copy_from_user()` if necessary.
Signed-off-by: Ian Abbott <[email protected]>
---
drivers/staging/comedi/comedi_fops.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 8c784c4..4dd4289 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2346,6 +2346,7 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
add_wait_queue(&async->wait_head, &wait);
while (count == 0 && !retval) {
unsigned runflags;
+ unsigned int wp, n1, n2;
set_current_state(TASK_INTERRUPTIBLE);
@@ -2360,9 +2361,6 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
/* Allocate all free buffer space. */
comedi_buf_write_alloc(s, async->prealloc_bufsz);
m = comedi_buf_write_n_allocated(s);
- /* Avoid buffer wraparound. */
- if (async->buf_write_ptr + m > async->prealloc_bufsz)
- m = async->prealloc_bufsz - async->buf_write_ptr;
n = min_t(size_t, m, nbytes);
if (n == 0) {
@@ -2388,8 +2386,14 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
continue;
}
- m = copy_from_user(async->prealloc_buf + async->buf_write_ptr,
- buf, n);
+ wp = async->buf_write_ptr;
+ n1 = min(n, async->prealloc_bufsz - wp);
+ n2 = n - n1;
+ m = copy_from_user(async->prealloc_buf + wp, buf, n1);
+ if (m)
+ m += n2;
+ else if (n2)
+ m = copy_from_user(async->prealloc_buf, buf + n1, n2);
if (m) {
n -= m;
retval = -EFAULT;
--
2.6.2
The "write" file operation handler, `comedi_write()` returns an error
for pretty much any condition that prevents a "write" going ahead. One
of the conditions that prevents a "write" going ahead is that no
asynchronous command has been set up, but that currently results in a
return value of 0 (unless COMEDI instructions are being processed or an
asynchronous command has been set up by a different file object).
Change it to return `-EINVAL` in this case.
Signed-off-by: Ian Abbott <[email protected]>
---
drivers/staging/comedi/comedi_fops.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 4dd4289..2a2b5a0 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2331,9 +2331,12 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
}
async = s->async;
-
- if (!s->busy || !nbytes)
+ if (!nbytes)
+ goto out;
+ if (!s->busy) {
+ retval = -EINVAL;
goto out;
+ }
if (s->busy != file) {
retval = -EACCES;
goto out;
@@ -2373,8 +2376,10 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
retval = -ERESTARTSYS;
break;
}
- if (!s->busy)
+ if (!s->busy) {
+ retval = -EINVAL;
break;
+ }
if (s->busy != file) {
retval = -EACCES;
break;
--
2.6.2
In order to perform a "write" file operation, an asynchronous COMEDI
command in the "write" direction needs to have been set up by the
current file object on the COMEDI "write" subdevice associated with the
file object. If there is a "write" subdevice, but a command has not
been set up by the file object (or is has been set-up in the wrong
direction), `comedi_write()` currently returns one of two error values
`-EINVAL` or `-EACCES`. `-EACCES` is returned if the command was set up
by a different subdevice, or somewhat randomly, if a COMEDI
"instruction" is currently being processed. `-EINVAL` is returned in
other cases. Simplify it by returning `-EINVAL` for all these cases.
Signed-off-by: Ian Abbott <[email protected]>
---
drivers/staging/comedi/comedi_fops.c | 21 +++------------------
1 file changed, 3 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 2a2b5a0..5a9c9d9 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2333,15 +2333,7 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
async = s->async;
if (!nbytes)
goto out;
- if (!s->busy) {
- retval = -EINVAL;
- goto out;
- }
- if (s->busy != file) {
- retval = -EACCES;
- goto out;
- }
- if (!(async->cmd.flags & CMDF_WRITE)) {
+ if (s->busy != file || !(async->cmd.flags & CMDF_WRITE)) {
retval = -EINVAL;
goto out;
}
@@ -2376,15 +2368,8 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
retval = -ERESTARTSYS;
break;
}
- if (!s->busy) {
- retval = -EINVAL;
- break;
- }
- if (s->busy != file) {
- retval = -EACCES;
- break;
- }
- if (!(async->cmd.flags & CMDF_WRITE)) {
+ if (s->busy != file ||
+ !(async->cmd.flags & CMDF_WRITE)) {
retval = -EINVAL;
break;
}
--
2.6.2
If the "write" file operation handler, `comedi_write()` is passed 0 for
the amount to write, some error conditions are currently skipped and the
function just returns 0. Change it to check those error conditions and
return an error value if appropriate. The trickiest case is the check
for when the previously set up asynchronous command has terminated with
an error. In that case, `-EPIPE` is returned (as it is for a write of
non-zero length) and the subdevice gets marked as non-busy.
A zero-length write that returns 0 has no other effects, in particular,
it does not cause the subdevice to be marked as non-busy.
Signed-off-by: Ian Abbott <[email protected]>
---
drivers/staging/comedi/comedi_fops.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 5a9c9d9..d57fade 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2331,8 +2331,6 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
}
async = s->async;
- if (!nbytes)
- goto out;
if (s->busy != file || !(async->cmd.flags & CMDF_WRITE)) {
retval = -EINVAL;
goto out;
@@ -2349,9 +2347,12 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
if (!comedi_is_runflags_running(runflags)) {
if (comedi_is_runflags_in_error(runflags))
retval = -EPIPE;
- become_nonbusy = true;
+ if (retval || nbytes)
+ become_nonbusy = true;
break;
}
+ if (nbytes == 0)
+ break;
/* Allocate all free buffer space. */
comedi_buf_write_alloc(s, async->prealloc_bufsz);
--
2.6.2
On Wednesday, November 18, 2015 10:55 AM, Ian Abbott wrote:
> Rearrange the code in `comedi_write()` to reduce the amount of
> indentation. The code never reiterates the `while` loop once `count`
> has become non-zero, so we can check that in the `while` condition to
> save an indentation level. (Note that `nbytes` has been checked to be
> non-zero before entering the loop, so we can remove that check.) Move
> the code that makes the subdevice "become non-busy" outside the `while`
> loop, using a new flag variable `become_nonbusy` to decide whether it
> needs to be done. This simplifies the wait queue handling so there is a
> single place where the task is removed from the wait queue, and we can
> remove the `on_wait_queue` flag variable.
>
> Signed-off-by: Ian Abbott <[email protected]>
> ---
> drivers/staging/comedi/comedi_fops.c | 71 +++++++++++++++---------------------
> 1 file changed, 30 insertions(+), 41 deletions(-)
Ian,
Minor nit-pick...
[snip]
> -out:
> - if (on_wait_queue)
> - remove_wait_queue(&async->wait_head, &wait);
> + remove_wait_queue(&async->wait_head, &wait);
> set_current_state(TASK_RUNNING);
> + if (become_nonbusy && count == 0) {
It looks like 'count' will always be 0 here.
Regards
Hartley
On Wednesday, November 18, 2015 10:55 AM, Ian Abbott wrote:
> Tidy up the "write" file operation handler, `comedi_write()` a bit and
> improve the error handling.
>
> 1) staging: comedi: rearrange comedi_write() code
> 2) staging: comedi: do extra checks for becoming non-busy for "write"
> 3) staging: comedi: make some variables unsigned in comedi_write()
> 4) staging: comedi: avoid bad truncation of a size_t in comedi_write()
> 5) staging: comedi: allow buffer wraparound in comedi_write()
> 6) staging: comedi: return error on "write" if no command set up
> 7) staging: comedi: simplify returned errors for comedi_write()
> 8) staging: comedi: check for more errors for zero-length write
>
> drivers/staging/comedi/comedi_fops.c | 124 ++++++++++++++++-------------------
> 1 file changed, 56 insertions(+), 68 deletions(-)
Ian,
Other than the minor nit-pick in patch 1 about the 'count == 0'
when becoming non-busy (the same situation is in comedi_read),
this looks good to me. It also makes the 'write' look more like the
'read'.
Thanks,
Reviewed-by: H Hartley Sweeten <[email protected]>