Skip to contentSkip to conversation

x86/efi: Add necessary checks before iterating over efi.memmap

PATCH In review Chao Gao 4 participants 5 messages Sep 13, 2016 – Sep 20, 2016 2 messages not in the archive

Subsystems (2)
x86 EFI
lore
Share & tools
lore

Chao Gao

author
#0 lore raw

PATCH 1/1 x86/efi: Add necessary checks before iterating over efi.memmap

Commit 78ce248f (efi: Iterate over efi.memmap in for_each_efi_memory_desc()) replaces the old loop by for_each_efi_memory_desc() which will encounter #PF when efi.memap are not initialized.

In boot process, xen set EFI_PARAVIRT in xen_efi_init() before setup_arch() is called. This leads efi_memmap_init() will not initialize structures related to efi.memmap. However, the following functions e.g. efi_find_mirror(), efi_print_memmap() and efi_free_boot_services() access efi.memmap without necessary checks. kernel and xen crash in this case. After adding these checks, xen and kernel boot up normally.

Signed-off-by Chao Gao <[email protected]>
---
 arch/x86/platform/efi/efi.c    | 6 ++++++
 arch/x86/platform/efi/quirks.c | 3 +++
 2 files changed, 9 insertions(+)
arch/x86/platform/efi/efi.c +6 -0
index 1fbb408..68966dc 100644
@@ -102,6 +102,9 @@ void __init efi_find_mirror(void)
 	efi_memory_desc_t *md;
 	u64 mirror_size = 0, total_size = 0;
 
+	if (efi_enabled(EFI_PARAVIRT))
+		return;
+
 	for_each_efi_memory_desc(md) {
 		unsigned long long start = md->phys_addr;
 		unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;

@@ -207,6 +210,9 @@ void __init efi_print_memmap(void)
 	efi_memory_desc_t *md;
 	int i = 0;
 
+	if (efi_enabled(EFI_PARAVIRT))
+		return;
+
 	for_each_efi_memory_desc(md) {
 		char buf[64];
arch/x86/platform/efi/quirks.c +3 -0
index 89d1146..4fa1e4d 100644
@@ -251,6 +251,9 @@ void __init efi_free_boot_services(void)
 {
 	efi_memory_desc_t *md;
 
+	if (efi_enabled(EFI_PARAVIRT))
+		return;
+
 	for_each_efi_memory_desc(md) {
 		unsigned long long start = md->phys_addr;
 		unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
Signature · 1 line
1.8.3.1

A message that is not in the archive (sent off-list or not yet received): 20160913032813.GA11652-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]

#2 lore raw
Quoted 11 lines · Chao Gao

Commit 78ce248f (efi: Iterate over efi.memmap in for_each_efi_memory_desc()) replaces the old loop by for_each_efi_memory_desc() which will encounter #PF when efi.memap are not initialized.

In boot process, xen set EFI_PARAVIRT in xen_efi_init() before setup_arch() is called. This leads efi_memmap_init() will not initialize structures related to efi.memmap. However, the following functions e.g. efi_find_mirror(), efi_print_memmap() and efi_free_boot_services() access efi.memmap without necessary checks. kernel and xen crash in this case. After adding these checks, xen and kernel boot up normally.

OK, so looking at the hunks below, it looks to me like we should initialize the efi.memmap to sane values in the EFI_PARAVIRT case, rather than open coding this test each time for_each_efi_memory_desc() is invoked.

Quoted 51 lines
Signed-off-by Chao Gao <[email protected]>
---
 arch/x86/platform/efi/efi.c    | 6 ++++++
 arch/x86/platform/efi/quirks.c | 3 +++
 2 files changed, 9 insertions(+)
arch/x86/platform/efi/efi.c +6 -0
index 1fbb408..68966dc 100644
@@ -102,6 +102,9 @@ void __init efi_find_mirror(void)
        efi_memory_desc_t *md;
        u64 mirror_size = 0, total_size = 0;
 
+       if (efi_enabled(EFI_PARAVIRT))
+               return;
+
        for_each_efi_memory_desc(md) {
                unsigned long long start = md->phys_addr;
                unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;

@@ -207,6 +210,9 @@ void __init efi_print_memmap(void)
        efi_memory_desc_t *md;
        int i = 0;
 
+       if (efi_enabled(EFI_PARAVIRT))
+               return;
+
        for_each_efi_memory_desc(md) {
                char buf[64];
arch/x86/platform/efi/quirks.c +3 -1
index 89d1146..4fa1e4d 100644
@@ -251,6 +251,9 @@ void __init efi_free_boot_services(void)
 {
        efi_memory_desc_t *md;
 
+       if (efi_enabled(EFI_PARAVIRT))
+               return;
+
        for_each_efi_memory_desc(md) {
                unsigned long long start = md->phys_addr;
                unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
--

1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): CAKv+Gu_NrMGSu-9DxyZdurLWNbkHx2D480WS3gupazzC8U67EQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

author
#4 lore raw
Quoted 13 lines · Ard Biesheuvel
Quoted 11 lines · Chao Gao

Commit 78ce248f (efi: Iterate over efi.memmap in for_each_efi_memory_desc()) replaces the old loop by for_each_efi_memory_desc() which will encounter #PF when efi.memap are not initialized.

In boot process, xen set EFI_PARAVIRT in xen_efi_init() before setup_arch() is called. This leads efi_memmap_init() will not initialize structures related to efi.memmap. However, the following functions e.g. efi_find_mirror(), efi_print_memmap() and efi_free_boot_services() access efi.memmap without necessary checks. kernel and xen crash in this case. After adding these checks, xen and kernel boot up normally.

Quoted 5 lines

OK, so looking at the hunks below, it looks to me like we should initialize the efi.memmap to sane values in the EFI_PARAVIRT case, rather than open coding this test each time for_each_efi_memory_desc() is invoked.

Ard, thanks for you reply.
Yes, I think what you say is also a solution. This patch just uses the same method using by some existed functions such as efi_memblock_x86_reserve_range() and efi_runtime_init(). we can handle cases with existed flags correctly and the checks seems not too much. Maybe we can try what you say when it's really needed.

Quoted 51 lines
Quoted 51 lines
Signed-off-by Chao Gao <[email protected]>
---
 arch/x86/platform/efi/efi.c    | 6 ++++++
 arch/x86/platform/efi/quirks.c | 3 +++
 2 files changed, 9 insertions(+)
arch/x86/platform/efi/efi.c +6 -0
index 1fbb408..68966dc 100644
@@ -102,6 +102,9 @@ void __init efi_find_mirror(void)
        efi_memory_desc_t *md;
        u64 mirror_size = 0, total_size = 0;
 
+       if (efi_enabled(EFI_PARAVIRT))
+               return;
+
        for_each_efi_memory_desc(md) {
                unsigned long long start = md->phys_addr;
                unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;

@@ -207,6 +210,9 @@ void __init efi_print_memmap(void)
        efi_memory_desc_t *md;
        int i = 0;
 
+       if (efi_enabled(EFI_PARAVIRT))
+               return;
+
        for_each_efi_memory_desc(md) {
                char buf[64];
arch/x86/platform/efi/quirks.c +3 -1
index 89d1146..4fa1e4d 100644
@@ -251,6 +251,9 @@ void __init efi_free_boot_services(void)
 {
        efi_memory_desc_t *md;
 
+       if (efi_enabled(EFI_PARAVIRT))
+               return;
+
        for_each_efi_memory_desc(md) {
                unsigned long long start = md->phys_addr;
                unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
--

1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw
In reply to Chao Gao

Sorry for bothering you. There is a regression since commit 78ce248f that if booting xen in UEFI mode, dom0 will crash and xen reboot constantly. This patch tries to fix it. Please take a look at it.

Quoted 62 lines · Chao Gao

Commit 78ce248f (efi: Iterate over efi.memmap in for_each_efi_memory_desc()) replaces the old loop by for_each_efi_memory_desc() which will encounter #PF when efi.memap are not initialized.

In boot process, xen set EFI_PARAVIRT in xen_efi_init() before setup_arch() is called. This leads efi_memmap_init() will not initialize structures related to efi.memmap. However, the following functions e.g. efi_find_mirror(), efi_print_memmap() and efi_free_boot_services() access efi.memmap without necessary checks. kernel and xen crash in this case. After adding these checks, xen and kernel boot up normally.

Signed-off-by Chao Gao <[email protected]>

---
arch/x86/platform/efi/efi.c | 6 ++++++
arch/x86/platform/efi/quirks.c | 3 +++

2 files changed, 9 insertions(+)
arch/x86/platform/efi/efi.c +6 -0
index 1fbb408..68966dc 100644
@@ -102,6 +102,9 @@ void __init efi_find_mirror(void)
 	efi_memory_desc_t *md;
 	u64 mirror_size = 0, total_size = 0;
 
+	if (efi_enabled(EFI_PARAVIRT))
+		return;
+
 	for_each_efi_memory_desc(md) {
 		unsigned long long start = md->phys_addr;
 		unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;

@@ -207,6 +210,9 @@ void __init efi_print_memmap(void)
 	efi_memory_desc_t *md;
 	int i = 0;
 
+	if (efi_enabled(EFI_PARAVIRT))
+		return;
+
 	for_each_efi_memory_desc(md) {
 		char buf[64];
arch/x86/platform/efi/quirks.c +3 -0
index 89d1146..4fa1e4d 100644
@@ -251,6 +251,9 @@ void __init efi_free_boot_services(void)
 {
 	efi_memory_desc_t *md;
 
+	if (efi_enabled(EFI_PARAVIRT))
+		return;
+
 	for_each_efi_memory_desc(md) {
 		unsigned long long start = md->phys_addr;
 		unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
Signature · 6 lines
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#6 lore raw
Quoted 3 lines · Chao Gao

Sorry for bothering you. There is a regression since commit 78ce248f that if booting xen in UEFI mode, dom0 will crash and xen reboot constantly. This patch tries to fix it. Please take a look at it.

Can you try commit d4c4fed08f31 ("efi: Make for_each_efi_memory_desc_in_map() cope with running on Xen") and see if that fixes things for you?

That patch is already in Linus' tree and is included in v4.8-rc7.