Re: [PATCH v2 2/2] mm/memory_hotplug: fix hwpoisoned large folio handling in do_migrate_range

[David Hildenbrand <david@redhat.com>]

On 27.06.25 14:57, Jinjiang Tu wrote:
> In do_migrate_range(), the hwpoisoned folio may be large folio, which
> can't be handled by unmap_poisoned_folio().
> 
> I can reproduce this issue in qemu after adding delay in memory_failure()
> 
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> Workqueue: kacpi_hotplug acpi_hotplug_work_fn
> RIP: 0010:try_to_unmap_one+0x16a/0xfc0
>   <TASK>
>   rmap_walk_anon+0xda/0x1f0
>   try_to_unmap+0x78/0x80
>   ? __pfx_try_to_unmap_one+0x10/0x10
>   ? __pfx_folio_not_mapped+0x10/0x10
>   ? __pfx_folio_lock_anon_vma_read+0x10/0x10
>   unmap_poisoned_folio+0x60/0x140
>   do_migrate_range+0x4d1/0x600
>   ? slab_memory_callback+0x6a/0x190
>   ? notifier_call_chain+0x56/0xb0
>   offline_pages+0x3e6/0x460
>   memory_subsys_offline+0x130/0x1f0
>   device_offline+0xba/0x110
>   acpi_bus_offline+0xb7/0x130
>   acpi_scan_hot_remove+0x77/0x290
>   acpi_device_hotplug+0x1e0/0x240
>   acpi_hotplug_work_fn+0x1a/0x30
>   process_one_work+0x186/0x340
> 
> In this case, just make offline_pages() fail.
> 
> Besides, do_migrate_range() may be called between memory_failure set
> hwposion flag and ioslate the folio from lru, so remove WARN_ON(). In other
> places, unmap_poisoned_folio() is called when the folio is isolated, obey
> it in do_migrate_range() too.
> 
> Fixes: b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned pages to be offlined")
> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
> ---
>   mm/memory_hotplug.c | 21 +++++++++++++--------
>   1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index b1caedbade5b..5fab212e3a2c 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1795,7 +1795,7 @@ static int scan_movable_pages(unsigned long start, unsigned long end,
>   	return 0;
>   }
>   
> -static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
> +static int do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>   {
>   	struct folio *folio;
>   	unsigned long pfn;
> @@ -1819,8 +1819,10 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>   			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>   
>   		if (folio_contain_hwpoisoned_page(folio)) {
> -			if (WARN_ON(folio_test_lru(folio)))
> -				folio_isolate_lru(folio);
> +			if (folio_test_large(folio) && !folio_test_hugetlb(folio))
> +				goto err_out;
> +			if (folio_test_lru(folio) && !folio_isolate_lru(folio))
> +				goto err_out;
>   			if (folio_mapped(folio)) {
>   				folio_lock(folio);
>   				unmap_poisoned_folio(folio, pfn, false);
> @@ -1877,6 +1879,11 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>   			putback_movable_pages(&source);
>   		}
>   	}
> +	return 0;
> +err_out:
> +	folio_put(folio);
> +	putback_movable_pages(&source);
> +	return -EBUSY;
>   }
 >   >   static int __init cmdline_parse_movable_node(char *p)
> @@ -2041,11 +2048,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>   
>   			ret = scan_movable_pages(pfn, end_pfn, &pfn);
>   			if (!ret) {
> -				/*
> -				 * TODO: fatal migration failures should bail
> -				 * out
> -				 */
> -				do_migrate_range(pfn, end_pfn);
> +				ret = do_migrate_range(pfn, end_pfn);
> +				if (ret)
> +					break;
>   			}
>   		} while (!ret);
>   


I think this could simply be

...
	ret = scan_movable_pages(pfn, end_pfn, &pfn);
	if (!ret)
		ret = do_migrate_range(pfn, end_pfn);
} while (!ret);

because the while (!ret) will take care of the "break".

-- 
Cheers,

David / dhildenb