* [patch 1/6] mm: page_isolation: check pfn validity before access
@ 2014-12-15 23:03 akpm
2014-12-17 23:56 ` David Rientjes
0 siblings, 1 reply; 3+ messages in thread
From: akpm @ 2014-12-15 23:03 UTC (permalink / raw)
To: linux-mm, akpm, weijie.yang, isimatu.yasuaki, kamezawa.hiroyu,
mel, mhocko, mina86, minchan, stable
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;
}
_
--
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>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [patch 1/6] mm: page_isolation: check pfn validity before access
2014-12-15 23:03 [patch 1/6] mm: page_isolation: check pfn validity before access akpm
@ 2014-12-17 23:56 ` David Rientjes
2014-12-19 2:01 ` Minchan Kim
0 siblings, 1 reply; 3+ messages in thread
From: David Rientjes @ 2014-12-17 23:56 UTC (permalink / raw)
To: akpm
Cc: linux-mm, weijie.yang, isimatu.yasuaki, kamezawa.hiroyu, mel,
mhocko, mina86, minchan, stable
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>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [patch 1/6] mm: page_isolation: check pfn validity before access
2014-12-17 23:56 ` David Rientjes
@ 2014-12-19 2:01 ` Minchan Kim
0 siblings, 0 replies; 3+ messages in thread
From: Minchan Kim @ 2014-12-19 2:01 UTC (permalink / raw)
To: David Rientjes
Cc: akpm, linux-mm, weijie.yang, isimatu.yasuaki, kamezawa.hiroyu,
mel, mhocko, mina86, stable
On Wed, Dec 17, 2014 at 03:56:08PM -0800, David Rientjes wrote:
> 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.
Can CMA have memory hole?
CMA user should allocate CMA area with cma_declare_contiguous which uses
memblock. I'm not familiar with memblock but I don't think it's possible.
>
> 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>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-12-19 2:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-15 23:03 [patch 1/6] mm: page_isolation: check pfn validity before access akpm
2014-12-17 23:56 ` David Rientjes
2014-12-19 2:01 ` Minchan Kim
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.