* [PATCH v2 1/1] check that order of free pages falls within valid range @ 2022-04-08 8:52 Alexander Egorenkov 2022-04-08 13:32 ` Alexander Egorenkov 0 siblings, 1 reply; 5+ messages in thread From: Alexander Egorenkov @ 2022-04-08 8:52 UTC (permalink / raw) To: kexec This change makes __exclude_unnecessary_pages() more robust by verifying that the order of a free page is valid before computing the size of its memory block in the buddy system. The order of a free page cannot be larger than (MAX_ORDER - 1) because the array 'zone.free_area' is of size MAX_ORDER. This situation is reproducible with some s390x dumps: __exclude_unnecessary_pages: Invalid free page order: pfn=2690c0, order=52, max order=8 References: - https://listman.redhat.com/archives/crash-utility/2021-September/009204.html - https://www.kernel.org/doc/gorman/html/understand/understand009.html Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com> --- makedumpfile.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/makedumpfile.c b/makedumpfile.c index 2ef345879524..56aa026e7b34 100644 --- a/makedumpfile.c +++ b/makedumpfile.c @@ -6457,6 +6457,12 @@ __exclude_unnecessary_pages(unsigned long mem_map, else if ((info->dump_level & DL_EXCLUDE_FREE) && info->page_is_buddy && info->page_is_buddy(flags, _mapcount, private, _count)) { + if (private >= ARRAY_LENGTH(zone.free_area)) { + ERRMSG("Invalid free page order: pfn=%llx, order=%lu, max order=%lu\n", + pfn, private, ARRAY_LENGTH(zone.free_area) - 1); + free(page_cache); + return FALSE; + } nr_pages = 1 << private; pfn_counter = &pfn_free; } -- 2.34.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 1/1] check that order of free pages falls within valid range 2022-04-08 8:52 [PATCH v2 1/1] check that order of free pages falls within valid range Alexander Egorenkov @ 2022-04-08 13:32 ` Alexander Egorenkov 2022-04-11 1:53 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 0 siblings, 1 reply; 5+ messages in thread From: Alexander Egorenkov @ 2022-04-08 13:32 UTC (permalink / raw) To: kexec Alexander Egorenkov <egorenar@linux.ibm.com> writes: > This change makes __exclude_unnecessary_pages() more robust by > verifying that the order of a free page is valid before computing the size > of its memory block in the buddy system. > > The order of a free page cannot be larger than (MAX_ORDER - 1) because > the array 'zone.free_area' is of size MAX_ORDER. > > This situation is reproducible with some s390x dumps: > > __exclude_unnecessary_pages: Invalid free page order: pfn=2690c0, order=52, max order=8 > > References: > - https://listman.redhat.com/archives/crash-utility/2021-September/009204.html > - https://www.kernel.org/doc/gorman/html/understand/understand009.html > > Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com> > --- > makedumpfile.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/makedumpfile.c b/makedumpfile.c > index 2ef345879524..56aa026e7b34 100644 > --- a/makedumpfile.c > +++ b/makedumpfile.c > @@ -6457,6 +6457,12 @@ __exclude_unnecessary_pages(unsigned long mem_map, > else if ((info->dump_level & DL_EXCLUDE_FREE) > && info->page_is_buddy > && info->page_is_buddy(flags, _mapcount, private, _count)) { > + if (private >= ARRAY_LENGTH(zone.free_area)) { > + ERRMSG("Invalid free page order: pfn=%llx, order=%lu, max order=%lu\n", > + pfn, private, ARRAY_LENGTH(zone.free_area) - 1); > + free(page_cache); > + return FALSE; > + } > nr_pages = 1 << private; > pfn_counter = &pfn_free; > } > -- > 2.34.1 > > > _______________________________________________ > kexec mailing list > kexec at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kexec I found out when this can happen. If e.g. a driver calls free_pages() and gives an order > max page order, then __free_one_page() stores the given invalid page order in the 'private' member of struct page and gives it back to the buddy allocator. This is what actually happened in the dump i used to reproduce this issue with makedumpfile. Regards Alex ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/1] check that order of free pages falls within valid range 2022-04-08 13:32 ` Alexander Egorenkov @ 2022-04-11 1:53 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 2022-04-11 5:33 ` Alexander Egorenkov 0 siblings, 1 reply; 5+ messages in thread From: HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= @ 2022-04-11 1:53 UTC (permalink / raw) To: kexec Hi Alex, thanks for the patch, -----Original Message----- > Alexander Egorenkov <egorenar@linux.ibm.com> writes: > > > This change makes __exclude_unnecessary_pages() more robust by > > verifying that the order of a free page is valid before computing the size > > of its memory block in the buddy system. > > > > The order of a free page cannot be larger than (MAX_ORDER - 1) because > > the array 'zone.free_area' is of size MAX_ORDER. > > > > This situation is reproducible with some s390x dumps: > > > > __exclude_unnecessary_pages: Invalid free page order: pfn=2690c0, order=52, max order=8 > > > > References: > > - https://listman.redhat.com/archives/crash-utility/2021-September/009204.html > > - https://www.kernel.org/doc/gorman/html/understand/understand009.html > > > > Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com> > > --- > > makedumpfile.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/makedumpfile.c b/makedumpfile.c > > index 2ef345879524..56aa026e7b34 100644 > > --- a/makedumpfile.c > > +++ b/makedumpfile.c > > @@ -6457,6 +6457,12 @@ __exclude_unnecessary_pages(unsigned long mem_map, > > else if ((info->dump_level & DL_EXCLUDE_FREE) > > && info->page_is_buddy > > && info->page_is_buddy(flags, _mapcount, private, _count)) { > > + if (private >= ARRAY_LENGTH(zone.free_area)) { > > + ERRMSG("Invalid free page order: pfn=%llx, order=%lu, max order=%lu\n", > > + pfn, private, ARRAY_LENGTH(zone.free_area) - 1); > > + free(page_cache); > > + return FALSE; > > + } > > nr_pages = 1 << private; > > pfn_counter = &pfn_free; > > } > > -- > > 2.34.1 > > > > > > _______________________________________________ > > kexec mailing list > > kexec at lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/kexec > > I found out when this can happen. > > If e.g. a driver calls free_pages() and gives an order > max page order, > then __free_one_page() stores the given invalid page order in the > 'private' member of struct page and gives it back to the buddy > allocator. > > This is what actually happened in the dump i used to reproduce this issue > with makedumpfile. Good catch, though I could not reproduce it so far.. but I wonder whether we have no other choice than returning FALSE? in other words, can't we skip (include) the invalid page with a warning message? As I said before, I think that capturing more pages than expected will be better than not capturing a dump, and that is "robust" against unexpected values. Thanks, Kazu ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/1] check that order of free pages falls within valid range 2022-04-11 1:53 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= @ 2022-04-11 5:33 ` Alexander Egorenkov 2022-04-11 6:11 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 0 siblings, 1 reply; 5+ messages in thread From: Alexander Egorenkov @ 2022-04-11 5:33 UTC (permalink / raw) To: kexec Hi Kazu, HAGIO KAZUHITO(?????) <k-hagio-ab@nec.com> writes: > Hi Alex, > > thanks for the patch, > > -----Original Message----- >> Alexander Egorenkov <egorenar@linux.ibm.com> writes: >> >> > This change makes __exclude_unnecessary_pages() more robust by >> > verifying that the order of a free page is valid before computing the size >> > of its memory block in the buddy system. >> > >> > The order of a free page cannot be larger than (MAX_ORDER - 1) because >> > the array 'zone.free_area' is of size MAX_ORDER. >> > >> > This situation is reproducible with some s390x dumps: >> > >> > __exclude_unnecessary_pages: Invalid free page order: pfn=2690c0, order=52, max order=8 >> > >> > References: >> > - https://listman.redhat.com/archives/crash-utility/2021-September/009204.html >> > - https://www.kernel.org/doc/gorman/html/understand/understand009.html >> > >> > Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com> >> > --- >> > makedumpfile.c | 6 ++++++ >> > 1 file changed, 6 insertions(+) >> > >> > diff --git a/makedumpfile.c b/makedumpfile.c >> > index 2ef345879524..56aa026e7b34 100644 >> > --- a/makedumpfile.c >> > +++ b/makedumpfile.c >> > @@ -6457,6 +6457,12 @@ __exclude_unnecessary_pages(unsigned long mem_map, >> > else if ((info->dump_level & DL_EXCLUDE_FREE) >> > && info->page_is_buddy >> > && info->page_is_buddy(flags, _mapcount, private, _count)) { >> > + if (private >= ARRAY_LENGTH(zone.free_area)) { >> > + ERRMSG("Invalid free page order: pfn=%llx, order=%lu, max order=%lu\n", >> > + pfn, private, ARRAY_LENGTH(zone.free_area) - 1); >> > + free(page_cache); >> > + return FALSE; >> > + } >> > nr_pages = 1 << private; >> > pfn_counter = &pfn_free; >> > } >> > -- >> > 2.34.1 >> > >> > >> > _______________________________________________ >> > kexec mailing list >> > kexec at lists.infradead.org >> > http://lists.infradead.org/mailman/listinfo/kexec >> >> I found out when this can happen. >> >> If e.g. a driver calls free_pages() and gives an order > max page order, >> then __free_one_page() stores the given invalid page order in the >> 'private' member of struct page and gives it back to the buddy >> allocator. >> >> This is what actually happened in the dump i used to reproduce this issue >> with makedumpfile. > > Good catch, though I could not reproduce it so far.. > > but I wonder whether we have no other choice than returning FALSE? > in other words, can't we skip (include) the invalid page with a > warning message? > > As I said before, I think that capturing more pages than expected > will be better than not capturing a dump, and that is "robust" > against unexpected values. I chose failing to an recover attempt because i was not sure that it won't have negative effects on other pages. Your suggestion is replacing ERRMSG with a warning and then just continue ? I can rework the patch. Thanks Regards Alex ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/1] check that order of free pages falls within valid range 2022-04-11 5:33 ` Alexander Egorenkov @ 2022-04-11 6:11 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 0 siblings, 0 replies; 5+ messages in thread From: HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= @ 2022-04-11 6:11 UTC (permalink / raw) To: kexec -----Original Message----- > > Hi Kazu, > > HAGIO KAZUHITO(?????) <k-hagio-ab@nec.com> writes: > > > Hi Alex, > > > > thanks for the patch, > > > > -----Original Message----- > >> Alexander Egorenkov <egorenar@linux.ibm.com> writes: > >> > >> > This change makes __exclude_unnecessary_pages() more robust by > >> > verifying that the order of a free page is valid before computing the size > >> > of its memory block in the buddy system. > >> > > >> > The order of a free page cannot be larger than (MAX_ORDER - 1) because > >> > the array 'zone.free_area' is of size MAX_ORDER. > >> > > >> > This situation is reproducible with some s390x dumps: > >> > > >> > __exclude_unnecessary_pages: Invalid free page order: pfn=2690c0, order=52, max order=8 > >> > > >> > References: > >> > - https://listman.redhat.com/archives/crash-utility/2021-September/009204.html > >> > - https://www.kernel.org/doc/gorman/html/understand/understand009.html > >> > > >> > Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com> > >> > --- > >> > makedumpfile.c | 6 ++++++ > >> > 1 file changed, 6 insertions(+) > >> > > >> > diff --git a/makedumpfile.c b/makedumpfile.c > >> > index 2ef345879524..56aa026e7b34 100644 > >> > --- a/makedumpfile.c > >> > +++ b/makedumpfile.c > >> > @@ -6457,6 +6457,12 @@ __exclude_unnecessary_pages(unsigned long mem_map, > >> > else if ((info->dump_level & DL_EXCLUDE_FREE) > >> > && info->page_is_buddy > >> > && info->page_is_buddy(flags, _mapcount, private, _count)) { > >> > + if (private >= ARRAY_LENGTH(zone.free_area)) { > >> > + ERRMSG("Invalid free page order: pfn=%llx, order=%lu, max order=%lu\n", > >> > + pfn, private, ARRAY_LENGTH(zone.free_area) - 1); > >> > + free(page_cache); > >> > + return FALSE; > >> > + } > >> > nr_pages = 1 << private; > >> > pfn_counter = &pfn_free; > >> > } > >> > -- > >> > 2.34.1 > >> > > >> > > >> > _______________________________________________ > >> > kexec mailing list > >> > kexec at lists.infradead.org > >> > http://lists.infradead.org/mailman/listinfo/kexec > >> > >> I found out when this can happen. > >> > >> If e.g. a driver calls free_pages() and gives an order > max page order, > >> then __free_one_page() stores the given invalid page order in the > >> 'private' member of struct page and gives it back to the buddy > >> allocator. > >> > >> This is what actually happened in the dump i used to reproduce this issue > >> with makedumpfile. > > > > Good catch, though I could not reproduce it so far.. > > > > but I wonder whether we have no other choice than returning FALSE? > > in other words, can't we skip (include) the invalid page with a > > warning message? > > > > As I said before, I think that capturing more pages than expected > > will be better than not capturing a dump, and that is "robust" > > against unexpected values. > > I chose failing to an recover attempt because i was not sure > that it won't have negative effects on other pages. ah, I forgot but the issue was an infinite loop, so you chose failing to avoid the same situation with negative effects on other pages, right? I understand it. hmm, but I think that it's too sensitive to stop makedumpfile with an invalid value, it might be possible to complete dumping. If it has negative effects, let's fix it again. > Your suggestion is replacing ERRMSG with a warning and then just > continue ? I can rework the patch. Yes, please. Thanks, Kazu ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-04-11 6:11 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-08 8:52 [PATCH v2 1/1] check that order of free pages falls within valid range Alexander Egorenkov 2022-04-08 13:32 ` Alexander Egorenkov 2022-04-11 1:53 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 2022-04-11 5:33 ` Alexander Egorenkov 2022-04-11 6:11 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?=
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.