[PATCH] efi: Fix the size not consistent issue when unmapping memory map

[PATCH] efi: Fix the size not consistent issue when unmapping memory map

[Lee, Chun-Yi]

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


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

Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map

[Dave Young]

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

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

Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map

[Dave Young]

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;

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

Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map

[joeyli]

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;

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

Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map

[Randy Wright]

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

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

Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map

[joeyli]

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 

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

Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map

[Dave Young]

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

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

Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map

[Randy Wright]

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

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

Re: [PATCH] efi: Fix the size not consistent issue when unmapping memory map

[joeyli]

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

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