3.2.50-rc1 review patch. If anyone has any objections, please let me know. ------------------ From: Ian Abbott <abbotti@xxxxxxxxx> 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@xxxxxxxxx> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> [bwh: Backported to 3.2: adjust context] Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx> --- 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); -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html