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

[David Hildenbrand <david@redhat.com>]

On 16.07.25 17:24, Zi Yan wrote:
> 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.

Please send on (you'll manage to clean this up further).

Thanks!

-- 
Cheers,

David / dhildenb