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