Re: [PATCH v2] mm/memory_hotplug: don't offline a hwpoisoned page still in the page cache

[David Hildenbrand (Arm)]

On 11/5/26 21:50, Mani Tofigh via B4 Relay wrote:
> From: Mani Tofigh <mt4022@xxxxxxxxxxxx>
> 
> Memory offline skips hwpoisoned pages, assuming nobody uses them
> anymore.  memory_failure() however sets PG_hwpoison before it takes the
> page, and taking it waits on pcp_batch_high_lock, which offline_pages()
> holds for the whole offline.  A memory error during the offline thus
> marks the page and then waits while the offline skips it:
> 
>   Task A (offline_pages)               Task B (memory_failure)
>   zone_pcp_disable()
>                                        TestSetPageHWPoison(P)
>                                        get_hwpoison_page()
>                                          /* waits for Task A */
>   do_migrate_range()
>     /* P is poisoned (not migrated) */
>   test_pages_isolated()
>     /* P is poisoned (accepted) */
>   zone_pcp_enable()
>                                        /* P is off the LRU (Ignored) */
> 
> The block goes offline while the file still points to P, and the next
> walk of the file's pages (here drop_caches on ext4) crashes on a struct
> page of offlined memory:
> 
>   [   11.710348] Memory failure: 0x24224: unhandlable page.
>   [   11.710372] Memory failure: 0x24224: recovery action for get hwpoison page: Ignored
>   [   11.765371] page:000000008c304eee is uninitialized and poisoned
>   [   11.765389] page dumped because: VM_BUG_ON_PAGE(page_ref_count(page) == 0)
>   [   11.765440] kernel BUG at include/linux/mm.h:1990!
>   [   11.765458] Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
>   [   11.772284] CPU: 3 UID: 0 PID: 85 Comm: mfoffline Tainted: G        W            7.3.0-17087-gd865f2677c44 #1 PREEMPT
>   [   11.777970] RIP: 0010:find_lock_entries+0x8ee/0xad0
>   [   11.798761] Call Trace:
>   [   11.799297]  <TASK>
>   [   11.803284]  mapping_try_invalidate+0xfd/0x2c0
>   [   11.810571]  drop_pagecache_sb+0x111/0x330
>   [   11.811572]  __iterate_supers+0x2e2/0x370
>   [   11.815863]  drop_caches_sysctl_handler+0x74/0xb0
>   [   11.816994]  proc_sys_call_handler+0x2ec/0x440
>   [   11.821360]  vfs_write+0x545/0x1250
>   [   11.825220]  ksys_write+0xfa/0x200
>   [   11.830202]  do_syscall_64+0xfb/0x590
>   [   11.832084]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
>   [   11.848423]  </TASK>
> 
> Once the memory is also removed it's a use-after-free of the memory map.
> tmpfs and the swap cache can end up the same way without the race.
> Since commit a76054266661 ("mm: shmem: don't truncate page if memory
> failure happens"), memory_failure() leaves a poisoned tmpfs folio in the
> page cache on purpose, and it does the same for a dirty folio in the
> swap cache.
> 
> Treat a hwpoisoned page whose folio is still in a page cache or swap
> cache as busy in scan_movable_pages() and
> __test_page_isolated_in_pageblock() so that the offline fails and a
> waiting memory_failure() can go on.  Also have do_migrate_range() leave
> such a folio alone instead of taking it off the LRU, so that
> memory_failure() can still remove it from the cache.  A later offline
> succeeds once the page is out of the cache.  With the fix, the offline
> fails with -EBUSY and the file doesn't point to offlined memory anymore.
> 
> This bug was identified and confirmed with the help of a deterministic
> hypervisor currently under development.  The C reproducer below however
> hits it within about 10s.
> 
> Fixes: b023f46813cd ("memory-hotplug: skip HWPoisoned page when offlining pages")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Mani Tofigh <mt4022@xxxxxxxxxxxx>
> ---
> Changes in v2:
> - Leave a hwpoisoned folio that is still in a page cache or swap cache
>   alone in do_migrate_range(), so that a waiting memory_failure() can
>   still remove it from the cache (Sashiko)
> - Rebase onto current mm-new
> - Link to v1: https://lore.kernel.org/all/20261101-memhp-hwpoison-pagecache-v1-1-ed8785ca44eb@xxxxxxxxxxxx/
> 
> A pinned hwpoisoned page still isn't covered.  I'll send the refcount
> check David suggested as a follow-up.

I didn't understand why just checking for the refcount is not sufficient. Can
you elaborate once more (and please not send a new revision before the
discussion is over).

Memory poisoning must grab a reference when setting the folio hwpoison. Any
other reference (pin, pagecache, swapcache, mapping) keeps it alive.

Why can't we build on and?


-- 
Cheers,

David