Re: [PATCH] hugetlbfs: fix anon huge page migration race

From: HORIGUCHI NAOYA(堀口 直也)
Date: Fri Nov 13 2020 - 00:34:44 EST


On Thu, Nov 05, 2020 at 11:50:58AM -0800, Mike Kravetz wrote:
> Qian Cai reported the following BUG in [1]
>
> [ 6147.019063][T45242] LTP: starting move_pages12
> [ 6147.475680][T64921] BUG: unable to handle page fault for address: ffffffffffffffe0
> ...
> [ 6147.525866][T64921] RIP: 0010:anon_vma_interval_tree_iter_first+0xa2/0x170
> avc_start_pgoff at mm/interval_tree.c:63
> [ 6147.620914][T64921] Call Trace:
> [ 6147.624078][T64921] rmap_walk_anon+0x141/0xa30
> rmap_walk_anon at mm/rmap.c:1864
> [ 6147.628639][T64921] try_to_unmap+0x209/0x2d0
> try_to_unmap at mm/rmap.c:1763
> [ 6147.633026][T64921] ? rmap_walk_locked+0x140/0x140
> [ 6147.637936][T64921] ? page_remove_rmap+0x1190/0x1190
> [ 6147.643020][T64921] ? page_not_mapped+0x10/0x10
> [ 6147.647668][T64921] ? page_get_anon_vma+0x290/0x290
> [ 6147.652664][T64921] ? page_mapcount_is_zero+0x10/0x10
> [ 6147.657838][T64921] ? hugetlb_page_mapping_lock_write+0x97/0x180
> [ 6147.663972][T64921] migrate_pages+0x1005/0x1fb0
> [ 6147.668617][T64921] ? remove_migration_pte+0xac0/0xac0
> [ 6147.673875][T64921] move_pages_and_store_status.isra.47+0xd7/0x1a0
> [ 6147.680181][T64921] ? migrate_pages+0x1fb0/0x1fb0
> [ 6147.685002][T64921] __x64_sys_move_pages+0xa5c/0x1100
> [ 6147.690176][T64921] ? trace_hardirqs_on+0x20/0x1b5
> [ 6147.695084][T64921] ? move_pages_and_store_status.isra.47+0x1a0/0x1a0
> [ 6147.701653][T64921] ? rcu_read_lock_sched_held+0xaa/0xd0
> [ 6147.707088][T64921] ? switch_fpu_return+0x196/0x400
> [ 6147.712083][T64921] ? lockdep_hardirqs_on_prepare+0x38c/0x550
> [ 6147.717954][T64921] ? do_syscall_64+0x24/0x310
> [ 6147.722513][T64921] do_syscall_64+0x5f/0x310
> [ 6147.726897][T64921] ? trace_hardirqs_off+0x12/0x1a0
> [ 6147.731894][T64921] ? asm_exc_page_fault+0x8/0x30
> [ 6147.736714][T64921] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Hugh Dickens diagnosed this as a migration bug caused by code introduced
> to use i_mmap_rwsem for pmd sharing synchronization. Specifically, the
> routine unmap_and_move_huge_page() is always passing the TTU_RMAP_LOCKED
> flag to try_to_unmap() while holding i_mmap_rwsem. This is wrong for
> anon pages as the anon_vma_lock should be held in this case. Further
> analysis suggested that i_mmap_rwsem was not required to he held at all
> when calling try_to_unmap for anon pages as an anon page could never be
> part of a shared pmd mapping.
>
> Discussion also revealed that the hack in hugetlb_page_mapping_lock_write
> to drop page lock and acquire i_mmap_rwsem is wrong. There is no way to
> keep mapping valid while dropping page lock.
>
> This patch does the following:
> - Do not take i_mmap_rwsem and set TTU_RMAP_LOCKED for anon pages when
> calling try_to_unmap.
> - Remove the hacky code in hugetlb_page_mapping_lock_write. The routine
> will now simply do a 'trylock' while still holding the page lock. If
> the trylock fails, it will return NULL. This could impact the callers:
> - migration calling code will receive -EAGAIN and retry up to the hard
> coded limit (10).
> - memory error code will treat the page as BUSY. This will force
> killing (SIGKILL) instead of SIGBUS any mapping tasks.
> Do note that this change in behavior only happens when there is a race.
> None of the standard kernel testing suites actually hit this race, but
> it is possible.
>
> [1] https://lore.kernel.org/lkml/20200708012044.GC992@xxxxxx/
> [2] https://lore.kernel.org/linux-mm/alpine.LSU.2.11.2010071833100.2214@eggly.anvils/
>
> Reported-by: Qian Cai <cai@xxxxxx>
> Suggested-by: Hugh Dickins <hughd@xxxxxxxxxx>
> Fixes: c0d0381ade79 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing synchronization")
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>

This approch looks simpler and better than former ones.
Thank you for the update.

Acked-by: Naoya Horiguchi <naoya.horiguchi@xxxxxxx>