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

PATCH In review Lee, Chun-Yi 4 participants 9 messages Apr 13, 2018 – Apr 17, 2018

Subsystems (4)

x86 EFI Live update kexec handover (KHO)

lore

Share & tools

lore

Lee, Chun-Yi

author

2018-04-13 06:27 UTC #0 lore raw

PATCH 1/1 efi: Fix the size not consistent issue when unmapping memory map

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 <[email protected]>

Cc Randy Wright <[email protected]>

Cc Takashi Iwai <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc Ingo Molnar <[email protected]>

Signed-off-by "Lee, Chun-Yi" <[email protected]>

---
 drivers/firmware/efi/memmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/firmware/efi/memmap.c +1 -1

index 5fc7052..1f592d8 100644
@@ -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);

Signature · 1 line

2.10.2

Dave Young

2018-04-16 02:57 UTC #1 lore raw

Quoted 28 lines · 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()

Quoted 8 lines

        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.

Quoted 32 lines

Cc Ard Biesheuvel <[email protected]>
Cc Randy Wright <[email protected]>
Cc Takashi Iwai <[email protected]>
Cc Vivek Goyal <[email protected]>
Cc Ingo Molnar <[email protected]>
Signed-off-by "Lee, Chun-Yi" <[email protected]>
---
 drivers/firmware/efi/memmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/firmware/efi/memmap.c +1 -1
index 5fc7052..1f592d8 100644
@@ -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);
Signature · 7 lines
2.10.2


_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Thanks
Dave

Dave Young

2018-04-16 03:09 UTC #2 lore raw

Quoted 35 lines · Dave Young

Quoted 34 lines · 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.

Quoted 6 lines

Quoted 2 lines
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?

arch/x86/kernel/kexec-bzimage64.c +2 -3

index 3182908b7e6c..eaee37c54b7b 100644
@@ -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;

joeyli

2018-04-16 06:37 UTC #3 lore raw

Hi Randy,

Quoted 8 lines · Dave Young

Quoted 7 lines · Dave Young
Quoted 6 lines · 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

[...snip]

Quoted 7 lines

Quoted 4 lines
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

Quoted 27 lines

arch/x86/kernel/kexec-bzimage64.c +2 -3

index 3182908b7e6c..eaee37c54b7b 100644
@@ -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;

Randy Wright

2018-04-17 00:35 UTC #4 lore raw

Quoted 7 lines · joeyli

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.

Signature · 2 lines

Randy Wright            Hewlett Packard Enterprise
Phone: (970) 898-0998   Mail: [email protected]

joeyli

2018-04-17 01:20 UTC #5 lore raw

Quoted 9 lines · Randy Wright

Quoted 7 lines · joeyli
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

Quoted 9 lines

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.

Quoted 4 lines

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

Dave Young

2018-04-17 02:41 UTC #6 lore raw

In reply to Randy Wright

Quoted 9 lines · Randy Wright

Quoted 7 lines · joeyli
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

Quoted 8 lines

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

Randy Wright

2018-04-17 20:34 UTC #7 lore raw

In reply to Randy Wright

Quoted 3 lines · Randy Wright

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

Signature · 2 lines

Randy Wright            Hewlett Packard Enterprise
Phone: (970) 898-0998   Mail: [email protected]

joeyli

2018-04-16 06:34 UTC #8 lore raw

In reply to Dave Young

Quoted 35 lines · Dave Young

Quoted 34 lines · 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.

Quoted 7 lines

Quoted 2 lines
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!

Quoted 4 lines

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