[PATCH] adjust /dev/{kmem,mem,port} write handlers

[PATCH] adjust /dev/{kmem,mem,port} write handlers

[Jan Beulich]

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 <jbeulich@novell.com>

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++;

Re: [PATCH] adjust /dev/{kmem,mem,port} write handlers

[Arjan van de Ven]

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?

Re: [PATCH] adjust /dev/{kmem,mem,port} write handlers

[Robin Holt]

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

Re: [PATCH] adjust /dev/{kmem,mem,port} write handlers

[Andi Kleen]

Arjan van de Ven <arjan@infradead.org> 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

Re: [PATCH] adjust /dev/{kmem,mem,port} write handlers

[Anton Blanchard]

 
> 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

Re: [PATCH] adjust /dev/{kmem,mem,port} write handlers

[Andrew Morton]

"Jan Beulich" <JBeulich@novell.com> 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;
 				}
_

Re: [PATCH] adjust /dev/{kmem,mem,port} write handlers

[Jan Beulich]

>> @@ -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