linux-kernel - Re: [PATCH v2] crash: Fix x86_32 and arm32 memory reserve bug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZpdNtfWCfOwmUWZl@MiWiFi-R3L-srv>
Date: Wed, 17 Jul 2024 12:51:01 +0800
From: Baoquan He <bhe@...hat.com>
To: Jinjie Ruan <ruanjinjie@...wei.com>
Cc: akpm@...ux-foundation.org, vgoyal@...hat.com, dyoung@...hat.com,
	austindh.kim@...il.com, rmk+kernel@...linux.org.uk,
	kexec@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] crash: Fix x86_32 and arm32 memory reserve bug

On 07/16/24 at 08:46pm, Jinjie Ruan wrote:
> 
> 
> On 2024/7/16 14:22, Baoquan He wrote:
> > On 07/16/24 at 11:44am, Jinjie Ruan wrote:
> >>
> >>
> >> On 2024/7/15 22:48, Baoquan He wrote:
> >>> On 07/13/24 at 09:48am, Jinjie Ruan wrote:
> >>>> On x86_32 Qemu machine with 1GB memory, the cmdline "crashkernel=4G" is ok
> >>>> as below:
> >>>> 	crashkernel reserved: 0x0000000020000000 - 0x0000000120000000 (4096 MB)
> >>>>
> >>>> And on Qemu vexpress-a9 with 1GB memory, the crash kernel "crashkernel=4G"
> >>>> is also ok as below:
> >>>> 	Reserving 4096MB of memory at 2432MB for crashkernel (System RAM: 1024MB)
> >>>>
> >>>> The cause is that the crash_size is parsed and printed with "unsigned long
> >>>> long" data type which is 8 bytes but allocated used with "phys_addr_t"
> >>>> which is 4 bytes in memblock_phys_alloc_range().
> >>>>
> >>>> Fix it by limiting the "crash_size" to phys_addr_t and bypass the invalid
> >>>> input size.
> >>>
> >>> I am not sure if this is a good idea. Shouldn't we handle this in
> >>> arch_reserve_crashkernel() to check the system RAM size?
> >>>
> >>> With this patch, if you specify crashkernel=4352M (namely 4G+256M) in
> >>> kernel cmdline, then you will reserve 256M crashkernel in system, don't
> >>> you think that is confusing?
> >>
> >> You are right!
> >>
> >> In the case you mentioned, it can still allocate 256M successfully, but
> >> the log shows 4352M successfully allocated, which is not taken into
> >> account by this patch.
> >>
> >> And handle this in arch_reserve_crashkernel() is a good idea, which will
> >>  bypass all these corner case, I'll do it next version.
> >>
> >>>
> >>> By the way, I am considering changing code to apply generic crashkernel
> >>> reservation to 32bit system. Maybe below draft code can prevent
> >>> crashkernel=,high from being parsed successfully on 32bit system.
> >>>
> >>> What do you think?
> >>
> >> I agree with you, I've thought about passing in a parameter in the
> >> generic interface whether high is supported or not to implement it,
> >> which is so incompatible. An architecture-defined macro to filter out
> >> parsing of "high" fundamentally avoid using the generic interface to
> >> allocate memory in "high" for the architecture that does not support
> >> "high". The below code do prevent "crashkernel=,high" from being parsed
> >> successfully on 32bit system.
> >>
> >> But if it is to support 32 bit system to use generic crash memory
> >> reserve interface, reserve_crashkernel_generic() needs more modification
> >> , as it may try to allocate memory at high.
> > 
> > You are right. Below change may be able to fix that. 
> > 
> > And I have been thinking if one case need be taken off in which the
> > first attempt was for high memory, then fall back to low memory. Surely,
> > this is not related to the 32bit crashkernel reservation.
> 
> It seems that ARM64 has the possibility before the refactoring. However,
> x86 supports only the "low" -> "high" retry but not the "high" -> "low"
> retry before the refactoring. In my opinion, "low" -> "high" retry is
> more usefull, but I'm not sure if we should get rid of the other way.

Thanks for the valuable input, I will think more about this.

> 
> > 
> > By the way, do you figure out these issues from code reading and qemu
> > testing, or there's need for deploying kdump on 32bit system, e.g i386
> > or arm32? Just curious.
> 
> I found these problems during testing on QEMU when trying to support
> this generic crash memory retention on ARM32, and I further found that
> x86_32 also has the same problem by code reading and uncommon
> configuration test.

I see, thanks for the explanation.

Could you help test the earlier patch and below draft patch, to see if
it fixes the issues you saw up to now? If they works, the generic
crashkernel reservation can be taken on 32bit system like arm32 based 
on them.

> 
> > 
> > diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c
> > index 5b2722a93a48..ac087ba442cd 100644
> > --- a/kernel/crash_reserve.c
> > +++ b/kernel/crash_reserve.c
> > @@ -414,7 +414,8 @@ void __init reserve_crashkernel_generic(char *cmdline,
> >  			search_end = CRASH_ADDR_HIGH_MAX;
> >  			search_base = CRASH_ADDR_LOW_MAX;
> >  			crash_low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE;
> > -			goto retry;
> > +			if (search_base != search_end)
> > +				goto retry;
> >  		}
> >  
> >  		/*
> > 
> > 
>