linux-kernel - Re: [PATCH v1] mm/gup: Revert "mm: gup: fix infinite loop within __get_longterm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <56b5e235-6fa5-49d8-8083-42021cdbd12e@nvidia.com>
Date: Wed, 18 Jun 2025 18:43:50 -0700
From: John Hubbard <jhubbard@...dia.com>
To: David Hildenbrand <david@...hat.com>, linux-kernel@...r.kernel.org
Cc: linux-mm@...ck.org, Hyesoo Yu <hyesoo.yu@...sung.com>,
 Stable@...r.kernel.org, Andrew Morton <akpm@...ux-foundation.org>,
 Jason Gunthorpe <jgg@...pe.ca>, Peter Xu <peterx@...hat.com>,
 Zhaoyang Huang <zhaoyang.huang@...soc.com>, Aijun Sun
 <aijun.sun@...soc.com>, Alistair Popple <apopple@...dia.com>
Subject: Re: [PATCH v1] mm/gup: Revert "mm: gup: fix infinite loop within
 __get_longterm_locked"

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@...sung.com>
> Cc: <Stable@...r.kernel.org>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Jason Gunthorpe <jgg@...pe.ca>
> Cc: Peter Xu <peterx@...hat.com>
> Cc: Zhaoyang Huang <zhaoyang.huang@...soc.com>
> Cc: Aijun Sun <aijun.sun@...soc.com>
> Cc: Alistair Popple <apopple@...dia.com>
> Cc: John Hubbard <jhubbard@...dia.com>
> Signed-off-by: David Hildenbrand <david@...hat.com>
> ---
>  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@...dia.com>


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