Commit c6cf24ba30c7225667827245cfd2bc98f7f5ed2b

Authored by Andrea Arcangeli
Committed by Greg Kroah-Hartman
1 parent 273fb194e8

mm: thp: fix pmd_bad() triggering in code paths holding mmap_sem read mode

commit 1a5a9906d4e8d1976b701f889d8f35d54b928f25 upstream.

In some cases it may happen that pmd_none_or_clear_bad() is called with
the mmap_sem hold in read mode.  In those cases the huge page faults can
allocate hugepmds under pmd_none_or_clear_bad() and that can trigger a
false positive from pmd_bad() that will not like to see a pmd
materializing as trans huge.

It's not khugepaged causing the problem, khugepaged holds the mmap_sem
in write mode (and all those sites must hold the mmap_sem in read mode
to prevent pagetables to go away from under them, during code review it
seems vm86 mode on 32bit kernels requires that too unless it's
restricted to 1 thread per process or UP builds).  The race is only with
the huge pagefaults that can convert a pmd_none() into a
pmd_trans_huge().

Effectively all these pmd_none_or_clear_bad() sites running with
mmap_sem in read mode are somewhat speculative with the page faults, and
the result is always undefined when they run simultaneously.  This is
probably why it wasn't common to run into this.  For example if the
madvise(MADV_DONTNEED) runs zap_page_range() shortly before the page
fault, the hugepage will not be zapped, if the page fault runs first it
will be zapped.

Altering pmd_bad() not to error out if it finds hugepmds won't be enough
to fix this, because zap_pmd_range would then proceed to call
zap_pte_range (which would be incorrect if the pmd become a
pmd_trans_huge()).

The simplest way to fix this is to read the pmd in the local stack
(regardless of what we read, no need of actual CPU barriers, only
compiler barrier needed), and be sure it is not changing under the code
that computes its value.  Even if the real pmd is changing under the
value we hold on the stack, we don't care.  If we actually end up in
zap_pte_range it means the pmd was not none already and it was not huge,
and it can't become huge from under us (khugepaged locking explained
above).

All we need is to enforce that there is no way anymore that in a code
path like below, pmd_trans_huge can be false, but pmd_none_or_clear_bad
can run into a hugepmd.  The overhead of a barrier() is just a compiler
tweak and should not be measurable (I only added it for THP builds).  I
don't exclude different compiler versions may have prevented the race
too by caching the value of *pmd on the stack (that hasn't been
verified, but it wouldn't be impossible considering
pmd_none_or_clear_bad, pmd_bad, pmd_trans_huge, pmd_none are all inlines
and there's no external function called in between pmd_trans_huge and
pmd_none_or_clear_bad).

		if (pmd_trans_huge(*pmd)) {
			if (next-addr != HPAGE_PMD_SIZE) {
				VM_BUG_ON(!rwsem_is_locked(&tlb->mm->mmap_sem));
				split_huge_page_pmd(vma->vm_mm, pmd);
			} else if (zap_huge_pmd(tlb, vma, pmd, addr))
				continue;
			/* fall through */
		}
		if (pmd_none_or_clear_bad(pmd))

Because this race condition could be exercised without special
privileges this was reported in CVE-2012-1179.

The race was identified and fully explained by Ulrich who debugged it.
I'm quoting his accurate explanation below, for reference.

====== start quote =======
      mapcount 0 page_mapcount 1
      kernel BUG at mm/huge_memory.c:1384!

    At some point prior to the panic, a "bad pmd ..." message similar to the
    following is logged on the console:

      mm/memory.c:145: bad pmd ffff8800376e1f98(80000000314000e7).

    The "bad pmd ..." message is logged by pmd_clear_bad() before it clears
    the page's PMD table entry.

        143 void pmd_clear_bad(pmd_t *pmd)
        144 {
    ->  145         pmd_ERROR(*pmd);
        146         pmd_clear(pmd);
        147 }

    After the PMD table entry has been cleared, there is an inconsistency
    between the actual number of PMD table entries that are mapping the page
    and the page's map count (_mapcount field in struct page). When the page
    is subsequently reclaimed, __split_huge_page() detects this inconsistency.

       1381         if (mapcount != page_mapcount(page))
       1382                 printk(KERN_ERR "mapcount %d page_mapcount %d\n",
       1383                        mapcount, page_mapcount(page));
    -> 1384         BUG_ON(mapcount != page_mapcount(page));

    The root cause of the problem is a race of two threads in a multithreaded
    process. Thread B incurs a page fault on a virtual address that has never
    been accessed (PMD entry is zero) while Thread A is executing an madvise()
    system call on a virtual address within the same 2 MB (huge page) range.

               virtual address space
              .---------------------.
              |                     |
              |                     |
            .-|---------------------|
            | |                     |
            | |                     |<-- B(fault)
            | |                     |
      2 MB  | |/////////////////////|-.
      huge <  |/////////////////////|  > A(range)
      page  | |/////////////////////|-'
            | |                     |
            | |                     |
            '-|---------------------|
              |                     |
              |                     |
              '---------------------'

    - Thread A is executing an madvise(..., MADV_DONTNEED) system call
      on the virtual address range "A(range)" shown in the picture.

    sys_madvise
      // Acquire the semaphore in shared mode.
      down_read(&current->mm->mmap_sem)
      ...
      madvise_vma
        switch (behavior)
        case MADV_DONTNEED:
             madvise_dontneed
               zap_page_range
                 unmap_vmas
                   unmap_page_range
                     zap_pud_range
                       zap_pmd_range
                         //
                         // Assume that this huge page has never been accessed.
                         // I.e. content of the PMD entry is zero (not mapped).
                         //
                         if (pmd_trans_huge(*pmd)) {
                             // We don't get here due to the above assumption.
                         }
                         //
                         // Assume that Thread B incurred a page fault and
             .---------> // sneaks in here as shown below.
             |           //
             |           if (pmd_none_or_clear_bad(pmd))
             |               {
             |                 if (unlikely(pmd_bad(*pmd)))
             |                     pmd_clear_bad
             |                     {
             |                       pmd_ERROR
             |                         // Log "bad pmd ..." message here.
             |                       pmd_clear
             |                         // Clear the page's PMD entry.
             |                         // Thread B incremented the map count
             |                         // in page_add_new_anon_rmap(), but
             |                         // now the page is no longer mapped
             |                         // by a PMD entry (-> inconsistency).
             |                     }
             |               }
             |
             v
    - Thread B is handling a page fault on virtual address "B(fault)" shown
      in the picture.

    ...
    do_page_fault
      __do_page_fault
        // Acquire the semaphore in shared mode.
        down_read_trylock(&mm->mmap_sem)
        ...
        handle_mm_fault
          if (pmd_none(*pmd) && transparent_hugepage_enabled(vma))
              // We get here due to the above assumption (PMD entry is zero).
              do_huge_pmd_anonymous_page
                alloc_hugepage_vma
                  // Allocate a new transparent huge page here.
                ...
                __do_huge_pmd_anonymous_page
                  ...
                  spin_lock(&mm->page_table_lock)
                  ...
                  page_add_new_anon_rmap
                    // Here we increment the page's map count (starts at -1).
                    atomic_set(&page->_mapcount, 0)
                  set_pmd_at
                    // Here we set the page's PMD entry which will be cleared
                    // when Thread A calls pmd_clear_bad().
                  ...
                  spin_unlock(&mm->page_table_lock)

    The mmap_sem does not prevent the race because both threads are acquiring
    it in shared mode (down_read).  Thread B holds the page_table_lock while
    the page's map count and PMD table entry are updated.  However, Thread A
    does not synchronize on that lock.

====== end quote =======

[akpm@linux-foundation.org: checkpatch fixes]
Reported-by: Ulrich Obergfell <uobergfe@redhat.com>
Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Hugh Dickins <hughd@google.com>
Cc: Dave Jones <davej@redhat.com>
Acked-by: Larry Woodman <lwoodman@redhat.com>
Acked-by: Rik van Riel <riel@redhat.com>
Cc: Mark Salter <msalter@redhat.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Showing 9 changed files with 92 additions and 10 deletions Side-by-side Diff

arch/x86/kernel/vm86_32.c
... ... @@ -172,6 +172,7 @@
172 172 spinlock_t *ptl;
173 173 int i;
174 174  
  175 + down_write(&mm->mmap_sem);
175 176 pgd = pgd_offset(mm, 0xA0000);
176 177 if (pgd_none_or_clear_bad(pgd))
177 178 goto out;
... ... @@ -190,6 +191,7 @@
190 191 }
191 192 pte_unmap_unlock(pte, ptl);
192 193 out:
  194 + up_write(&mm->mmap_sem);
193 195 flush_tlb();
194 196 }
195 197  
... ... @@ -409,6 +409,9 @@
409 409 } else {
410 410 spin_unlock(&walk->mm->page_table_lock);
411 411 }
  412 +
  413 + if (pmd_trans_unstable(pmd))
  414 + return 0;
412 415 /*
413 416 * The mmap_sem held all the way back in m_start() is what
414 417 * keeps khugepaged out of here and from collapsing things
... ... @@ -507,6 +510,8 @@
507 510 struct page *page;
508 511  
509 512 split_huge_page_pmd(walk->mm, pmd);
  513 + if (pmd_trans_unstable(pmd))
  514 + return 0;
510 515  
511 516 pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
512 517 for (; addr != end; pte++, addr += PAGE_SIZE) {
... ... @@ -670,6 +675,8 @@
670 675 int err = 0;
671 676  
672 677 split_huge_page_pmd(walk->mm, pmd);
  678 + if (pmd_trans_unstable(pmd))
  679 + return 0;
673 680  
674 681 /* find the first VMA at or above 'addr' */
675 682 vma = find_vma(walk->mm, addr);
... ... @@ -961,6 +968,8 @@
961 968 spin_unlock(&walk->mm->page_table_lock);
962 969 }
963 970  
  971 + if (pmd_trans_unstable(pmd))
  972 + return 0;
964 973 orig_pte = pte = pte_offset_map_lock(walk->mm, pmd, addr, &ptl);
965 974 do {
966 975 struct page *page = can_gather_numa_stats(*pte, md->vma, addr);
include/asm-generic/pgtable.h
... ... @@ -425,6 +425,8 @@
425 425 unsigned long size);
426 426 #endif
427 427  
  428 +#ifdef CONFIG_MMU
  429 +
428 430 #ifndef CONFIG_TRANSPARENT_HUGEPAGE
429 431 static inline int pmd_trans_huge(pmd_t pmd)
430 432 {
431 433  
... ... @@ -441,7 +443,66 @@
441 443 return 0;
442 444 }
443 445 #endif /* __HAVE_ARCH_PMD_WRITE */
  446 +#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
  447 +
  448 +/*
  449 + * This function is meant to be used by sites walking pagetables with
  450 + * the mmap_sem hold in read mode to protect against MADV_DONTNEED and
  451 + * transhuge page faults. MADV_DONTNEED can convert a transhuge pmd
  452 + * into a null pmd and the transhuge page fault can convert a null pmd
  453 + * into an hugepmd or into a regular pmd (if the hugepage allocation
  454 + * fails). While holding the mmap_sem in read mode the pmd becomes
  455 + * stable and stops changing under us only if it's not null and not a
  456 + * transhuge pmd. When those races occurs and this function makes a
  457 + * difference vs the standard pmd_none_or_clear_bad, the result is
  458 + * undefined so behaving like if the pmd was none is safe (because it
  459 + * can return none anyway). The compiler level barrier() is critically
  460 + * important to compute the two checks atomically on the same pmdval.
  461 + */
  462 +static inline int pmd_none_or_trans_huge_or_clear_bad(pmd_t *pmd)
  463 +{
  464 + /* depend on compiler for an atomic pmd read */
  465 + pmd_t pmdval = *pmd;
  466 + /*
  467 + * The barrier will stabilize the pmdval in a register or on
  468 + * the stack so that it will stop changing under the code.
  469 + */
  470 +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
  471 + barrier();
444 472 #endif
  473 + if (pmd_none(pmdval))
  474 + return 1;
  475 + if (unlikely(pmd_bad(pmdval))) {
  476 + if (!pmd_trans_huge(pmdval))
  477 + pmd_clear_bad(pmd);
  478 + return 1;
  479 + }
  480 + return 0;
  481 +}
  482 +
  483 +/*
  484 + * This is a noop if Transparent Hugepage Support is not built into
  485 + * the kernel. Otherwise it is equivalent to
  486 + * pmd_none_or_trans_huge_or_clear_bad(), and shall only be called in
  487 + * places that already verified the pmd is not none and they want to
  488 + * walk ptes while holding the mmap sem in read mode (write mode don't
  489 + * need this). If THP is not enabled, the pmd can't go away under the
  490 + * code even if MADV_DONTNEED runs, but if THP is enabled we need to
  491 + * run a pmd_trans_unstable before walking the ptes after
  492 + * split_huge_page_pmd returns (because it may have run when the pmd
  493 + * become null, but then a page fault can map in a THP and not a
  494 + * regular page).
  495 + */
  496 +static inline int pmd_trans_unstable(pmd_t *pmd)
  497 +{
  498 +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
  499 + return pmd_none_or_trans_huge_or_clear_bad(pmd);
  500 +#else
  501 + return 0;
  502 +#endif
  503 +}
  504 +
  505 +#endif /* CONFIG_MMU */
445 506  
446 507 #endif /* !__ASSEMBLY__ */
447 508  
... ... @@ -5237,6 +5237,8 @@
5237 5237 spinlock_t *ptl;
5238 5238  
5239 5239 split_huge_page_pmd(walk->mm, pmd);
  5240 + if (pmd_trans_unstable(pmd))
  5241 + return 0;
5240 5242  
5241 5243 pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
5242 5244 for (; addr != end; pte++, addr += PAGE_SIZE)
... ... @@ -5398,6 +5400,8 @@
5398 5400 spinlock_t *ptl;
5399 5401  
5400 5402 split_huge_page_pmd(walk->mm, pmd);
  5403 + if (pmd_trans_unstable(pmd))
  5404 + return 0;
5401 5405 retry:
5402 5406 pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
5403 5407 for (; addr != end; addr += PAGE_SIZE) {
... ... @@ -1228,16 +1228,24 @@
1228 1228 do {
1229 1229 next = pmd_addr_end(addr, end);
1230 1230 if (pmd_trans_huge(*pmd)) {
1231   - if (next-addr != HPAGE_PMD_SIZE) {
  1231 + if (next - addr != HPAGE_PMD_SIZE) {
1232 1232 VM_BUG_ON(!rwsem_is_locked(&tlb->mm->mmap_sem));
1233 1233 split_huge_page_pmd(vma->vm_mm, pmd);
1234 1234 } else if (zap_huge_pmd(tlb, vma, pmd))
1235   - continue;
  1235 + goto next;
1236 1236 /* fall through */
1237 1237 }
1238   - if (pmd_none_or_clear_bad(pmd))
1239   - continue;
  1238 + /*
  1239 + * Here there can be other concurrent MADV_DONTNEED or
  1240 + * trans huge page faults running, and if the pmd is
  1241 + * none or trans huge it can change under us. This is
  1242 + * because MADV_DONTNEED holds the mmap_sem in read
  1243 + * mode.
  1244 + */
  1245 + if (pmd_none_or_trans_huge_or_clear_bad(pmd))
  1246 + goto next;
1240 1247 next = zap_pte_range(tlb, vma, pmd, addr, next, details);
  1248 +next:
1241 1249 cond_resched();
1242 1250 } while (pmd++, addr = next, addr != end);
1243 1251  
... ... @@ -512,7 +512,7 @@
512 512 do {
513 513 next = pmd_addr_end(addr, end);
514 514 split_huge_page_pmd(vma->vm_mm, pmd);
515   - if (pmd_none_or_clear_bad(pmd))
  515 + if (pmd_none_or_trans_huge_or_clear_bad(pmd))
516 516 continue;
517 517 if (check_pte_range(vma, pmd, addr, next, nodes,
518 518 flags, private))
... ... @@ -164,7 +164,7 @@
164 164 }
165 165 /* fall through */
166 166 }
167   - if (pmd_none_or_clear_bad(pmd))
  167 + if (pmd_none_or_trans_huge_or_clear_bad(pmd))
168 168 mincore_unmapped_range(vma, addr, next, vec);
169 169 else
170 170 mincore_pte_range(vma, pmd, addr, next, vec);
... ... @@ -59,7 +59,7 @@
59 59 continue;
60 60  
61 61 split_huge_page_pmd(walk->mm, pmd);
62   - if (pmd_none_or_clear_bad(pmd))
  62 + if (pmd_none_or_trans_huge_or_clear_bad(pmd))
63 63 goto again;
64 64 err = walk_pte_range(pmd, addr, next, walk);
65 65 if (err)
... ... @@ -931,9 +931,7 @@
931 931 pmd = pmd_offset(pud, addr);
932 932 do {
933 933 next = pmd_addr_end(addr, end);
934   - if (unlikely(pmd_trans_huge(*pmd)))
935   - continue;
936   - if (pmd_none_or_clear_bad(pmd))
  934 + if (pmd_none_or_trans_huge_or_clear_bad(pmd))
937 935 continue;
938 936 ret = unuse_pte_range(vma, pmd, addr, next, entry, page);
939 937 if (ret)