* [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.