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

["David Hildenbrand (Red Hat)" <david@kernel.org>]

On 19.11.25 13:42, Wei Yang wrote:
> On Wed, Nov 19, 2025 at 09:57:58AM +0100, David Hildenbrand (Red Hat) wrote:
>> On 19.11.25 02:26, Wei Yang wrote:
>>> Commit c010d47f107f ("mm: thp: split huge page to any lower order
>>> pages") introduced an early check on the folio's order via
>>> mapping->flags before proceeding with the split work.
>>>
>>> This check introduced a bug: for shmem folios in the swap cache, the
>>> mapping pointer can be NULL. Accessing mapping->flags in this state
>>> leads directly to a NULL pointer dereference.
>>
>> Under which circumstances would that be the case? Only for large shmem folios
>> in the swapcache or also for truncated folios? So I'd assume this
>> would also affect truncated folios and we should spell that out here?
>>
>>>
>>> This commit fixes the issue by moving the check for mapping != NULL
>>> before any attempt to access mapping->flags.
>>>
>>> This fix necessarily changes the return value from -EBUSY to -EINVAL
>>> when mapping is NULL. After reviewing current callers, they do not
>>> differentiate between these two error codes, making this change safe.
>>
>> The doc of __split_huge_page_to_list_to_order() would now be outdated and has
>> to be updated.
>>
>> Also, take a look at s390_wiggle_split_folio(): returning -EINVAL instead of
>> -EBUSY will make a difference on concurrent truncation. -EINVAL will be
>> propagated and make the operation fail, while -EBUSY will be translated to
>> -EAGAIN and the caller will simply lookup the folio again and retry.
>>
>> So I think we should try to keep truncation return -EBUSY. For the shmem
>> case, I think it's ok to return -EINVAL. I guess we can identify such folios
>> by checking for folio_test_swapcache().
>>
> 
> I come up a draft:
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 7c69572b6c3f..3e140fa1ca13 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3696,6 +3696,18 @@ bool folio_split_supported(struct folio *folio, unsigned int new_order,
>                                  "Cannot split to order-1 folio");
>                  if (new_order == 1)
>                          return -EINVAL;
> +       } else if (!folio->mapping) {
> +               /*
> +                * If there is no mapping that the folio was truncated and we
> +                * cannot split.
> +                *
> +                * TODO: large shmem folio in the swap cache also don't
> +                * currently have a mapping but folio_test_swapcache() is true
> +                * for them.
> +                */
> +               if (folio_test_swapcache(folio))

As per discussions, that would likely have to be

	folio_test_swapbacked() && folio_test_swapcache()


> +                       return -EINVAL;
> +               return -EBUSY;
>          } else if (split_type == SPLIT_TYPE_NON_UNIFORM || new_order) {
>                  if (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) &&
>                      !mapping_large_folio_support(folio->mapping)) {
> @@ -3931,8 +3943,9 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>          if (new_order >= old_order)
>                  return -EINVAL;
>   
> -       if (!folio_split_supported(folio, new_order, split_type, /* warn = */ true))
> -               return -EINVAL;
> +       ret = folio_split_supported((folio, new_order, split_type, /* warn = */ true));
> +       if (ret)

I'd prefer if folio_split_supported() would keep returning a boolen, 
such that we detect the truncation case earlier and just return -EBUSY.

But no strong opinion. Important part is that truncation handling is not 
changed without taking a lot of care.

-- 
Cheers

David