| Message ID | 20181013002430.698-2-aarcange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migrate_misplaced_transhuge_page race conditions | expand |
On Fri, Oct 12, 2018 at 08:24:28PM -0400, Andrea Arcangeli wrote: > This is a corollary of ced108037c2aa542b3ed8b7afd1576064ad1362a, > 58ceeb6bec86d9140f9d91d71a710e963523d063, > 5b7abeae3af8c08c577e599dd0578b9e3ee6687b. > > When the above three fixes where posted Dave asked > https://lkml.kernel.org/r/929b3844-aec2-0111-fef7-8002f9d4e2b9@intel.com > but apparently this was missed. > > The pmdp_clear_flush* in migrate_misplaced_transhuge_page was > introduced in commit a54a407fbf7735fd8f7841375574f5d9b0375f93. > > The important part of such commit is only the part where the page lock > is not released until the first do_huge_pmd_numa_page() finished > disarming the pagenuma/protnone. > > The addition of pmdp_clear_flush() wasn't beneficial to such commit > and there's no commentary about such an addition either. > > I guess the pmdp_clear_flush() in such commit was added just in case for > safety, but it ended up introducing the MADV_DONTNEED race condition > found by Aaron. > > At that point in time nobody thought of such kind of MADV_DONTNEED > race conditions yet (they were fixed later) so the code may have > looked more robust by adding the pmdp_clear_flush(). > > This specific race condition won't destabilize the kernel, but it can > confuse userland because after MADV_DONTNEED the memory won't be > zeroed out. > > This also optimizes the code and removes a superflous TLB flush. > > Reported-by: Aaron Tomlin <atomlin@redhat.com> > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com> Acked-by: Mel Gorman <mgorman@suse.de>
On Fri, Oct 12, 2018 at 08:24:28PM -0400, Andrea Arcangeli wrote: > This is a corollary of ced108037c2aa542b3ed8b7afd1576064ad1362a, > 58ceeb6bec86d9140f9d91d71a710e963523d063, > 5b7abeae3af8c08c577e599dd0578b9e3ee6687b. > > When the above three fixes where posted Dave asked > https://lkml.kernel.org/r/929b3844-aec2-0111-fef7-8002f9d4e2b9@intel.com > but apparently this was missed. > > The pmdp_clear_flush* in migrate_misplaced_transhuge_page was > introduced in commit a54a407fbf7735fd8f7841375574f5d9b0375f93. > > The important part of such commit is only the part where the page lock > is not released until the first do_huge_pmd_numa_page() finished > disarming the pagenuma/protnone. > > The addition of pmdp_clear_flush() wasn't beneficial to such commit > and there's no commentary about such an addition either. > > I guess the pmdp_clear_flush() in such commit was added just in case for > safety, but it ended up introducing the MADV_DONTNEED race condition > found by Aaron. > > At that point in time nobody thought of such kind of MADV_DONTNEED > race conditions yet (they were fixed later) so the code may have > looked more robust by adding the pmdp_clear_flush(). > > This specific race condition won't destabilize the kernel, but it can > confuse userland because after MADV_DONTNEED the memory won't be > zeroed out. > > This also optimizes the code and removes a superflous TLB flush. > > Reported-by: Aaron Tomlin <atomlin@redhat.com> > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
diff --git a/mm/migrate.c b/mm/migrate.c index d6a2e89b086a..180e3d0ed16d 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -2082,15 +2082,27 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm, entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma); /* - * Clear the old entry under pagetable lock and establish the new PTE. - * Any parallel GUP will either observe the old page blocking on the - * page lock, block on the page table lock or observe the new page. - * The SetPageUptodate on the new page and page_add_new_anon_rmap - * guarantee the copy is visible before the pagetable update. + * Overwrite the old entry under pagetable lock and establish + * the new PTE. Any parallel GUP will either observe the old + * page blocking on the page lock, block on the page table + * lock or observe the new page. The SetPageUptodate on the + * new page and page_add_new_anon_rmap guarantee the copy is + * visible before the pagetable update. */ flush_cache_range(vma, mmun_start, mmun_end); page_add_anon_rmap(new_page, vma, mmun_start, true); - pmdp_huge_clear_flush_notify(vma, mmun_start, pmd); + /* + * At this point the pmd is numa/protnone (i.e. non present) + * and the TLB has already been flushed globally. So no TLB + * can be currently caching this non present pmd mapping. + * There's no need of clearing the pmd before doing + * set_pmd_at(), nor to flush the TLB after + * set_pmd_at(). Clearing the pmd here would introduce a race + * condition against MADV_DONTNEED, beacuse MADV_DONTNEED only + * holds the mmap_sem for reading. If the pmd is set to NULL + * at any given time, MADV_DONTNEED won't wait on the pmd lock + * and it'll skip clearing this pmd. + */ set_pmd_at(mm, mmun_start, pmd, entry); update_mmu_cache_pmd(vma, address, &entry); @@ -2104,7 +2116,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm, * No need to double call mmu_notifier->invalidate_range() callback as * the above pmdp_huge_clear_flush_notify() did already call it. */ - mmu_notifier_invalidate_range_only_end(mm, mmun_start, mmun_end); + mmu_notifier_invalidate_range_end(mm, mmun_start, mmun_end); /* Take an "isolate" reference and put new page on the LRU. */ get_page(new_page);
This is a corollary of ced108037c2aa542b3ed8b7afd1576064ad1362a, 58ceeb6bec86d9140f9d91d71a710e963523d063, 5b7abeae3af8c08c577e599dd0578b9e3ee6687b. When the above three fixes where posted Dave asked https://lkml.kernel.org/r/929b3844-aec2-0111-fef7-8002f9d4e2b9@intel.com but apparently this was missed. The pmdp_clear_flush* in migrate_misplaced_transhuge_page was introduced in commit a54a407fbf7735fd8f7841375574f5d9b0375f93. The important part of such commit is only the part where the page lock is not released until the first do_huge_pmd_numa_page() finished disarming the pagenuma/protnone. The addition of pmdp_clear_flush() wasn't beneficial to such commit and there's no commentary about such an addition either. I guess the pmdp_clear_flush() in such commit was added just in case for safety, but it ended up introducing the MADV_DONTNEED race condition found by Aaron. At that point in time nobody thought of such kind of MADV_DONTNEED race conditions yet (they were fixed later) so the code may have looked more robust by adding the pmdp_clear_flush(). This specific race condition won't destabilize the kernel, but it can confuse userland because after MADV_DONTNEED the memory won't be zeroed out. This also optimizes the code and removes a superflous TLB flush. Reported-by: Aaron Tomlin <atomlin@redhat.com> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com> --- mm/migrate.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)