1. Patchew
  2. linux
  3. View series

[PATCH] mm/huge_memory: Fix iterator variable usage after swap()

zenghongling posted 1 patch 2 weeks, 4 days ago 
mm/huge_memory.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] mm/huge_memory: Fix iterator variable usage after swap()
Posted by zenghongling 2 weeks, 4 days ago 
The iterator variable 'folio' is swapped with 'prev' in the else
branch. Using 'folio' after swap() checks the potentially NULL
'prev' value, not the original iterator value.

Fix by moving folio_put() call before the swap operation in the
path where swap() occurs.

Found by:
./huge_memory.c:4225:6-11: ERROR: iterator variable bound on line 4178 cannot be NULL

Signed-off-by: zenghongling <zenghongling@kylinos.cn>
---
 mm/huge_memory.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 6cba1cb14b23..258bf4725aea 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -4212,6 +4212,7 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
 			; /* folio already removed from list */
 		} else if (!folio_test_partially_mapped(folio)) {
 			list_del_init(&folio->_deferred_list);
+			folio_put(folio);
 			removed++;
 		} else {
 			/*
@@ -4220,10 +4221,9 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
 			 * left on the list (which may be concurrently unqueued)
 			 * by one safe folio with refcount still raised.
 			 */
+			folio_put(folio);
 			swap(folio, prev);
 		}
-		if (folio)
-			folio_put(folio);
 	}
 
 	spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
-- 
2.25.1
Re: [PATCH] mm/huge_memory: Fix iterator variable usage after swap()
Posted by Lance Yang 2 weeks, 4 days ago 

On 2026/1/21 16:13, zenghongling wrote:
> The iterator variable 'folio' is swapped with 'prev' in the else
> branch. Using 'folio' after swap() checks the potentially NULL
> 'prev' value, not the original iterator value.
> 
> Fix by moving folio_put() call before the swap operation in the
> path where swap() occurs.
> 
> Found by:
> ./huge_memory.c:4225:6-11: ERROR: iterator variable bound on line 4178 cannot be NULL

Good catch!

But which tree is your patch based on?

Seems like that was already fixed in commit 776bde7caf80[1]. The
whole thing deferred_split_scan() was refactored using folio_batch,
so the buggy code with swap(folio, prev) is gone ...

Ccing Muchun and Qi who fixed that.

[1] 
https://lore.kernel.org/all/59cb6b6fb5ffcff9d23b81890b252960139ad8e7.1762762324.git.zhengqi.arch@bytedance.com/

Thanks,
Lance

> 
> Signed-off-by: zenghongling <zenghongling@kylinos.cn>
> ---
>   mm/huge_memory.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 6cba1cb14b23..258bf4725aea 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -4212,6 +4212,7 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
>   			; /* folio already removed from list */
>   		} else if (!folio_test_partially_mapped(folio)) {
>   			list_del_init(&folio->_deferred_list);
> +			folio_put(folio);
>   			removed++;
>   		} else {
>   			/*
> @@ -4220,10 +4221,9 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
>   			 * left on the list (which may be concurrently unqueued)
>   			 * by one safe folio with refcount still raised.
>   			 */
> +			folio_put(folio);
>   			swap(folio, prev);
>   		}
> -		if (folio)
> -			folio_put(folio);
>   	}
>   
>   	spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
Re: [PATCH] mm/huge_memory: Fix iterator variable usage after swap()
Posted by David Hildenbrand (Red Hat) 2 weeks, 4 days ago 
On 1/21/26 10:25, Lance Yang wrote:
> 
> 
> On 2026/1/21 16:13, zenghongling wrote:
>> The iterator variable 'folio' is swapped with 'prev' in the else
>> branch. Using 'folio' after swap() checks the potentially NULL
>> 'prev' value, not the original iterator value.
>>
>> Fix by moving folio_put() call before the swap operation in the
>> path where swap() occurs.
>>
>> Found by:
>> ./huge_memory.c:4225:6-11: ERROR: iterator variable bound on line 4178 cannot be NULL
> 

Which tool did find that? A compiler?

> Good catch!
> 
> But which tree is your patch based on?
> 
> Seems like that was already fixed in commit 776bde7caf80[1]. The
> whole thing deferred_split_scan() was refactored using folio_batch,
> so the buggy code with swap(folio, prev) is gone ...
> 
> Ccing Muchun and Qi who fixed that.
> 
> [1]
> https://lore.kernel.org/all/59cb6b6fb5ffcff9d23b81890b252960139ad8e7.1762762324.git.zhengqi.arch@bytedance.com/

Right, in

commit 776bde7caf80f6af72b087cafe7d9f607b14716d
Author: Muchun Song <muchun.song@linux.dev>
Date:   Mon Nov 10 16:17:57 2025 +0800

     mm: thp: use folio_batch to handle THP splitting in deferred_split_scan()


Which raises the question whether we would want to backport that patch to stable kernels
if there was indeed a problem?


But: I don't immediately see the problem.

If pref is NULL (and folio obviously !+NULL), we'll end up with
	* pref != NULL
	* folio == NULL

	The "if (folio)" check will do nothing, because we defer the freeing to the

		if (prev)
			folio_put(prev);

	later

If pref is != NULL (and folio obviously !+NULL), we'll end up with
	* pref = NULL
	* folio = NULL

	The if (folio) and if (prev) handling will care of it all.


So ... this pretty much looks like working as expected?

-- 
Cheers

David
Re: [PATCH] mm/huge_memory: Fix iterator variable usage after swap()
Posted by Lance Yang 2 weeks, 4 days ago 

On 2026/1/21 20:28, David Hildenbrand (Red Hat) wrote:
> On 1/21/26 10:25, Lance Yang wrote:
>>
>>
>> On 2026/1/21 16:13, zenghongling wrote:
>>> The iterator variable 'folio' is swapped with 'prev' in the else
>>> branch. Using 'folio' after swap() checks the potentially NULL
>>> 'prev' value, not the original iterator value.
>>>
>>> Fix by moving folio_put() call before the swap operation in the
>>> path where swap() occurs.
>>>
>>> Found by:
>>> ./huge_memory.c:4225:6-11: ERROR: iterator variable bound on line 
>>> 4178 cannot be NULL
>>
> 
> Which tool did find that? A compiler?

Looks like a false positive from coccinelle? The pattern is tricky to
follow statically :)

> 
>> Good catch!
>>
>> But which tree is your patch based on?
>>
>> Seems like that was already fixed in commit 776bde7caf80[1]. The
>> whole thing deferred_split_scan() was refactored using folio_batch,
>> so the buggy code with swap(folio, prev) is gone ...
>>
>> Ccing Muchun and Qi who fixed that.
>>
>> [1]
>> https://lore.kernel.org/ 
>> all/59cb6b6fb5ffcff9d23b81890b252960139ad8e7.1762762324.git.zhengqi.arch@bytedance.com/
> 
> Right, in
> 
> commit 776bde7caf80f6af72b087cafe7d9f607b14716d
> Author: Muchun Song <muchun.song@linux.dev>
> Date:   Mon Nov 10 16:17:57 2025 +0800
> 
>      mm: thp: use folio_batch to handle THP splitting in 
> deferred_split_scan()
> 
> 
> Which raises the question whether we would want to backport that patch 
> to stable kernels
> if there was indeed a problem?
> 
> 
> But: I don't immediately see the problem.
> 
> If pref is NULL (and folio obviously !+NULL), we'll end up with
>      * pref != NULL
>      * folio == NULL
> 
>      The "if (folio)" check will do nothing, because we defer the 
> freeing to the
> 
>          if (prev)
>              folio_put(prev);
> 
>      later
> 
> If pref is != NULL (and folio obviously !+NULL), we'll end up with
>      * pref = NULL
>      * folio = NULL
> 
>      The if (folio) and if (prev) handling will care of it all.
> 
> 
> So ... this pretty much looks like working as expected?

Right! After reading through the old code, the swap() design is
correct and working as intended, IIUC.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.