The /dev/mem and /dev/kmem write handlers weren't fully POSIX compliant in
that they wouldn't always force the file pointer to be updated when returning
success status.
The /dev/port write handler was inconsistent with the /dev/mem and /dev/kmem
handlers in that when encountering a -EFAULT condition after already having
written a number of items it would return -EFAULT rather than the number of
bytes written.
Signed-Off-By: Jan Beulich <[email protected]>
diff -Npru /home/jbeulich/tmp/linux-2.6.16-rc5/drivers/char/mem.c 2.6.16-rc5-posix-dev-mem/drivers/char/mem.c
--- /home/jbeulich/tmp/linux-2.6.16-rc5/drivers/char/mem.c 2006-03-03 09:55:50.000000000 +0100
+++ 2.6.16-rc5-posix-dev-mem/drivers/char/mem.c 2006-03-02 11:45:28.000000000 +0100
@@ -216,11 +216,9 @@ static ssize_t write_mem(struct file * f
copied = copy_from_user(ptr, buf, sz);
if (copied) {
- ssize_t ret;
-
- ret = written + (sz - copied);
- if (ret)
- return ret;
+ written += sz - copied;
+ if (written)
+ break;
return -EFAULT;
}
buf += sz;
@@ -456,11 +454,9 @@ do_write_kmem(void *p, unsigned long rea
copied = copy_from_user(ptr, buf, sz);
if (copied) {
- ssize_t ret;
-
- ret = written + (sz - copied);
- if (ret)
- return ret;
+ written += sz - copied;
+ if (written)
+ break;
return -EFAULT;
}
buf += sz;
@@ -514,11 +510,10 @@ static ssize_t write_kmem(struct file *
if (len) {
written = copy_from_user(kbuf, buf, len);
if (written) {
- ssize_t ret;
-
+ if (wrote + virtr)
+ break;
free_page((unsigned long)kbuf);
- ret = wrote + virtr + (len - written);
- return ret ? ret : -EFAULT;
+ return -EFAULT;
}
}
len = vwrite(kbuf, (char *)p, len);
@@ -563,8 +558,11 @@ static ssize_t write_port(struct file *
return -EFAULT;
while (count-- > 0 && i < 65536) {
char c;
- if (__get_user(c, tmp))
+ if (__get_user(c, tmp)) {
+ if (tmp > buf)
+ break;
return -EFAULT;
+ }
outb(c,i);
i++;
tmp++;
On Fri, 2006-03-03 at 10:31 +0100, Jan Beulich wrote:
> The /dev/mem and /dev/kmem write handlers weren't fully POSIX compliant in
> that they wouldn't always force the file pointer to be updated when returning
> success status.
should we instead just remove the /dev/mem and/or /dev/kmem write
handlers entirely?
On Fri, Mar 03, 2006 at 10:38:17AM +0100, Arjan van de Ven wrote:
> On Fri, 2006-03-03 at 10:31 +0100, Jan Beulich wrote:
> > The /dev/mem and /dev/kmem write handlers weren't fully POSIX compliant in
> > that they wouldn't always force the file pointer to be updated when returning
> > success status.
>
> should we instead just remove the /dev/mem and/or /dev/kmem write
> handlers entirely?
As long as I can still mmap offsets and write directly, I don't think
SGI will have any issue. For diagnostics, SGI uses mmap of architecture
specific registers and injects errors to ensure signals get completely
through and processed correctly. Of course, the last time I talked
with anybody about that stuff was more than a year ago and things may
have changed. I can ask around to see if anybody is using write, but I
would be very surprised.
Thanks,
Robin Holt
Arjan van de Ven <[email protected]> writes:
> On Fri, 2006-03-03 at 10:31 +0100, Jan Beulich wrote:
> > The /dev/mem and /dev/kmem write handlers weren't fully POSIX compliant in
> > that they wouldn't always force the file pointer to be updated when returning
> > success status.
>
> should we instead just remove the /dev/mem and/or /dev/kmem write
> handlers entirely?
If you mmap it doesn't make any sense to not have read/write.
And they are sometimes quite useful for debugging.
-Andi
"Jan Beulich" <[email protected]> wrote:
>
> The /dev/mem and /dev/kmem write handlers weren't fully POSIX compliant in
> that they wouldn't always force the file pointer to be updated when returning
> success status.
> The /dev/port write handler was inconsistent with the /dev/mem and /dev/kmem
> handlers in that when encountering a -EFAULT condition after already having
> written a number of items it would return -EFAULT rather than the number of
> bytes written.
>
> ...
>
> @@ -514,11 +510,10 @@ static ssize_t write_kmem(struct file *
> if (len) {
> written = copy_from_user(kbuf, buf, len);
> if (written) {
> - ssize_t ret;
> -
> + if (wrote + virtr)
> + break;
> free_page((unsigned long)kbuf);
> - ret = wrote + virtr + (len - written);
> - return ret ? ret : -EFAULT;
> + return -EFAULT;
> }
> }
> len = vwrite(kbuf, (char *)p, len);
I think write_kmem() still isn't quie right - it needs to update `p' (and
hence *ppos) to account for a partial copy_from_user(). (Please double-check)
--- devel/drivers/char/mem.c~adjust-dev-kmemmemport-write-handlers-fix 2006-03-04 22:10:55.000000000 -0800
+++ devel-akpm/drivers/char/mem.c 2006-03-04 22:15:19.000000000 -0800
@@ -504,8 +504,11 @@ static ssize_t write_kmem(struct file *
if (len) {
written = copy_from_user(kbuf, buf, len);
if (written) {
- if (wrote + virtr)
+ if (wrote + virtr) {
+ p += len - written;
+ virtr += len - written;
break;
+ }
free_page((unsigned long)kbuf);
return -EFAULT;
}
_
> If you mmap it doesn't make any sense to not have read/write.
> And they are sometimes quite useful for debugging.
Agreed, I use them quite a lot too.
Anton
>> @@ -514,11 +510,10 @@ static ssize_t write_kmem(struct file *
>> if (len) {
>> written = copy_from_user(kbuf, buf, len);
>> if (written) {
>> - ssize_t ret;
>> -
>> + if (wrote + virtr)
>> + break;
>> free_page((unsigned long)kbuf);
>> - ret = wrote + virtr + (len - written);
>> - return ret ? ret : -EFAULT;
>> + return -EFAULT;
>> }
>> }
>> len = vwrite(kbuf, (char *)p, len);
>
>
>I think write_kmem() still isn't quie right - it needs to update `p' (and
>hence *ppos) to account for a partial copy_from_user(). (Please double-check)
No, I disagree. The actual write happens with the call to vwrite(), independent of the reading of the user buffer. If
any adjustment might be needed, then it would be to account for the potential of vwrite() (and similarly vread()) to
return 0, resulting in no progress being possible to be made anymore.
Jan