[PATCH v2] mm: gup: fix infinite loop within __get_longterm_locked

[PATCH v2] mm: gup: fix infinite loop within __get_longterm_locked

[zhaoyang.huang]

From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>

We can run into an infinite loop in __get_longterm_locked() when
collect_longterm_unpinnable_folios() finds only folios that are isolated
from the LRU or were never added to the LRU. This can happen when all
folios to be pinned are never added to the LRU, for example when
vm_ops->fault allocated pages using cma_alloc() and never added them to
the LRU.

We incorrectly update the "collected" variable even if nothing was
collected. Fix it by incrementing "collected" only when we isolated a
folio and added it to the list of folios to migrate.

Fixes: 67e139b02d99 ("mm/gup.c: refactor check_and_migrate_movable_pages()")

Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
---
v2: judge if there are unpinnable pages via list_empty and update commit
message
---
---
 mm/gup.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 3b75e631f369..37951fba929a 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2323,11 +2323,11 @@ static void pofs_unpin(struct pages_or_folios *pofs)
 /*
  * Returns the number of collected folios. Return value is always >= 0.
  */
-static unsigned long collect_longterm_unpinnable_folios(
+static void collect_longterm_unpinnable_folios(
 		struct list_head *movable_folio_list,
 		struct pages_or_folios *pofs)
 {
-	unsigned long i, collected = 0;
+	unsigned long i;
 	struct folio *prev_folio = NULL;
 	bool drain_allow = true;
 
@@ -2341,8 +2341,6 @@ static unsigned long collect_longterm_unpinnable_folios(
 		if (folio_is_longterm_pinnable(folio))
 			continue;
 
-		collected++;
-
 		if (folio_is_device_coherent(folio))
 			continue;
 
@@ -2364,8 +2362,6 @@ static unsigned long collect_longterm_unpinnable_folios(
 				    NR_ISOLATED_ANON + folio_is_file_lru(folio),
 				    folio_nr_pages(folio));
 	}
-
-	return collected;
 }
 
 /*
@@ -2442,11 +2438,9 @@ static long
 check_and_migrate_movable_pages_or_folios(struct pages_or_folios *pofs)
 {
 	LIST_HEAD(movable_folio_list);
-	unsigned long collected;
 
-	collected = collect_longterm_unpinnable_folios(&movable_folio_list,
-						       pofs);
-	if (!collected)
+	collect_longterm_unpinnable_folios(&movable_folio_list, pofs);
+	if (list_empty(&movable_folio_list))
 		return 0;
 
 	return migrate_longterm_unpinnable_folios(&movable_folio_list, pofs);
-- 
2.25.1

Re: [PATCH v2] mm: gup: fix infinite loop within __get_longterm_locked

[David Hildenbrand]

On 21.01.25 03:01, zhaoyang.huang wrote:
> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> 
> We can run into an infinite loop in __get_longterm_locked() when
> collect_longterm_unpinnable_folios() finds only folios that are isolated
> from the LRU or were never added to the LRU. This can happen when all
> folios to be pinned are never added to the LRU, for example when
> vm_ops->fault allocated pages using cma_alloc() and never added them to
> the LRU.
> 
> We incorrectly update the "collected" variable even if nothing was
> collected. Fix it by incrementing "collected" only when we isolated a
> folio and added it to the list of folios to migrate.

Last sentence now has to be "Fix it by simply taking a look at the list 
in the single caller, to see if anything was added.".

> 
> Fixes: 67e139b02d99 ("mm/gup.c: refactor check_and_migrate_movable_pages()")
> 
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> ---
> v2: judge if there are unpinnable pages via list_empty and update commit
> message
> ---
> ---
>   mm/gup.c | 14 ++++----------
>   1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 3b75e631f369..37951fba929a 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2323,11 +2323,11 @@ static void pofs_unpin(struct pages_or_folios *pofs)
>   /*
>    * Returns the number of collected folios. Return value is always >= 0.
>    */
> -static unsigned long collect_longterm_unpinnable_folios(
> +static void collect_longterm_unpinnable_folios(
>   		struct list_head *movable_folio_list,
>   		struct pages_or_folios *pofs)
>   {
> -	unsigned long i, collected = 0;
> +	unsigned long i;

Nit: "reverse christmas tree" parameter format wants this to come after 
the "drain_allow" variable now.

>   	struct folio *prev_folio = NULL;
>   	bool drain_allow = true;
>   
> @@ -2341,8 +2341,6 @@ static unsigned long collect_longterm_unpinnable_folios(
>   		if (folio_is_longterm_pinnable(folio))
>   			continue;
>   
> -		collected++;
> -
>   		if (folio_is_device_coherent(folio))
>   			continue;
>   
> @@ -2364,8 +2362,6 @@ static unsigned long collect_longterm_unpinnable_folios(
>   				    NR_ISOLATED_ANON + folio_is_file_lru(folio),
>   				    folio_nr_pages(folio));
>   	}
> -
> -	return collected;
>   }
>   
>   /*
> @@ -2442,11 +2438,9 @@ static long
>   check_and_migrate_movable_pages_or_folios(struct pages_or_folios *pofs)
>   {
>   	LIST_HEAD(movable_folio_list);
> -	unsigned long collected;
>   
> -	collected = collect_longterm_unpinnable_folios(&movable_folio_list,
> -						       pofs);
> -	if (!collected)
> +	collect_longterm_unpinnable_folios(&movable_folio_list, pofs);
> +	if (list_empty(&movable_folio_list))
>   		return 0;
>   
>   	return migrate_longterm_unpinnable_folios(&movable_folio_list, pofs);

Nice, exactly what I just recommended as reply to #1 after waking up :)

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb