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

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

[Antonio Quartulli]

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(
-- 
2.49.1

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

[Lorenzo Stoakes]

On Wed, Jul 16, 2025 at 04:58:04PM +0200, 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>

Thanks for the patch :) but sorry it is in practice not a thing that can
happen. See below for analysis.

> ---
>  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) {

It's kind of not _hugely_ clear but, if anon - which implies no mapping - we set
end == -1.

At which point this condition cannot evaluate true (index is at page granularity
so even MAX_UINT64 would be page shifted and still not equal -1).

Under all other circumstances, mapping will be non-NULL.

> -				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(
> --
> 2.49.1
>
>

Cheers, Lorenzo

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

[Zi Yan]

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.

Dan Carpenter reported the issue yesterday and I explained it is no issue[1].
The same report showed up when I added __split_unmapped_folio()
back in February[2]

[1] https://lore.kernel.org/oe-kbuild/64b54034-f311-4e7d-b935-c16775dbb642@suswa.mountain/
[2] https://lore.kernel.org/linux-mm/2afe3d59-aca5-40f7-82a3-a6d976fb0f4f@stanley.mountain/

I wonder how many times I need to explain this, although I appreciate
your effort to improve kernel.

Best Regards,
Yan, Zi

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

[David Hildenbrand]

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

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

[Zi Yan]

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

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

[David Hildenbrand]

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

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

[Dan Carpenter]

On Wed, Jul 16, 2025 at 11:10:00AM -0400, 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.
> 
> Dan Carpenter reported the issue yesterday and I explained it is no issue[1].
> The same report showed up when I added __split_unmapped_folio()
> back in February[2]
> 
> [1] https://lore.kernel.org/oe-kbuild/64b54034-f311-4e7d-b935-c16775dbb642@suswa.mountain/
> [2] https://lore.kernel.org/linux-mm/2afe3d59-aca5-40f7-82a3-a6d976fb0f4f@stanley.mountain/
> 
> I wonder how many times I need to explain this, although I appreciate
> your effort to improve kernel.

The way to disable static analysis is to wrap the line or function in
#ifndef __CHECKER__.  This is a bad option.  We could probably count
how many places do this using one set of fingers and toes.  We don't
like defines in .c files.  I hate when static analysis makes the code
messier and worse.  We always say to fix the checker and leave the code
alone.

We generally try to only report warnings once, but in this case you
changed the name of the function so it shows up as new.  All the scripts
say if you change the name of the file or the function that's a new
warning.  When I'm searching for duplicate reports on lore, I use the
name of the function as well.  Plus, I looked at the surrounding code
and everything on my screen was from Jul 7 so I assumed it was new.

The best option is to add a comment to the code.  The next best option
is to just endure the occasional false positive.

One idea might be to add a hash tag to static checker warnings/patches
so we can manually search lore.kernel.org for the filename and hash tag
before sending new bug reports/fixes.

regards,
dan carpenter

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

[Antonio Quartulli]

Hi Lorenzo,

On 16/07/2025 17:07, Lorenzo Stoakes wrote:
>> 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) {
> 
> It's kind of not _hugely_ clear but, if anon - which implies no mapping - we set
> end == -1.
> 
> At which point this condition cannot evaluate true (index is at page granularity
> so even MAX_UINT64 would be page shifted and still not equal -1).

I may be missing something, but why can't "index >= -1" be true?

In any case, with Zi's patch the NULL check comes before the rest so the 
problem does not exist anymore there.

Regards,

> 
> Under all other circumstances, mapping will be non-NULL.
> 
>> -				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(
>> --
>> 2.49.1
>>
>>
> 
> Cheers, Lorenzo

-- 
Antonio Quartulli

CEO and Co-Founder
Mandelbit Srl
https://www.mandelbit.com

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

[Lorenzo Stoakes]

On Wed, Jul 16, 2025 at 09:05:14PM +0200, Antonio Quartulli wrote:
> Hi Lorenzo,
>
> On 16/07/2025 17:07, Lorenzo Stoakes wrote:
> > > 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) {
> >
> > It's kind of not _hugely_ clear but, if anon - which implies no mapping - we set
> > end == -1.
> >
> > At which point this condition cannot evaluate true (index is at page granularity
> > so even MAX_UINT64 would be page shifted and still not equal -1).
>
> I may be missing something, but why can't "index >= -1" be true?

These are unsigned long's, -1 in two's complement this means -1 translates
to the maximum possible unsigned long. Index cannot == the maximum number
as this is a _page_ index so a folio would need to reference a page larger
than could be represented in a 64-bit system for that to be so.

>
> In any case, with Zi's patch the NULL check comes before the rest so the
> problem does not exist anymore there.
>
> Regards,
>
> >
> > Under all other circumstances, mapping will be non-NULL.
> >
> > > -				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(
> > > --
> > > 2.49.1
> > >
> > >
> >
> > Cheers, Lorenzo
>
> --
> Antonio Quartulli
>
> CEO and Co-Founder
> Mandelbit Srl
> https://www.mandelbit.com
>
>

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

[Antonio Quartulli]

On 16/07/2025 21:10, Lorenzo Stoakes wrote:
> On Wed, Jul 16, 2025 at 09:05:14PM +0200, Antonio Quartulli wrote:
>> Hi Lorenzo,
>>
>> On 16/07/2025 17:07, Lorenzo Stoakes wrote:
>>>> 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) {
>>>
>>> It's kind of not _hugely_ clear but, if anon - which implies no mapping - we set
>>> end == -1.
>>>
>>> At which point this condition cannot evaluate true (index is at page granularity
>>> so even MAX_UINT64 would be page shifted and still not equal -1).
>>
>> I may be missing something, but why can't "index >= -1" be true?
> 
> These are unsigned long's, -1 in two's complement this means -1 translates
> to the maximum possible unsigned long. Index cannot == the maximum number
> as this is a _page_ index so a folio would need to reference a page larger
> than could be represented in a 64-bit system for that to be so.

Got it!
Thanks a lot for the clarification.

Cheers,


-- 
Antonio Quartulli

CEO and Co-Founder
Mandelbit Srl
https://www.mandelbit.com