RE: [PATCH 4/7] staging: comedi: don't allow read() on async command set up for write
On Thursday, October 30, 2014 5:42 AM, Ian Abbott wrote: If a Comedi asynchronous command has been set up for data transfer in the write direction on the current read subdevice (for those subdevices that support both directions), don't allow the read file operation as that would mess with the data in the comedi data buffer that is read by the low-level comedi hardware driver. Signed-off-by: Ian Abbott abbo...@mev.co.uk --- drivers/staging/comedi/comedi_fops.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index fef68da..6805ec9 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -2210,6 +2210,10 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes, retval = -EACCES; goto out; } + if (async-cmd.flags CMDF_WRITE) { + retval = -EINVAL; + goto out; + } add_wait_queue(async-wait_head, wait); while (nbytes 0 !retval) { @@ -2249,6 +2253,10 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes, retval = -EACCES; break; } + if (async-cmd.flags CMDF_WRITE) { + retval = -EINVAL; + break; + } Is this second test really needed in the while() loop? For that matter, are the s-busy tests needed in the while() loop? continue; } m = copy_to_user(buf, async-prealloc_buf + Regards, Hartley ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/7] staging: comedi: don't allow read() on async command set up for write
On 30/10/14 18:05, Hartley Sweeten wrote: On Thursday, October 30, 2014 5:42 AM, Ian Abbott wrote: [snip] add_wait_queue(async-wait_head, wait); while (nbytes 0 !retval) { @@ -2249,6 +2253,10 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes, retval = -EACCES; break; } + if (async-cmd.flags CMDF_WRITE) { + retval = -EINVAL; + break; + } Is this second test really needed in the while() loop? For that matter, are the s-busy tests needed in the while() loop? To answer your second question, some other thread using the same file object might have cancelled the asynchronous command, causing the current thread to see that the command is no longer active when it wakes up. To answer your first question, that other thread might have managed to set up another asynchronous command in before we wake up, and it might have been set up as a write command (if the subdevice supports commands in both directions). This doesn't detect the case when the other thread has managed to set up another read command, but since the current read() call hasn't read any data yet, we can just pretend we didn't know about the original command and read data from the new command instead. (After all, the calling thread can't prove the read() started before the first command was cancelled, so we can just pretend it didn't.) -- -=( Ian Abbott @ MEV Ltd.E-mail: abbo...@mev.co.uk )=- -=( Web: http://www.mev.co.uk/ )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 4/7] staging: comedi: don't allow read() on async command set up for write
On Thursday, October 30, 2014 1:28 PM, Ian Abbott wrote: On 30/10/14 18:05, Hartley Sweeten wrote: On Thursday, October 30, 2014 5:42 AM, Ian Abbott wrote: [snip] add_wait_queue(async-wait_head, wait); while (nbytes 0 !retval) { @@ -2249,6 +2253,10 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes, retval = -EACCES; break; } + if (async-cmd.flags CMDF_WRITE) { + retval = -EINVAL; + break; + } Is this second test really needed in the while() loop? For that matter, are the s-busy tests needed in the while() loop? To answer your second question, some other thread using the same file object might have cancelled the asynchronous command, causing the current thread to see that the command is no longer active when it wakes up. To answer your first question, that other thread might have managed to set up another asynchronous command in before we wake up, and it might have been set up as a write command (if the subdevice supports commands in both directions). This doesn't detect the case when the other thread has managed to set up another read command, but since the current read() call hasn't read any data yet, we can just pretend we didn't know about the original command and read data from the new command instead. (After all, the calling thread can't prove the read() started before the first command was cancelled, so we can just pretend it didn't.) But when the command is first started by do_cmd_ioctl() we have this sequence: if (s-busy) return -EBUSY; ... s-busy = file; ret = s-do_cmd(dev, s); From then on the s-busy pointer can only be cleared in do_become_nonbusy() (by way of a (*cancel)). So another command cannot be started until the current command is completed. The user could do a (*do_cmdtest) while a command is running but that does not effect the read/write of the async buffer. Hartley ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/7] staging: comedi: don't allow read() on async command set up for write
On 30/10/14 20:45, Hartley Sweeten wrote: On Thursday, October 30, 2014 1:28 PM, Ian Abbott wrote: On 30/10/14 18:05, Hartley Sweeten wrote: On Thursday, October 30, 2014 5:42 AM, Ian Abbott wrote: [snip] add_wait_queue(async-wait_head, wait); while (nbytes 0 !retval) { @@ -2249,6 +2253,10 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes, retval = -EACCES; break; } + if (async-cmd.flags CMDF_WRITE) { + retval = -EINVAL; + break; + } Is this second test really needed in the while() loop? For that matter, are the s-busy tests needed in the while() loop? To answer your second question, some other thread using the same file object might have cancelled the asynchronous command, causing the current thread to see that the command is no longer active when it wakes up. To answer your first question, that other thread might have managed to set up another asynchronous command in before we wake up, and it might have been set up as a write command (if the subdevice supports commands in both directions). This doesn't detect the case when the other thread has managed to set up another read command, but since the current read() call hasn't read any data yet, we can just pretend we didn't know about the original command and read data from the new command instead. (After all, the calling thread can't prove the read() started before the first command was cancelled, so we can just pretend it didn't.) But when the command is first started by do_cmd_ioctl() we have this sequence: if (s-busy) return -EBUSY; ... s-busy = file; ret = s-do_cmd(dev, s); From then on the s-busy pointer can only be cleared in do_become_nonbusy() (by way of a (*cancel)). So another command cannot be started until the current command is completed. The other thread could do its own read() after it cancelled the command, which would clear the busy condition (once it returns 0 to indicate end-of-file), so the current thread's read() still needs to check it. -- -=( Ian Abbott @ MEV Ltd.E-mail: abbo...@mev.co.uk )=- -=( Web: http://www.mev.co.uk/ )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel