linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] efi: Fix the size not consistent issue when unmapping memory map
@ 2018-04-13  6:27 Lee, Chun-Yi
  2018-04-16  2:57 ` Dave Young
  0 siblings, 1 reply; 12+ messages in thread
From: Lee, Chun-Yi @ 2018-04-13  6:27 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: linux-efi, linux-kernel, kexec, akpm, Lee, Chun-Yi, Randy Wright,
	Takashi Iwai, Vivek Goyal, Ingo Molnar

When using kdump, SOMETIMES the "size not consistent" warning message
shows up when the crash kernel boots with early_ioremap_debug parameter:

WARNING: CPU: 0 PID: 0 at ../mm/early_ioremap.c:182 early_iounmap+0x4f/0x12c()
early_iounmap(ffffffffff200180, 00000118) [0] size not consistent 00000120

The root cause is that the unmapping size of memory map doesn't
match with the original size when mapping:

in __efi_memmap_init()
	map.map = early_memremap(phys_map, data->size);

in efi_memmap_unmap()
        size = efi.memmap.desc_size * efi.memmap.nr_map;
        early_memunmap(efi.memmap.map, size);

But the efi.memmap.nr_map is from __efi_memmap_init(). The remainder
of size was discarded when calculating the nr_map:
        map.nr_map = data->size / data->desc_size;

When the original size of memory map region does not equal to the
result of multiplication. The "size not consistent" warning
will be triggered.

This issue sometimes was hit by kdump because kexec set the efi map
size to align with 16 when loading crash kernel image:

in bzImage64_load()
        efi_map_sz = efi_get_runtime_map_size();
        efi_map_sz = ALIGN(efi_map_sz, 16);

This patch changes the logic in the unmapping function. Using the
end address of map to calcuate original size.

Thank Randy Wright for his report and testing. And also thank
Takashi Iwai for his help to trace issue.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Randy Wright <rwright@hpe.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
 drivers/firmware/efi/memmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index 5fc7052..1f592d8 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -121,7 +121,7 @@ void __init efi_memmap_unmap(void)
 	if (!efi.memmap.late) {
 		unsigned long size;
 
-		size = efi.memmap.desc_size * efi.memmap.nr_map;
+		size = efi.memmap.map_end - efi.memmap.map;
 		early_memunmap(efi.memmap.map, size);
 	} else {
 		memunmap(efi.memmap.map);
-- 
2.10.2

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map
  2018-04-13  6:27 [PATCH] efi: Fix the size not consistent issue when unmapping memory map Lee, Chun-Yi
@ 2018-04-16  2:57 ` Dave Young
  2018-04-16  3:09   ` Dave Young
  2018-04-16  6:34   ` joeyli
  0 siblings, 2 replies; 12+ messages in thread
From: Dave Young @ 2018-04-16  2:57 UTC (permalink / raw)
  To: Lee, Chun-Yi
  Cc: Ard Biesheuvel, linux-efi, Takashi Iwai, kexec, linux-kernel,
	Randy Wright, Lee, Chun-Yi, Ingo Molnar, akpm, Vivek Goyal

On 04/13/18 at 02:27pm, Lee, Chun-Yi wrote:
> When using kdump, SOMETIMES the "size not consistent" warning message
> shows up when the crash kernel boots with early_ioremap_debug parameter:
> 
> WARNING: CPU: 0 PID: 0 at ../mm/early_ioremap.c:182 early_iounmap+0x4f/0x12c()
> early_iounmap(ffffffffff200180, 00000118) [0] size not consistent 00000120
> 
> The root cause is that the unmapping size of memory map doesn't
> match with the original size when mapping:
> 
> in __efi_memmap_init()
> 	map.map = early_memremap(phys_map, data->size);
> 
> in efi_memmap_unmap()
>         size = efi.memmap.desc_size * efi.memmap.nr_map;
>         early_memunmap(efi.memmap.map, size);
> 
> But the efi.memmap.nr_map is from __efi_memmap_init(). The remainder
> of size was discarded when calculating the nr_map:
>         map.nr_map = data->size / data->desc_size;
> 
> When the original size of memory map region does not equal to the
> result of multiplication. The "size not consistent" warning
> will be triggered.
> 
> This issue sometimes was hit by kdump because kexec set the efi map
> size to align with 16 when loading crash kernel image:
> 
> in bzImage64_load()
>         efi_map_sz = efi_get_runtime_map_size();
>         efi_map_sz = ALIGN(efi_map_sz, 16);
> 
> This patch changes the logic in the unmapping function. Using the
> end address of map to calcuate original size.
> 
> Thank Randy Wright for his report and testing. And also thank
> Takashi Iwai for his help to trace issue.

Good catch.  The kexec code need to be fixed to use a separate buffer so
avoid the alignment like what kexec-tools did.  I can submit a fix for
that.

But this issue could be a potential issue even if kexec get fixed so it
looks worth a fix in efi code as well.  How about mapping only nr_maps
*desc_size in __efi_memmap_init?  It looks easier to understand.

> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Randy Wright <rwright@hpe.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> ---
>  drivers/firmware/efi/memmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
> index 5fc7052..1f592d8 100644
> --- a/drivers/firmware/efi/memmap.c
> +++ b/drivers/firmware/efi/memmap.c
> @@ -121,7 +121,7 @@ void __init efi_memmap_unmap(void)
>  	if (!efi.memmap.late) {
>  		unsigned long size;
>  
> -		size = efi.memmap.desc_size * efi.memmap.nr_map;
> +		size = efi.memmap.map_end - efi.memmap.map;
>  		early_memunmap(efi.memmap.map, size);
>  	} else {
>  		memunmap(efi.memmap.map);
> -- 
> 2.10.2
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Thanks
Dave

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map
  2018-04-16  2:57 ` Dave Young
@ 2018-04-16  3:09   ` Dave Young
  2018-04-16  6:37     ` joeyli
  2018-04-16  6:34   ` joeyli
  1 sibling, 1 reply; 12+ messages in thread
From: Dave Young @ 2018-04-16  3:09 UTC (permalink / raw)
  To: Lee, Chun-Yi
  Cc: linux-efi, Ard Biesheuvel, Takashi Iwai, kexec, linux-kernel,
	Randy Wright, Lee, Chun-Yi, Ingo Molnar, akpm, Vivek Goyal

On 04/16/18 at 10:57am, Dave Young wrote:
> On 04/13/18 at 02:27pm, Lee, Chun-Yi wrote:
> > When using kdump, SOMETIMES the "size not consistent" warning message
> > shows up when the crash kernel boots with early_ioremap_debug parameter:
> > 
> > WARNING: CPU: 0 PID: 0 at ../mm/early_ioremap.c:182 early_iounmap+0x4f/0x12c()
> > early_iounmap(ffffffffff200180, 00000118) [0] size not consistent 00000120
> > 
> > The root cause is that the unmapping size of memory map doesn't
> > match with the original size when mapping:
> > 
> > in __efi_memmap_init()
> > 	map.map = early_memremap(phys_map, data->size);
> > 
> > in efi_memmap_unmap()
> >         size = efi.memmap.desc_size * efi.memmap.nr_map;
> >         early_memunmap(efi.memmap.map, size);
> > 
> > But the efi.memmap.nr_map is from __efi_memmap_init(). The remainder
> > of size was discarded when calculating the nr_map:
> >         map.nr_map = data->size / data->desc_size;
> > 
> > When the original size of memory map region does not equal to the
> > result of multiplication. The "size not consistent" warning
> > will be triggered.
> > 
> > This issue sometimes was hit by kdump because kexec set the efi map
> > size to align with 16 when loading crash kernel image:
> > 
> > in bzImage64_load()
> >         efi_map_sz = efi_get_runtime_map_size();
> >         efi_map_sz = ALIGN(efi_map_sz, 16);
> > 
> > This patch changes the logic in the unmapping function. Using the
> > end address of map to calcuate original size.
> > 
> > Thank Randy Wright for his report and testing. And also thank
> > Takashi Iwai for his help to trace issue.
> 
> Good catch.  The kexec code need to be fixed to use a separate buffer so
> avoid the alignment like what kexec-tools did.  I can submit a fix for
> that.

Can you try below code, see if it works?


diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index 3182908b7e6c..eaee37c54b7b 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -398,11 +398,10 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
 	 * little bit simple
 	 */
 	efi_map_sz = efi_get_runtime_map_size();
-	efi_map_sz = ALIGN(efi_map_sz, 16);
 	params_cmdline_sz = sizeof(struct boot_params) + cmdline_len +
 				MAX_ELFCOREHDR_STR_LEN;
 	params_cmdline_sz = ALIGN(params_cmdline_sz, 16);
-	kbuf.bufsz = params_cmdline_sz + efi_map_sz +
+	kbuf.bufsz = params_cmdline_sz + ALIGN(efi_map_sz, 16)+
 				sizeof(struct setup_data) +
 				sizeof(struct efi_setup_data);
 
@@ -410,7 +409,7 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
 	if (!params)
 		return ERR_PTR(-ENOMEM);
 	efi_map_offset = params_cmdline_sz;
-	efi_setup_data_offset = efi_map_offset + efi_map_sz;
+	efi_setup_data_offset = efi_map_offset + ALIGN(efi_map_sz, 16);
 
 	/* Copy setup header onto bootparams. Documentation/x86/boot.txt */
 	setup_header_size = 0x0202 + kernel[0x0201] - setup_hdr_offset;

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map
  2018-04-16  2:57 ` Dave Young
  2018-04-16  3:09   ` Dave Young
@ 2018-04-16  6:34   ` joeyli
  1 sibling, 0 replies; 12+ messages in thread
From: joeyli @ 2018-04-16  6:34 UTC (permalink / raw)
  To: Dave Young
  Cc: Lee, Chun-Yi, Ard Biesheuvel, linux-efi, Takashi Iwai, kexec,
	linux-kernel, Randy Wright, Ingo Molnar, akpm, Vivek Goyal

On Mon, Apr 16, 2018 at 10:57:34AM +0800, Dave Young wrote:
> On 04/13/18 at 02:27pm, Lee, Chun-Yi wrote:
> > When using kdump, SOMETIMES the "size not consistent" warning message
> > shows up when the crash kernel boots with early_ioremap_debug parameter:
> > 
> > WARNING: CPU: 0 PID: 0 at ../mm/early_ioremap.c:182 early_iounmap+0x4f/0x12c()
> > early_iounmap(ffffffffff200180, 00000118) [0] size not consistent 00000120
> > 
> > The root cause is that the unmapping size of memory map doesn't
> > match with the original size when mapping:
> > 
> > in __efi_memmap_init()
> > 	map.map = early_memremap(phys_map, data->size);
> > 
> > in efi_memmap_unmap()
> >         size = efi.memmap.desc_size * efi.memmap.nr_map;
> >         early_memunmap(efi.memmap.map, size);
> > 
> > But the efi.memmap.nr_map is from __efi_memmap_init(). The remainder
> > of size was discarded when calculating the nr_map:
> >         map.nr_map = data->size / data->desc_size;
> > 
> > When the original size of memory map region does not equal to the
> > result of multiplication. The "size not consistent" warning
> > will be triggered.
> > 
> > This issue sometimes was hit by kdump because kexec set the efi map
> > size to align with 16 when loading crash kernel image:
> > 
> > in bzImage64_load()
> >         efi_map_sz = efi_get_runtime_map_size();
> >         efi_map_sz = ALIGN(efi_map_sz, 16);
> > 
> > This patch changes the logic in the unmapping function. Using the
> > end address of map to calcuate original size.
> > 
> > Thank Randy Wright for his report and testing. And also thank
> > Takashi Iwai for his help to trace issue.
> 
> Good catch.  The kexec code need to be fixed to use a separate buffer so
> avoid the alignment like what kexec-tools did.  I can submit a fix for
> that.
>

Thanks!
 
> But this issue could be a potential issue even if kexec get fixed so it
> looks worth a fix in efi code as well.  How about mapping only nr_maps
> *desc_size in __efi_memmap_init?  It looks easier to understand.
> 

Takashi has another patch as you said. Finally I sent this patch because
it's smaller.   

Thanks a lot!
Joey Lee

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map
  2018-04-16  3:09   ` Dave Young
@ 2018-04-16  6:37     ` joeyli
  2018-04-17  0:35       ` Randy Wright
  0 siblings, 1 reply; 12+ messages in thread
From: joeyli @ 2018-04-16  6:37 UTC (permalink / raw)
  To: Randy Wright
  Cc: Lee, Chun-Yi, linux-efi, Ard Biesheuvel, Takashi Iwai, kexec,
	linux-kernel, Randy Wright, Ingo Molnar, akpm, Vivek Goyal,
	Dave Young

Hi Randy,

On Mon, Apr 16, 2018 at 11:09:04AM +0800, Dave Young wrote:
> On 04/16/18 at 10:57am, Dave Young wrote:
> > On 04/13/18 at 02:27pm, Lee, Chun-Yi wrote:
> > > When using kdump, SOMETIMES the "size not consistent" warning message
> > > shows up when the crash kernel boots with early_ioremap_debug parameter:
> > > 
> > > WARNING: CPU: 0 PID: 0 at ../mm/early_ioremap.c:182 early_iounmap+0x4f/0x12c()
> > > early_iounmap(ffffffffff200180, 00000118) [0] size not consistent 00000120
> > >
[...snip] 
> > 
> > Good catch.  The kexec code need to be fixed to use a separate buffer so
> > avoid the alignment like what kexec-tools did.  I can submit a fix for
> > that.
> 
> Can you try below code, see if it works?
>

Randy, do you want to try Dave's kexec patch on your environment? Please remove
my patch first.  

Thanks a lot!
Joey Lee
 
> 
> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> index 3182908b7e6c..eaee37c54b7b 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -398,11 +398,10 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
>  	 * little bit simple
>  	 */
>  	efi_map_sz = efi_get_runtime_map_size();
> -	efi_map_sz = ALIGN(efi_map_sz, 16);
>  	params_cmdline_sz = sizeof(struct boot_params) + cmdline_len +
>  				MAX_ELFCOREHDR_STR_LEN;
>  	params_cmdline_sz = ALIGN(params_cmdline_sz, 16);
> -	kbuf.bufsz = params_cmdline_sz + efi_map_sz +
> +	kbuf.bufsz = params_cmdline_sz + ALIGN(efi_map_sz, 16)+
>  				sizeof(struct setup_data) +
>  				sizeof(struct efi_setup_data);
>  
> @@ -410,7 +409,7 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
>  	if (!params)
>  		return ERR_PTR(-ENOMEM);
>  	efi_map_offset = params_cmdline_sz;
> -	efi_setup_data_offset = efi_map_offset + efi_map_sz;
> +	efi_setup_data_offset = efi_map_offset + ALIGN(efi_map_sz, 16);
>  
>  	/* Copy setup header onto bootparams. Documentation/x86/boot.txt */
>  	setup_header_size = 0x0202 + kernel[0x0201] - setup_hdr_offset;

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map
  2018-04-16  6:37     ` joeyli
@ 2018-04-17  0:35       ` Randy Wright
  2018-04-17  1:20         ` joeyli
                           ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Randy Wright @ 2018-04-17  0:35 UTC (permalink / raw)
  To: joeyli
  Cc: Lee, Chun-Yi, linux-efi, Ard Biesheuvel, Takashi Iwai, kexec,
	linux-kernel, Ingo Molnar, akpm, Vivek Goyal, Dave Young

On Mon, Apr 16, 2018 at 02:37:38PM +0800, joeyli wrote:
> Hi Randy,
> ...
> Randy, do you want to try Dave's kexec patch on your environment? Please remove
> my patch first.  
> 
> Thanks a lot!
> Joey Lee

Hi Joey, 

I tried Dave's patch to kexec-bzimage64.c on my build of the SuSE
4.12.14-15 kernel.   I ran the same test as I did with your patch: I
verified the early_ioremap.c warnings occurred with a crash triggered
from a kexec boot of the unmodified kernel. Then I applied the patch to
kexec-bzimage64.c, rebuilt, re-ran the test to crash from the kexec'ed
kernel, and verified the warnings are no longer seen.

I'm out of time today, but I will plan to run the same  test tomorrow on
a build of the SuSE 4.4.120-94.17 kernel, on which I had also reported
the original bug.

-- 
Randy Wright            Hewlett Packard Enterprise
Phone: (970) 898-0998   Mail: rwright@hpe.com

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map
  2018-04-17  0:35       ` Randy Wright
@ 2018-04-17  1:20         ` joeyli
  2018-04-17  2:41         ` Dave Young
  2018-04-17 20:34         ` Randy Wright
  2 siblings, 0 replies; 12+ messages in thread
From: joeyli @ 2018-04-17  1:20 UTC (permalink / raw)
  To: Randy Wright
  Cc: Lee, Chun-Yi, linux-efi, Ard Biesheuvel, Takashi Iwai, kexec,
	linux-kernel, Ingo Molnar, akpm, Vivek Goyal, Dave Young

On Mon, Apr 16, 2018 at 06:35:22PM -0600, Randy Wright wrote:
> On Mon, Apr 16, 2018 at 02:37:38PM +0800, joeyli wrote:
> > Hi Randy,
> > ...
> > Randy, do you want to try Dave's kexec patch on your environment? Please remove
> > my patch first.  
> > 
> > Thanks a lot!
> > Joey Lee
> 
> Hi Joey, 
> 
> I tried Dave's patch to kexec-bzimage64.c on my build of the SuSE
> 4.12.14-15 kernel.   I ran the same test as I did with your patch: I
> verified the early_ioremap.c warnings occurred with a crash triggered
> from a kexec boot of the unmodified kernel. Then I applied the patch to
> kexec-bzimage64.c, rebuilt, re-ran the test to crash from the kexec'ed
> kernel, and verified the warnings are no longer seen.
>

Thanks for your help! Looks that Dave's kexec patch works to prevent the
warning message. 
 
> I'm out of time today, but I will plan to run the same  test tomorrow on
> a build of the SuSE 4.4.120-94.17 kernel, on which I had also reported
> the original bug.
>

Both kexec and efi sides will be applied patches.

Thanks a lot!
Joey Lee 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map
  2018-04-17  0:35       ` Randy Wright
  2018-04-17  1:20         ` joeyli
@ 2018-04-17  2:41         ` Dave Young
  2018-04-17 20:34         ` Randy Wright
  2 siblings, 0 replies; 12+ messages in thread
From: Dave Young @ 2018-04-17  2:41 UTC (permalink / raw)
  To: Randy Wright
  Cc: joeyli, Lee, Chun-Yi, linux-efi, Ard Biesheuvel, Takashi Iwai,
	kexec, linux-kernel, Ingo Molnar, akpm, Vivek Goyal

On 04/16/18 at 06:35pm, Randy Wright wrote:
> On Mon, Apr 16, 2018 at 02:37:38PM +0800, joeyli wrote:
> > Hi Randy,
> > ...
> > Randy, do you want to try Dave's kexec patch on your environment? Please remove
> > my patch first.  
> > 
> > Thanks a lot!
> > Joey Lee
> 
> Hi Joey, 
> 
> I tried Dave's patch to kexec-bzimage64.c on my build of the SuSE
> 4.12.14-15 kernel.   I ran the same test as I did with your patch: I
> verified the early_ioremap.c warnings occurred with a crash triggered
> from a kexec boot of the unmodified kernel. Then I applied the patch to
> kexec-bzimage64.c, rebuilt, re-ran the test to crash from the kexec'ed
> kernel, and verified the warnings are no longer seen.

Great, thanks for the testing, will send out the patch after some local
tests.

Thanks
Dave

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map
  2018-04-17  0:35       ` Randy Wright
  2018-04-17  1:20         ` joeyli
  2018-04-17  2:41         ` Dave Young
@ 2018-04-17 20:34         ` Randy Wright
  2 siblings, 0 replies; 12+ messages in thread
From: Randy Wright @ 2018-04-17 20:34 UTC (permalink / raw)
  To: joeyli, dyoung
  Cc: Lee, Chun-Yi, linux-efi, Ard Biesheuvel, Takashi Iwai, kexec,
	linux-kernel, Ingo Molnar, akpm, Vivek Goyal, Dave Young

On Mon, Apr 16, 2018 at 06:35:22PM -0600, Randy Wright wrote:

> ... I will plan to run the same test tomorrow on
> a build of the SuSE 4.4.120-94.17 kernel, on which I had also reported
> the original bug.

I carried out the test on the older kernel today. I found the version of
arch/x86/kernel/kexec-bzimage64.c in the SuSE kernel version
4.4.120-94.17 was sufficiently different from the newer version that one
chunk of the patch under discussion was rejected.  Specifically, the
'struct kexec_buf kbuf' is not found in that older SuSE version of
kexec-bzimage64.c.   But I made the same modification to the
calculation of variable params_misc_sz in the older source, ran the same
test, and there were no longer any warnings from early_ioremap.c.

-- 
Randy Wright            Hewlett Packard Enterprise
Phone: (970) 898-0998   Mail: rwright@hpe.com

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map
  2018-05-03 12:05 ` Ard Biesheuvel
@ 2018-05-04  7:29   ` joeyli
  0 siblings, 0 replies; 12+ messages in thread
From: joeyli @ 2018-05-04  7:29 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Lee, Chun-Yi, linux-efi, Linux Kernel Mailing List, Takashi Iwai,
	Vivek Goyal, Ingo Molnar

Hi Ard,

On Thu, May 03, 2018 at 02:05:51PM +0200, Ard Biesheuvel wrote:
> On 2 May 2018 at 08:17, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> > When using kdump, SOMETIMES the "size not consistent" warning message
> > shows up when the crash kernel boots with early_ioremap_debug parameter:
> >
> > WARNING: CPU: 0 PID: 0 at ../mm/early_ioremap.c:182 early_iounmap+0x4f/0x12c()
> > early_iounmap(ffffffffff200180, 00000118) [0] size not consistent 00000120
> >
> > The root cause is that the unmapping size of memory map doesn't
> > match with the original size when mapping:
> >
> > in __efi_memmap_init()
> >         map.map = early_memremap(phys_map, data->size);
> >
> > in efi_memmap_unmap()
> >         size = efi.memmap.desc_size * efi.memmap.nr_map;
> >         early_memunmap(efi.memmap.map, size);
> >
> > But the efi.memmap.nr_map is from __efi_memmap_init(). The remainder
> > of size was discarded when calculating the nr_map:
> >         map.nr_map = data->size / data->desc_size;
> >
> > When the original size of memory map region does not equal to the
> > result of multiplication. The "size not consistent" warning
> > will be triggered.
> >
> > This issue sometimes was hit by kdump because kexec set the efi map
> > size to align with 16 when loading crash kernel image:
> >
> > in bzImage64_load()
> >         efi_map_sz = efi_get_runtime_map_size();
> >         efi_map_sz = ALIGN(efi_map_sz, 16);
> >
> > Dave Young's a841aa83d patch fixed kexec issue. On UEFI side, this
> > patch changes the logic in the unmapping function. Using the end
> > address of map to calcuate original size.
> >
> 
> Why do we still need this patch? I.e., in which circumstances will
> efi_memory_map_data::size assume a value that is rounded up or
> otherwise incorrect?
>

There have no other case except kexec. But I think that it's better
to sync mapping/unmapping size between __efi_memmap_init() and 
efi_memmap_unmap(). 


Thanks
Joey Lee
 
> > Thank Randy Wright for his report and testing. And also thank
> > Takashi Iwai for his help to trace issue.
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Cc: Vivek Goyal <vgoyal@redhat.com>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Tested-by: Randy Wright <rwright@hpe.com>
> > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> > ---
> >  drivers/firmware/efi/memmap.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
> > index 5fc7052..1f592d8 100644
> > --- a/drivers/firmware/efi/memmap.c
> > +++ b/drivers/firmware/efi/memmap.c
> > @@ -121,7 +121,7 @@ void __init efi_memmap_unmap(void)
> >         if (!efi.memmap.late) {
> >                 unsigned long size;
> >
> > -               size = efi.memmap.desc_size * efi.memmap.nr_map;
> > +               size = efi.memmap.map_end - efi.memmap.map;
> >                 early_memunmap(efi.memmap.map, size);
> >         } else {
> >                 memunmap(efi.memmap.map);
> > --
> > 2.10.2
> >

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map
  2018-05-02  6:17 Lee, Chun-Yi
@ 2018-05-03 12:05 ` Ard Biesheuvel
  2018-05-04  7:29   ` joeyli
  0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2018-05-03 12:05 UTC (permalink / raw)
  To: Lee, Chun-Yi
  Cc: linux-efi, Linux Kernel Mailing List, Lee, Chun-Yi, Takashi Iwai,
	Vivek Goyal, Ingo Molnar

On 2 May 2018 at 08:17, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> When using kdump, SOMETIMES the "size not consistent" warning message
> shows up when the crash kernel boots with early_ioremap_debug parameter:
>
> WARNING: CPU: 0 PID: 0 at ../mm/early_ioremap.c:182 early_iounmap+0x4f/0x12c()
> early_iounmap(ffffffffff200180, 00000118) [0] size not consistent 00000120
>
> The root cause is that the unmapping size of memory map doesn't
> match with the original size when mapping:
>
> in __efi_memmap_init()
>         map.map = early_memremap(phys_map, data->size);
>
> in efi_memmap_unmap()
>         size = efi.memmap.desc_size * efi.memmap.nr_map;
>         early_memunmap(efi.memmap.map, size);
>
> But the efi.memmap.nr_map is from __efi_memmap_init(). The remainder
> of size was discarded when calculating the nr_map:
>         map.nr_map = data->size / data->desc_size;
>
> When the original size of memory map region does not equal to the
> result of multiplication. The "size not consistent" warning
> will be triggered.
>
> This issue sometimes was hit by kdump because kexec set the efi map
> size to align with 16 when loading crash kernel image:
>
> in bzImage64_load()
>         efi_map_sz = efi_get_runtime_map_size();
>         efi_map_sz = ALIGN(efi_map_sz, 16);
>
> Dave Young's a841aa83d patch fixed kexec issue. On UEFI side, this
> patch changes the logic in the unmapping function. Using the end
> address of map to calcuate original size.
>

Why do we still need this patch? I.e., in which circumstances will
efi_memory_map_data::size assume a value that is rounded up or
otherwise incorrect?

> Thank Randy Wright for his report and testing. And also thank
> Takashi Iwai for his help to trace issue.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Tested-by: Randy Wright <rwright@hpe.com>
> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> ---
>  drivers/firmware/efi/memmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
> index 5fc7052..1f592d8 100644
> --- a/drivers/firmware/efi/memmap.c
> +++ b/drivers/firmware/efi/memmap.c
> @@ -121,7 +121,7 @@ void __init efi_memmap_unmap(void)
>         if (!efi.memmap.late) {
>                 unsigned long size;
>
> -               size = efi.memmap.desc_size * efi.memmap.nr_map;
> +               size = efi.memmap.map_end - efi.memmap.map;
>                 early_memunmap(efi.memmap.map, size);
>         } else {
>                 memunmap(efi.memmap.map);
> --
> 2.10.2
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] efi: Fix the size not consistent issue when unmapping memory map
@ 2018-05-02  6:17 Lee, Chun-Yi
  2018-05-03 12:05 ` Ard Biesheuvel
  0 siblings, 1 reply; 12+ messages in thread
From: Lee, Chun-Yi @ 2018-05-02  6:17 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: linux-efi, linux-kernel, Lee, Chun-Yi, Takashi Iwai, Vivek Goyal,
	Ingo Molnar

When using kdump, SOMETIMES the "size not consistent" warning message
shows up when the crash kernel boots with early_ioremap_debug parameter:

WARNING: CPU: 0 PID: 0 at ../mm/early_ioremap.c:182 early_iounmap+0x4f/0x12c()
early_iounmap(ffffffffff200180, 00000118) [0] size not consistent 00000120

The root cause is that the unmapping size of memory map doesn't
match with the original size when mapping:

in __efi_memmap_init()
	map.map = early_memremap(phys_map, data->size);

in efi_memmap_unmap()
        size = efi.memmap.desc_size * efi.memmap.nr_map;
        early_memunmap(efi.memmap.map, size);

But the efi.memmap.nr_map is from __efi_memmap_init(). The remainder
of size was discarded when calculating the nr_map:
        map.nr_map = data->size / data->desc_size;

When the original size of memory map region does not equal to the
result of multiplication. The "size not consistent" warning
will be triggered.

This issue sometimes was hit by kdump because kexec set the efi map
size to align with 16 when loading crash kernel image:

in bzImage64_load()
        efi_map_sz = efi_get_runtime_map_size();
        efi_map_sz = ALIGN(efi_map_sz, 16);

Dave Young's a841aa83d patch fixed kexec issue. On UEFI side, this
patch changes the logic in the unmapping function. Using the end
address of map to calcuate original size.

Thank Randy Wright for his report and testing. And also thank
Takashi Iwai for his help to trace issue.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Ingo Molnar <mingo@redhat.com>
Tested-by: Randy Wright <rwright@hpe.com>
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
 drivers/firmware/efi/memmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index 5fc7052..1f592d8 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -121,7 +121,7 @@ void __init efi_memmap_unmap(void)
 	if (!efi.memmap.late) {
 		unsigned long size;
 
-		size = efi.memmap.desc_size * efi.memmap.nr_map;
+		size = efi.memmap.map_end - efi.memmap.map;
 		early_memunmap(efi.memmap.map, size);
 	} else {
 		memunmap(efi.memmap.map);
-- 
2.10.2

^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-05-04  7:29 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-13  6:27 [PATCH] efi: Fix the size not consistent issue when unmapping memory map Lee, Chun-Yi
2018-04-16  2:57 ` Dave Young
2018-04-16  3:09   ` Dave Young
2018-04-16  6:37     ` joeyli
2018-04-17  0:35       ` Randy Wright
2018-04-17  1:20         ` joeyli
2018-04-17  2:41         ` Dave Young
2018-04-17 20:34         ` Randy Wright
2018-04-16  6:34   ` joeyli
2018-05-02  6:17 Lee, Chun-Yi
2018-05-03 12:05 ` Ard Biesheuvel
2018-05-04  7:29   ` joeyli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).