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

[Baoquan He <bhe@redhat.com>]

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.

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.

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;
 		}
 
 		/*