Re: [RFC] mm/huge_memory: prevent potential NULL pointer dereference

[Zi Yan <ziy@nvidia.com>]

On 16 Jul 2025, at 11:18, David Hildenbrand wrote:

> On 16.07.25 17:10, Zi Yan wrote:
>> On 16 Jul 2025, at 10:58, Antonio Quartulli wrote:
>>
>>> I just found this issue in the last linux-next Coverity report and it
>>> caught my attention.
>>> I am not familiar with this code, therefore I am sending this patch
>>> as RFC because I am not 100% sure whether this is a false positive or
>>> not.
>>> However, it seems potentially legit to me:
>>>
>>> In __folio_split(), when looping over folios we dereference
>>> `mapping` before ensuring it is non-NULL.
>>>
>>> Following code in the loop body performs such check, thus
>>> suggesting that `mapping` may be NULL and accessing it
>>> without any check may be dangerous.
>>>
>>> Add NULL check before passing it to shmem_mapping().
>>>
>>> Cc: Zi Yan <ziy@nvidia.com>
>>> Fixes: 00527733d0dc ("mm/huge_memory: add two new (not yet used) functions for folio_split()")
>>> Addresses-Coverity-ID: 1647614 ("FORWARD_NULL")
>>> Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
>>> ---
>>>   mm/huge_memory.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>> index 389620c65a5f..d649026db95a 100644
>>> --- a/mm/huge_memory.c
>>> +++ b/mm/huge_memory.c
>>> @@ -3802,7 +3802,7 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>>>
>>>   			/* Some pages can be beyond EOF: drop them from cache */
>>>   			if (new_folio->index >= end) {
>>> -				if (shmem_mapping(mapping))
>>> +				if (mapping && shmem_mapping(mapping))
>>>   					nr_shmem_dropped += folio_nr_pages(new_folio);
>>>   				else if (folio_test_clear_dirty(new_folio))
>>>   					folio_account_cleaned(
>> Hi Antonio,
>>
>> Is there a way of preventing Coverity/sparse from checking certain code?
>> This non-NULL mapping issue pops up every time I touch the code.
>
> Probably we could make that code more readable and achieve it:
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 24aff14d22a1e..acf56aae1a2ef 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3775,6 +3775,7 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>                  */
>                 for (new_folio = folio_next(folio); new_folio != next_folio;
>                      new_folio = next) {
> +                       unsigned long nr_pages = folio_nr_pages(new_folio);
>                         next = folio_next(new_folio);
>                          expected_refs = folio_expected_ref_count(new_folio) + 1;
> @@ -3782,20 +3783,20 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>                          lru_add_split_folio(folio, new_folio, lruvec, list);
>  -                       /* Some pages can be beyond EOF: drop them from cache */
> -                       if (new_folio->index >= end) {
> -                               if (shmem_mapping(mapping))
> -                                       nr_shmem_dropped += folio_nr_pages(new_folio);
> -                               else if (folio_test_clear_dirty(new_folio))
> -                                       folio_account_cleaned(
> -                                               new_folio,
> -                                               inode_to_wb(mapping->host));
> -                               __filemap_remove_folio(new_folio, NULL);
> -                               folio_put_refs(new_folio,
> -                                              folio_nr_pages(new_folio));
> -                       } else if (mapping) {
> -                               __xa_store(&mapping->i_pages, new_folio->index,
> -                                          new_folio, 0);
> +                       /* Add the new folio to the page cache ... */
> +                       if (mapping) {
> +                               /* ... however, drop folios beyond EOF. */
> +                               if (new_folio->index >= end) {
> +                                       if (shmem_mapping(mapping))
> +                                               nr_shmem_dropped += nr_pages;
> +                                       else if (folio_test_clear_dirty(new_folio))
> +                                               folio_account_cleaned(new_folio, inode_to_wb(mapping->host));
> +                                       __filemap_remove_folio(new_folio, NULL);
> +                                       folio_put_refs(new_folio, nr_pages);
> +                               } else {
> +                                       __xa_store(&mapping->i_pages,
> +                                                  new_folio->index, new_folio, 0);
> +                               }
>                         } else if (swap_cache) {
>                                 __xa_store(&swap_cache->i_pages,
>                                            swap_cache_index(new_folio->swap),
>
>
> Having that logic in a helper would be cleaner, but not straight-forward due
> to things like nr_shmem_dropped.

Looks good to me. Do you want to send a patch? Otherwise, I can send one.

Best Regards,
Yan, Zi