kern/68017: fork with INHERIT_NONE miscounts VM map sizes

[Mark W. Krentel]

>Number:         68017
>Category:       kern
>Synopsis:       fork with INHERIT_NONE miscounts VM map sizes
>Confidential:   no
>Severity:       serious
>Priority:       low
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Wed Jun 16 18:00:40 GMT 2004
>Closed-Date:
>Last-Modified:
>Originator:     Mark W. Krentel
>Release:        5.2-Current as of May 26, 2004
>Organization:
none
>Environment:
5.2-Current as of May 26, 2004
>Description:
During fork(), vmspace_fork() in sys/vm/vm_map.c constructs the new
vmspace and vm_map by copying map entries according to their
inheritance.  Map entries with VM_INHERIT_NONE are correctly skipped,
but the size of the new map is incorrectly and always set to the size
of the old map, thus ingoring the INHERIT_NONE regions.

    new_map->size = old_map->size;

This results in a new size that is too large whenever the original
process has any region with inheritance INHERIT_NONE.

Furthermore, the following fields in struct vmspace are also bcopy()'d
directly from the old vmspace without regard to the inheritance of the
segments.

    #define vm_startcopy vm_rssize
            segsz_t vm_rssize;      /* current resident set size in pages */
            segsz_t vm_swrss;       /* resident set size before last swap */
            segsz_t vm_tsize;       /* text size (pages) XXX */
            segsz_t vm_dsize;       /* data size (pages) XXX */
            segsz_t vm_ssize;       /* stack size (pages) */
            caddr_t vm_taddr;       /* (c) user virtual address of text */
            caddr_t vm_daddr;       /* (c) user virtual address of data */
            caddr_t vm_maxsaddr;    /* user VA at max stack growth */
    #define vm_endcopy vm_exitingcnt

vm_taddr and vm_daddr are probably ok, but some of the others may also
be incorrect if the original process has regions with INHERIT_NONE.

Btw, nentries in struct vm_map is computed correctly because it is
initialized to 0 in vm_map_zinit() and then incremented in
vm_map_entry_link().

I guess no one ever uses INHERIT_NONE.  In 5.2 with INVARIANTS turned
on, vm_map_zdtor() notices the discrepancy in vm_map.size and panics,
which is pretty noticeable.

>How-To-Repeat:
In 5.2 with INVARIANTS turned on, the following program produces a
panic in vm_map_zdtor() because map->size > 0 when the child exits.

#include <sys/mman.h>
#include <sys/types.h>
#include <unistd.h>

int main()
{
        void *p;

        p = mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_ANON, -1, 0);
        minherit(p, 4096, INHERIT_NONE);
        fork();

        return (0);
}

>Fix:
This patch fixes the vm_map.size problem.  Note that size (along with
nentries) is initialized to 0 in vm_map_zinit().

I don't really know enough about the other vmspace entries (vm_tsize,
vm_dsize, etc.) to offer an intelligent patch for them.

Index: sys/vm/vm_map.c
===================================================================
RCS file: /data/ncvs/src/sys/vm/vm_map.c,v
retrieving revision 1.338
diff -u -r1.338 vm_map.c
--- sys/vm/vm_map.c     25 May 2004 18:28:52 -0000      1.338
+++ sys/vm/vm_map.c     16 Jun 2004 17:22:49 -0000
@@ -2431,6 +2431,7 @@
                         */
                        vm_map_entry_link(new_map, new_map->header.prev,
                            new_entry);
+                       new_map->size += new_entry->end - new_entry->start;
 
                        /*
                         * Update the physical map
@@ -2452,6 +2453,7 @@
                        new_entry->object.vm_object = NULL;
                        vm_map_entry_link(new_map, new_map->header.prev,
                            new_entry);
+                       new_map->size += new_entry->end - new_entry->start;
                        vm_map_copy_entry(old_map, new_map, old_entry,
                            new_entry);
                        break;
@@ -2459,7 +2461,6 @@
                old_entry = old_entry->next;
        }
 
-       new_map->size = old_map->size;
        old_map->infork = 0;
        vm_map_unlock(old_map);

>Release-Note:
>Audit-Trail:
>Unformatted: