Re: [PATCH v1] mm/gup: Revert "mm: gup: fix infinite loop within __get_longterm_locked"

[John Hubbard]

On 6/11/25 6:13 AM, David Hildenbrand wrote:
> After commit 1aaf8c122918 ("mm: gup: fix infinite loop within
> __get_longterm_locked") we are able to longterm pin folios that are not
> supposed to get longterm pinned, simply because they temporarily have
> the LRU flag cleared (esp. temporarily isolated).
> 
> For example, two __get_longterm_locked() callers can race, or
> __get_longterm_locked() can race with anything else that temporarily
> isolates folios.
> 
> The introducing commit mentions the use case of a driver that uses
> vm_ops->fault to insert pages allocated through cma_alloc() into the
> page tables, assuming they can later get longterm pinned. These pages/
> folios would never have the LRU flag set and consequently cannot get
> isolated. There is no known in-tree user making use of that so far,
> fortunately.
> 
> To handle that in the future -- and avoid retrying forever to
> isolate/migrate them -- we will need a different mechanism for the CMA
> area *owner* to indicate that it actually already allocated the page and
> is fine with longterm pinning it. The LRU flag is not suitable for that.
> 
> Probably we can lookup the relevant CMA area and query the bitmap; we
> only have have to care about some races, probably. If already allocated,
> we could just allow longterm pinning)
> 
> Anyhow, let's fix the "must not be longterm pinned" problem first by
> reverting the original commit.

Really great commit description, I appreciate the time and effort spent
summarizing what happened here.

> 
> Fixes: 1aaf8c122918 ("mm: gup: fix infinite loop within __get_longterm_locked")
> Closes: https://lore.kernel.org/all/20250522092755.GA3277597@tiffany/
> Reported-by: Hyesoo Yu <hyesoo.yu@xxxxxxxxxxx>
> Cc: <Stable@xxxxxxxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Jason Gunthorpe <jgg@xxxxxxxx>
> Cc: Peter Xu <peterx@xxxxxxxxxx>
> Cc: Zhaoyang Huang <zhaoyang.huang@xxxxxxxxxx>
> Cc: Aijun Sun <aijun.sun@xxxxxxxxxx>
> Cc: Alistair Popple <apopple@xxxxxxxxxx>
> Cc: John Hubbard <jhubbard@xxxxxxxxxx>
> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> ---
>  mm/gup.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
I've verified that this is an exact revert of 1aaf8c122918, yes.

Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx>


thanks,
-- 
John Hubbard

> diff --git a/mm/gup.c b/mm/gup.c
> index e065a49842a87..3c39cbbeebef1 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2303,13 +2303,13 @@ static void pofs_unpin(struct pages_or_folios *pofs)
>  /*
>   * Returns the number of collected folios. Return value is always >= 0.
>   */
> -static void collect_longterm_unpinnable_folios(
> +static unsigned long collect_longterm_unpinnable_folios(
>  		struct list_head *movable_folio_list,
>  		struct pages_or_folios *pofs)
>  {
> +	unsigned long i, collected = 0;
>  	struct folio *prev_folio = NULL;
>  	bool drain_allow = true;
> -	unsigned long i;
>  
>  	for (i = 0; i < pofs->nr_entries; i++) {
>  		struct folio *folio = pofs_get_folio(pofs, i);
> @@ -2321,6 +2321,8 @@ static void collect_longterm_unpinnable_folios(
>  		if (folio_is_longterm_pinnable(folio))
>  			continue;
>  
> +		collected++;
> +
>  		if (folio_is_device_coherent(folio))
>  			continue;
>  
> @@ -2342,6 +2344,8 @@ static void collect_longterm_unpinnable_folios(
>  				    NR_ISOLATED_ANON + folio_is_file_lru(folio),
>  				    folio_nr_pages(folio));
>  	}
> +
> +	return collected;
>  }
>  
>  /*
> @@ -2418,9 +2422,11 @@ static long
>  check_and_migrate_movable_pages_or_folios(struct pages_or_folios *pofs)
>  {
>  	LIST_HEAD(movable_folio_list);
> +	unsigned long collected;
>  
> -	collect_longterm_unpinnable_folios(&movable_folio_list, pofs);
> -	if (list_empty(&movable_folio_list))
> +	collected = collect_longterm_unpinnable_folios(&movable_folio_list,
> +						       pofs);
> +	if (!collected)
>  		return 0;
>  
>  	return migrate_longterm_unpinnable_folios(&movable_folio_list, pofs);