Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
On Mon, Jan 07, 2019 at 07:38:20PM +0100, Vitaly Kuznetsov wrote: Sasha Levin writes: On Mon, Jan 07, 2019 at 02:44:30PM +0100, Vitaly Kuznetsov wrote: P.S. I still think about bringing mem_hotplug_begin()/done() to hv_balloon but that's going to be a separate discussion, here I want to have a small fix backportable to stable. This should probably be marked for stable then :) Yes, please :-) Queued up for hyperv-fixes with a -stable tag, thank you! -- Thanks, Sasha ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
Dan Carpenter writes: > On Mon, Jan 07, 2019 at 07:38:20PM +0100, Vitaly Kuznetsov wrote: >> (I remember Greg disliked when people were tagging patches for stable@ >> themselves, he prefered maintainers deciding if the particular commit >> deserves stable@ or not - but as you have a tree now we may as well have >> different rules :-) > > You're thinking of Dave Miller. So far as I know he's the only > maintainer who handles stable that way. This may actually be the case, you think "one of the the mighty Linux overlords" but your fingers type "Greg" instread. Sorry for the confusion :-) -- Vitaly ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
On Mon, Jan 07, 2019 at 07:38:20PM +0100, Vitaly Kuznetsov wrote: > (I remember Greg disliked when people were tagging patches for stable@ > themselves, he prefered maintainers deciding if the particular commit > deserves stable@ or not - but as you have a tree now we may as well have > different rules :-) You're thinking of Dave Miller. So far as I know he's the only maintainer who handles stable that way. It's a lot of work to do the things Dave does. #understatement regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
David Hildenbrand writes: > On 07.01.19 14:44, Vitaly Kuznetsov wrote: >> David Hildenbrand writes: >> ... >>> On 04.01.19 15:19, Vitaly Kuznetsov wrote: if (start_pfn > has->start_pfn && - !PageReserved(pfn_to_page(start_pfn - 1))) + online_section_nr(pfn_to_section_nr(start_pfn))) hv_bring_pgs_online(has, start_pfn, pgs_ol); } >>> >>> I wonder if you should use pfn_to_online_page() and check for PageOffline(). >>> >>> (I guess online_section_nr() should also do the trick) >> >> I'm worried a bit about racing with mm code here as we're not doing >> mem_hotplug_begin()/done() so I'd slightly prefer keeping >> online_section_nr() (pfn_to_online_page() also uses it but then it gets >> to the particular struct page). Moreover, with pfn_to_online_page() we >> will be looking at some other pfn - because the start_pfn is definitelly >> offline (pre-patch we were looking at start_pfn-1). Just looking at the >> whole section seems cleaner. > > Fine with me. I guess the section can never be offlined as it still > contains reserved pages if not fully "fake-onlined" by HV code already. > > But we could still consider mem_hotplug_begin()/done() as we could have > a online section although online_pages() has not completed yet. So we > could actually touch some "semi onlined section". Yes, exactly, if we race with section onlining here we may never online the tail so it will remain 'semi onlined'. I'm going to propose exporting mem_hotplug_begin()/done() to modules to fix this (in a separate patch because I anticipate some pushback :-) > > Acked-by: David Hildenbrand > Thanks! -- Vitaly ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
On 07.01.19 14:44, Vitaly Kuznetsov wrote: > David Hildenbrand writes: > >> On 04.01.19 15:19, Vitaly Kuznetsov wrote: >>> Hyper-V memory hotplug protocol has 2M granularity and in Linux x86 we use >>> 128M. To deal with it we implement partial section onlining by registering >>> custom page onlining callback (hv_online_page()). Later, when more memory >>> arrives we try to online the 'tail' (see hv_bring_pgs_online()). >>> >>> It was found that in some cases this 'tail' onlining causes issues: >>> >>> BUG: Bad page state in process kworker/0:2 pfn:109e3a >>> page:e08344278e80 count:0 mapcount:1 mapping: index:0x0 >>> flags: 0xf8000() >>> raw: 000f8000 dead0100 dead0200 >>> raw: >>> page dumped because: nonzero mapcount >>> ... >>> Workqueue: events hot_add_req [hv_balloon] >>> Call Trace: >>> dump_stack+0x5c/0x80 >>> bad_page.cold.112+0x7f/0xb2 >>> free_pcppages_bulk+0x4b8/0x690 >>> free_unref_page+0x54/0x70 >>> hv_page_online_one+0x5c/0x80 [hv_balloon] >>> hot_add_req.cold.24+0x182/0x835 [hv_balloon] >>> ... >>> >>> Turns out that we now have deferred struct page initialization for memory >>> hotplug so e.g. memory_block_action() in drivers/base/memory.c does >>> pages_correctly_probed() check and in that check it avoids inspecting >>> struct pages and checks sections instead. But in Hyper-V balloon driver we >>> do PageReserved(pfn_to_page()) check and this is now wrong. >>> >>> Switch to checking online_section_nr() instead. >>> >>> Signed-off-by: Vitaly Kuznetsov >>> --- >>> drivers/hv/hv_balloon.c | 10 ++ >>> 1 file changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c >>> index 5301fef16c31..7c6349a50ef1 100644 >>> --- a/drivers/hv/hv_balloon.c >>> +++ b/drivers/hv/hv_balloon.c >>> @@ -888,12 +888,14 @@ static unsigned long handle_pg_range(unsigned long >>> pg_start, >>> pfn_cnt -= pgs_ol; >>> /* >>> * Check if the corresponding memory block is already >>> -* online by checking its last previously backed page. >>> -* In case it is we need to bring rest (which was not >>> -* backed previously) online too. >>> +* online. It is possible to observe struct pages still >>> +* being uninitialized here so check section instead. >>> +* In case the section is online we need to bring the >>> +* rest of pfns (which were not backed previously) >>> +* online too. >>> */ >>> if (start_pfn > has->start_pfn && >>> - !PageReserved(pfn_to_page(start_pfn - 1))) >>> + online_section_nr(pfn_to_section_nr(start_pfn))) >>> hv_bring_pgs_online(has, start_pfn, pgs_ol); >>> >>> } >>> >> >> I wonder if you should use pfn_to_online_page() and check for PageOffline(). >> >> (I guess online_section_nr() should also do the trick) > > I'm worried a bit about racing with mm code here as we're not doing > mem_hotplug_begin()/done() so I'd slightly prefer keeping > online_section_nr() (pfn_to_online_page() also uses it but then it gets > to the particular struct page). Moreover, with pfn_to_online_page() we > will be looking at some other pfn - because the start_pfn is definitelly > offline (pre-patch we were looking at start_pfn-1). Just looking at the > whole section seems cleaner. Fine with me. I guess the section can never be offlined as it still contains reserved pages if not fully "fake-onlined" by HV code already. But we could still consider mem_hotplug_begin()/done() as we could have a online section although online_pages() has not completed yet. So we could actually touch some "semi onlined section". Acked-by: David Hildenbrand > > P.S. I still think about bringing mem_hotplug_begin()/done() to > hv_balloon but that's going to be a separate discussion, here I want to > have a small fix backportable to stable. > > Thanks, > -- Thanks, David / dhildenb ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
On Mon, Jan 07, 2019 at 07:38:20PM +0100, Vitaly Kuznetsov wrote: > (I remember Greg disliked when people were tagging patches for stable@ > themselves, he prefered maintainers deciding if the particular commit > deserves stable@ or not - but as you have a tree now we may as well have > different rules :-) I don't recall ever saying I disliked that, unless people were tagging stuff that were obviously not stable material. Which, given the history of this codebase, was probably the case :) greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
Sasha Levin writes: > On Mon, Jan 07, 2019 at 02:44:30PM +0100, Vitaly Kuznetsov wrote: >>P.S. I still think about bringing mem_hotplug_begin()/done() to >>hv_balloon but that's going to be a separate discussion, here I want to >>have a small fix backportable to stable. > > This should probably be marked for stable then :) > Yes, please :-) I didn't test old kernels but this seems to be an old issue, e.g. DEFERRED_STRUCT_PAGE_INIT appeared in 4.2 - not sure if memory hotplug code was using it back then but, oh well, it's all history now. (I remember Greg disliked when people were tagging patches for stable@ themselves, he prefered maintainers deciding if the particular commit deserves stable@ or not - but as you have a tree now we may as well have different rules :-) -- Vitaly ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
On Mon, Jan 07, 2019 at 02:44:30PM +0100, Vitaly Kuznetsov wrote: David Hildenbrand writes: On 04.01.19 15:19, Vitaly Kuznetsov wrote: Hyper-V memory hotplug protocol has 2M granularity and in Linux x86 we use 128M. To deal with it we implement partial section onlining by registering custom page onlining callback (hv_online_page()). Later, when more memory arrives we try to online the 'tail' (see hv_bring_pgs_online()). It was found that in some cases this 'tail' onlining causes issues: BUG: Bad page state in process kworker/0:2 pfn:109e3a page:e08344278e80 count:0 mapcount:1 mapping: index:0x0 flags: 0xf8000() raw: 000f8000 dead0100 dead0200 raw: page dumped because: nonzero mapcount ... Workqueue: events hot_add_req [hv_balloon] Call Trace: dump_stack+0x5c/0x80 bad_page.cold.112+0x7f/0xb2 free_pcppages_bulk+0x4b8/0x690 free_unref_page+0x54/0x70 hv_page_online_one+0x5c/0x80 [hv_balloon] hot_add_req.cold.24+0x182/0x835 [hv_balloon] ... Turns out that we now have deferred struct page initialization for memory hotplug so e.g. memory_block_action() in drivers/base/memory.c does pages_correctly_probed() check and in that check it avoids inspecting struct pages and checks sections instead. But in Hyper-V balloon driver we do PageReserved(pfn_to_page()) check and this is now wrong. Switch to checking online_section_nr() instead. Signed-off-by: Vitaly Kuznetsov --- drivers/hv/hv_balloon.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c index 5301fef16c31..7c6349a50ef1 100644 --- a/drivers/hv/hv_balloon.c +++ b/drivers/hv/hv_balloon.c @@ -888,12 +888,14 @@ static unsigned long handle_pg_range(unsigned long pg_start, pfn_cnt -= pgs_ol; /* * Check if the corresponding memory block is already -* online by checking its last previously backed page. -* In case it is we need to bring rest (which was not -* backed previously) online too. +* online. It is possible to observe struct pages still +* being uninitialized here so check section instead. +* In case the section is online we need to bring the +* rest of pfns (which were not backed previously) +* online too. */ if (start_pfn > has->start_pfn && - !PageReserved(pfn_to_page(start_pfn - 1))) + online_section_nr(pfn_to_section_nr(start_pfn))) hv_bring_pgs_online(has, start_pfn, pgs_ol); } I wonder if you should use pfn_to_online_page() and check for PageOffline(). (I guess online_section_nr() should also do the trick) I'm worried a bit about racing with mm code here as we're not doing mem_hotplug_begin()/done() so I'd slightly prefer keeping online_section_nr() (pfn_to_online_page() also uses it but then it gets to the particular struct page). Moreover, with pfn_to_online_page() we will be looking at some other pfn - because the start_pfn is definitelly offline (pre-patch we were looking at start_pfn-1). Just looking at the whole section seems cleaner. P.S. I still think about bringing mem_hotplug_begin()/done() to hv_balloon but that's going to be a separate discussion, here I want to have a small fix backportable to stable. This should probably be marked for stable then :) -- Thanks, Sasha ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
David Hildenbrand writes: > On 04.01.19 15:19, Vitaly Kuznetsov wrote: >> Hyper-V memory hotplug protocol has 2M granularity and in Linux x86 we use >> 128M. To deal with it we implement partial section onlining by registering >> custom page onlining callback (hv_online_page()). Later, when more memory >> arrives we try to online the 'tail' (see hv_bring_pgs_online()). >> >> It was found that in some cases this 'tail' onlining causes issues: >> >> BUG: Bad page state in process kworker/0:2 pfn:109e3a >> page:e08344278e80 count:0 mapcount:1 mapping: index:0x0 >> flags: 0xf8000() >> raw: 000f8000 dead0100 dead0200 >> raw: >> page dumped because: nonzero mapcount >> ... >> Workqueue: events hot_add_req [hv_balloon] >> Call Trace: >> dump_stack+0x5c/0x80 >> bad_page.cold.112+0x7f/0xb2 >> free_pcppages_bulk+0x4b8/0x690 >> free_unref_page+0x54/0x70 >> hv_page_online_one+0x5c/0x80 [hv_balloon] >> hot_add_req.cold.24+0x182/0x835 [hv_balloon] >> ... >> >> Turns out that we now have deferred struct page initialization for memory >> hotplug so e.g. memory_block_action() in drivers/base/memory.c does >> pages_correctly_probed() check and in that check it avoids inspecting >> struct pages and checks sections instead. But in Hyper-V balloon driver we >> do PageReserved(pfn_to_page()) check and this is now wrong. >> >> Switch to checking online_section_nr() instead. >> >> Signed-off-by: Vitaly Kuznetsov >> --- >> drivers/hv/hv_balloon.c | 10 ++ >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c >> index 5301fef16c31..7c6349a50ef1 100644 >> --- a/drivers/hv/hv_balloon.c >> +++ b/drivers/hv/hv_balloon.c >> @@ -888,12 +888,14 @@ static unsigned long handle_pg_range(unsigned long >> pg_start, >> pfn_cnt -= pgs_ol; >> /* >> * Check if the corresponding memory block is already >> - * online by checking its last previously backed page. >> - * In case it is we need to bring rest (which was not >> - * backed previously) online too. >> + * online. It is possible to observe struct pages still >> + * being uninitialized here so check section instead. >> + * In case the section is online we need to bring the >> + * rest of pfns (which were not backed previously) >> + * online too. >> */ >> if (start_pfn > has->start_pfn && >> -!PageReserved(pfn_to_page(start_pfn - 1))) >> +online_section_nr(pfn_to_section_nr(start_pfn))) >> hv_bring_pgs_online(has, start_pfn, pgs_ol); >> >> } >> > > I wonder if you should use pfn_to_online_page() and check for PageOffline(). > > (I guess online_section_nr() should also do the trick) I'm worried a bit about racing with mm code here as we're not doing mem_hotplug_begin()/done() so I'd slightly prefer keeping online_section_nr() (pfn_to_online_page() also uses it but then it gets to the particular struct page). Moreover, with pfn_to_online_page() we will be looking at some other pfn - because the start_pfn is definitelly offline (pre-patch we were looking at start_pfn-1). Just looking at the whole section seems cleaner. P.S. I still think about bringing mem_hotplug_begin()/done() to hv_balloon but that's going to be a separate discussion, here I want to have a small fix backportable to stable. Thanks, -- Vitaly ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
On 04.01.19 15:19, Vitaly Kuznetsov wrote: > Hyper-V memory hotplug protocol has 2M granularity and in Linux x86 we use > 128M. To deal with it we implement partial section onlining by registering > custom page onlining callback (hv_online_page()). Later, when more memory > arrives we try to online the 'tail' (see hv_bring_pgs_online()). > > It was found that in some cases this 'tail' onlining causes issues: > > BUG: Bad page state in process kworker/0:2 pfn:109e3a > page:e08344278e80 count:0 mapcount:1 mapping: index:0x0 > flags: 0xf8000() > raw: 000f8000 dead0100 dead0200 > raw: > page dumped because: nonzero mapcount > ... > Workqueue: events hot_add_req [hv_balloon] > Call Trace: > dump_stack+0x5c/0x80 > bad_page.cold.112+0x7f/0xb2 > free_pcppages_bulk+0x4b8/0x690 > free_unref_page+0x54/0x70 > hv_page_online_one+0x5c/0x80 [hv_balloon] > hot_add_req.cold.24+0x182/0x835 [hv_balloon] > ... > > Turns out that we now have deferred struct page initialization for memory > hotplug so e.g. memory_block_action() in drivers/base/memory.c does > pages_correctly_probed() check and in that check it avoids inspecting > struct pages and checks sections instead. But in Hyper-V balloon driver we > do PageReserved(pfn_to_page()) check and this is now wrong. > > Switch to checking online_section_nr() instead. > > Signed-off-by: Vitaly Kuznetsov > --- > drivers/hv/hv_balloon.c | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c > index 5301fef16c31..7c6349a50ef1 100644 > --- a/drivers/hv/hv_balloon.c > +++ b/drivers/hv/hv_balloon.c > @@ -888,12 +888,14 @@ static unsigned long handle_pg_range(unsigned long > pg_start, > pfn_cnt -= pgs_ol; > /* >* Check if the corresponding memory block is already > - * online by checking its last previously backed page. > - * In case it is we need to bring rest (which was not > - * backed previously) online too. > + * online. It is possible to observe struct pages still > + * being uninitialized here so check section instead. > + * In case the section is online we need to bring the > + * rest of pfns (which were not backed previously) > + * online too. >*/ > if (start_pfn > has->start_pfn && > - !PageReserved(pfn_to_page(start_pfn - 1))) > + online_section_nr(pfn_to_section_nr(start_pfn))) > hv_bring_pgs_online(has, start_pfn, pgs_ol); > > } > I wonder if you should use pfn_to_online_page() and check for PageOffline(). (I guess online_section_nr() should also do the trick) -- Thanks, David / dhildenb ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
