Re: [PATCH] mm: rearrange exit_mmap() to unlock before arch_exit_mmap

[Lee Schermerhorn <Lee.Schermerhorn@hp.com>]

On Tue, 2009-02-10 at 13:31 -0800, Andrew Morton wrote:
> On Mon, 09 Feb 2009 12:29:48 -0500
> Lee Schermerhorn <Lee.Schermerhorn@hp.com> wrote:
> 
> > From: Jeremy Fitzhardinge <jeremy@goop.org>
> > 
> > Subject: mm: rearrange exit_mmap() to unlock before arch_exit_mmap
> > 
> > Applicable to 29-rc4 and 28-stable
> > 
> > Christophe Saout reported [in precursor to:
> > http://marc.info/?l=linux-kernel&m=123209902707347&w=4]:
> > 
> > > Note that I also some a different issue with CONFIG_UNEVICTABLE_LRU.
> > > Seems like Xen tears down current->mm early on process termination, so
> > > that __get_user_pages in exit_mmap causes nasty messages when the
> > > process had any mlocked pages.  (in fact, it somehow manages to get into
> > > the swapping code and produces a null pointer dereference trying to get
> > > a swap token)
> > 
> > Jeremy explained:
> > 
> > Yes.  In the normal case under Xen, an in-use pagetable is "pinned", 
> > meaning that it is RO to the kernel, and all updates must go via 
> > hypercall (or writes are trapped and emulated, which is much the same 
> > thing).  An unpinned pagetable is not currently in use by any process, 
> > and can be directly accessed as normal RW pages.
> > 
> > As an optimisation at process exit time, we unpin the pagetable as early 
> > as possible (switching the process to init_mm), so that all the normal 
> > pagetable teardown can happen with direct memory accesses.
> > 
> > This happens in exit_mmap() -> arch_exit_mmap().  The munlocking happens 
> > a few lines below.  The obvious thing to do would be to move 
> > arch_exit_mmap() to below the munlock code, but I think we'd want to 
> > call it even if mm->mmap is NULL, just to be on the safe side.
> > 
> > Thus, this patch:
> > 
> > exit_mmap() needs to unlock any locked vmas before calling
> > arch_exit_mmap, as the latter may switch the current mm to init_mm,
> > which would cause the former to fail.
> > 
> > Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> > Acked-by:  Lee Schermerhorn <lee.schermerhorn@hp.com>
> > 
> > ---
> >  mm/mmap.c |   10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > ===================================================================
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -2078,12 +2078,8 @@
> >  	unsigned long end;
> >  
> >  	/* mm's last user has gone, and its about to be pulled down */
> > -	arch_exit_mmap(mm);
> >  	mmu_notifier_release(mm);
> >  
> > -	if (!mm->mmap)	/* Can happen if dup_mmap() received an OOM */
> > -		return;
> > -
> >  	if (mm->locked_vm) {
> >  		vma = mm->mmap;
> >  		while (vma) {
> > @@ -2092,7 +2088,13 @@
> >  			vma = vma->vm_next;
> >  		}
> >  	}
> > +
> > +	arch_exit_mmap(mm);
> > +
> >  	vma = mm->mmap;
> > +	if (!vma)	/* Can happen if dup_mmap() received an OOM */
> > +		return;
> > +
> >  	lru_add_drain();
> >  	flush_cache_mm(mm);
> >  	tlb = tlb_gather_mmu(mm, 1);
> 
> The patch as it stands doesn't apply cleanly to 2.6.28.  I didn't look
> into what needs to be done to fix it up.  Presumably the stable beavers
> would like a fixed-up and tested version for backporting sometime.

The only difference I can see in this area, looking at a 28.2 tree I
have, the 28.4 patch on kernel.org and the context in the patch, is that
"flush_cache_mm(mm);" line.

Lee