This (trivial) patch prevents the topdown allocator from allocating
mmap areas all the way down to address zero. It's not the prettiest
patch, so suggestions for improvement are welcome ;)
Signed-off-by: Rik van Riel <[email protected]>
--- linux-2.6.11/mm/mmap.c.nullptr 2005-05-17 23:07:26.000000000 -0400
+++ linux-2.6.11/mm/mmap.c 2005-05-17 23:18:53.000000000 -0400
@@ -1251,7 +1251,7 @@
addr = mm->free_area_cache;
/* make sure it can fit in the remaining address space */
- if (addr >= len) {
+ if (addr >= (len + mm->brk)) {
vma = find_vma(mm, addr-len);
if (!vma || addr <= vma->vm_start)
/* remember the address as a hint for next time */
@@ -1267,13 +1267,13 @@
* return with success:
*/
vma = find_vma(mm, addr);
- if (!vma || addr+len <= vma->vm_start)
+ if ((!vma || addr+len <= vma->vm_start) && addr > mm->brk)
/* remember the address as a hint for next time */
return (mm->free_area_cache = addr);
/* try just below the current vma->vm_start */
addr = vma->vm_start-len;
- } while (len <= vma->vm_start);
+ } while (len <= vma->vm_start && addr > mm->brk);
/*
* A failed mmap() very likely causes application failure,
On Wed, 2005-05-18 at 15:57 -0400, Rik van Riel wrote:
> This (trivial) patch prevents the topdown allocator from allocating
> mmap areas all the way down to address zero. It's not the prettiest
> patch, so suggestions for improvement are welcome ;)
it looks like you stop at brk() time.. isn't it better to just stop just
above NULL instead?? Gives you more space and is less of an artificial
barrier..
On Wed, 18 May 2005, Arjan van de Ven wrote:
> On Wed, 2005-05-18 at 15:57 -0400, Rik van Riel wrote:
> > This (trivial) patch prevents the topdown allocator from allocating
> > mmap areas all the way down to address zero. It's not the prettiest
> > patch, so suggestions for improvement are welcome ;)
>
> it looks like you stop at brk() time.. isn't it better to just stop just
> above NULL instead?? Gives you more space and is less of an artificial
> barrier..
Firstly, there isn't much below brk() at all. Secondly, do we
really want to fill the randomized hole between the executable
and the brk area with data ?
Thirdly, we do want to continue detecting NULL pointer dereferences
inside large structs, ie. dereferencing an element 700kB into some
large struct...
--
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan
On Wed, 18 May 2005, Rik van Riel wrote:
>
> This (trivial) patch prevents the topdown allocator from allocating
> mmap areas all the way down to address zero. It's not the prettiest
> patch, so suggestions for improvement are welcome ;)
Why not just change the "addr >= len" test into "addr > len" and be done
with it?
Ie something as simple as the appended..
Linus
---
diff --git a/mm/mmap.c b/mm/mmap.c
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1244,7 +1244,7 @@ arch_get_unmapped_area_topdown(struct fi
addr = mm->free_area_cache;
/* make sure it can fit in the remaining address space */
- if (addr >= len) {
+ if (addr > len) {
vma = find_vma(mm, addr-len);
if (!vma || addr <= vma->vm_start)
/* remember the address as a hint for next time */
@@ -1266,7 +1266,7 @@ arch_get_unmapped_area_topdown(struct fi
/* try just below the current vma->vm_start */
addr = vma->vm_start-len;
- } while (len <= vma->vm_start);
+ } while (len < vma->vm_start);
/*
* A failed mmap() very likely causes application failure,
On Wed, 18 May 2005, Rik van Riel wrote:
>
> On Wed, 18 May 2005, Arjan van de Ven wrote:
> > On Wed, 2005-05-18 at 15:57 -0400, Rik van Riel wrote:
> > > This (trivial) patch prevents the topdown allocator from allocating
> > > mmap areas all the way down to address zero. It's not the prettiest
> > > patch, so suggestions for improvement are welcome ;)
> >
> > it looks like you stop at brk() time.. isn't it better to just stop just
> > above NULL instead?? Gives you more space and is less of an artificial
> > barrier..
>
> Firstly, there isn't much below brk() at all.
Guaranteed? What about executables that have fixed code addresses?
Sounds like a dubious approach, in other words.
If you want to, you could make the "how low do you go" thing be an
rlimit-like thing, but I really doubt "brk" makes much sense as the limit.
Linus
On Wed, 18 May 2005, Linus Torvalds wrote:
> Why not just change the "addr >= len" test into "addr > len" and be done
> with it?
If you're fine with not catching dereferences of a struct
member further than PAGE_SIZE into a struct when the struct
pointer is NULL, sure ...
--
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan
On Wed, 18 May 2005, Rik van Riel wrote:
>
> On Wed, 18 May 2005, Linus Torvalds wrote:
>
> > Why not just change the "addr >= len" test into "addr > len" and be done
> > with it?
>
> If you're fine with not catching dereferences of a struct
> member further than PAGE_SIZE into a struct when the struct
> pointer is NULL, sure ...
I'm certainly ok with that, especially since it should never be a problem
in practice (ie the virtual memory map getting so full that we even get to
these low allocations should be basically something that never happens
under normal load).
However, it would be good to have even the trivial patch tested.
Especially since what it tries to fix is a total corner-case in the first
place..
Linus
* Linus Torvalds ([email protected]) wrote:
> However, it would be good to have even the trivial patch tested.
> Especially since what it tries to fix is a total corner-case in the first
> place..
I gave it a quick and simple test. Worked as expected. Last page got
mapped at 0x1000, leaving first page unmapped. Of course, either with
MAP_FIXED or w/out MAP_FIXED but proper hint (like -1) you can still
map first page. This isn't to say I was extra creative in testing.
On Wed, 2005-05-18 at 23:46 -0700, Chris Wright wrote:
> * Linus Torvalds ([email protected]) wrote:
> > However, it would be good to have even the trivial patch tested.
> > Especially since what it tries to fix is a total corner-case in the first
> > place..
>
> I gave it a quick and simple test. Worked as expected. Last page got
> mapped at 0x1000, leaving first page unmapped. Of course, either with
> MAP_FIXED or w/out MAP_FIXED but proper hint (like -1) you can still
> map first page. This isn't to say I was extra creative in testing.
sure. Making it *impossible* to mmap that page is bad. People should be
able to do that if they really want to, just doing it if they don't ask
for it is bad.
There are plenty of reasons people may want that page mmaped, one of
them being that the compiler can then do more speculative loads around
null pointer checks. Not saying it's a brilliant idea always, but making
such things impossible makes no sense.
* Arjan van de Ven ([email protected]) wrote:
> On Wed, 2005-05-18 at 23:46 -0700, Chris Wright wrote:
> > I gave it a quick and simple test. Worked as expected. Last page got
> > mapped at 0x1000, leaving first page unmapped. Of course, either with
> > MAP_FIXED or w/out MAP_FIXED but proper hint (like -1) you can still
> > map first page. This isn't to say I was extra creative in testing.
>
> sure. Making it *impossible* to mmap that page is bad. People should be
> able to do that if they really want to, just doing it if they don't ask
> for it is bad.
Heh, that was actually my intended point ;-) At any rate, you made it
clearer, thanks.
Arjan van de Ven <[email protected]> writes:
> On Wed, 2005-05-18 at 23:46 -0700, Chris Wright wrote:
>
> sure. Making it *impossible* to mmap that page is bad. People should be
> able to do that if they really want to, just doing it if they don't ask
> for it is bad.
>
> There are plenty of reasons people may want that page mmaped, one of
> them being that the compiler can then do more speculative loads around
> null pointer checks. Not saying it's a brilliant idea always, but making
> such things impossible makes no sense.
More realistically, iirc either Wine or dosemu, i forget which, actually has
to map page 0 to work properly.
--
greg
* Greg Stark ([email protected]) wrote:
> More realistically, iirc either Wine or dosemu, i forget which, actually has
> to map page 0 to work properly.
Yup, this is well-known, and the patch does not effect MAP_FIXED.
http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=49a43876b935c811cfd29d8fe998a6912a1cc5c4
thanks,
-chris