proc_file_read() hack?

proc_file_read() hack?

[Todd Inglett]

[-- Attachment #1: Type: text/plain, Size: 791 bytes --]

I'm trying to understand this little hack in 2.4.18's (and earlier) 
fs/proc/generic.c:

/* This is a hack to allow mangling of file pos independent
  * of actual bytes read.  Simply place the data at page,
  * return the bytes, and set `start' to the desired offset
  * as an unsigned int. - Paul.Russell@rustcorp.com.au
  */

I take it to mean that if I return a small integer for "start" instead 
of a ptr the hack will kick in.  However it compares pointers instead. 
I'll attach a patch that does it the way I am thinking -- but I may be 
misunderstanding the comment.

Or perhaps it is always assumed that I am copying my data to the given 
page?  If that is true, then the way it is coded will work but the patch 
trivially allows me to point "start" anywhere without copying.

-todd

[-- Attachment #2: fs-proc-generic.patch --]
[-- Type: text/plain, Size: 887 bytes --]

Index: fs/proc/generic.c
===================================================================
RCS file: /cvs/linuxppc64/linuxppc64_2_4/fs/proc/generic.c,v
retrieving revision 1.6
diff -u -r1.6 generic.c
--- fs/proc/generic.c	8 Oct 2001 19:19:59 -0000	1.6
+++ fs/proc/generic.c	22 Mar 2002 14:34:47 -0000
@@ -104,14 +104,14 @@
  		 * return the bytes, and set `start' to the desired offset
  		 * as an unsigned int. - Paul.Russell@rustcorp.com.au
 		 */
- 		n -= copy_to_user(buf, start < page ? page : start, n);
+ 		n -= copy_to_user(buf, (unsigned long)start < PROC_BLOCK_SIZE ? page : start, n);
 		if (n == 0) {
 			if (retval == 0)
 				retval = -EFAULT;
 			break;
 		}
 
-		*ppos += start < page ? (long)start : n; /* Move down the file */
+		*ppos += (unsigned long)start < PROC_BLOCK_SIZE ? (long)start : n; /* Move down the file */
 		nbytes -= n;
 		buf += n;
 		retval += n;

Re: proc_file_read() hack?

[J.D. Hood]

In http://marc.theaimsgroup.com/?l=linux-kernel&m=101304602932539&w=2
I posted a patch which includes the following comment:

+			/*
+			 * How to be a proc read function
+			 * ------------------------------
+			 * Prototype:
+			 *    int f(char *buffer, char **start, off_t offset,
+			 *          int count, int *peof, void *dat)
+			 *
+			 * Assume that the buffer is "count" bytes in size.
+			 *
+			 * If you know you have supplied all the data you
+			 * have, set *peof.
+			 *
+			 * You have three ways to return data:
+			 * 0) Leave *start = NULL.  (This is the default.)
+			 *    Put the data of the requested offset at that
+			 *    offset within the buffer.  Return the number (n)
+			 *    of bytes there are from the beginning of the
+			 *    buffer up to the last byte of data.  If the
+			 *    number of supplied bytes (= n - offset) is 
+			 *    greater than zero and you didn't signal eof
+			 *    and the reader is prepared to take more data
+			 *    you will be called again with the requested
+			 *    offset advanced by the number of bytes 
+			 *    absorbed.  This interface is useful for files
+			 *    no larger than the buffer.
+			 * 1) Set *start = an unsigned long value less than
+			 *    the buffer address but greater than zero.
+			 *    Put the data of the requested offset at the
+			 *    beginning of the buffer.  Return the number of
+			 *    bytes of data placed there.  If this number is
+			 *    greater than zero and you didn't signal eof
+			 *    and the reader is prepared to take more data
+			 *    you will be called again with the requested
+			 *    offset advanced by *start.  This interface is
+			 *    useful when you have a large file consisting
+			 *    of a series of blocks which you want to count
+			 *    and return as wholes.
+			 *    (Hack by Paul.Russell@rustcorp.com.au)
+			 * 2) Set *start = an address within the buffer.
+			 *    Put the data of the requested offset at *start.
+			 *    Return the number of bytes of data placed there.
+			 *    If this number is greater than zero and you
+			 *    didn't signal eof and the reader is prepared to
+			 *    take more data you will be called again with the
+			 *    requested offset advanced by the number of bytes
+			 *    absorbed.
+			 */

This patch was included in David Jones's patches for the 2.5
series.

> I'm trying to understand this little hack in 2.4.18's (and earlier)
> fs/proc/generic.c:
> /* This is a hack to allow mangling of file pos independent
>  * of actual bytes read.  Simply place the data at page,
>  * return the bytes, and set `start' to the desired offset
>  * as an unsigned int. - Paul.Russell@rustcorp.com.au
>  */
> I take it to mean that if I return a small integer for "start" instead 
> of a ptr the hack will kick in.  However it compares pointers instead. 

That's why it's called a "hack".

> I'll attach a patch that does it the way I am thinking -- but I may be 
> misunderstanding the comment.

You are misunderstanding it.

--
Thomas Hood


__________________________________________________
Do You Yahoo!?
Everything you'll ever need on one web page
from News and Sport to Email and Music Charts
http://uk.my.yahoo.com

Re: proc_file_read() hack?

[Todd Inglett]

J.D. Hood wrote:

> I posted a patch which includes the following comment:
> 
> +			/*
> +			 * How to be a proc read function
> +			 * ------------------------------
[...]


How about applying my trivial patch and then adding this to your nice 
comment?

3) Set *start = an address outside the buffer.
    Put the data of the requested offset at *start.
    Return the number of bytes of data placed there.
    If this number is greater than zero and you
    didn't signal eof and the reader is prepared to
    take more data you will be called again with the
    requested offset advanced by the number ob tyes
    absorbed.

The code should still work with the other cases now that the hack is 
fixed.  Of course, rather than add 3), it would be better to re-word 2) 
(e.g. "Set *start = address of the buffer which may or may not be in the 
given buffer.).

There are cases where the data is available and need not be copied.  My 
code got simpler when I got rid of the need to copy my data around.

-todd

Re: proc_file_read() hack?

[Thomas Hood]

Unfortunately, your method #3 conflicts with methods #0 through #2,
which exhaust the range of possible values that may be returned
in *start.  Any value greater than buffer is regarded as being
"within the buffer".

Introducing method #1 was a bad idea because this hack made it
impossible cleanly to implement what you suggest.

--
Thomas Hood

On Mon, 2002-03-25 at 13:18, Todd Inglett wrote:
> How about applying my trivial patch and then adding this to your nice 
> comment?
> 
> 3) Set *start = an address outside the buffer.
>     Put the data of the requested offset at *start.
>     Return the number of bytes of data placed there.
>     If this number is greater than zero and you
>     didn't signal eof and the reader is prepared to
>     take more data you will be called again with the
>     requested offset advanced by the number ob tyes
>     absorbed.
> 
> The code should still work with the other cases now that the hack is 
> fixed.  Of course, rather than add 3), it would be better to re-word 2) 
> (e.g. "Set *start = address of the buffer which may or may not be in the 
> given buffer.).
> 
> There are cases where the data is available and need not be copied.  My 
> code got simpler when I got rid of the need to copy my data around.

Re: proc_file_read() hack?

[Todd Inglett]

[-- Attachment #1: Type: text/plain, Size: 2246 bytes --]

Thomas Hood wrote:

> Unfortunately, your method #3 conflicts with methods #0 through #2,
> which exhaust the range of possible values that may be returned
> in *start.  Any value greater than buffer is regarded as being
> "within the buffer".


I guess I don't understand the conflict.

There is case #0 where start is NULL.  In this case start is computed in 
proc_file_read as page + *ppos so (unsigned long)start < PROC_BLOCK_SIZE 
is NOT true and so start is used as before.

In case #1 (unsigned long)start < PROC_BLOCK_SIZE so it will grab the 
data from page and use start as the length to copy as it did before.

And finally cases #2 and #3 are the same:  use start as an explicit data 
address.  It doesn't matter whether this points into page or if it is 
space provided by read_proc (which is why I suggested not even 
mentioning a "case #3").


Did you get a chance to read the patch?  I'll attach it again just in 
case.  Or is there a chance that start >= PROC_BLOCK_SIZE (but start < 
page) in case #1?  If that is true I am wondering how it could possibly 
be correct since start will be used as a length which is greater than 
the size of the page.

-todd


> 
> Introducing method #1 was a bad idea because this hack made it
> impossible cleanly to implement what you suggest.
> 
> --
> Thomas Hood
> 
> On Mon, 2002-03-25 at 13:18, Todd Inglett wrote:
> 
>>How about applying my trivial patch and then adding this to your nice 
>>comment?
>>
>>3) Set *start = an address outside the buffer.
>>    Put the data of the requested offset at *start.
>>    Return the number of bytes of data placed there.
>>    If this number is greater than zero and you
>>    didn't signal eof and the reader is prepared to
>>    take more data you will be called again with the
>>    requested offset advanced by the number ob tyes
>>    absorbed.
>>
>>The code should still work with the other cases now that the hack is 
>>fixed.  Of course, rather than add 3), it would be better to re-word 2) 
>>(e.g. "Set *start = address of the buffer which may or may not be in the 
>>given buffer.).
>>
>>There are cases where the data is available and need not be copied.  My 
>>code got simpler when I got rid of the need to copy my data around.
>>



[-- Attachment #2: fs-proc-generic.patch --]
[-- Type: text/plain, Size: 887 bytes --]

Index: fs/proc/generic.c
===================================================================
RCS file: /cvs/linuxppc64/linuxppc64_2_4/fs/proc/generic.c,v
retrieving revision 1.6
diff -u -r1.6 generic.c
--- fs/proc/generic.c	8 Oct 2001 19:19:59 -0000	1.6
+++ fs/proc/generic.c	22 Mar 2002 14:34:47 -0000
@@ -104,14 +104,14 @@
  		 * return the bytes, and set `start' to the desired offset
  		 * as an unsigned int. - Paul.Russell@rustcorp.com.au
 		 */
- 		n -= copy_to_user(buf, start < page ? page : start, n);
+ 		n -= copy_to_user(buf, (unsigned long)start < PROC_BLOCK_SIZE ? page : start, n);
 		if (n == 0) {
 			if (retval == 0)
 				retval = -EFAULT;
 			break;
 		}
 
-		*ppos += start < page ? (long)start : n; /* Move down the file */
+		*ppos += (unsigned long)start < PROC_BLOCK_SIZE ? (long)start : n; /* Move down the file */
 		nbytes -= n;
 		buf += n;
 		retval += n;

Re: proc_file_read() hack?

[Thomas Hood]

On Wed, 2002-03-27 at 13:26, Todd Inglett wrote:
> I guess I don't understand the conflict.

There are three cases:
0)  start == 0
1)  0 < start < buffer
2)  start >= buffer

These exhaust all the possible values that can be returned
in *start.

You propose to change the code so that there are three cases:
0)  start == 0
1') 0 < start < PROC_BLOCK_SIZE
2'/3) start >= PROC_BLOCK_SIZE

However, we can't make the change you propose because it would
break functions that use case #1 with a *start value greater
than PROC_BLOCK_SIZE.

>... is there a chance that start >= PROC_BLOCK_SIZE (but start < page)
> in case #1?

Yes.

> If that is true I am wondering how it could possibly be correct
> since start will be used as a length which is greater than the
> size of the page.

start will be used as an offset, not as a length.

If you think the hack was a bad idea, I agree with you.
But we can't change it without auditing all the proc read
functions that use case #1.

--
Thomas Hood

Re: proc_file_read() hack?

[Todd Inglett]

Thomas Hood wrote:


>>If that is true I am wondering how it could possibly be correct
>>since start will be used as a length which is greater than the
>>size of the page.
>>
> 
> start will be used as an offset, not as a length.
> 
> If you think the hack was a bad idea, I agree with you.
> But we can't change it without auditing all the proc read
> functions that use case #1.


Ahh...thanks for taking the trouble to answer all these questions :).  I 
obviously wasn't paying attention enough to see that start was *not* 
used as a length.  Sigh.  Anyway, yes I agree we cannot patch this 
without some serious review and I'm not going to volunteer today :).


-todd