Re: [PATCH] mm/khugepaged: Fix race with folio splitting in hpage_collapse_scan_file()

[Shivank Garg <shivankg@amd.com>]

On 5/22/2025 5:29 PM, David Hildenbrand wrote:
> On 22.05.25 12:01, Baolin Wang wrote:
>>
>>
>> On 2025/5/22 17:34, Shivank Garg wrote:
>>> folio_mapcount() checks folio_test_large() before proceeding to
>>> folio_large_mapcount(), but there exists a race window where a folio
>>> could be split between these checks which triggered the
>>> VM_WARN_ON_FOLIO(!folio_test_large(folio), folio) in
>>> folio_large_mapcount().
>>>
>>> Take a temporary folio reference in hpage_collapse_scan_file() to prevent
>>> races with concurrent folio splitting/freeing. This prevent potential
>>> incorrect large folio detection.
>>>
>>> Reported-by: syzbot+2b99589e33edbe9475ca@syzkaller.appspotmail.com
>>> Closes: https://lore.kernel.org/all/6828470d.a70a0220.38f255.000c.GAE@google.com
>>> Fixes: 05c5323b2a34 ("mm: track mapcount of large folios in single value")
>>> Suggested-by: David Hildenbrand <david@redhat.com>
>>> Signed-off-by: Shivank Garg <shivankg@amd.com>
>>> ---
>>>    mm/khugepaged.c | 16 ++++++++++++++++
>>>    1 file changed, 16 insertions(+)
>>>
>>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>>> index cc945c6ab3bd..6e8902f9d88c 100644
>>> --- a/mm/khugepaged.c
>>> +++ b/mm/khugepaged.c
>>> @@ -2295,6 +2295,17 @@ static int hpage_collapse_scan_file(struct mm_struct *mm, unsigned long addr,
>>>                continue;
>>>            }
>>>    +        if (!folio_try_get(folio)) {
>>> +            xas_reset(&xas);
>>> +            continue;
>>> +        }
>>> +
>>> +        if (unlikely(folio != xas_reload(&xas))) {
>>> +            folio_put(folio);
>>> +            xas_reset(&xas);
>>> +            continue;
>>> +        }
>>> +
>>>            if (folio_order(folio) == HPAGE_PMD_ORDER &&
>>>                folio->index == start) {
>>>                /* Maybe PMD-mapped */
>>> @@ -2305,23 +2316,27 @@ static int hpage_collapse_scan_file(struct mm_struct *mm, unsigned long addr,
>>>                 * it's safe to skip LRU and refcount checks before
>>>                 * returning.
>>>                 */
>>> +            folio_put(folio);
>>>                break;
>>>            }
>>>               node = folio_nid(folio);
>>>            if (hpage_collapse_scan_abort(node, cc)) {
>>>                result = SCAN_SCAN_ABORT;
>>> +            folio_put(folio);
>>>                break;
>>>            }
>>>            cc->node_load[node]++;
>>>               if (!folio_test_lru(folio)) {
>>>                result = SCAN_PAGE_LRU;
>>> +            folio_put(folio);
>>>                break;
>>>            }
>>>               if (!is_refcount_suitable(folio)) {
>>
>> You add a temporary refcnt for the folio, then the
>> is_refcount_suitable() will always fail, right?
You're right. Good Catch!

> Indeed. Would one of our MADV_COLLAPSE selftests catch that?

The status of this one test case changed from PASS to SKIP
with the patch:

./tools/testing/selftests/mm/uffd-unit-tests
Testing minor-collapse on shmem... skipped [reason: MADV_COLLAPSE failed]
Userfaults unit tests: pass=65, skip=1, fail=0 (total=66)

All test cases in khugepaged.c PASS.

In cow.c, Test Case status with or without patch remain unchanged for me.

# [RUN] Basic COW after fork() when collapsing after fork() (fully shared)
ok 758 # SKIP MADV_COLLAPSE failed: Invalid argument
# 1 skipped test(s) detected. Consider enabling relevant config options to improve coverage.
# Totals: pass:769 fail:0 xfail:8 xpass:0 skip:1 error:0

> 
> We should also be converting that code to use folio_expected_ref_count() -- either directly or wrapped in is_refcount_suitable().
> 
> Likely just here through
> 
> if (folio_expected_ref_count(folio) + 1 != folio_ref_count(folio))
>     ...
> 

This makes sense. Then is_refcount_suitable() will not be needed.

Thanks,
Shivank