lkml.org 
[lkml]   [2013]   [Jul]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    Subject[72/84] staging: comedi: fix a race between do_cmd_ioctl() and read/write
    3.2.50-rc1 review patch.  If anyone has any objections, please let me know.

    ------------------

    From: Ian Abbott <abbotti@mev.co.uk>

    commit 4b18f08be01a7b3c7b6df497137b6e3cb28adaa3 upstream.

    `do_cmd_ioctl()` is called with the comedi device's mutex locked to
    process the `COMEDI_CMD` ioctl to set up comedi's asynchronous command
    handling on a comedi subdevice. `comedi_read()` and `comedi_write()`
    are the `read` and `write` handlers for the comedi device, but do not
    lock the mutex (for performance reasons, as some things can hold the
    mutex for quite a long time).

    There is a race condition if `comedi_read()` or `comedi_write()` is
    running at the same time and for the same file object and comedi
    subdevice as `do_cmd_ioctl()`. `do_cmd_ioctl()` sets the subdevice's
    `busy` pointer to the file object way before it sets the `SRF_RUNNING` flag
    in the subdevice's `runflags` member. `comedi_read() and
    `comedi_write()` check the subdevice's `busy` pointer is pointing to the
    current file object, then if the `SRF_RUNNING` flag is not set, will call
    `do_become_nonbusy()` to shut down the asyncronous command. Bad things
    can happen if the asynchronous command is being shutdown and set up at
    the same time.

    To prevent the race, don't set the `busy` pointer until
    after the `SRF_RUNNING` flag has been set. Also, make sure the mutex is
    held in `comedi_read()` and `comedi_write()` while calling
    `do_become_nonbusy()` in order to avoid moving the race condition to a
    point within that function.

    Change some error handling `goto cleanup` statements in `do_cmd_ioctl()`
    to simple `return -ERRFOO` statements as a result of changing when the
    `busy` pointer is set.

    Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    [bwh: Backported to 3.2: adjust context]
    Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
    ---
    drivers/staging/comedi/comedi_fops.c | 25 +++++++++++++++----------
    1 file changed, 15 insertions(+), 10 deletions(-)

    --- a/drivers/staging/comedi/comedi_fops.c
    +++ b/drivers/staging/comedi/comedi_fops.c
    @@ -1078,22 +1078,19 @@ static int do_cmd_ioctl(struct comedi_de
    DPRINTK("subdevice busy\n");
    return -EBUSY;
    }
    - s->busy = file;

    /* make sure channel/gain list isn't too long */
    if (user_cmd.chanlist_len > s->len_chanlist) {
    DPRINTK("channel/gain list too long %u > %d\n",
    user_cmd.chanlist_len, s->len_chanlist);
    - ret = -EINVAL;
    - goto cleanup;
    + return -EINVAL;
    }

    /* make sure channel/gain list isn't too short */
    if (user_cmd.chanlist_len < 1) {
    DPRINTK("channel/gain list too short %u < 1\n",
    user_cmd.chanlist_len);
    - ret = -EINVAL;
    - goto cleanup;
    + return -EINVAL;
    }

    async->cmd = user_cmd;
    @@ -1103,8 +1100,7 @@ static int do_cmd_ioctl(struct comedi_de
    kmalloc(async->cmd.chanlist_len * sizeof(int), GFP_KERNEL);
    if (!async->cmd.chanlist) {
    DPRINTK("allocation failed\n");
    - ret = -ENOMEM;
    - goto cleanup;
    + return -ENOMEM;
    }

    if (copy_from_user(async->cmd.chanlist, user_cmd.chanlist,
    @@ -1156,6 +1152,9 @@ static int do_cmd_ioctl(struct comedi_de

    comedi_set_subdevice_runflags(s, ~0, SRF_USER | SRF_RUNNING);

    + /* set s->busy _after_ setting SRF_RUNNING flag to avoid race with
    + * comedi_read() or comedi_write() */
    + s->busy = file;
    ret = s->do_cmd(dev, s);
    if (ret == 0)
    return 0;
    @@ -1658,6 +1657,7 @@ static ssize_t comedi_write(struct file

    if (!(comedi_get_subdevice_runflags(s) & SRF_RUNNING)) {
    if (count == 0) {
    + mutex_lock(&dev->mutex);
    if (comedi_get_subdevice_runflags(s) &
    SRF_ERROR) {
    retval = -EPIPE;
    @@ -1665,6 +1665,7 @@ static ssize_t comedi_write(struct file
    retval = 0;
    }
    do_become_nonbusy(dev, s);
    + mutex_unlock(&dev->mutex);
    }
    break;
    }
    @@ -1779,6 +1780,7 @@ static ssize_t comedi_read(struct file *

    if (n == 0) {
    if (!(comedi_get_subdevice_runflags(s) & SRF_RUNNING)) {
    + mutex_lock(&dev->mutex);
    do_become_nonbusy(dev, s);
    if (comedi_get_subdevice_runflags(s) &
    SRF_ERROR) {
    @@ -1786,6 +1788,7 @@ static ssize_t comedi_read(struct file *
    } else {
    retval = 0;
    }
    + mutex_unlock(&dev->mutex);
    break;
    }
    if (file->f_flags & O_NONBLOCK) {
    @@ -1823,9 +1826,11 @@ static ssize_t comedi_read(struct file *
    buf += n;
    break; /* makes device work like a pipe */
    }
    - if (!(comedi_get_subdevice_runflags(s) & (SRF_ERROR | SRF_RUNNING)) &&
    - async->buf_read_count - async->buf_write_count == 0) {
    - do_become_nonbusy(dev, s);
    + if (!(comedi_get_subdevice_runflags(s) & (SRF_ERROR | SRF_RUNNING))) {
    + mutex_lock(&dev->mutex);
    + if (async->buf_read_count - async->buf_write_count == 0)
    + do_become_nonbusy(dev, s);
    + mutex_unlock(&dev->mutex);
    }
    set_current_state(TASK_RUNNING);
    remove_wait_queue(&async->wait_head, &wait);


    \
     
     \ /
      Last update: 2013-07-31 23:21    [W:4.085 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site