Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx> writes:
> When running stress-ng testing, we found below kernel crash after a few hours:
>
> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> pc : dentry_name+0xd8/0x224
> lr : pointer+0x22c/0x370
> sp : ffff800025f134c0
> ......
> Call trace:
> dentry_name+0xd8/0x224
> pointer+0x22c/0x370
> vsnprintf+0x1ec/0x730
> vscnprintf+0x2c/0x60
> vprintk_store+0x70/0x234
> vprintk_emit+0xe0/0x24c
> vprintk_default+0x3c/0x44
> vprintk_func+0x84/0x2d0
> printk+0x64/0x88
> __dump_page+0x52c/0x530
> dump_page+0x14/0x20
> set_migratetype_isolate+0x110/0x224
> start_isolate_page_range+0xc4/0x20c
> offline_pages+0x124/0x474
> memory_block_offline+0x44/0xf4
> memory_subsys_offline+0x3c/0x70
> device_offline+0xf0/0x120
> ......
>
> After analyzing the vmcore, I found this issue is caused by page migration.
> The scenario is that, one thread is doing page migration, and we will use the
> target page's ->mapping field to save 'anon_vma' pointer between page unmap and
> page move, and now the target page is locked and refcount is 1.
>
> Currently, there is another stress-ng thread performing memory hotplug,
> attempting to offline the target page that is being migrated. It discovers that
> the refcount of this target page is 1, preventing the offline operation, thus
> proceeding to dump the page. However, page_mapping() of the target page may
> return an incorrect file mapping to crash the system in dump_mapping(), since
> the target page->mapping only saves 'anon_vma' pointer without setting
> PAGE_MAPPING_ANON flag.
>
> There are seveval ways to fix this issue:
> (1) Setting the PAGE_MAPPING_ANON flag for target page's ->mapping when saving
> 'anon_vma', but this can confuse PageAnon() for PFN walkers, since the target
> page has not built mappings yet.
> (2) Getting the page lock to call page_mapping() in __dump_page() to avoid crashing
> the system, however, there are still some PFN walkers that call page_mapping()
> without holding the page lock, such as compaction.
> (3) Using target page->private field to save the 'anon_vma' pointer and 2 bits
> page state, just as page->mapping records an anonymous page, which can remove
> the page_mapping() impact for PFN walkers and also seems a simple way.
>
> So I choose option 3 to fix this issue, and this can also fix other potential
> issues for PFN walkers, such as compaction.
>
> Fixes: 64c8902ed441 ("migrate_pages: split unmap_and_move() to _unmap() and _move()")
> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
Good catch! Thanks!
Reviewed-by: "Huang, Ying" <ying.huang@xxxxxxxxx>
> ---
> mm/migrate.c | 27 ++++++++++-----------------
> 1 file changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 397f2a6e34cb..bad3039d165e 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1025,38 +1025,31 @@ static int move_to_new_folio(struct folio *dst, struct folio *src,
> }
>
> /*
> - * To record some information during migration, we use some unused
> - * fields (mapping and private) of struct folio of the newly allocated
> - * destination folio. This is safe because nobody is using them
> - * except us.
> + * To record some information during migration, we use unused private
> + * field of struct folio of the newly allocated destination folio.
> + * This is safe because nobody is using it except us.
> */
> -union migration_ptr {
> - struct anon_vma *anon_vma;
> - struct address_space *mapping;
> -};
> -
> enum {
> PAGE_WAS_MAPPED = BIT(0),
> PAGE_WAS_MLOCKED = BIT(1),
> + PAGE_OLD_STATES = PAGE_WAS_MAPPED | PAGE_WAS_MLOCKED,
> };
>
> static void __migrate_folio_record(struct folio *dst,
> - unsigned long old_page_state,
> + int old_page_state,
> struct anon_vma *anon_vma)
> {
> - union migration_ptr ptr = { .anon_vma = anon_vma };
> - dst->mapping = ptr.mapping;
> - dst->private = (void *)old_page_state;
> + dst->private = (void *)anon_vma + old_page_state;
> }
>
> static void __migrate_folio_extract(struct folio *dst,
> int *old_page_state,
> struct anon_vma **anon_vmap)
> {
> - union migration_ptr ptr = { .mapping = dst->mapping };
> - *anon_vmap = ptr.anon_vma;
> - *old_page_state = (unsigned long)dst->private;
> - dst->mapping = NULL;
> + unsigned long private = (unsigned long)dst->private;
> +
> + *anon_vmap = (struct anon_vma *)(private & ~PAGE_OLD_STATES);
> + *old_page_state = private & PAGE_OLD_STATES;
> dst->private = NULL;
> }
