Re: [PATCH] mm/vmscan: fix hwpoisoned large folio handling in shrink_folio_list

[David Hildenbrand <david@redhat.com>]

On 11.06.25 11:20, David Hildenbrand wrote:
> On 11.06.25 11:00, Jinjiang Tu wrote:
>>
>> 在 2025/6/11 16:35, David Hildenbrand 写道:
>>> On 11.06.25 10:29, Jinjiang Tu wrote:
>>>>
>>>> 在 2025/6/11 15:59, David Hildenbrand 写道:
>>>>> On 11.06.25 09:46, Jinjiang Tu wrote:
>>>>>> In shrink_folio_list(), the hwpoisoned folio may be large folio, which
>>>>>> can't be handled by unmap_poisoned_folio().
>>>>>>
>>>>>> Since UCE is rare in real world, and race with reclaimation is more
>>>>>> rare,
>>>>>> just skipping the hwpoisoned large folio is enough. memory_failure()
>>>>>> will
>>>>>> handle it if the UCE is triggered again.
>>>>>>
>>>>>> Fixes: 1b0449544c64 ("mm/vmscan: don't try to reclaim hwpoison folio")
>>>>>
>>>>> Please also add
>>>>>
>>>>> Closes:
>>>>>
>>>>> with a link to the report
>>>> Thanks, I will add it.
>>>>>
>>>>>> Reported-by: syzbot+3b220254df55d8ca8a61@syzkaller.appspotmail.com
>>>>>> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
>>>>>> ---
>>>>>>      mm/vmscan.c | 8 ++++++++
>>>>>>      1 file changed, 8 insertions(+)
>>>>>> /home/tujinjiang/hulk-repo/hulk/mm/mempolicy.c
>>>>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>>>>> index b6f4db6c240f..3a4e8d7419ae 100644
>>>>>> --- a/mm/vmscan.c
>>>>>> +++ b/mm/vmscan.c
>>>>>> @@ -1131,6 +1131,14 @@ static unsigned int shrink_folio_list(struct
>>>>>> list_head *folio_list,
>>>>>>                  goto keep;
>>>>>>                if (folio_contain_hwpoisoned_page(folio)) {
>>>>>> +            /*
>>>>>> +             * unmap_poisoned_folio() can't handle large
>>>>>> +             * folio, just skip it. memory_failure() will
>>>>>> +             * handle it if the UCE is triggered again.
>>>>>> +             */
>>>>>> +            if (folio_test_large(folio))
>>>>>> +                goto keep_locked;
>>>>>> +
>>>>>>                  unmap_poisoned_folio(folio, folio_pfn(folio), false);
>>>>>>                  folio_unlock(folio);
>>>>>>                  folio_put(folio);
>>>>>
>>>>> Why not handle that in unmap_poisoned_folio() to make that limitation
>>>>> clear and avoid?
>>>> I tried to put the check in unmap_poisoned_folio(), but it still exists
>>>> other issues.
>>>
>>>
>>>
>>>> The calltrace in v6.6 kernel:
>>>>
>>>> Unable to handle kernel paging request at virtual address
>>>> fbd5200000000024
>>>> KASAN: maybe wild-memory-access in range
>>>> [0xdead000000000120-0xdead000000000127]
>>>> pc : __list_add_valid_or_report+0x50/0x158 lib/list_debug.c:32
>>>> lr : __list_add_valid include/linux/list.h:88 [inline]
>>>> lr : __list_add include/linux/list.h:150 [inline]
>>>> lr : list_add_tail include/linux/list.h:183 [inline]
>>>> lr : lru_add_page_tail.constprop.0+0x4ac/0x640 mm/huge_memory.c:3187
>>>> Call trace:
>>>>      __list_add_valid_or_report+0x50/0x158 lib/list_debug.c:32
>>>>      __list_add_valid include/linux/list.h:88 [inline]
>>>>      __list_add include/linux/list.h:150 [inline]
>>>>      list_add_tail include/linux/list.h:183 [inline]
>>>>      lru_add_page_tail.constprop.0+0x4ac/0x640 mm/huge_memory.c:3187
>>>>      __split_huge_page_tail.isra.0+0x344/0x508 mm/huge_memory.c:3286
>>>>      __split_huge_page+0x244/0x1270 mm/huge_memory.c:3317
>>>>      split_huge_page_to_list_to_order+0x1038/0x1620 mm/huge_memory.c:3625
>>>>      split_folio_to_list_to_order include/linux/huge_mm.h:638 [inline]
>>>>      split_folio_to_order include/linux/huge_mm.h:643 [inline]
>>>>      deferred_split_scan+0x5f8/0xb70 mm/huge_memory.c:3778
>>>>      do_shrink_slab+0x2a0/0x828 mm/vmscan.c:927
>>>>      shrink_slab_memcg+0x2c0/0x558 mm/vmscan.c:996
>>>>      shrink_slab+0x228/0x250 mm/vmscan.c:1075
>>>>      shrink_node_memcgs+0x34c/0x6a0 mm/vmscan.c:6630
>>>>      shrink_node+0x21c/0x1378 mm/vmscan.c:6664
>>>>      shrink_zones.constprop.0+0x24c/0xab0 mm/vmscan.c:6906
>>>>      do_try_to_free_pages+0x150/0x880 mm/vmscan.c:6968
>>>>
>>>>
>>>> The folio is deleted from lru and the folio->lru can't be accessed. If
>>>> the folio is splitted later,
>>>> lru_add_split_folio() assumes the folio is on lru.
>>>
>>> Not sure if something like the following would be appropriate:
>>>
>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>> index b91a33fb6c694..fdd58c8ba5254 100644
>>> --- a/mm/memory-failure.c
>>> +++ b/mm/memory-failure.c
>>> @@ -1566,6 +1566,9 @@ int unmap_poisoned_folio(struct folio *folio,
>>> unsigned long pfn, bool must_kill)
>>>           enum ttu_flags ttu = TTU_IGNORE_MLOCK | TTU_SYNC | TTU_HWPOISON;
>>>           struct address_space *mapping;
>>>
>>> +       if (folio_test_large && !folio_test_hugetlb(folio))
>>> +               return -EBUSY;
>>> +
>>>           if (folio_test_swapcache(folio)) {
>>>                   pr_err("%#lx: keeping poisoned page in swap cache\n",
>>> pfn);
>>>                   ttu &= ~TTU_HWPOISON;
>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>> index f8dfd2864bbf4..6a3426bc9e9d7 100644
>>> --- a/mm/vmscan.c
>>> +++ b/mm/vmscan.c
>>> @@ -1138,7 +1138,8 @@ static unsigned int shrink_folio_list(struct
>>> list_head *folio_list,
>>>                           goto keep;
>>>
>>>                   if (folio_contain_hwpoisoned_page(folio)) {
>>> -                       unmap_poisoned_folio(folio, folio_pfn(folio),
>>> false);
>>> +                       if (unmap_poisoned_folio(folio,
>>> folio_pfn(folio), false)){
>>> +                               list_add(&folio->lru, &ret_folios);
>>>                           folio_unlock(folio);
>>>                           folio_put(folio);
>>>                           continue;
>>
>> The expected behaviour is keeping the folio on lru if
>> unmap_poisoned_folio fails?
> 
> Good question, it's a mess.
> 
> If we keep the LRU bit cleared (kept isolated), we wouldn't have to add
> it to the list.
> 
> But now I wonder where deferred_split_scan() would check for the LRU flag?
> 
> It seems to trylock, to then call split_folio().
> 
> In __folio_split(), I don't find any checks for the lru flag ... :(
> 
> We call lru_add_split_folio() where we
> VM_BUG_ON_FOLIO(folio_test_lru(new_folio), folio);

Oh, that's for the new folio. So the checks in the other two paths apply.

When we come through deferred, we don't expect a "list" and consequently 
assume that it is still on the LRU.

	VM_WARN_ON(!folio_test_lru(folio));

So not adding it back to the LRU will be problematic for 
lru_add_split_folio() when called through deferred shrinking I guess ...

-- 
Cheers,

David / dhildenb