Re: [PATCH] mm/huge_memory: fix NULL pointer deference when splitting shmem folio in swap cache

[Zi Yan <ziy@nvidia.com>]

On 19 Nov 2025, at 19:47, Wei Yang wrote:

> On Wed, Nov 19, 2025 at 03:46:14PM +0100, David Hildenbrand (Red Hat) wrote:
>> On 19.11.25 15:37, David Hildenbrand (Red Hat) wrote:
>>>>> Given folio_test_swapcache() might have false positives,
>>>>> I assume we'd need a
>>>>>
>>>>> 	folio_test_swapbacked() && folio_test_swapcache(folio)
>>>>>
>>>>> To detect large large shmem folios in the swapcache in all cases here.
>>>>>
>>>>> Something like the following would hopefully do:
>>>>>
>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>>> index 2f2a521e5d683..57aab66bedbea 100644
>>>>> --- a/mm/huge_memory.c
>>>>> +++ b/mm/huge_memory.c
>>>>> @@ -3515,6 +3515,13 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
>>>>>           return ret;
>>>>>    }
>>>>>    +static bool folio_test_shmem_swapcache(struct folio *folio)
>>>>> +{
>>>>> +       VM_WARN_ON_ONCE_FOLIO(folio_test_anon(folio), folio);
>>>>> +       /* These folios do not have folio->mapping set. */
>>>>> +       return folio_test_swapbacked(folio) && folio_test_swapcache(folio);
>>>>> +}
>>>>> +
>>>>>    bool non_uniform_split_supported(struct folio *folio, unsigned int new_order,
>>>>>                   bool warns)
>>>>>    {
>>>>> @@ -3524,6 +3531,9 @@ bool non_uniform_split_supported(struct folio *folio, unsigned int new_order,
>>>>>                                   "Cannot split to order-1 folio");
>>>>>                   if (new_order == 1)
>>>>>                           return false;
>>>>> +       } else if (folio_test_shmem_swapcache(folio)) {
>>>>> +               /* TODO: support shmem folios that are in the swapcache. */
>>>>> +               return false;
>>>>
>>>> With this, truncated shmem returns -EINVALID instead of -EBUSY now.
>>>> Can s390_wiggle_split_folio() such folios?
>>>
>>> [noting that s390_wiggle_split_folio() was just one caller where I new
>>> the return value differs. I suspect there might be more.]
>>>
>>> I am still not clear on that one.
>>>
>>> s390x obtains the folio while walking the page tables. In case it gets
>>> -EBUSY it simply retries to obtain the folio from the page tables.
>>>
>>> So assuming there was concurrent truncation and we returned -EBUSY, it
>>> would just retry walking the page tables (trigger a fault to map a
>>> folio) and retry with that one.
>>>
>>> I would assume that the shmem folio in the swapcache could never have
>>> worked before, and that there is no way to make progress really.
>>>
>>> In other words: do we know how we can end up with a shmem folio that is
>>> in the swapcache and does not have folio->mapping set?
>>>
>>> Could that think still be mapped into the page tables? (I hope not, but
>>> right now I am confused how that can happen )
>>>
>>
>> Ah, my memory comes back.
>>
>> vmscan triggers shmem_writeout() after unmapping the folio and after making sure that there are no unexpected folio references.
>>
>> shmem_writeout() will do the shmem_delete_from_page_cache() where we set folio->mapping = NULL.
>>
>> So anything walking the page tables (like s390x) could never find it.
>>
>>
>> Such shmem folios really cannot get split right now until we either reclaimed them (-> freed) or until shmem_swapin_folio() re-obtained them from the swapcache to re-add them to the swapcache through shmem_add_to_page_cache().
>>
>> So maybe we can just make our life easy and just keep returning -EBUSY for this scenario for the time being?
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 2f2a521e5d683..5ce86882b2727 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -3619,6 +3619,16 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>>        if (folio != page_folio(split_at) || folio != page_folio(lock_at))
>>                return -EINVAL;
>> +       /*
>> +        * Folios that just got truncated cannot get split. Signal to the
>> +        * caller that there was a race.
>> +        *
>> +        * TODO: this will also currently refuse shmem folios that are in
>> +        * the swapcache.
>> +        */
>> +       if (!is_anon && !folio->mapping)
>> +               return -EBUSY;
>> +
>>        if (new_order >= folio_order(folio))
>>                return -EINVAL;
>> @@ -3659,17 +3669,7 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>>                gfp_t gfp;
>>                mapping = folio->mapping;
>> -
>> -               /* Truncated ? */
>> -               /*
>> -                * TODO: add support for large shmem folio in swap cache.
>> -                * When shmem is in swap cache, mapping is NULL and
>> -                * folio_test_swapcache() is true.
>> -                */
>> -               if (!mapping) {
>> -                       ret = -EBUSY;
>> -                       goto out;
>> -               }
>> +               VM_WARN_ON_ONCE_FOLIO(!mapping, folio);
>>                min_order = mapping_min_folio_order(folio->mapping);
>>                if (new_order < min_order) {
>>
>
> One more thing come up my mind.
>
> Current folio_split_supported() is used in try_folio_split_to_order().
>
> Here are related commits:
>
> [1] commit 7460b470a131f985a70302a322617121efdd7caa
>     Author: Zi Yan <ziy@nvidia.com>
>     Date:   Fri Mar 7 12:40:00 2025 -0500
>
>         mm/truncate: use folio_split() in truncate operation
>
> [2] commit 77008e1b2ef73249bceb078a321a3ff6bc087afb
>     Author: Zi Yan <ziy@nvidia.com>
>     Date:   Thu Oct 16 21:36:30 2025 -0400
>
>         mm/huge_memory: do not change split_huge_page*() target order silently
>
> [1] looks fine, because before calling folio_split_supported(),
> min_order_for_split() would return negative if !folio->mapping.
>
> But [2] moves min_order_for_split() from try_folio_split_to_order() to it
> caller.
>
> Currently it looks good, but not sure it will leave potential misuse.

I am sending patches to handle it. Thank you for spotting it.
Best Regards,
Yan, Zi