Bug 118510
| Summary: | [vm] [patch] munmap(2) doesn't remove all mappings | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Tijl Coosemans <tijl> |
| Component: | kern | Assignee: | Alan Cox <alc> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | ||
| Priority: | Normal | ||
| Version: | 7.0-BETA3 | ||
| Hardware: | Any | ||
| OS: | Any | ||
proposed patch:
--- sys/vm/vm_mmap.c.orig 2007-12-10 15:57:00.000000000 +0100
+++ sys/vm/vm_mmap.c 2007-12-10 20:40:42.000000000 +0100
@@ -555,13 +555,6 @@
if (addr < vm_map_min(map) || addr + size > vm_map_max(map))
return (EINVAL);
vm_map_lock(map);
- /*
- * Make sure entire range is allocated.
- */
- if (!vm_map_check_protection(map, addr, addr + size, VM_PROT_NONE)) {
- vm_map_unlock(map);
- return (EINVAL);
- }
#ifdef HWPMC_HOOKS
/*
* Inform hwpmc if the address range being unmapped contains
Tijl Coosemans wrote:
>> Description:
> When a memory region has been partially munmap()ed, subsequent
> calls to munmap() on this region have no effect.
>> How-To-Repeat:
> The following program should segfault, but doesn't.
>
> It mmap()s 2 pages and munmap()s them again but the second page
> is still accessible.
>
> --- test.c begins here ---
> #include <sys/mman.h>
> #include <stdio.h>
> #include <unistd.h>
>
> int main( int argc, char **argv ) {
>
> unsigned int const page_size = getpagesize();
> void *map;
> char volatile *cmap;
>
> map = mmap( NULL, 2 * page_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0 );
> munmap( map, page_size );
> munmap( map, 2 * page_size );
>
> cmap = map;
> cmap[ page_size ] = 'a';
> printf( "%c\n", cmap[ page_size ] );
>
> return 0;
> }
> --- test.c ends here ---
I believe this is an incorrect use of munmap. After the first call to
munmap, you have removed the first page of the mapping, leaving a
mapping at offset map + page_size of size page_size. The second munmap
is being called with an offset that is no longer mapped, and indeed it
returns an EINVAL that your sample code is not testing for.
Kris
State Changed From-To: open->analyzed Believed not to be a bug Responsible Changed From-To: freebsd-bugs->kris Take this pending further feedback On Thursday 10 January 2008 22:48:46 Kris Kennaway wrote: > Tijl Coosemans wrote: >>> Description: >> When a memory region has been partially munmap()ed, subsequent >> calls to munmap() on this region have no effect. >> >>> How-To-Repeat: >> The following program should segfault, but doesn't. >> >> It mmap()s 2 pages and munmap()s them again but the second page >> is still accessible. >> >> --- test.c begins here --- >> #include <sys/mman.h> >> #include <stdio.h> >> #include <unistd.h> >> >> int main( int argc, char **argv ) { >> >> unsigned int const page_size = getpagesize(); >> void *map; >> char volatile *cmap; >> >> map = mmap( NULL, 2 * page_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0 ); >> munmap( map, page_size ); >> munmap( map, 2 * page_size ); >> >> cmap = map; >> cmap[ page_size ] = 'a'; >> printf( "%c\n", cmap[ page_size ] ); >> >> return 0; >> } >> --- test.c ends here --- > > I believe this is an incorrect use of munmap. After the first call > to munmap, you have removed the first page of the mapping, leaving a > mapping at offset map + page_size of size page_size. The second > munmap is being called with an offset that is no longer mapped, and > indeed it returns an EINVAL that your sample code is not testing for. Yes, but this EINVAL case isn't documented. I guess my question is: why does it return EINVAL if it could simply remove all mappings in the given address range like it says in the manpage (and how it seems to work on Linux at least)? In the NetBSD CVS log I also found a similar patch to the one I submitted: #if 0 the call to uvm_map_checkprot() in sys_munmap() -- it's not documented, and programs do not expect it. Also fixes memory leaks in dlopen()/dlclose(). http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/uvm/uvm_mmap.c#rev1.66 Responsible Changed From-To: kris->alc Assign to alc for evaluation. Submitter suggests modifying the behaviour of munmap so that it will unmap all mappings inside a range, even if it encompasses regions that are unmapped. It is documented in the man page. See the last clause of sentence
describing EINVAL.
ERRORS
The munmap() system call will fail if:
[EINVAL] The addr argument was not page aligned, the len
argu-
ment was zero or negative, or some part of the
region
being unmapped is outside the valid address
range for
a process.
This is also consistent with the description provided by the SuSv2 at
http://www.opengroup.org/pubs/online/7908799/xsh/munmap.html
That said, the subtle difference between our man page and the Open
Group's description is the use of "some part" in our man page. In other
words, it's unclear whether the Open Group intends for munmap() to
return EINVAL if some part of the address range is invalid or only if
the whole address range is invalid. I'm not aware of them issuing any
clarifications on this question.
Regards,
Alan
On Friday 11 January 2008 17:50:15 Alan Cox wrote:
> It is documented in the man page. See the last clause of sentence
> describing EINVAL.
>
> ERRORS
> The munmap() system call will fail if:
>
> [EINVAL] The addr argument was not page aligned, the len argu-
> ment was zero or negative, or some part of the region
> being unmapped is outside the valid address range for
> a process.
>
> This is also consistent with the description provided by the SuSv2 at
> http://www.opengroup.org/pubs/online/7908799/xsh/munmap.html
>
> That said, the subtle difference between our man page and the Open
> Group's description is the use of "some part" in our man page. In
> other words, it's unclear whether the Open Group intends for munmap()
> to return EINVAL if some part of the address range is invalid or only
> if the whole address range is invalid. I'm not aware of them issuing
> any clarifications on this question.
Ok, we are interpreting things differently then. I understand both
manpages as simply saying that the given address range has to be within
user space limits, and not that additionally, the full range has to be
mapped as well. In my opinion, the EINVAL is only meant for simple
bounds checking of the given arguments.
The description part of both manpages also simply states munmap()
removes any mappings for pages within the given range. I don't see
where that implies the full range has to be mapped. What would the
benefit of such a precondition be?
Tijl Coosemans wrote: >On Friday 11 January 2008 17:50:15 Alan Cox wrote: > > >>It is documented in the man page. See the last clause of sentence >>describing EINVAL. >> >>ERRORS >> The munmap() system call will fail if: >> >> [EINVAL] The addr argument was not page aligned, the len argu- >> ment was zero or negative, or some part of the region >> being unmapped is outside the valid address range for >> a process. >> >>This is also consistent with the description provided by the SuSv2 at >>http://www.opengroup.org/pubs/online/7908799/xsh/munmap.html >> >>That said, the subtle difference between our man page and the Open >>Group's description is the use of "some part" in our man page. In >>other words, it's unclear whether the Open Group intends for munmap() >>to return EINVAL if some part of the address range is invalid or only >>if the whole address range is invalid. I'm not aware of them issuing >>any clarifications on this question. >> >> > >Ok, we are interpreting things differently then. I understand both >manpages as simply saying that the given address range has to be within >user space limits, and not that additionally, the full range has to be >mapped as well. In my opinion, the EINVAL is only meant for simple >bounds checking of the given arguments. > >The description part of both manpages also simply states munmap() >removes any mappings for pages within the given range. I don't see >where that implies the full range has to be mapped. What would the >benefit of such a precondition be? > > After a more careful reading of the SuSv2 pages, I've changed my mind. I now agree with your interpretation. So, I'll be committing your patch. Regards, Alan P.S. I've also verified that both Solaris and Linux behave as you describe. alc 2008-05-24 21:57:16 UTC
FreeBSD src repository
Modified files:
sys/vm vm_mmap.c
Log:
To date, our implementation of munmap(2) has required that the
entirety of the specified range be mapped. Specifically, it has
returned EINVAL if the entire range is not mapped. There is not,
however, any basis for this in either SuSv2 or our own man page.
Moreover, neither Linux nor Solaris impose this requirement. This
revision removes this requirement.
Submitted by: Tijl Coosemans
PR: 118510
MFC after: 6 weeks
Revision Changes Path
1.221 +0 -7 src/sys/vm/vm_mmap.c
_______________________________________________
cvs-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/cvs-all
To unsubscribe, send any mail to "cvs-all-unsubscribe@freebsd.org"
State Changed From-To: analyzed->patched Patch applied to HEAD State Changed From-To: patched->closed Fix merged to 6.x and 7.x. |
When a memory region has been partially munmap()ed, subsequent calls to munmap() on this region have no effect. How-To-Repeat: The following program should segfault, but doesn't. It mmap()s 2 pages and munmap()s them again but the second page is still accessible. --- test.c begins here --- #include <sys/mman.h> #include <stdio.h> #include <unistd.h> int main( int argc, char **argv ) { unsigned int const page_size = getpagesize(); void *map; char volatile *cmap; map = mmap( NULL, 2 * page_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0 ); munmap( map, page_size ); munmap( map, 2 * page_size ); cmap = map; cmap[ page_size ] = 'a'; printf( "%c\n", cmap[ page_size ] ); return 0; } --- test.c ends here ---