Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)
> 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 <[EMAIL PROTECTED]> FSC EP PS DS1, Paderborn Tel. +49 5251 8 15113 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)
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 <[EMAIL PROTECTED]> 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. - [EMAIL PROTECTED] */ - 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; - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)
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 [EMAIL PROTECTED] 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. - [EMAIL PROTECTED] */ - 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; - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)
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 [EMAIL PROTECTED] FSC EP PS DS1, Paderborn Tel. +49 5251 8 15113 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)
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 <[EMAIL PROTECTED]> >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. - [EMAIL PROTECTED] >*/ >- 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] proc_file_read() (Was: Re: proc_file_read() question)
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 <[EMAIL PROTECTED]> 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. - [EMAIL PROTECTED] */ - 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; - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] proc_file_read() (Was: Re: proc_file_read() question)
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 [EMAIL PROTECTED] 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. - [EMAIL PROTECTED] */ - 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; - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)
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 [EMAIL PROTECTED] 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. - [EMAIL PROTECTED] */ - 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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/