| Message ID | alpine.LSU.2.11.2008021215400.27773@eggly.anvils (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | khugepaged: retract_page_tables() remember to test exit | expand |
On Sun, Aug 02, 2020 at 12:16:53PM -0700, Hugh Dickins wrote: > Only once have I seen this scenario (and forgot even to notice what > forced the eventual crash): a sequence of "BUG: Bad page map" alerts > from vm_normal_page(), from zap_pte_range() servicing exit_mmap(); > pmd:00000000, pte values corresponding to data in physical page 0. > > The pte mappings being zapped in this case were supposed to be from a > huge page of ext4 text (but could as well have been shmem): my belief > is that it was racing with collapse_file()'s retract_page_tables(), > found *pmd pointing to a page table, locked it, but *pmd had become > 0 by the time start_pte was decided. > > In most cases, that possibility is excluded by holding mmap lock; > but exit_mmap() proceeds without mmap lock. Most of what's run by > khugepaged checks khugepaged_test_exit() after acquiring mmap lock: > khugepaged_collapse_pte_mapped_thps() and hugepage_vma_revalidate() > do so, for example. But retract_page_tables() did not: fix that > (using an mm variable instead of vma->vm_mm repeatedly). Hm. I'm not sure I follow. vma->vm_mm has to be valid as long as we hold i_mmap lock, no? Unlinking a VMA requires it.
On Mon, 3 Aug 2020, Kirill A. Shutemov wrote: > On Sun, Aug 02, 2020 at 12:16:53PM -0700, Hugh Dickins wrote: > > Only once have I seen this scenario (and forgot even to notice what > > forced the eventual crash): a sequence of "BUG: Bad page map" alerts > > from vm_normal_page(), from zap_pte_range() servicing exit_mmap(); > > pmd:00000000, pte values corresponding to data in physical page 0. > > > > The pte mappings being zapped in this case were supposed to be from a > > huge page of ext4 text (but could as well have been shmem): my belief > > is that it was racing with collapse_file()'s retract_page_tables(), > > found *pmd pointing to a page table, locked it, but *pmd had become > > 0 by the time start_pte was decided. > > > > In most cases, that possibility is excluded by holding mmap lock; > > but exit_mmap() proceeds without mmap lock. Most of what's run by > > khugepaged checks khugepaged_test_exit() after acquiring mmap lock: > > khugepaged_collapse_pte_mapped_thps() and hugepage_vma_revalidate() > > do so, for example. But retract_page_tables() did not: fix that > > (using an mm variable instead of vma->vm_mm repeatedly). > > Hm. I'm not sure I follow. vma->vm_mm has to be valid as long as we hold > i_mmap lock, no? Unlinking a VMA requires it. Ah, my wording is misleading, yes. That comment "(using an mm variable instead of vma->vm_mm repeatedly)" was nothing more than a note, that the patch is bigger than it could be, because I decided to use an mm variable, instead of vma->vm_mm repeatedly. But it looks as if I'm saying there used to be a need for READ_ONCE() or something, and by using the mm variable I was fixing the problem. No, sorry: delete that line now the point is made: the mm variable is just a patch detail, it's not important. The fix (as the subject suggested) is for retract_page_tables() to check khugepaged_test_exit(), after acquiring mmap lock, before doing anything to the page table. Getting the mmap lock serializes with __mmput(), which briefly takes and drops it in __khugepaged_exit(); then the khugepaged_test_exit() check on mm_users makes sure we don't touch the page table once exit_mmap() might reach it, since exit_mmap() will be proceeding without mmap lock, not expecting anyone to be racing with it. (I devised that protocol for ksmd, then Andrea adopted it for khugepaged: back then it was important for these daemons to have a hold on the mm, without an actual reference to mm_users, because that would prevent the OOM killer from reaching exit_mmap(). Nowadays with the OOM reaper, it's probably less crucial to avoid mm_users, but I think still worthwhile.) Thanks a lot for looking at these patches so quickly, Hugh
On Sun, Aug 02, 2020 at 05:35:23PM -0700, Hugh Dickins wrote: > On Mon, 3 Aug 2020, Kirill A. Shutemov wrote: > > On Sun, Aug 02, 2020 at 12:16:53PM -0700, Hugh Dickins wrote: > > > Only once have I seen this scenario (and forgot even to notice what > > > forced the eventual crash): a sequence of "BUG: Bad page map" alerts > > > from vm_normal_page(), from zap_pte_range() servicing exit_mmap(); > > > pmd:00000000, pte values corresponding to data in physical page 0. > > > > > > The pte mappings being zapped in this case were supposed to be from a > > > huge page of ext4 text (but could as well have been shmem): my belief > > > is that it was racing with collapse_file()'s retract_page_tables(), > > > found *pmd pointing to a page table, locked it, but *pmd had become > > > 0 by the time start_pte was decided. > > > > > > In most cases, that possibility is excluded by holding mmap lock; > > > but exit_mmap() proceeds without mmap lock. Most of what's run by > > > khugepaged checks khugepaged_test_exit() after acquiring mmap lock: > > > khugepaged_collapse_pte_mapped_thps() and hugepage_vma_revalidate() > > > do so, for example. But retract_page_tables() did not: fix that > > > (using an mm variable instead of vma->vm_mm repeatedly). > > > > Hm. I'm not sure I follow. vma->vm_mm has to be valid as long as we hold > > i_mmap lock, no? Unlinking a VMA requires it. > > Ah, my wording is misleading, yes. That comment > "(using an mm variable instead of vma->vm_mm repeatedly)" > was nothing more than a note, that the patch is bigger than it could be, > because I decided to use an mm variable, instead of vma->vm_mm repeatedly. > But it looks as if I'm saying there used to be a need for READ_ONCE() or > something, and by using the mm variable I was fixing the problem. > > No, sorry: delete that line now the point is made: the mm variable is > just a patch detail, it's not important. > > The fix (as the subject suggested) is for retract_page_tables() to check > khugepaged_test_exit(), after acquiring mmap lock, before doing anything > to the page table. Getting the mmap lock serializes with __mmput(), > which briefly takes and drops it in __khugepaged_exit(); then the > khugepaged_test_exit() check on mm_users makes sure we don't touch the > page table once exit_mmap() might reach it, since exit_mmap() will be > proceeding without mmap lock, not expecting anyone to be racing with it. Okay, makes sense. Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
--- 5.8-rc7/mm/khugepaged.c 2020-07-26 16:58:02.189038680 -0700 +++ linux/mm/khugepaged.c 2020-08-02 10:53:37.892660983 -0700 @@ -1538,6 +1538,7 @@ out: static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) { struct vm_area_struct *vma; + struct mm_struct *mm; unsigned long addr; pmd_t *pmd, _pmd; @@ -1566,7 +1567,8 @@ static void retract_page_tables(struct a continue; if (vma->vm_end < addr + HPAGE_PMD_SIZE) continue; - pmd = mm_find_pmd(vma->vm_mm, addr); + mm = vma->vm_mm; + pmd = mm_find_pmd(mm, addr); if (!pmd) continue; /* @@ -1576,17 +1578,19 @@ static void retract_page_tables(struct a * mmap_lock while holding page lock. Fault path does it in * reverse order. Trylock is a way to avoid deadlock. */ - if (mmap_write_trylock(vma->vm_mm)) { - spinlock_t *ptl = pmd_lock(vma->vm_mm, pmd); - /* assume page table is clear */ - _pmd = pmdp_collapse_flush(vma, addr, pmd); - spin_unlock(ptl); - mmap_write_unlock(vma->vm_mm); - mm_dec_nr_ptes(vma->vm_mm); - pte_free(vma->vm_mm, pmd_pgtable(_pmd)); + if (mmap_write_trylock(mm)) { + if (!khugepaged_test_exit(mm)) { + spinlock_t *ptl = pmd_lock(mm, pmd); + /* assume page table is clear */ + _pmd = pmdp_collapse_flush(vma, addr, pmd); + spin_unlock(ptl); + mm_dec_nr_ptes(mm); + pte_free(mm, pmd_pgtable(_pmd)); + } + mmap_write_unlock(mm); } else { /* Try again later */ - khugepaged_add_pte_mapped_thp(vma->vm_mm, addr); + khugepaged_add_pte_mapped_thp(mm, addr); } } i_mmap_unlock_write(mapping);
Only once have I seen this scenario (and forgot even to notice what forced the eventual crash): a sequence of "BUG: Bad page map" alerts from vm_normal_page(), from zap_pte_range() servicing exit_mmap(); pmd:00000000, pte values corresponding to data in physical page 0. The pte mappings being zapped in this case were supposed to be from a huge page of ext4 text (but could as well have been shmem): my belief is that it was racing with collapse_file()'s retract_page_tables(), found *pmd pointing to a page table, locked it, but *pmd had become 0 by the time start_pte was decided. In most cases, that possibility is excluded by holding mmap lock; but exit_mmap() proceeds without mmap lock. Most of what's run by khugepaged checks khugepaged_test_exit() after acquiring mmap lock: khugepaged_collapse_pte_mapped_thps() and hugepage_vma_revalidate() do so, for example. But retract_page_tables() did not: fix that (using an mm variable instead of vma->vm_mm repeatedly). Fixes: f3f0e1d2150b ("khugepaged: add support of collapse for tmpfs/shmem pages") Signed-off-by: Hugh Dickins <hughd@google.com> Cc: stable@vger.kernel.org # v4.8+ --- mm/khugepaged.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)