| Message ID | 20210904092053.33037-1-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/page_isolation: fix potential missing call to unset_migratetype_isolate() | expand |
On 04.09.21 11:20, Miaohe Lin wrote: > In start_isolate_page_range() undo path, pfn_to_online_page() just checks > the first pfn in a pageblock while __first_valid_page() will traverse the > pageblock until the first online pfn is found. So we may miss the call to > unset_migratetype_isolate() in undo path and pages will remain isolated > unexpectedly. Fix this by calling undo_isolate_page_range() and this will > also help to remove some duplicated codes. > > Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages") While that is true, we shouldn't ever trigger, neither via cma, virtio-mem nor memory offlining, because essentially all operate on MAX_ORDER -1 -aligned ranges without memory holes. > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/page_isolation.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/mm/page_isolation.c b/mm/page_isolation.c > index 471e3a13b541..9bb562d5d194 100644 > --- a/mm/page_isolation.c > +++ b/mm/page_isolation.c > @@ -202,14 +202,7 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn, > } > return 0; > undo: > - for (pfn = start_pfn; > - pfn < undo_pfn; > - pfn += pageblock_nr_pages) { > - struct page *page = pfn_to_online_page(pfn); > - if (!page) > - continue; > - unset_migratetype_isolate(page, migratetype); > - } > + undo_isolate_page_range(start_pfn, undo_pfn, migratetype); > It'd be even cleaner to drop the label completely and call it from the single callsite. We can even avoid undo_pfn ... if (page && set_migratetype_isolate(page, migratetype, flags)) { undo_isolate_page_range(start_pfn, pfn, migratetype); return -EBUSY; } If pfn == start_pfn, undo_isolate_page_range() will simply do nothing. > return -EBUSY; > } >
On 2021/9/6 17:27, David Hildenbrand wrote: > On 04.09.21 11:20, Miaohe Lin wrote: >> In start_isolate_page_range() undo path, pfn_to_online_page() just checks >> the first pfn in a pageblock while __first_valid_page() will traverse the >> pageblock until the first online pfn is found. So we may miss the call to >> unset_migratetype_isolate() in undo path and pages will remain isolated >> unexpectedly. Fix this by calling undo_isolate_page_range() and this will >> also help to remove some duplicated codes. >> >> Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages") > > While that is true, we shouldn't ever trigger, neither via cma, virtio-mem nor memory offlining, because essentially all operate on MAX_ORDER -1 -aligned ranges without memory holes. I think this should never trigger too. It's a theoretical issue. So is the Fixes tag necessary ? > >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/page_isolation.c | 9 +-------- >> 1 file changed, 1 insertion(+), 8 deletions(-) >> >> diff --git a/mm/page_isolation.c b/mm/page_isolation.c >> index 471e3a13b541..9bb562d5d194 100644 >> --- a/mm/page_isolation.c >> +++ b/mm/page_isolation.c >> @@ -202,14 +202,7 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn, >> } >> return 0; >> undo: >> - for (pfn = start_pfn; >> - pfn < undo_pfn; >> - pfn += pageblock_nr_pages) { >> - struct page *page = pfn_to_online_page(pfn); >> - if (!page) >> - continue; >> - unset_migratetype_isolate(page, migratetype); >> - } >> + undo_isolate_page_range(start_pfn, undo_pfn, migratetype); >> > > It'd be even cleaner to drop the label completely and call it from the single callsite. We can even avoid undo_pfn ... > > if (page && set_migratetype_isolate(page, migratetype, flags)) { > undo_isolate_page_range(start_pfn, pfn, migratetype); > return -EBUSY; > } > Looks much better. Will do it later. Many thanks. :) > If pfn == start_pfn, undo_isolate_page_range() will simply do nothing. > >> return -EBUSY; >> } >> >
On 06.09.21 11:38, Miaohe Lin wrote: > On 2021/9/6 17:27, David Hildenbrand wrote: >> On 04.09.21 11:20, Miaohe Lin wrote: >>> In start_isolate_page_range() undo path, pfn_to_online_page() just checks >>> the first pfn in a pageblock while __first_valid_page() will traverse the >>> pageblock until the first online pfn is found. So we may miss the call to >>> unset_migratetype_isolate() in undo path and pages will remain isolated >>> unexpectedly. Fix this by calling undo_isolate_page_range() and this will >>> also help to remove some duplicated codes. >>> >>> Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages") >> >> While that is true, we shouldn't ever trigger, neither via cma, virtio-mem nor memory offlining, because essentially all operate on MAX_ORDER -1 -aligned ranges without memory holes. > > I think this should never trigger too. It's a theoretical issue. So is the Fixes tag necessary ? > I think it's one of these "let's add Fixes: but no need for Cc: stable". BUT in older kernels we could have triggered this via memory offlining eventually ... before c5e79ef561b0 ("mm/memory_hotplug.c: don't allow to online/offline memory blocks with holes") ... so maybe even a Cc: stable?
On 2021/9/6 17:46, David Hildenbrand wrote: > On 06.09.21 11:38, Miaohe Lin wrote: >> On 2021/9/6 17:27, David Hildenbrand wrote: >>> On 04.09.21 11:20, Miaohe Lin wrote: >>>> In start_isolate_page_range() undo path, pfn_to_online_page() just checks >>>> the first pfn in a pageblock while __first_valid_page() will traverse the >>>> pageblock until the first online pfn is found. So we may miss the call to >>>> unset_migratetype_isolate() in undo path and pages will remain isolated >>>> unexpectedly. Fix this by calling undo_isolate_page_range() and this will >>>> also help to remove some duplicated codes. >>>> >>>> Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages") >>> >>> While that is true, we shouldn't ever trigger, neither via cma, virtio-mem nor memory offlining, because essentially all operate on MAX_ORDER -1 -aligned ranges without memory holes. >> >> I think this should never trigger too. It's a theoretical issue. So is the Fixes tag necessary ? >> > > I think it's one of these "let's add Fixes: but no need for Cc: stable". > > BUT in older kernels we could have triggered this via memory offlining eventually ... before c5e79ef561b0 ("mm/memory_hotplug.c: don't allow to online/offline memory blocks with holes") ... so maybe even a Cc: stable? Looks like it could happen in older kernels. Maybe a Cc: stable is necessary. Many thanks. >
diff --git a/mm/page_isolation.c b/mm/page_isolation.c index 471e3a13b541..9bb562d5d194 100644 --- a/mm/page_isolation.c +++ b/mm/page_isolation.c @@ -202,14 +202,7 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn, } return 0; undo: - for (pfn = start_pfn; - pfn < undo_pfn; - pfn += pageblock_nr_pages) { - struct page *page = pfn_to_online_page(pfn); - if (!page) - continue; - unset_migratetype_isolate(page, migratetype); - } + undo_isolate_page_range(start_pfn, undo_pfn, migratetype); return -EBUSY; }
In start_isolate_page_range() undo path, pfn_to_online_page() just checks the first pfn in a pageblock while __first_valid_page() will traverse the pageblock until the first online pfn is found. So we may miss the call to unset_migratetype_isolate() in undo path and pages will remain isolated unexpectedly. Fix this by calling undo_isolate_page_range() and this will also help to remove some duplicated codes. Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages") Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/page_isolation.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)