RE: [PATCH 1/1] fix left bit-shift overflow in __exclude_unnecessary_pages()

RE: [PATCH 1/1] fix left bit-shift overflow in __exclude_unnecessary_pages()

[HAGIO KAZUHITO(萩尾 一仁)]

Hi Alex,
+cc kexec list (the right one for makedumpfile patch)

-----Original Message-----
> Whenever the variables compound_order or private become greater than
> 31, left bit-shift of 1 overflows, and nr_pages becomes zero. If nr_pages
> becomes 0 and pages are being excluded at the end of the PFN loop, the
> else branch of the last if statement is entered and pfn is decremented by
> 1 because nr_pages is 0. Finally, this causes the loop variable pfn to
> be assigned the same value as before when the next loop iteration begins
> which results in an infinite loop.
> 
> This issue appeared on s390 64bit architecture with a dump of 16GB RAM.

The patch looks good to me, but just out of curiosity, when do the
compound_order or private become greater than 31 on s390?

Thanks,
Kazu

> 
> This is a simple program to demonstrate the primary issue:
> 
> void main(void)
> {
>         unsigned long long n;
>         unsigned long m;
> 
>         m = 32;
>         n = 1 << m;
>         fprintf(stderr, "%llx\n", n);
>         n = 1UL << m;
>         fprintf(stderr, "%llx\n", n);
> }
> 
> Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
> ---
>  makedumpfile.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/makedumpfile.c b/makedumpfile.c
> index c063267f15bb..863840b13608 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -6210,7 +6210,7 @@ __exclude_unnecessary_pages(unsigned long mem_map,
>  		if (OFFSET(page.private) != NOT_FOUND_STRUCTURE)
>  			private = ULONG(pcache + OFFSET(page.private));
> 
> -		nr_pages = 1 << compound_order;
> +		nr_pages = 1UL << compound_order;
>  		pfn_counter = NULL;
> 
>  		/*
> @@ -6227,7 +6227,7 @@ __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)) {
> -			nr_pages = 1 << private;
> +			nr_pages = 1UL << private;
>  			pfn_counter = &pfn_free;
>  		}
>  		/*
> --
> 2.31.1
> 
> --
> Crash-utility mailing list
> Crash-utility@redhat.com
> https://listman.redhat.com/mailman/listinfo/crash-utility

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCH 1/1] fix left bit-shift overflow in __exclude_unnecessary_pages()

[Alexander Egorenkov]

Hi Kazu,

HAGIO KAZUHITO(萩尾　一仁)	<k-hagio-ab@nec.com> writes:

> Hi Alex,
> +cc kexec list (the right one for makedumpfile patch)
>

Thanks, sorry for the mix up :(

> -----Original Message-----
>> Whenever the variables compound_order or private become greater than
>> 31, left bit-shift of 1 overflows, and nr_pages becomes zero. If nr_pages
>> becomes 0 and pages are being excluded at the end of the PFN loop, the
>> else branch of the last if statement is entered and pfn is decremented by
>> 1 because nr_pages is 0. Finally, this causes the loop variable pfn to
>> be assigned the same value as before when the next loop iteration begins
>> which results in an infinite loop.
>> 
>> This issue appeared on s390 64bit architecture with a dump of 16GB RAM.
>
> The patch looks good to me, but just out of curiosity, when do the
> compound_order or private become greater than 31 on s390?
>
> Thanks,
> Kazu
>

I added some debug statements and this what i got:

compound_order 0
compound_order 1
compound_order 2
compound_order 3
compound_order 4
compound_order 5
compound_order 6
compound_order 7
compound_order 8
private 0
private 1
private 2
private 3
private 4
private 5
private 52
private 6
private 7
private 8

It seems that not compound_order but private is at fault here and
triggers the bug. Not sure yet what that exactly means and whether we
have here another bug which triggers this one :/

Regards
Alex

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCH 1/1] fix left bit-shift overflow in __exclude_unnecessary_pages()

[HAGIO KAZUHITO(萩尾 一仁)]

-----Original Message-----
> > -----Original Message-----
> >> Whenever the variables compound_order or private become greater than
> >> 31, left bit-shift of 1 overflows, and nr_pages becomes zero. If nr_pages
> >> becomes 0 and pages are being excluded at the end of the PFN loop, the
> >> else branch of the last if statement is entered and pfn is decremented by
> >> 1 because nr_pages is 0. Finally, this causes the loop variable pfn to
> >> be assigned the same value as before when the next loop iteration begins
> >> which results in an infinite loop.
> >>
> >> This issue appeared on s390 64bit architecture with a dump of 16GB RAM.
> >
> > The patch looks good to me, but just out of curiosity, when do the
> > compound_order or private become greater than 31 on s390?
> >
> > Thanks,
> > Kazu
> >
> 
> I added some debug statements and this what i got:
> 
> compound_order 0
> compound_order 1
> compound_order 2
> compound_order 3
> compound_order 4
> compound_order 5
> compound_order 6
> compound_order 7
> compound_order 8
> private 0
> private 1
> private 2
> private 3
> private 4
> private 5
> private 52
> private 6
> private 7
> private 8
> 
> It seems that not compound_order but private is at fault here and
> triggers the bug. Not sure yet what that exactly means and whether we
> have here another bug which triggers this one :/

Hmm, so makedumpfile will exclude many pages wrongly with the patch?
Excluding pages wrongly is better than failing with an infinite loop,
but not better than including pages wrongly, because it might lose
necessary data for investigation.

So I think we should have a sanity check also for the private.  AFAIK,
the private value (buddy allocator's order) should be less than MAX_ORDER.
If this is correct, we can use LENGTH(zone.free_area) in vmcoreinfo.

Thanks,
Kazu

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCH 1/1] fix left bit-shift overflow in __exclude_unnecessary_pages()

[Alexander Egorenkov]

Hi Kazu,

HAGIO KAZUHITO(萩尾　一仁)	<k-hagio-ab@nec.com> writes:

> -----Original Message-----
>> > -----Original Message-----
>> >> Whenever the variables compound_order or private become greater than
>> >> 31, left bit-shift of 1 overflows, and nr_pages becomes zero. If nr_pages
>> >> becomes 0 and pages are being excluded at the end of the PFN loop, the
>> >> else branch of the last if statement is entered and pfn is decremented by
>> >> 1 because nr_pages is 0. Finally, this causes the loop variable pfn to
>> >> be assigned the same value as before when the next loop iteration begins
>> >> which results in an infinite loop.
>> >>
>> >> This issue appeared on s390 64bit architecture with a dump of 16GB RAM.
>> >
>> > The patch looks good to me, but just out of curiosity, when do the
>> > compound_order or private become greater than 31 on s390?
>> >
>> > Thanks,
>> > Kazu
>> >
>> 
>> I added some debug statements and this what i got:
>> 
>> compound_order 0
>> compound_order 1
>> compound_order 2
>> compound_order 3
>> compound_order 4
>> compound_order 5
>> compound_order 6
>> compound_order 7
>> compound_order 8
>> private 0
>> private 1
>> private 2
>> private 3
>> private 4
>> private 5
>> private 52
>> private 6
>> private 7
>> private 8
>> 
>> It seems that not compound_order but private is at fault here and
>> triggers the bug. Not sure yet what that exactly means and whether we
>> have here another bug which triggers this one :/
>
> Hmm, so makedumpfile will exclude many pages wrongly with the patch?
> Excluding pages wrongly is better than failing with an infinite loop,
> but not better than including pages wrongly, because it might lose
> necessary data for investigation.
>
> So I think we should have a sanity check also for the private.  AFAIK,
> the private value (buddy allocator's order) should be less than MAX_ORDER.
> If this is correct, we can use LENGTH(zone.free_area) in vmcoreinfo.
>

Completely agree with this. We need to fix the real cause of this and
not the effect. First need to understand better what the loop is exactly doing.

Regards
Alex

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec