Re: [patch 1/6] mm: page_isolation: check pfn validity before access

[David Rientjes <rientjes@google.com>]

On Mon, 15 Dec 2014, akpm@linux-foundation.org wrote:

> From: Weijie Yang <weijie.yang@samsung.com>
> Subject: mm: page_isolation: check pfn validity before access
> 
> In the undo path of start_isolate_page_range(), we need to check the pfn
> validity before accessing its page, or it will trigger an addressing
> exception if there is hole in the zone.
> 
> This issue is found by code-review not a test-trigger.  In
> "CONFIG_HOLES_IN_ZONE" environment, there is a certain chance that it
> would casue an addressing exception when start_isolate_page_range()
> fails, this could affect CMA, hugepage and memory-hotplug function.
> 
> Signed-off-by: Weijie Yang <weijie.yang@samsung.com>
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
> Reviewed-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Mel Gorman <mel@csn.ul.ie>
> Cc: Michal Hocko <mhocko@suse.cz>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  mm/page_isolation.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff -puN mm/page_isolation.c~mm-page_isolation-check-pfn-validity-before-access mm/page_isolation.c
> --- a/mm/page_isolation.c~mm-page_isolation-check-pfn-validity-before-access
> +++ a/mm/page_isolation.c
> @@ -176,8 +176,11 @@ int start_isolate_page_range(unsigned lo
>  undo:
>  	for (pfn = start_pfn;
>  	     pfn < undo_pfn;
> -	     pfn += pageblock_nr_pages)
> -		unset_migratetype_isolate(pfn_to_page(pfn), migratetype);
> +	     pfn += pageblock_nr_pages) {
> +		page = __first_valid_page(pfn, pageblock_nr_pages);
> +		if (page)
> +			unset_migratetype_isolate(page, migratetype);
> +	}
>  
>  	return -EBUSY;
>  }

This is such an interesting patch because of who acked it and the two 
callers of the function that seem to want different behavior.

The behavior of start_isolate_page_range() is currently to either set the 
migratetype of the pageblocks to MIGRATE_ISOLATE or allow the pageblocks 
to have no valid pages due to a memory hole.

The memory hotplug usecase makes perfect sense since it's entirely 
legitimate to offline memory holes and we would not want to return -EBUSY, 
but that doesn't seem to be what the implementation of 
start_isolate_page_range() is this undo behavior expects pfn_to_page(pfn) 
to be valid up to undo_pfn.

I'm not a CMA expert, but I'm surprised that we want to return success 
here if some pageblocks are actually memory holes.  Don't we want to 
return -EBUSY for such a range?  That seems to be more in line with the 
comment for start_isolate_page_range() which specifies it returns "-EBUSY 
if any part of range cannot be isolated", which would seem to imply memory 
holes as well, but that doesn't match its implementation.

So there's two radically different expectations for this function with 
regard to invalid pfns.  Which one do we want?

If we want it to simply disregard memory holes (memory hotplug), then ack 
the patch with a follow-up to fix the comment.  If we want it to undo on 
memory holes (CMA), then nack the patch since its current implementation 
is correct and we need to fix memory hotplug.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>