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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 15 Dec 2014, akpm@xxxxxxxxxxxxxxxxxxxx wrote:

> From: Weijie Yang <weijie.yang@xxxxxxxxxxx>
> 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@xxxxxxxxxxx>
> Acked-by: Michal Nazarewicz <mina86@xxxxxxxxxx>
> Reviewed-by: Yasuaki Ishimatsu <isimatu.yasuaki@xxxxxxxxxxxxxx>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> Cc: Minchan Kim <minchan@xxxxxxxxxx>
> Cc: Mel Gorman <mel@xxxxxxxxx>
> Cc: Michal Hocko <mhocko@xxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
> 
>  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 from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]