diff mbox series

[v3,1/2] mm: protect free_pgtables with mmap_lock write lock in exit_mmap

Message ID 20211207215031.2251719-1-surenb@google.com (mailing list archive)
State New
Headers show
Series [v3,1/2] mm: protect free_pgtables with mmap_lock write lock in exit_mmap | expand

Commit Message

Suren Baghdasaryan Dec. 7, 2021, 9:50 p.m. UTC
oom-reaper and process_mrelease system call should protect against
races with exit_mmap which can destroy page tables while they
walk the VMA tree. oom-reaper protects from that race by setting
MMF_OOM_VICTIM and by relying on exit_mmap to set MMF_OOM_SKIP
before taking and releasing mmap_write_lock. process_mrelease has
to elevate mm->mm_users to prevent such race. Both oom-reaper and
process_mrelease hold mmap_read_lock when walking the VMA tree.
The locking rules and mechanisms could be simpler if exit_mmap takes
mmap_write_lock while executing destructive operations such as
free_pgtables.
Change exit_mmap to hold the mmap_write_lock when calling
free_pgtables and remove_vma. Operations like unmap_vmas and
unlock_range are not destructive and could run under mmap_read_lock
but for simplicity we take one mmap_write_lock during almost the entire
operation. Note also that because oom-reaper checks VM_LOCKED flag,
unlock_range() should not be allowed to race with it.
Before this patch, remove_vma used to be called with no locks held,
however with fput being executed asynchronously and vm_ops->close
not being allowed to hold mmap_lock (it is called from __split_vma
with mmap_sem held for write), changing that should be fine.
In most cases this lock should be uncontended. Previously, Kirill
reported ~4% regression caused by a similar change [1]. We reran the
same test and although the individual results are quite noisy, the
percentiles show lower regression with 1.6% being the worst case [2].
The change allows oom-reaper and process_mrelease to execute safely
under mmap_read_lock without worries that exit_mmap might destroy page
tables from under them.

[1] https://lore.kernel.org/all/20170725141723.ivukwhddk2voyhuc@node.shutemov.name/
[2] https://lore.kernel.org/all/CAJuCfpGC9-c9P40x7oy=jy5SphMcd0o0G_6U1-+JAziGKG6dGA@mail.gmail.com/

Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
changes in v3
- Amended patch description to explain why remove_vma can be called while
holding mmap_write_lock, per Michal Hocko
- Added a comment for vm_operations_struct::close, documenting restriction for
taking mmap_lock in the callback, per Michal Hocko and Matthew Wilcox

 include/linux/mm.h |  4 ++++
 mm/mmap.c          | 16 ++++++++--------
 2 files changed, 12 insertions(+), 8 deletions(-)

Comments

Matthew Wilcox Dec. 7, 2021, 10:07 p.m. UTC | #1
On Tue, Dec 07, 2021 at 01:50:30PM -0800, Suren Baghdasaryan wrote:
> - Added a comment for vm_operations_struct::close, documenting restriction for
> taking mmap_lock in the callback, per Michal Hocko and Matthew Wilcox

This should be a separate patch because it stands alone, but ...

>  struct vm_operations_struct {
>  	void (*open)(struct vm_area_struct * area);
> +    /*
> +     * Called with mmap_lock lock held for write from __split_vma and
> +     * remove_vma, therefore should never take that lock.
> +     */

Your whitespace indentation is weird.  And it'd be nice to make this a
kernel-doc comment (I know none of the others are, but that should be
fixed too).  And naming the functions that call it is wrong too.

	/**
	 * @close: Called when the VMA is being removed from the MM.
	 * Context: Caller holds mmap_lock.
	 */
Suren Baghdasaryan Dec. 7, 2021, 10:47 p.m. UTC | #2
On Tue, Dec 7, 2021 at 2:07 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Dec 07, 2021 at 01:50:30PM -0800, Suren Baghdasaryan wrote:
> > - Added a comment for vm_operations_struct::close, documenting restriction for
> > taking mmap_lock in the callback, per Michal Hocko and Matthew Wilcox
>
> This should be a separate patch because it stands alone, but ...

I thought about it and since it was relevant to the change in
remove_vma locking, I thought it would fit here. However, if you
insist on splitting it, I'll post it as a separate patch. Please let
me know.

>
> >  struct vm_operations_struct {
> >       void (*open)(struct vm_area_struct * area);
> > +    /*
> > +     * Called with mmap_lock lock held for write from __split_vma and
> > +     * remove_vma, therefore should never take that lock.
> > +     */
>
> Your whitespace indentation is weird.  And it'd be nice to make this a
> kernel-doc comment (I know none of the others are, but that should be
> fixed too).  And naming the functions that call it is wrong too.
>
>         /**
>          * @close: Called when the VMA is being removed from the MM.
>          * Context: Caller holds mmap_lock.
>          */

Ack. Will change and include in the next respin once I hear from you
on the preference for a separate patch.
Thanks!
Suren Baghdasaryan Dec. 7, 2021, 11:08 p.m. UTC | #3
On Tue, Dec 7, 2021 at 2:47 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Tue, Dec 7, 2021 at 2:07 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Tue, Dec 07, 2021 at 01:50:30PM -0800, Suren Baghdasaryan wrote:
> > > - Added a comment for vm_operations_struct::close, documenting restriction for
> > > taking mmap_lock in the callback, per Michal Hocko and Matthew Wilcox
> >
> > This should be a separate patch because it stands alone, but ...
>
> I thought about it and since it was relevant to the change in
> remove_vma locking, I thought it would fit here. However, if you
> insist on splitting it, I'll post it as a separate patch. Please let
> me know.
>
> >
> > >  struct vm_operations_struct {
> > >       void (*open)(struct vm_area_struct * area);
> > > +    /*
> > > +     * Called with mmap_lock lock held for write from __split_vma and
> > > +     * remove_vma, therefore should never take that lock.
> > > +     */
> >
> > Your whitespace indentation is weird.  And it'd be nice to make this a
> > kernel-doc comment (I know none of the others are, but that should be
> > fixed too).  And naming the functions that call it is wrong too.
> >
> >         /**
> >          * @close: Called when the VMA is being removed from the MM.
> >          * Context: Caller holds mmap_lock.

BTW, is the caller always required to hold mmap_lock for write or it
*might* hold it?

> >          */
>
> Ack. Will change and include in the next respin once I hear from you
> on the preference for a separate patch.
> Thanks!
Michal Hocko Dec. 8, 2021, 8:56 a.m. UTC | #4
On Tue 07-12-21 15:08:19, Suren Baghdasaryan wrote:
> On Tue, Dec 7, 2021 at 2:47 PM Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > On Tue, Dec 7, 2021 at 2:07 PM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Tue, Dec 07, 2021 at 01:50:30PM -0800, Suren Baghdasaryan wrote:
> > > > - Added a comment for vm_operations_struct::close, documenting restriction for
> > > > taking mmap_lock in the callback, per Michal Hocko and Matthew Wilcox
> > >
> > > This should be a separate patch because it stands alone, but ...
> >
> > I thought about it and since it was relevant to the change in
> > remove_vma locking, I thought it would fit here. However, if you
> > insist on splitting it, I'll post it as a separate patch. Please let
> > me know.
> >
> > >
> > > >  struct vm_operations_struct {
> > > >       void (*open)(struct vm_area_struct * area);
> > > > +    /*
> > > > +     * Called with mmap_lock lock held for write from __split_vma and
> > > > +     * remove_vma, therefore should never take that lock.
> > > > +     */
> > >
> > > Your whitespace indentation is weird.  And it'd be nice to make this a
> > > kernel-doc comment (I know none of the others are, but that should be
> > > fixed too).  And naming the functions that call it is wrong too.
> > >
> > >         /**
> > >          * @close: Called when the VMA is being removed from the MM.
> > >          * Context: Caller holds mmap_lock.
> 
> BTW, is the caller always required to hold mmap_lock for write or it
> *might* hold it?

I would go with might
Matthew Wilcox Dec. 8, 2021, 3:01 p.m. UTC | #5
On Tue, Dec 07, 2021 at 03:08:19PM -0800, Suren Baghdasaryan wrote:
> > >         /**
> > >          * @close: Called when the VMA is being removed from the MM.
> > >          * Context: Caller holds mmap_lock.
> 
> BTW, is the caller always required to hold mmap_lock for write or it
> *might* hold it?

__do_munmap() might hold it for read, thanks to:

        if (downgrade)
                mmap_write_downgrade(mm);

Should probably say:

	* Context: User context.  May sleep.  Caller holds mmap_lock.

I don't think we should burden the implementor of the vm_ops with the
knowledge that the VM chooses to not hold the mmap_lock under certain
circumstances when it doesn't matter whether it's holding the mmap_lock
or not.
Michal Hocko Dec. 8, 2021, 3:51 p.m. UTC | #6
On Wed 08-12-21 15:01:24, Matthew Wilcox wrote:
> On Tue, Dec 07, 2021 at 03:08:19PM -0800, Suren Baghdasaryan wrote:
> > > >         /**
> > > >          * @close: Called when the VMA is being removed from the MM.
> > > >          * Context: Caller holds mmap_lock.
> > 
> > BTW, is the caller always required to hold mmap_lock for write or it
> > *might* hold it?
> 
> __do_munmap() might hold it for read, thanks to:
> 
>         if (downgrade)
>                 mmap_write_downgrade(mm);
> 
> Should probably say:
> 
> 	* Context: User context.  May sleep.  Caller holds mmap_lock.
> 
> I don't think we should burden the implementor of the vm_ops with the
> knowledge that the VM chooses to not hold the mmap_lock under certain
> circumstances when it doesn't matter whether it's holding the mmap_lock
> or not.

If we document it like that some code might depend on that lock to be
held. I think we only want to document that the holder itself is not
allowed to take mmap sem or a depending lock.
Matthew Wilcox Dec. 8, 2021, 4:05 p.m. UTC | #7
On Wed, Dec 08, 2021 at 04:51:58PM +0100, Michal Hocko wrote:
> On Wed 08-12-21 15:01:24, Matthew Wilcox wrote:
> > On Tue, Dec 07, 2021 at 03:08:19PM -0800, Suren Baghdasaryan wrote:
> > > > >         /**
> > > > >          * @close: Called when the VMA is being removed from the MM.
> > > > >          * Context: Caller holds mmap_lock.
> > > 
> > > BTW, is the caller always required to hold mmap_lock for write or it
> > > *might* hold it?
> > 
> > __do_munmap() might hold it for read, thanks to:
> > 
> >         if (downgrade)
> >                 mmap_write_downgrade(mm);
> > 
> > Should probably say:
> > 
> > 	* Context: User context.  May sleep.  Caller holds mmap_lock.
> > 
> > I don't think we should burden the implementor of the vm_ops with the
> > knowledge that the VM chooses to not hold the mmap_lock under certain
> > circumstances when it doesn't matter whether it's holding the mmap_lock
> > or not.
> 
> If we document it like that some code might depend on that lock to be
> held. I think we only want to document that the holder itself is not
> allowed to take mmap sem or a depending lock.

The only place where we're not currently holding the mmap_lock is at
task exit, where the mmap_lock is effectively held because nobody else
can modify the task's mm.  Besides, Suren is changing that in this patch
series anyway, so it will be always true.
Suren Baghdasaryan Dec. 8, 2021, 4:50 p.m. UTC | #8
On Wed, Dec 8, 2021 at 8:05 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Dec 08, 2021 at 04:51:58PM +0100, Michal Hocko wrote:
> > On Wed 08-12-21 15:01:24, Matthew Wilcox wrote:
> > > On Tue, Dec 07, 2021 at 03:08:19PM -0800, Suren Baghdasaryan wrote:
> > > > > >         /**
> > > > > >          * @close: Called when the VMA is being removed from the MM.
> > > > > >          * Context: Caller holds mmap_lock.
> > > >
> > > > BTW, is the caller always required to hold mmap_lock for write or it
> > > > *might* hold it?
> > >
> > > __do_munmap() might hold it for read, thanks to:
> > >
> > >         if (downgrade)
> > >                 mmap_write_downgrade(mm);
> > >
> > > Should probably say:
> > >
> > >     * Context: User context.  May sleep.  Caller holds mmap_lock.
> > >
> > > I don't think we should burden the implementor of the vm_ops with the
> > > knowledge that the VM chooses to not hold the mmap_lock under certain
> > > circumstances when it doesn't matter whether it's holding the mmap_lock
> > > or not.
> >
> > If we document it like that some code might depend on that lock to be
> > held. I think we only want to document that the holder itself is not
> > allowed to take mmap sem or a depending lock.
>
> The only place where we're not currently holding the mmap_lock is at
> task exit, where the mmap_lock is effectively held because nobody else
> can modify the task's mm.  Besides, Suren is changing that in this patch
> series anyway, so it will be always true.

Ok, I'll make it a separate patch after the patch that changes
exit_mmap and this statement will become always true. Sounds
reasonable?
Suren Baghdasaryan Dec. 8, 2021, 7:13 p.m. UTC | #9
On Wed, Dec 8, 2021 at 8:50 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Wed, Dec 8, 2021 at 8:05 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Wed, Dec 08, 2021 at 04:51:58PM +0100, Michal Hocko wrote:
> > > On Wed 08-12-21 15:01:24, Matthew Wilcox wrote:
> > > > On Tue, Dec 07, 2021 at 03:08:19PM -0800, Suren Baghdasaryan wrote:
> > > > > > >         /**
> > > > > > >          * @close: Called when the VMA is being removed from the MM.
> > > > > > >          * Context: Caller holds mmap_lock.
> > > > >
> > > > > BTW, is the caller always required to hold mmap_lock for write or it
> > > > > *might* hold it?
> > > >
> > > > __do_munmap() might hold it for read, thanks to:
> > > >
> > > >         if (downgrade)
> > > >                 mmap_write_downgrade(mm);
> > > >
> > > > Should probably say:
> > > >
> > > >     * Context: User context.  May sleep.  Caller holds mmap_lock.
> > > >
> > > > I don't think we should burden the implementor of the vm_ops with the
> > > > knowledge that the VM chooses to not hold the mmap_lock under certain
> > > > circumstances when it doesn't matter whether it's holding the mmap_lock
> > > > or not.
> > >
> > > If we document it like that some code might depend on that lock to be
> > > held. I think we only want to document that the holder itself is not
> > > allowed to take mmap sem or a depending lock.
> >
> > The only place where we're not currently holding the mmap_lock is at
> > task exit, where the mmap_lock is effectively held because nobody else
> > can modify the task's mm.  Besides, Suren is changing that in this patch
> > series anyway, so it will be always true.
>
> Ok, I'll make it a separate patch after the patch that changes
> exit_mmap and this statement will become always true. Sounds
> reasonable?

Actually, while today vma_ops->close is called with mmap_lock held, I
believe we want this comment to reflect the restrictions on the
callback itself, not on the user. IOW, we want to say that the
callback should not take mmap_lock while the caller might or might not
hold it. If so, I think *might* would make more sense here, like this:

* Context: User context.  May sleep.  Caller might hold mmap_lock.

WDYT?
Matthew Wilcox Dec. 8, 2021, 7:22 p.m. UTC | #10
On Wed, Dec 08, 2021 at 11:13:42AM -0800, Suren Baghdasaryan wrote:
> On Wed, Dec 8, 2021 at 8:50 AM Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > On Wed, Dec 8, 2021 at 8:05 AM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Wed, Dec 08, 2021 at 04:51:58PM +0100, Michal Hocko wrote:
> > > > On Wed 08-12-21 15:01:24, Matthew Wilcox wrote:
> > > > > On Tue, Dec 07, 2021 at 03:08:19PM -0800, Suren Baghdasaryan wrote:
> > > > > > > >         /**
> > > > > > > >          * @close: Called when the VMA is being removed from the MM.
> > > > > > > >          * Context: Caller holds mmap_lock.
> > > > > >
> > > > > > BTW, is the caller always required to hold mmap_lock for write or it
> > > > > > *might* hold it?
> > > > >
> > > > > __do_munmap() might hold it for read, thanks to:
> > > > >
> > > > >         if (downgrade)
> > > > >                 mmap_write_downgrade(mm);
> > > > >
> > > > > Should probably say:
> > > > >
> > > > >     * Context: User context.  May sleep.  Caller holds mmap_lock.
> > > > >
> > > > > I don't think we should burden the implementor of the vm_ops with the
> > > > > knowledge that the VM chooses to not hold the mmap_lock under certain
> > > > > circumstances when it doesn't matter whether it's holding the mmap_lock
> > > > > or not.
> > > >
> > > > If we document it like that some code might depend on that lock to be
> > > > held. I think we only want to document that the holder itself is not
> > > > allowed to take mmap sem or a depending lock.
> > >
> > > The only place where we're not currently holding the mmap_lock is at
> > > task exit, where the mmap_lock is effectively held because nobody else
> > > can modify the task's mm.  Besides, Suren is changing that in this patch
> > > series anyway, so it will be always true.
> >
> > Ok, I'll make it a separate patch after the patch that changes
> > exit_mmap and this statement will become always true. Sounds
> > reasonable?
> 
> Actually, while today vma_ops->close is called with mmap_lock held, I
> believe we want this comment to reflect the restrictions on the
> callback itself, not on the user. IOW, we want to say that the
> callback should not take mmap_lock while the caller might or might not
> hold it. If so, I think *might* would make more sense here, like this:
> 
> * Context: User context.  May sleep.  Caller might hold mmap_lock.
> 
> WDYT?

We're documenting the contract between the caller and the callee.
That implies responsibilities on both sides.  For example, we're
placing requirements on the caller that they're not going to tear
down the VMA in interrupt context.  So I preferred what previous-Suren
said to current-Suren, "this statement will become always true".
Suren Baghdasaryan Dec. 8, 2021, 9:24 p.m. UTC | #11
On Wed, Dec 8, 2021 at 11:22 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Dec 08, 2021 at 11:13:42AM -0800, Suren Baghdasaryan wrote:
> > On Wed, Dec 8, 2021 at 8:50 AM Suren Baghdasaryan <surenb@google.com> wrote:
> > >
> > > On Wed, Dec 8, 2021 at 8:05 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > >
> > > > On Wed, Dec 08, 2021 at 04:51:58PM +0100, Michal Hocko wrote:
> > > > > On Wed 08-12-21 15:01:24, Matthew Wilcox wrote:
> > > > > > On Tue, Dec 07, 2021 at 03:08:19PM -0800, Suren Baghdasaryan wrote:
> > > > > > > > >         /**
> > > > > > > > >          * @close: Called when the VMA is being removed from the MM.
> > > > > > > > >          * Context: Caller holds mmap_lock.
> > > > > > >
> > > > > > > BTW, is the caller always required to hold mmap_lock for write or it
> > > > > > > *might* hold it?
> > > > > >
> > > > > > __do_munmap() might hold it for read, thanks to:
> > > > > >
> > > > > >         if (downgrade)
> > > > > >                 mmap_write_downgrade(mm);
> > > > > >
> > > > > > Should probably say:
> > > > > >
> > > > > >     * Context: User context.  May sleep.  Caller holds mmap_lock.
> > > > > >
> > > > > > I don't think we should burden the implementor of the vm_ops with the
> > > > > > knowledge that the VM chooses to not hold the mmap_lock under certain
> > > > > > circumstances when it doesn't matter whether it's holding the mmap_lock
> > > > > > or not.
> > > > >
> > > > > If we document it like that some code might depend on that lock to be
> > > > > held. I think we only want to document that the holder itself is not
> > > > > allowed to take mmap sem or a depending lock.
> > > >
> > > > The only place where we're not currently holding the mmap_lock is at
> > > > task exit, where the mmap_lock is effectively held because nobody else
> > > > can modify the task's mm.  Besides, Suren is changing that in this patch
> > > > series anyway, so it will be always true.
> > >
> > > Ok, I'll make it a separate patch after the patch that changes
> > > exit_mmap and this statement will become always true. Sounds
> > > reasonable?
> >
> > Actually, while today vma_ops->close is called with mmap_lock held, I
> > believe we want this comment to reflect the restrictions on the
> > callback itself, not on the user. IOW, we want to say that the
> > callback should not take mmap_lock while the caller might or might not
> > hold it. If so, I think *might* would make more sense here, like this:
> >
> > * Context: User context.  May sleep.  Caller might hold mmap_lock.
> >
> > WDYT?
>
> We're documenting the contract between the caller and the callee.
> That implies responsibilities on both sides.  For example, we're
> placing requirements on the caller that they're not going to tear
> down the VMA in interrupt context.  So I preferred what previous-Suren
> said to current-Suren, "this statement will become always true".
>

previous-Suren posted v4 at
https://lore.kernel.org/all/20211208212211.2860249-1-surenb@google.com
Thanks!
diff mbox series

Patch

diff --git a/include/linux/mm.h b/include/linux/mm.h
index a7e4a9e7d807..97e1a05c3b2c 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -577,6 +577,10 @@  enum page_entry_size {
  */
 struct vm_operations_struct {
 	void (*open)(struct vm_area_struct * area);
+    /*
+     * Called with mmap_lock lock held for write from __split_vma and
+     * remove_vma, therefore should never take that lock.
+     */
 	void (*close)(struct vm_area_struct * area);
 	/* Called any time before splitting to check if it's allowed */
 	int (*may_split)(struct vm_area_struct *area, unsigned long addr);
diff --git a/mm/mmap.c b/mm/mmap.c
index bfb0ea164a90..f4e09d390a07 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -3142,25 +3142,27 @@  void exit_mmap(struct mm_struct *mm)
 		 * to mmu_notifier_release(mm) ensures mmu notifier callbacks in
 		 * __oom_reap_task_mm() will not block.
 		 *
-		 * This needs to be done before calling munlock_vma_pages_all(),
+		 * This needs to be done before calling unlock_range(),
 		 * which clears VM_LOCKED, otherwise the oom reaper cannot
 		 * reliably test it.
 		 */
 		(void)__oom_reap_task_mm(mm);
 
 		set_bit(MMF_OOM_SKIP, &mm->flags);
-		mmap_write_lock(mm);
-		mmap_write_unlock(mm);
 	}
 
+	mmap_write_lock(mm);
 	if (mm->locked_vm)
 		unlock_range(mm->mmap, ULONG_MAX);
 
 	arch_exit_mmap(mm);
 
 	vma = mm->mmap;
-	if (!vma)	/* Can happen if dup_mmap() received an OOM */
+	if (!vma) {
+		/* Can happen if dup_mmap() received an OOM */
+		mmap_write_unlock(mm);
 		return;
+	}
 
 	lru_add_drain();
 	flush_cache_mm(mm);
@@ -3171,16 +3173,14 @@  void exit_mmap(struct mm_struct *mm)
 	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
 	tlb_finish_mmu(&tlb);
 
-	/*
-	 * Walk the list again, actually closing and freeing it,
-	 * with preemption enabled, without holding any MM locks.
-	 */
+	/* Walk the list again, actually closing and freeing it. */
 	while (vma) {
 		if (vma->vm_flags & VM_ACCOUNT)
 			nr_accounted += vma_pages(vma);
 		vma = remove_vma(vma);
 		cond_resched();
 	}
+	mmap_write_unlock(mm);
 	vm_unacct_memory(nr_accounted);
 }