2.4.* mlockall(MCL_FUTURE) is broken -- child inherits VM_LOCKED

PATCH In review Dave Anderson 3 participants 4 messages Jan 8, 2002

lore

Share & tools

lore

Dave Anderson

author

2002-01-08 20:57 UTC #0 lore raw

PATCH 1/1 2.4.* mlockall(MCL_FUTURE) is broken -- child inherits VM_LOCKED

In 2.4.*, mlockall(MCL_FUTURE) is erroneously inherited by child processes across fork() and exec():

1. across a fork(), the inherited memory is not locked, but any new memory allocations by the child will be VM_LOCKED.
2. across a subsequent exec(), *all* of the exec'd program's memory except for its stack pages will be VM_LOCKED.

The problem is:

 1.  if MCL_FUTURE, mm->def_flags gets set to VM_LOCKED in do_mlockall().
 2.  mm->def_flags is not cleared during subsequent forks and execs.
 3   mm->def_flags, with the leftover VM_LOCKED flag set, is subsequently

utilized in calc_vm_flags() when called by do_brk() to extend the address space of a forked process, and by do_mmap_pgoff() when building the non-stack address space of an exec'd process.

The proposed patch puts the fix in mm_init(), which seems to be the most appropriate place since it's called by copy_mm(), and by mm_alloc() on behalf of exec_mmap():

# diff -u linux/kernel/fork.c linux-2.4.17/kernel/fork.c
--- linux/kernel/fork.c Tue Jan  8 15:11:13 2002
+++ linux-2.4.17/kernel/fork.c  Tue Jan  8 15:12:26 2002
@@ -219,6 +219,7 @@
        init_rwsem(&mm->mmap_sem);
        mm->page_table_lock = SPIN_LOCK_UNLOCKED;
        mm->pgd = pgd_alloc(mm);
+       mm->def_flags = 0;
        if (mm->pgd)
                return mm;
        free_mm(mm);

Note that it worked OK in 2.2 because mm->def_flags was explicitly cleared in mm_alloc(), which was called by both copy_mm() and exec_mmap(). But things were shuffled around a bit in 2.4, and it must have gotten lost in the translation...

Dave Anderson

==============================================================================

  David Anderson                             [email protected]
  Mission Critical Linux, Inc.               http://www.mclinux.com
  100 Foot of John St.                       Work: 978-606-0225
  Lowell, MA 01852                           Fax: 978-446-9470

==============================================================================

Andrew Morton

2002-01-08 22:03 UTC #1 lore raw

Quoted 3 lines · Dave Anderson

In 2.4.*, mlockall(MCL_FUTURE) is erroneously inherited by child processes across fork() and exec():

The Linux manpage says that it is not inherited across either.

However SUS says that it is not inherited across exec, and doesn't mention fork() at all.
http://www.opengroup.org/onlinepubs/007908799/xsh/mlockall.html

So... Shouldn't we be clearing it in the exec() path?

Quoted 13 lines

...

# diff -u linux/kernel/fork.c linux-2.4.17/kernel/fork.c

kernel/fork.c Tue Jan 8 15:12:26 2002 renamed from kernel/fork.c Tue Jan 8 15:11:13 2002 +1 -0

@@ -219,6 +219,7 @@
        init_rwsem(&mm->mmap_sem);
        mm->page_table_lock = SPIN_LOCK_UNLOCKED;
        mm->pgd = pgd_alloc(mm);
+       mm->def_flags = 0;
        if (mm->pgd)
                return mm;
        free_mm(mm);

Quoted 4 lines

Note that it worked OK in 2.2 because mm->def_flags was explicitly cleared in mm_alloc(), which was called by both copy_mm() and exec_mmap(). But things were shuffled around a bit in 2.4, and it must have gotten lost in the translation...

um. Is this correct? It seems that we'll be clearing things like VM_IO on device mappings across fork. Bad. Would an explicit clear of VM_LOCKED be better here? (Assuming we want to ignore SUS).

Bruce Blinn

2002-01-08 22:18 UTC #2 lore raw

In reply to Dave Anderson

Quoted 6 lines · Andrew Morton

Quoted 3 lines · Dave Anderson
In 2.4.*, mlockall(MCL_FUTURE) is erroneously inherited by child processes across fork() and exec():

Quoted 8 lines

The Linux manpage says that it is not inherited across either.
However SUS says that it is not inherited across exec, and doesn't mention fork() at all.
http://www.opengroup.org/onlinepubs/007908799/xsh/mlockall.html
So... Shouldn't we be clearing it in the exec() path?

But, the SUS documentation for fork() says that it does not inherit the memory locks of the parent. It explicitly mentions mlockall().

	http://www.opengroup.org/onlinepubs/007908799/xsh/fork.html

Andrew Morton

2002-01-08 22:44 UTC #3 lore raw

In reply to Dave Anderson

Quoted 14 lines · Bruce Blinn

Quoted 12 lines · Andrew Morton
Quoted 3 lines · Dave Anderson
In 2.4.*, mlockall(MCL_FUTURE) is erroneously inherited by child processes across fork() and exec():
The Linux manpage says that it is not inherited across either.
However SUS says that it is not inherited across exec, and doesn't mention fork() at all.
http://www.opengroup.org/onlinepubs/007908799/xsh/mlockall.html

Quoted 7 lines

Quoted 2 lines
So... Shouldn't we be clearing it in the exec() path?
But, the SUS documentation for fork() says that it does not inherit the memory locks of the parent. It explicitly mentions mlockall().
        http://www.opengroup.org/onlinepubs/007908799/xsh/fork.html

So it does. So clearing it on fork is correct. And my comment regarding def_flags was nonsense. Probably it's best to explicitly clear VM_LOCKED, just in case something else gets added to def_flags in the future.

Apart from that - ship it :)