Overlapping VMA regions not merged correctly #16
Comments
|
Wow Rick, good catch! I periodically do a massive build (all packages in the CDROM) when testing new features, basically to avoid regressions. After the changes introduced in c786079 I didn't see anything worse. I keep seeing the same 3 or 4 sporadic segfaults as always. They are pending to fix, of course, but seeing the same results and in the same place made me think I was in the good line. Your 7 test programs will be really of help. I'll double check the case 7 to see if I can fix the issue. I'll let you know the results. |
|
After the massive build, I still have seen the same number of segfaults in the same places. Perhaps this confirms that sys_mmap() is not working properly yet. Regarding your program tests:
I confirm all cases.
No, in all cases the heap is affected by the new segments and in the case 4, it even disappears. For instance, this is the output for the case 1: As you can see, the heap initially had I've executed the same program test 1 on a Linux, and this is the result: The segfault here would confirm that heap has been clobbered by a different segment (mmap): From the URL you provided above:
As I said, the rest of cases have similar results affecting heap directly. Also, after applying your patch I have seen a regression in the case of a new mmap() with an address above the first entry. I mean, imagine the following vm regions: Before apply your patch, I was able to include a new mmap before the first one, i.e., an address like After applying your patch, this is not possible and it caused a reboot: The following is the test program used to include a new mmap region: #include <stdio.h>
#include <string.h>
#include <sys/mman.h>
#include <errno.h>
int main(void)
{
int *addr, start, len;
int pid, flags, mode;
int fd;
char cmd[100];
for(;;) {
printf("\n");
printf("Address (hex): ");
scanf("%x", &start);
getchar(); /* eats a '\n' */
printf("Length (hex): ");
scanf("%x", &len);
getchar(); /* eats a '\n' */
printf("Mode (0=R,1=W,2=RW): ");
scanf("%d", &mode);
getchar(); /* eats a '\n' */
addr = (unsigned int *)start;
if(start) {
flags = MAP_FIXED | MAP_PRIVATE;
} else {
addr = NULL;
flags = MAP_PRIVATE | MAP_ANONYMOUS;
}
if(mode == 0) {
mode = PROT_READ;
} else if(mode == 1) {
mode = PROT_WRITE;
} else {
mode = PROT_READ | PROT_WRITE;
}
if(start >= 0) {
printf("mapping addr = %08x, len = %d\n", (int)addr, len);
if(flags == (MAP_FIXED | MAP_PRIVATE)) {
fd = open("/dev/zero", 0);
mmap(addr, len, mode, flags, fd, 0);
close(fd);
} else {
mmap(addr, len, mode, flags, -1, 0);
}
perror("");
} else {
/* if address is negative then remove the vm region */
start *= -1;
addr = (unsigned int *)start;
printf("unmapping addr = %08x, len = %d\n", (int)addr, len);
munmap(addr, len);
perror("");
}
printf("mmap regions:\n");
memset(cmd, 0, sizeof(cmd));
sprintf(cmd, "cat /proc/%d/maps", getpid());
system(cmd);
}
return 0;
}I think this could be related to the case 7. |
|
For the test cases, the heap should be changed by the mmap. When an mmap segment overlaps the heap, the overlapping part of the heap should be discarded. So, I think the test cases are working as expected (except for 7). From here: https://man7.org/linux/man-pages/man2/mmap.2.html The reason I filed this issue was because a program in live-bootstrap crashed because it was doing something similar to test case 3. It remapped part of its own heap using mmap. But the rest of the heap was deleted unexpectedly and it segfaulted when trying to access that. Honestly, I didn't look closely at why it was remapping the first page of its heap but it appears to be something the musl-1.2.3 library does internally related to dynamic linking. So, remapping the heap is something that should work and that is the reason I wrote the tests to overlap the heap. I also tried some of these tests on Linux and experienced segfaults, but that's probably because other standard libraries like glibc don't react well to the heap being remapped. But the kernel may be correctly doing what it was told to do according to the documentation of mmap. It would have been simpler for me to provide tests that avoided the heap, and those might not have segfaulted on Linux, but I was trying to address the specific situation I was experiencing with a musl-based program running on Fiwix. With regards to 0x60000000 I agree that if that worked before it should still work. I'll take a look at that today and report back later. |
|
I added a commit to the PR that handles the 0x06000000 situation. The previous code did not handle case where vma->prev wrapped around to the end of list or to itself. Sorry about that. |
Yes, the problem here is that even after applying your patch Fiwix wasn't managing correctly the discarded segments. It didn't discard the memory pages associated to those overlapped segments. That's why we don't get a segfault when we should.
I appreciate a lot the effort you put on fixing this. Fiwix indeed was lacking of a properly sys_mmap behavior. I'll include a call to |
Oh, yeah that's something I never thought of. The previous code adjusted the vma ranges without addressing the underlying pages so it didn't even occur to me. I don't have tests which verify pages are unmapped. I only have test 5 which verified a page was still mapped. You may need to add or remove a subset of pages from a vma or transfer some of them to that new vma region created in merge_vma_regions, which is more complicated than I thought. If you make that change I'll be sure to look it over and make sure it works for the situations I'm testing. |
|
After thinking about this a little bit more I am wondering why the test 5 program segfaulted before my fix was applied. The vma for the heap was shortened but the underlying pages for the end of the heap should have still been mapped... |
You mean this line? expansion[16000] = 0;Tests 3 and 5 have this line.
No, the system call
Oh!, you're right. It doesn't segfaults under Linux. The address of the variable |
|
Case 5 (continued): This is the result under Fiwix: and this is the result under Linux: I see two different things here:
|
|
This is the change in |
|
I recently noticed, while doing a complete package build, that some packages showed the following message when its Why that no? As far as I can tell, I was able to extract the program that tested #include <stdio.h>
/* Thanks to Mike Haertel and Jim Avera for this test.
Here is a matrix of mmap possibilities:
mmap private not fixed
mmap private fixed at somewhere currently unmapped
mmap private fixed at somewhere already mapped
mmap shared not fixed
mmap shared fixed at somewhere currently unmapped
mmap shared fixed at somewhere already mapped
For private mappings, we should verify that changes cannot be read()
back from the file, nor mmap's back from the file at a different
address. (There have been systems where private was not correctly
implemented like the infamous i386 svr4.0, and systems where the
VM page cache was not coherent with the file system buffer cache
like early versions of FreeBSD and possibly contemporary NetBSD.)
For shared mappings, we should conversely verify that changes get
propogated back to all the places they're supposed to be.
Grep wants private fixed already mapped.
The main things grep needs to know about mmap are:
* does it exist and is it safe to write into the mmap'd area
* how to use it (BSD variants) */
#include <fcntl.h>
#include <sys/mman.h>
#include <sys/errno.h>
#include <stdlib.h>
static void show_maps(void)
{
char buf[80];
FILE *f;
sprintf(buf, "/proc/self/maps");
f = fopen(buf, "r");
while(fgets(buf, 79, f)) {
printf("%s", buf);
}
fclose(f);
}
int
main (void)
{
char *data, *data2, *data3;
int i, pagesize;
int fd;
pagesize = getpagesize ();
/* First, make a file with some known garbage in it. */
show_maps();
data = (char *) malloc (pagesize);
printf("data = (0x%x) (pagesize = %d)\n", (unsigned int)data, pagesize);
show_maps();
if (!data) {
printf("ERR: first malloc failed!\n");
return (1);
}
for (i = 0; i < pagesize; ++i)
*(data + i) = rand ();
umask (0);
fd = creat ("conftest.mmap", 0600);
if (fd < 0) {
printf("ERR: creat(conftest.mmap)\n");
return (1);
}
if (write (fd, data, pagesize) != pagesize) {
printf("ERR: write(..., pagesize)\n");
return (1);
}
close (fd);
/* Next, try to mmap the file at a fixed address which already has
something else allocated at it. If we can, also make sure that
we see the same garbage. */
fd = open ("conftest.mmap", O_RDWR);
if (fd < 0) {
printf("ERR: open(conftest.mmap)\n");
return (1);
}
data2 = (char *) malloc (2 * pagesize);
printf("data2 = (0x%x) (pagesize = %d)\n", (unsigned int)data2, pagesize);
show_maps();
if (!data2) {
printf("ERR: not data2\n");
return (1);
}
data2 += (pagesize - ((int) data2 & (pagesize - 1))) & (pagesize - 1);
printf("data2 = (0x%x)\n", (unsigned int)data2);
show_maps();
if (data2 != mmap (data2, pagesize, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_FIXED, fd, 0L)) {
printf("ERR: mmap()\n");
return (1);
}
for (i = 0; i < pagesize; ++i)
if (*(data + i) != *(data2 + i)) {
printf("data != data2\n");
return (1);
}
/* Finally, make sure that changes to the mapped area do not
percolate back to the file as seen by read(). (This is a bug on
some variants of i386 svr4.0.) */
for (i = 0; i < pagesize; ++i)
*(data2 + i) = *(data2 + i) + 1;
data3 = (char *) malloc (pagesize);
perror("data3");
printf("data3 = (0x%x) (pagesize = %d)\n", (unsigned int)data3, pagesize);
show_maps();
if (!data3) {
printf("ERR: not data3\n");
return (1);
}
if (read (fd, data3, pagesize) != pagesize) {
printf("read != pagesize\n");
return (1);
}
for (i = 0; i < pagesize; ++i)
if (*(data + i) != *(data3 + i)) {
printf("data != data3\n");
return (1);
}
close (fd);
return (0);
}This is the output of this program under Fiwix: Why And this is the output under Linux (i386) : As you can see, there is again an offset on an anonymous mapping: I need to investigate further. |
|
OK, I've found the bug: Line 248 in 2fd0923 This line should be: free_vma_pages(a, b->start, b->end - b->start);otherwise it ends up freeing too many pages. After applying this patch the test 5 no longer segfaults and, after another complete package build, all mmap checkings now say that mmap works. |
|
I think we can safely close it now. |
sys_lchown, and creating the new #182 sys_chown which will follow (dereference) symbolic links. This might break the compatibility with Linux 2.0 ABI.
If a program calls mmap on memory already mapped the segments are not split/merged correctly. This can result in parts of an existing segment being lost. In the case where the segment was the heap, attempts to access that part of the heap results in a segfault. (See test case 3 below).
This is partly a regression introduced by c786079
The following is a list of various kinds of segment overlaps that can occur. ("old" refers to an existing segment).
The new segment should be mapped fully and any overlapping part of the old segment should be discarded. For case 1, if the segments are compatible, they should be merged into one.
Background information that describes the expected behavior is provided here: https://stackoverflow.com/questions/14943990/overlapping-pages-with-mmap-map-fixed
Prior to c786079 cases 2, 3, and 5 worked properly while 1, 4, 6, and 7 caused reboots.
After c786079 case 1 did not merge, 2 and 4 worked, while 3, 5, 6 and 7 left overlapping segments. Also 4 leaks memory and 1 subjects vma memory to a double-free.
With the PR provided, 1, 2, 3, 4, 5, and 6 work properly but 7 still leaves an overlapping segment.
The following are test programs for cases 1 through 7.
Case 1:
Case 2:
Case 3:
Case 4:
Case 5:
Case 6:
Case 7:
It can be helpful to print the vma regions before and after every mmap syscall by applying this patch to the kernel:
The text was updated successfully, but these errors were encountered: