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: <56fe9ae8-e9bf-4627-a10a-ba1d3e84e8c8@redhat.com>
Date: Tue, 21 Jan 2025 09:13:26 +0100
From: David Hildenbrand <david@...hat.com>
To: "zhaoyang.huang" <zhaoyang.huang@...soc.com>,
 Andrew Morton <akpm@...ux-foundation.org>,
 Alistair Popple <apopple@...dia.com>, John Hubbard <jhubbard@...dia.com>,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 Zhaoyang Huang <huangzhaoyang@...il.com>, steve.kang@...soc.com,
 aijun.sun@...soc.com
Subject: Re: [PATCH v2] mm: gup: fix infinite loop within
 __get_longterm_locked

On 21.01.25 03:01, zhaoyang.huang wrote:
> From: Zhaoyang Huang <zhaoyang.huang@...soc.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@...dia.com>
> Suggested-by: David Hildenbrand <david@...hat.com>
> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@...soc.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@...hat.com>

-- 
Cheers,

David / dhildenb


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ