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

[David Hildenbrand <david@redhat.com>]

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.

-- 
Cheers,

David / dhildenb