crash: Fix x86_32 and arm32 memory reserve bug

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

Posted by Jinjie Ruan 1 year, 5 months ago

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.

After this patch, it fail as expected and no above confusing reserve
success info.

Fixes: 9d17f3372306 ("ARM: 9190/1: kdump: add invalid input check for 'crashkernel=0'")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2:
- Also fix for x86_32.
- Update the fix method.
- Peel off the other two patches.
- Update the commit message.
---
 kernel/crash_reserve.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c
index 5073ae205f79..5c2148d89da6 100644
--- a/kernel/crash_reserve.c
+++ b/kernel/crash_reserve.c
@@ -314,7 +314,7 @@ int __init parse_crashkernel(char *cmdline,
 	if (high && ret == -ENOENT) {
 		ret = __parse_crashkernel(cmdline, 0, crash_size,
 				crash_base, suffix_tbl[SUFFIX_HIGH]);
-		if (ret || !*crash_size)
+		if (ret || !(phys_addr_t)*crash_size)
 			return -EINVAL;
 
 		/*
@@ -333,7 +333,7 @@ int __init parse_crashkernel(char *cmdline,
 		*high = true;
 	}
 #endif
-	if (!*crash_size)
+	if (!(phys_addr_t)*crash_size)
 		ret = -EINVAL;
 
 	return ret;
-- 
2.34.1

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

Posted by Baoquan He 1 year, 5 months ago

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?

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?

diff --git a/arch/arm64/include/asm/crash_reserve.h b/arch/arm64/include/asm/crash_reserve.h
index 4afe027a4e7b..bf362c1a612f 100644
--- a/arch/arm64/include/asm/crash_reserve.h
+++ b/arch/arm64/include/asm/crash_reserve.h
@@ -7,4 +7,6 @@
 
 #define CRASH_ADDR_LOW_MAX              arm64_dma_phys_limit
 #define CRASH_ADDR_HIGH_MAX             (PHYS_MASK + 1)
+
+#define HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH
 #endif
diff --git a/arch/riscv/include/asm/crash_reserve.h b/arch/riscv/include/asm/crash_reserve.h
index 013962e63587..8d7a8fc1d459 100644
--- a/arch/riscv/include/asm/crash_reserve.h
+++ b/arch/riscv/include/asm/crash_reserve.h
@@ -7,5 +7,7 @@
 #define CRASH_ADDR_LOW_MAX		dma32_phys_limit
 #define CRASH_ADDR_HIGH_MAX		memblock_end_of_DRAM()
 
+#define HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH
+
 extern phys_addr_t memblock_end_of_DRAM(void);
 #endif
diff --git a/arch/x86/include/asm/crash_reserve.h b/arch/x86/include/asm/crash_reserve.h
index 7835b2cdff04..24c2327f9a16 100644
--- a/arch/x86/include/asm/crash_reserve.h
+++ b/arch/x86/include/asm/crash_reserve.h
@@ -26,6 +26,7 @@ extern unsigned long swiotlb_size_or_default(void);
 #else
 # define CRASH_ADDR_LOW_MAX     SZ_4G
 # define CRASH_ADDR_HIGH_MAX    SZ_64T
+#define HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH
 #endif
 
 # define DEFAULT_CRASH_KERNEL_LOW_SIZE crash_low_size_default()
diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c
index 5b2722a93a48..c5213f123e19 100644
--- a/kernel/crash_reserve.c
+++ b/kernel/crash_reserve.c
@@ -306,7 +306,7 @@ int __init parse_crashkernel(char *cmdline,
 	/* crashkernel=X[@offset] */
 	ret = __parse_crashkernel(cmdline, system_ram, crash_size,
 				crash_base, NULL);
-#ifdef CONFIG_ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION
+#ifdef HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH
 	/*
 	 * If non-NULL 'high' passed in and no normal crashkernel
 	 * setting detected, try parsing crashkernel=,high|low.

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

Posted by Jinjie Ruan 1 year, 5 months ago


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?
> 
> 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'll test the below patch on ARM32 an x86_32 sooner.

> 
> diff --git a/arch/arm64/include/asm/crash_reserve.h b/arch/arm64/include/asm/crash_reserve.h
> index 4afe027a4e7b..bf362c1a612f 100644
> --- a/arch/arm64/include/asm/crash_reserve.h
> +++ b/arch/arm64/include/asm/crash_reserve.h
> @@ -7,4 +7,6 @@
>  
>  #define CRASH_ADDR_LOW_MAX              arm64_dma_phys_limit
>  #define CRASH_ADDR_HIGH_MAX             (PHYS_MASK + 1)
> +
> +#define HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH
>  #endif
> diff --git a/arch/riscv/include/asm/crash_reserve.h b/arch/riscv/include/asm/crash_reserve.h
> index 013962e63587..8d7a8fc1d459 100644
> --- a/arch/riscv/include/asm/crash_reserve.h
> +++ b/arch/riscv/include/asm/crash_reserve.h
> @@ -7,5 +7,7 @@
>  #define CRASH_ADDR_LOW_MAX		dma32_phys_limit
>  #define CRASH_ADDR_HIGH_MAX		memblock_end_of_DRAM()
>  
> +#define HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH
> +
>  extern phys_addr_t memblock_end_of_DRAM(void);
>  #endif
> diff --git a/arch/x86/include/asm/crash_reserve.h b/arch/x86/include/asm/crash_reserve.h
> index 7835b2cdff04..24c2327f9a16 100644
> --- a/arch/x86/include/asm/crash_reserve.h
> +++ b/arch/x86/include/asm/crash_reserve.h
> @@ -26,6 +26,7 @@ extern unsigned long swiotlb_size_or_default(void);
>  #else
>  # define CRASH_ADDR_LOW_MAX     SZ_4G
>  # define CRASH_ADDR_HIGH_MAX    SZ_64T
> +#define HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH
>  #endif
>  
>  # define DEFAULT_CRASH_KERNEL_LOW_SIZE crash_low_size_default()
> diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c
> index 5b2722a93a48..c5213f123e19 100644
> --- a/kernel/crash_reserve.c
> +++ b/kernel/crash_reserve.c
> @@ -306,7 +306,7 @@ int __init parse_crashkernel(char *cmdline,
>  	/* crashkernel=X[@offset] */
>  	ret = __parse_crashkernel(cmdline, system_ram, crash_size,
>  				crash_base, NULL);
> -#ifdef CONFIG_ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION
> +#ifdef HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH
>  	/*
>  	 * If non-NULL 'high' passed in and no normal crashkernel
>  	 * setting detected, try parsing crashkernel=,high|low.
> 
>

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

Posted by Jinjie Ruan 1 year, 5 months ago


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.

> 
> diff --git a/arch/arm64/include/asm/crash_reserve.h b/arch/arm64/include/asm/crash_reserve.h
> index 4afe027a4e7b..bf362c1a612f 100644
> --- a/arch/arm64/include/asm/crash_reserve.h
> +++ b/arch/arm64/include/asm/crash_reserve.h
> @@ -7,4 +7,6 @@
>  
>  #define CRASH_ADDR_LOW_MAX              arm64_dma_phys_limit
>  #define CRASH_ADDR_HIGH_MAX             (PHYS_MASK + 1)
> +
> +#define HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH
>  #endif
> diff --git a/arch/riscv/include/asm/crash_reserve.h b/arch/riscv/include/asm/crash_reserve.h
> index 013962e63587..8d7a8fc1d459 100644
> --- a/arch/riscv/include/asm/crash_reserve.h
> +++ b/arch/riscv/include/asm/crash_reserve.h
> @@ -7,5 +7,7 @@
>  #define CRASH_ADDR_LOW_MAX		dma32_phys_limit
>  #define CRASH_ADDR_HIGH_MAX		memblock_end_of_DRAM()
>  
> +#define HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH
> +
>  extern phys_addr_t memblock_end_of_DRAM(void);
>  #endif
> diff --git a/arch/x86/include/asm/crash_reserve.h b/arch/x86/include/asm/crash_reserve.h
> index 7835b2cdff04..24c2327f9a16 100644
> --- a/arch/x86/include/asm/crash_reserve.h
> +++ b/arch/x86/include/asm/crash_reserve.h
> @@ -26,6 +26,7 @@ extern unsigned long swiotlb_size_or_default(void);
>  #else
>  # define CRASH_ADDR_LOW_MAX     SZ_4G
>  # define CRASH_ADDR_HIGH_MAX    SZ_64T
> +#define HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH
>  #endif
>  
>  # define DEFAULT_CRASH_KERNEL_LOW_SIZE crash_low_size_default()
> diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c
> index 5b2722a93a48..c5213f123e19 100644
> --- a/kernel/crash_reserve.c
> +++ b/kernel/crash_reserve.c
> @@ -306,7 +306,7 @@ int __init parse_crashkernel(char *cmdline,
>  	/* crashkernel=X[@offset] */
>  	ret = __parse_crashkernel(cmdline, system_ram, crash_size,
>  				crash_base, NULL);
> -#ifdef CONFIG_ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION
> +#ifdef HAVE_ARCH_CRASHKERNEL_RESERVATION_HIGH
>  	/*
>  	 * If non-NULL 'high' passed in and no normal crashkernel
>  	 * setting detected, try parsing crashkernel=,high|low.
> 
>

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

Posted by Baoquan He 1 year, 5 months ago

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

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

Posted by Jinjie Ruan 1 year, 5 months ago


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.

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

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

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

Posted by Baoquan He 1 year, 5 months ago

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