Skip to content
  • Andrea Arcangeli's avatar
    mm: thp: fix pmd_bad() triggering in code paths holding mmap_sem read mode · 5a3e1f55
    Andrea Arcangeli authored
    commit 1a5a9906
    
     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: default avatarUlrich Obergfell <uobergfe@redhat.com>
    Signed-off-by: default avatarAndrea Arcangeli <aarcange@redhat.com>
    Acked-by: default avatarJohannes Weiner <hannes@cmpxchg.org>
    Cc: Mel Gorman <mgorman@suse.de>
    Cc: Hugh Dickins <hughd@google.com>
    Cc: Dave Jones <davej@redhat.com>
    Acked-by: default avatarLarry Woodman <lwoodman@redhat.com>
    Acked-by: default avatarRik van Riel <riel@redhat.com>
    Cc: Mark Salter <msalter@redhat.com>
    Signed-off-by: default avatarAndrew Morton <akpm@linux-foundation.org>
    Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
    Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
    5a3e1f55