Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9b31868d-cec2-121a-cd1c-008dd9880cca@samsung.com>
Date:   Wed, 25 May 2022 23:48:01 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0)
        Gecko/20100101 Thunderbird/91.9.1
Subject: Re: [PATCH] mm: fix a potential infinite loop in
 start_isolate_page_range().
Content-Language: en-US
To:     Zi Yan <ziy@nvidia.com>, Qian Cai <quic_qiancai@quicinc.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        David Hildenbrand <david@redhat.com>, linux-mm@kvack.org
Cc:     linux-kernel@vger.kernel.org,
        virtualization@lists.linux-foundation.org,
        Vlastimil Babka <vbabka@suse.cz>,
        Mel Gorman <mgorman@techsingularity.net>,
        Eric Ren <renzhengeek@gmail.com>,
        Mike Rapoport <rppt@kernel.org>,
        Oscar Salvador <osalvador@suse.de>,
        Christophe Leroy <christophe.leroy@csgroup.eu>
From:   Marek Szyprowski <m.szyprowski@samsung.com>
In-Reply-To: <20220524194756.1698351-1-zi.yan@sent.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <20220524194756.1698351-1-zi.yan@sent.com>
        <CGME20220525214802eucas1p1edda2be0c03b9048c3712af8ad425345@eucas1p1.samsung.com>
Precedence: bulk

On 24.05.2022 21:47, Zi Yan wrote:
> From: Zi Yan <ziy@nvidia.com>
>
> In isolate_single_pageblock() called by start_isolate_page_range(),
> there are some pageblock isolation issues causing a potential
> infinite loop when isolating a page range. This is reported by Qian Cai.
>
> 1. the pageblock was isolated by just changing pageblock migratetype
>     without checking unmovable pages. Calling set_migratetype_isolate() to
>     isolate pageblock properly.
> 2. an off-by-one error caused migrating pages unnecessarily, since the page
>     is not crossing pageblock boundary.
> 3. migrating a compound page across pageblock boundary then splitting the
>     free page later has a small race window that the free page might be
>     allocated again, so that the code will try again, causing an potential
>     infinite loop. Temporarily set the to-be-migrated page's pageblock to
>     MIGRATE_ISOLATE to prevent that and bail out early if no free page is
>     found after page migration.
>
> An additional fix to split_free_page() aims to avoid crashing in
> __free_one_page(). When the free page is split at the specified
> split_pfn_offset, free_page_order should check both the first bit of
> free_page_pfn and the last bit of split_pfn_offset and use the smaller one.
> For example, if free_page_pfn=0x10000, split_pfn_offset=0xc000,
> free_page_order should first be 0x8000 then 0x4000, instead of 0x4000 then
> 0x8000, which the original algorithm did.
>
> Fixes: b2c9e2fbba ("mm: make alloc_contig_range work at pageblock granularity")
> Reported-by: Qian Cai <quic_qiancai@quicinc.com>
> Signed-off-by: Zi Yan <ziy@nvidia.com>

This patch landed in linux next-20220525 as commit 29a8af92b874 ("mm: 
fix a potential infinite loop in start_isolate_page_range()"). 
Unfortunately it breaks all CMA allocations done by the DMA-mapping 
framework. I've observed this on ARM 32bit and 64bit. In the logs I only 
see messages like:

cma: cma_alloc: linux,cma: alloc failed, req-size: 128 pages, ret: -16

I will try to analyze it a bit more tomorrow, but it looks that 
isolation always fails.

> ---
>   mm/page_alloc.c     |  5 ++++-
>   mm/page_isolation.c | 52 ++++++++++++++++++++++++++++++++++-----------
>   2 files changed, 44 insertions(+), 13 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 267599dd9706..6eec0211e0be 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1114,13 +1114,16 @@ void split_free_page(struct page *free_page,
>   	unsigned long flags;
>   	int free_page_order;
>   
> +	if (split_pfn_offset == 0)
> +		return;
> +
>   	spin_lock_irqsave(&zone->lock, flags);
>   	del_page_from_free_list(free_page, zone, order);
>   	for (pfn = free_page_pfn;
>   	     pfn < free_page_pfn + (1UL << order);) {
>   		int mt = get_pfnblock_migratetype(pfn_to_page(pfn), pfn);
>   
> -		free_page_order = ffs(split_pfn_offset) - 1;
> +		free_page_order = min(pfn ? __ffs(pfn) : order, __fls(split_pfn_offset));
>   		__free_one_page(pfn_to_page(pfn), pfn, zone, free_page_order,
>   				mt, FPI_NONE);
>   		pfn += 1UL << free_page_order;
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index b3f074d1682e..c643c8420809 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -283,6 +283,7 @@ __first_valid_page(unsigned long pfn, unsigned long nr_pages)
>    * isolate_single_pageblock() -- tries to isolate a pageblock that might be
>    * within a free or in-use page.
>    * @boundary_pfn:		pageblock-aligned pfn that a page might cross
> + * @flags:			isolation flags
>    * @gfp_flags:			GFP flags used for migrating pages
>    * @isolate_before:	isolate the pageblock before the boundary_pfn
>    *
> @@ -298,14 +299,15 @@ __first_valid_page(unsigned long pfn, unsigned long nr_pages)
>    * either. The function handles this by splitting the free page or migrating
>    * the in-use page then splitting the free page.
>    */
> -static int isolate_single_pageblock(unsigned long boundary_pfn, gfp_t gfp_flags,
> -			bool isolate_before)
> +static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
> +			gfp_t gfp_flags, bool isolate_before)
>   {
>   	unsigned char saved_mt;
>   	unsigned long start_pfn;
>   	unsigned long isolate_pageblock;
>   	unsigned long pfn;
>   	struct zone *zone;
> +	int ret;
>   
>   	VM_BUG_ON(!IS_ALIGNED(boundary_pfn, pageblock_nr_pages));
>   
> @@ -325,7 +327,11 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, gfp_t gfp_flags,
>   				      zone->zone_start_pfn);
>   
>   	saved_mt = get_pageblock_migratetype(pfn_to_page(isolate_pageblock));
> -	set_pageblock_migratetype(pfn_to_page(isolate_pageblock), MIGRATE_ISOLATE);
> +	ret = set_migratetype_isolate(pfn_to_page(isolate_pageblock), saved_mt, flags,
> +			isolate_pageblock, isolate_pageblock + pageblock_nr_pages);
> +
> +	if (ret)
> +		return ret;
>   
>   	/*
>   	 * Bail out early when the to-be-isolated pageblock does not form
> @@ -374,7 +380,7 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, gfp_t gfp_flags,
>   			struct page *head = compound_head(page);
>   			unsigned long head_pfn = page_to_pfn(head);
>   
> -			if (head_pfn + nr_pages < boundary_pfn) {
> +			if (head_pfn + nr_pages <= boundary_pfn) {
>   				pfn = head_pfn + nr_pages;
>   				continue;
>   			}
> @@ -386,7 +392,8 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, gfp_t gfp_flags,
>   			if (PageHuge(page) || PageLRU(page) || __PageMovable(page)) {
>   				int order;
>   				unsigned long outer_pfn;
> -				int ret;
> +				int page_mt = get_pageblock_migratetype(page);
> +				bool isolate_page = !is_migrate_isolate_page(page);
>   				struct compact_control cc = {
>   					.nr_migratepages = 0,
>   					.order = -1,
> @@ -399,9 +406,31 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, gfp_t gfp_flags,
>   				};
>   				INIT_LIST_HEAD(&cc.migratepages);
>   
> +				/*
> +				 * XXX: mark the page as MIGRATE_ISOLATE so that
> +				 * no one else can grab the freed page after migration.
> +				 * Ideally, the page should be freed as two separate
> +				 * pages to be added into separate migratetype free
> +				 * lists.
> +				 */
> +				if (isolate_page) {
> +					ret = set_migratetype_isolate(page, page_mt,
> +						flags, head_pfn, head_pfn + nr_pages);
> +					if (ret)
> +						goto failed;
> +				}
> +
>   				ret = __alloc_contig_migrate_range(&cc, head_pfn,
>   							head_pfn + nr_pages);
>   
> +				/*
> +				 * restore the page's migratetype so that it can
> +				 * be split into separate migratetype free lists
> +				 * later.
> +				 */
> +				if (isolate_page)
> +					unset_migratetype_isolate(page, page_mt);
> +
>   				if (ret)
>   					goto failed;
>   				/*
> @@ -417,10 +446,9 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, gfp_t gfp_flags,
>   				order = 0;
>   				outer_pfn = pfn;
>   				while (!PageBuddy(pfn_to_page(outer_pfn))) {
> -					if (++order >= MAX_ORDER) {
> -						outer_pfn = pfn;
> -						break;
> -					}
> +					/* stop if we cannot find the free page */
> +					if (++order >= MAX_ORDER)
> +						goto failed;
>   					outer_pfn &= ~0UL << order;
>   				}
>   				pfn = outer_pfn;
> @@ -435,7 +463,7 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, gfp_t gfp_flags,
>   	return 0;
>   failed:
>   	/* restore the original migratetype */
> -	set_pageblock_migratetype(pfn_to_page(isolate_pageblock), saved_mt);
> +	unset_migratetype_isolate(pfn_to_page(isolate_pageblock), saved_mt);
>   	return -EBUSY;
>   }
>   
> @@ -496,12 +524,12 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
>   	int ret;
>   
>   	/* isolate [isolate_start, isolate_start + pageblock_nr_pages) pageblock */
> -	ret = isolate_single_pageblock(isolate_start, gfp_flags, false);
> +	ret = isolate_single_pageblock(isolate_start, flags, gfp_flags, false);
>   	if (ret)
>   		return ret;
>   
>   	/* isolate [isolate_end - pageblock_nr_pages, isolate_end) pageblock */
> -	ret = isolate_single_pageblock(isolate_end, gfp_flags, true);
> +	ret = isolate_single_pageblock(isolate_end, flags, gfp_flags, true);
>   	if (ret) {
>   		unset_migratetype_isolate(pfn_to_page(isolate_start), migratetype);
>   		return ret;

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland