proc_file_read() question

proc_file_read() question

[Martin Wilck]

Hi,

the "hack" below in proc_file_read() fs/proc/generic.c (2.4.5)
irritates me:

If I do use "start" for a pointer into a memory area
allocated in read_proc, will it be always guaranteed
that (start > page)?

If no, this will IMO lead to spuriously wrong output.
If yes, I'd like to understand why.

Regards & thanks,
Martin

		/* 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
		 */
 		n -= copy_to_user(buf, start < page ? page : start, n);
		if (n == 0) {
			if (retval == 0)
				retval = -EFAULT;
			break;
		}

		*ppos += start < page ? (long)start : n; /* Move down the file */

-- 
Martin Wilck     <Martin.Wilck@fujitsu-siemens.com>
FSC EP PS DS1, Paderborn      Tel. +49 5251 8 15113

Re: proc_file_read() question

[Mike Galbraith]

On Mon, 25 Jun 2001, Martin Wilck wrote:

> Hi,
>
> the "hack" below in proc_file_read() fs/proc/generic.c (2.4.5)
> irritates me:
>
> If I do use "start" for a pointer into a memory area
> allocated in read_proc, will it be always guaranteed
> that (start > page)?
>
> If no, this will IMO lead to spuriously wrong output.
> If yes, I'd like to understand why.

Shhh ;-)  Last time that hack was mentioned, someone wanted to _remove_
it.  It's a very nice little hack to have around, and IKD uses it.

	-Mike

diff -urN linux-2.4.4.virgin/fs/proc/generic.c linux-2.4.4.ikd.mike/fs/proc/generic.c
--- linux-2.4.4.virgin/fs/proc/generic.c	Mon Dec 11 22:45:42 2000
+++ linux-2.4.4.ikd/fs/proc/generic.c	Sun Dec 17 07:58:56 2000
@@ -104,6 +104,11 @@
  		 * return the bytes, and set `start' to the desired offset
  		 * as an unsigned int. - Paul.Russell@rustcorp.com.au
 		 */
+		/* Ensure that the data will fit when using the ppos hack,
+		 * otherwise userland receives truncated data.
+		 */
+		if (n > count-1 && start && start < page)
+			break;
  		n -= copy_to_user(buf, start < page ? page : start, n);
 		if (n == 0) {
 			if (retval == 0)

[PATCH] proc_file_read() (Was: Re: proc_file_read() question)

[Martin Wilck]

Hi,

> Shhh ;-)  Last time that hack was mentioned, someone wanted to _remove_
> it.  It's a very nice little hack to have around, and IKD uses it.

I am not saying it should be removed. But IMO it is a legitimate (if
not the originally intended) use of "start" to serve as a pointer to
a memory area allocated in the proc_read () function. This use is broken
with this hack in its current form, because reading from such a file
will fail depending on the (random) order of the page and start pointers.

If I understand the "hack" right, legitimate offsets generated for it
are always between 0 and PAGE_SIZE. Therefore the patch below would
not break it, while overcoming the abovementioned problem, because
legitimate page pointers will never be < PAGE_SIZE.

Please correct me if I'm wrong.

Cheers,
Martin

-- 
Martin Wilck     <Martin.Wilck@fujitsu-siemens.com>
FSC EP PS DS1, Paderborn      Tel. +49 5251 8 15113


--- linux-2.4.5/fs/proc/generic.c	Mon Jun 25 13:46:26 2001
+++ 2.4.5mw/fs/proc/generic.c	Tue Jun 26 20:42:22 2001
@@ -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 < PAGE_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 < PAGE_SIZE ? (unsigned long) start : n; /* Move down the file */
 		nbytes -= n;
 		buf += n;
 		retval += n;

Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)

[Jonathan Lundell]

At 7:14 PM +0200 2001-06-26, Martin Wilck wrote:
>Hi,
>
>>  Shhh ;-)  Last time that hack was mentioned, someone wanted to _remove_
>>  it.  It's a very nice little hack to have around, and IKD uses it.
>
>I am not saying it should be removed. But IMO it is a legitimate (if
>not the originally intended) use of "start" to serve as a pointer to
>a memory area allocated in the proc_read () function. This use is broken
>with this hack in its current form, because reading from such a file
>will fail depending on the (random) order of the page and start pointers.
>
>If I understand the "hack" right, legitimate offsets generated for it
>are always between 0 and PAGE_SIZE. Therefore the patch below would
>not break it, while overcoming the abovementioned problem, because
>legitimate page pointers will never be < PAGE_SIZE.
>
>Please correct me if I'm wrong.

I use the hack myself, to implement a record-oriented file where the 
file position is a record number. I could probably live with 
PAGE_SIZE, but the current hack works fine with start bigger than 
that, and it's possible that someone counts on it.

But if you're allocating your own buffer, you'd probably be better 
off writing your own file ops, and not using the default 
proc_file_read() at all. At the very least you'd save a redundant 
__get_free_page/free_page pair.

>Cheers,
>Martin
>
>--
>Martin Wilck     <Martin.Wilck@fujitsu-siemens.com>
>FSC EP PS DS1, Paderborn      Tel. +49 5251 8 15113
>
>
>--- linux-2.4.5/fs/proc/generic.c	Mon Jun 25 13:46:26 2001
>+++ 2.4.5mw/fs/proc/generic.c	Tue Jun 26 20:42:22 2001
>@@ -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 < 
>PAGE_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 < PAGE_SIZE ? 
>(unsigned long) start : n; /* Move down the file */
>  		nbytes -= n;
>  		buf += n;
>  		retval += n;

-- 
/Jonathan Lundell.

Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)

[Mike Galbraith]

On Tue, 26 Jun 2001, Martin Wilck wrote:

> Hi,
>
> > Shhh ;-)  Last time that hack was mentioned, someone wanted to _remove_
> > it.  It's a very nice little hack to have around, and IKD uses it.
>
> I am not saying it should be removed. But IMO it is a legitimate (if
> not the originally intended) use of "start" to serve as a pointer to
> a memory area allocated in the proc_read () function. This use is broken
> with this hack in its current form, because reading from such a file
> will fail depending on the (random) order of the page and start pointers.
>
> If I understand the "hack" right, legitimate offsets generated for it
> are always between 0 and PAGE_SIZE. Therefore the patch below would
> not break it, while overcoming the abovementioned problem, because
> legitimate page pointers will never be < PAGE_SIZE.

It's dead simple.  My variable length data often didn't quite fit into
the transport vehicle.. so I whacked the excess to avoid the fault.

	-Mike

Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)

[Martin Wilck]

On Tue, 26 Jun 2001, Jonathan Lundell wrote:

> I use the hack myself, to implement a record-oriented file where the
> file position is a record number. I could probably live with
> PAGE_SIZE, but the current hack works fine with start bigger than
> that, and it's possible that someone counts on it.

Ok, let's use PAGE_OFFSET instead of PAGE_SIZE, then (see new patch
below).

Unless I'm mislead, legitimate values of "start" as a pointer are always
larger than that, and I can hardly imagin e a case where the "unsigned
int" value of start must be greater than PAGE_OFFSET.

I insist that relying on the comparison of two pointers is the wrong
thing. If (as you suggest) the major use of "start" has migrated from the
original intention to that of the "hack", this should be reflected
in the interface by making the "start" parameter to read_proc ()
an unsigned long. Everything else is misleading and error-prone.
For now, "start" is a char* and should be treated as such.

> But if you're allocating your own buffer, you'd probably be better
> off writing your own file ops, and not using the default
> proc_file_read() at all. At the very least you'd save a redundant
> __get_free_page/free_page pair.

That's right, but nevertheless (repeat) comparing "start" and "page" is
wrong.

Regards,
Martin

-- 
Martin Wilck     <Martin.Wilck@fujitsu-siemens.com>
FSC EP PS DS1, Paderborn      Tel. +49 5251 8 15113

--- linux-2.4.5/fs/proc/generic.c	Mon Jun 25 13:46:26 2001
+++ 2.4.5mw/fs/proc/generic.c	Wed Jun 27 11:22:14 2001
@@ -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 < PAGE_OFFSET ? 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 < PAGE_OFFSET ? (unsigned long) start : n; /* Move down the file */
 		nbytes -= n;
 		buf += n;
 		retval += n;

Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)

[Jonathan Lundell]

At 10:07 AM +0200 2001-06-27, Martin Wilck wrote:
>On Tue, 26 Jun 2001, Jonathan Lundell wrote:
>
>>  I use the hack myself, to implement a record-oriented file where the
>>  file position is a record number. I could probably live with
>>  PAGE_SIZE, but the current hack works fine with start bigger than
>>  that, and it's possible that someone counts on it.
>
>Ok, let's use PAGE_OFFSET instead of PAGE_SIZE, then (see new patch
>below).
>Unless I'm mislead, legitimate values of "start" as a pointer are always
>larger than that, and I can hardly imagin e a case where the "unsigned
>int" value of start must be greater than PAGE_OFFSET.


PAGE_OFFSET definitely works for me, but a quick scan of the headers 
suggests that non-sun3 m68k builds define PAGE_OFFSET as 0, as does 
s390.

Maybe you want max(PAGE_SIZE, PAGE_OFFSET).

>I insist that relying on the comparison of two pointers is the wrong
>thing. If (as you suggest) the major use of "start" has migrated from the
>original intention to that of the "hack", this should be reflected
>in the interface by making the "start" parameter to read_proc ()
>an unsigned long. Everything else is misleading and error-prone.
>For now, "start" is a char* and should be treated as such.

That's the hack, though. Rusty should chime in, but the implicit 
restriction on start in the original hack (by the time we get to the 
test we're talking about) is that it's either a pointer of the form 
page+offset, where offset < PAGE_SIZE, or it's a (relatively) small 
file offset.

That's a reasonable assumption given that the procedure is 
dynamically allocating page. After all, why would you allocate the 
buffer and then not use it?

Sure, the overloading is self-admittedly hacky, but (again I assume) 
the motivation was to avoid breaking the clients, many of which are 
not in the kernel.org tree. Your proposed change overloads a third 
interpretation on start, namely an arbitrary pointer, outside the 
page allocation.

>  > But if you're allocating your own buffer, you'd probably be better
>>  off writing your own file ops, and not using the default
>>  proc_file_read() at all. At the very least you'd save a redundant
>>  __get_free_page/free_page pair.
>
>That's right, but nevertheless (repeat) comparing "start" and "page" is
>wrong.

Not given the implied restriction that, if start is a pointer at all, 
it's a pointer within page's allocation. And after all, PAGE_OFFSET 
is effectively a pointer.
-- 
/Jonathan Lundell.

Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)

[Martin Wilck]

> PAGE_OFFSET definitely works for me, but a quick scan of the headers
> suggests that non-sun3 m68k builds define PAGE_OFFSET as 0, as does
> s390.

Hum - is there no simple way to determine whether a pointer is
a valid pointer to something returned by __get_free_pages ()? You are
right, S390 in particular seems to allow arbitrary addresses starting from
0.

> Sure, the overloading is self-admittedly hacky, but (again I assume)
> the motivation was to avoid breaking the clients, many of which are
> not in the kernel.org tree. Your proposed change overloads a third
> interpretation on start, namely an arbitrary pointer, outside the
> page allocation.

For some reason I was convinced that this was the originally intended
use of start. The only quotes I find right now are Ori Pomerantz'
Module Programming Guide (http://www.linuxdoc.org/LDP/lkmpg/node16.html)
and Rubini's "Writing Device Drivers", chapter 4. Also, the comment in the
code
		if (!start) {
			/*
			 * For proc files that are less than 4k
			 */
		...
		}
supports this notion somehow (start only set if data size > page size).

After all, unless you want to mangle the file position as intended by
the hack, there is no point in touching start at all in proc_read (),
ppos will be updated automatically.

Perhaps I have misunderstood something here.
Who wrote the original code, after all?

Martin

-- 
Martin Wilck     <Martin.Wilck@fujitsu-siemens.com>
FSC EP PS DS1, Paderborn      Tel. +49 5251 8 15113

Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)

[Roman Zippel]

Hi,

Martin Wilck wrote:

> Hum - is there no simple way to determine whether a pointer is
> a valid pointer to something returned by __get_free_pages ()? You are
> right, S390 in particular seems to allow arbitrary addresses starting from
> 0.

M68k does so too, although the first page is never used and usually
unmapped to catch NULL pointers.

bye, Roman