mm: protect free_pgtables with mmap_lock write lock in exit_mmap

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. 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.
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/[email protected]/
[2] https://lore.kernel.org/all/CAJuCfpGC9-c9P40x7oy=jy5SphMcd0o0G_6U1-+JAziGKG6dGA@mail.gmail.com/

Signed-off-by Suren Baghdasaryan <[email protected]>

---
changes in v2
- Moved mmap_write_unlock to cover remove_vma loop as well, per Matthew Wilcox

 mm/mmap.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

mm/mmap.c +0 -0

index bfb0ea164a90..f4e09d390a07 100644
@@ -3142,25 +3142,27 @@ void exit_mmap(struct mm_struct *mm)
 		 * to mmu_notifier_release(mm) ensures mmu notifier callbacks in

Quoted 50 lines · Suren Baghdasaryan

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. 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.
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/[email protected]/
[2] https://lore.kernel.org/all/CAJuCfpGC9-c9P40x7oy=jy5SphMcd0o0G_6U1-+JAziGKG6dGA@mail.gmail.com/

Signed-off-by Suren Baghdasaryan <[email protected]>

---
changes in v2
- Moved mmap_write_unlock to cover remove_vma loop as well, per Matthew Wilcox

 mm/mmap.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

mm/mmap.c +1 -1

index bfb0ea164a90..f4e09d390a07 100644
@@ -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);
 

mm/mmap.c void exit_mmap(struct mm_struct *mm) · line 3151

Quoted 1 line

mm/mmap.c +0 -0

              set_bit(MMF_OOM_SKIP, &mm->flags);

Why do you keep this in place?

Other than that looks OK to me. Maybe we want to add an explicit note that vm_ops::close cannot take mmap_sem in any form. The changelog should also mention that you have considered remove_vma and its previous no MM locking assumption. You can argue that fput is async and close callback shouldn't really need mmap_sem.

--
Michal Hocko
SUSE Labs

Quoted 5 lines

Other than that looks OK to me. Maybe we want to add an explicit note that vm_ops::close cannot take mmap_sem in any form. The changelog should also mention that you have considered remove_vma and its previous no MM locking assumption. You can argue that fput is async and close callback shouldn't really need mmap_sem.

Should I post another version of this patch with the patch description clarifying these points and additional comments as you suggested?

Quoted 6 lines · Suren Baghdasaryan

Quoted 5 lines

Other than that looks OK to me. Maybe we want to add an explicit note that vm_ops::close cannot take mmap_sem in any form. The changelog should also mention that you have considered remove_vma and its previous no MM locking assumption. You can argue that fput is async and close callback shouldn't really need mmap_sem.

Quoted 2 lines

Should I post another version of this patch with the patch description clarifying these points and additional comments as you suggested?

fyi, vm_ops->close() is already called with the mmap_sem held for write in __split_vma(). If that needs to be documented, it's a separate patch because it's absolutely not a consequence of this patch.

Quoted 52 lines · Michal Hocko

Quoted 50 lines · Suren Baghdasaryan

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. 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.
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/[email protected]/
[2] https://lore.kernel.org/all/CAJuCfpGC9-c9P40x7oy=jy5SphMcd0o0G_6U1-+JAziGKG6dGA@mail.gmail.com/

Signed-off-by Suren Baghdasaryan <[email protected]>

---
changes in v2
- Moved mmap_write_unlock to cover remove_vma loop as well, per Matthew Wilcox

 mm/mmap.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

mm/mmap.c +1 -1

index bfb0ea164a90..f4e09d390a07 100644
@@ -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);
 

Quoted 3 lines

mm/mmap.c void exit_mmap(struct mm_struct *mm) · line 3151

Quoted 1 line

mm/mmap.c +0 -0

              set_bit(MMF_OOM_SKIP, &mm->flags);

Why do you keep this in place?

Sorry for the delay, I was out last week.
I missed your comment about removing MMF_OOM_SKIP at
https://lore.kernel.org/all/YYrO%[email protected]
I'll look into removing it in a separate patch, which I think would be cleaner.

Quoted 5 lines

Other than that looks OK to me. Maybe we want to add an explicit note that vm_ops::close cannot take mmap_sem in any form. The changelog should also mention that you have considered remove_vma and its previous no MM locking assumption. You can argue that fput is async and close callback shouldn't really need mmap_sem.

Should I post another version of this patch with the patch description clarifying these points and additional comments as you suggested?

Quoted 53 lines · Suren Baghdasaryan

Quoted 52 lines · Michal Hocko

Quoted 50 lines · Suren Baghdasaryan

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. 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.
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/[email protected]/
[2] https://lore.kernel.org/all/CAJuCfpGC9-c9P40x7oy=jy5SphMcd0o0G_6U1-+JAziGKG6dGA@mail.gmail.com/

Signed-off-by Suren Baghdasaryan <[email protected]>

---
changes in v2
- Moved mmap_write_unlock to cover remove_vma loop as well, per Matthew Wilcox

 mm/mmap.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

mm/mmap.c +1 -1

index bfb0ea164a90..f4e09d390a07 100644
@@ -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);
 

Quoted 8 lines

Quoted 3 lines

mm/mmap.c void exit_mmap(struct mm_struct *mm) · line 3151

Quoted 1 line

mm/mmap.c +0 -0

              set_bit(MMF_OOM_SKIP, &mm->flags);

Why do you keep this in place?

Sorry for the delay, I was out last week.
I missed your comment about removing MMF_OOM_SKIP at
https://lore.kernel.org/all/YYrO%[email protected]
I'll look into removing it in a separate patch, which I think would be cleaner.

The point of this code was to sync up the oom_repaer and exit_mmap. Now that your patch uses proper locking for that to happen then MMF_OOM_SKIP is not really necessary. IIRC all you need to guarantee is that the vma tree is empty when exit_mmap does all its work - i.e set mm->mmap to NULL. You can do that after remove_vma loop but it would be equally safe at any time after vma = mm->mmap as the loop relies on the vma chain. Doing that after would be slightly nicer if you ask me.

Quoted 9 lines

Quoted 6 lines

Other than that looks OK to me. Maybe we want to add an explicit note that vm_ops::close cannot take mmap_sem in any form. The changelog should also mention that you have considered remove_vma and its previous no MM locking assumption. You can argue that fput is async and close callback shouldn't really need mmap_sem.

Should I post another version of this patch with the patch description clarifying these points and additional comments as you suggested?

Yes please.

--
Michal Hocko
SUSE Labs

Quoted 64 lines · Michal Hocko

Quoted 53 lines · Suren Baghdasaryan

Quoted 52 lines · Michal Hocko

Quoted 50 lines · Suren Baghdasaryan

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. 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.
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/[email protected]/
[2] https://lore.kernel.org/all/CAJuCfpGC9-c9P40x7oy=jy5SphMcd0o0G_6U1-+JAziGKG6dGA@mail.gmail.com/

Signed-off-by Suren Baghdasaryan <[email protected]>

---
changes in v2
- Moved mmap_write_unlock to cover remove_vma loop as well, per Matthew Wilcox

 mm/mmap.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

mm/mmap.c +1 -1

index bfb0ea164a90..f4e09d390a07 100644
@@ -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);
 

Quoted 8 lines

Quoted 3 lines

mm/mmap.c void exit_mmap(struct mm_struct *mm) · line 3151

Quoted 1 line

mm/mmap.c +0 -0

              set_bit(MMF_OOM_SKIP, &mm->flags);

Why do you keep this in place?

Sorry for the delay, I was out last week.
I missed your comment about removing MMF_OOM_SKIP at
https://lore.kernel.org/all/YYrO%[email protected]
I'll look into removing it in a separate patch, which I think would be cleaner.

Quoted 7 lines

The point of this code was to sync up the oom_repaer and exit_mmap. Now that your patch uses proper locking for that to happen then MMF_OOM_SKIP is not really necessary. IIRC all you need to guarantee is that the vma tree is empty when exit_mmap does all its work - i.e set mm->mmap to NULL. You can do that after remove_vma loop but it would be equally safe at any time after vma = mm->mmap as the loop relies on the vma chain. Doing that after would be slightly nicer if you ask me.

Will do. But if you don't mind I'll post the removal of MMF_OOM_SKIP as a separate patch. This patchset has already been extensively tested and it will be easier for me to test MMF_OOM_SKIP removal separately.

Quoted 11 lines

Quoted 9 lines

Quoted 6 lines

Other than that looks OK to me. Maybe we want to add an explicit note that vm_ops::close cannot take mmap_sem in any form. The changelog should also mention that you have considered remove_vma and its previous no MM locking assumption. You can argue that fput is async and close callback shouldn't really need mmap_sem.

Should I post another version of this patch with the patch description clarifying these points and additional comments as you suggested?

Yes please.

Will re-post. So, to clarify, we want:
- Patch description to include explanation that remove_vma is now being called under MM lock but this should not be a problem because fput and vm_ops->close do not and should not take mmap_sem.
- Add a comment for vm_ops->close that the callback should not take mmap_sem, with explanation that __split_vma and exit_mmap use this callback with the mmap_sem write lock taken.
Is that correct?

Quoted 5 lines

--
Michal Hocko
SUSE Labs