* Re: [PATCH] efi/x86: fix boot regression on systems with invalid memmap entries
2020-02-01 23:33 [PATCH] efi/x86: fix boot regression on systems with invalid memmap entries Ard Biesheuvel
@ 2020-02-02 9:34 ` Ingo Molnar
2020-02-02 9:50 ` Ard Biesheuvel
2020-02-02 9:39 ` [tip: efi/urgent] efi/x86: Fix " tip-bot2 for Ard Biesheuvel
1 sibling, 1 reply; 4+ messages in thread
From: Ingo Molnar @ 2020-02-02 9:34 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-efi, x86, dan.j.williams, jrg.otte, torvalds, linux-kernel
* Ard Biesheuvel <ardb@kernel.org> wrote:
> In efi_clean_memmap(), we do a pass over the EFI memory map to remove
> bogus entries that may be returned on certain systems.
>
> Commit 1db91035d01aa8bf ("efi: Add tracking for dynamically allocated
> memmaps") refactored this code to pass the input to efi_memmap_install()
> via a temporary struct on the stack, which is populated using an
> initializer which inadvertently defines the value of its size field
> in terms of its desc_size field, which value cannot be relied upon yet
> in the initializer itself.
>
> Fix this by using efi.memmap.desc_size instead, which is where we get
> the value for desc_size from in the first place.
>
> Tested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> arch/x86/platform/efi/efi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 59f7f6d60cf6..ae923ee8e2b4 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -308,7 +308,7 @@ static void __init efi_clean_memmap(void)
> .phys_map = efi.memmap.phys_map,
> .desc_version = efi.memmap.desc_version,
> .desc_size = efi.memmap.desc_size,
> - .size = data.desc_size * (efi.memmap.nr_map - n_removal),
> + .size = efi.memmap.desc_size * (efi.memmap.nr_map - n_removal),
> .flags = 0,
> };
Applied, and I also added:
Reported-by: Jörg Otte <jrg.otte@gmail.com>
Tested-by: Jörg Otte <jrg.otte@gmail.com>
I presumptively added the Jörg's Tested-by tag: won't send the commit to
Linus if he still has trouble booting the laptop.
I'm still amazed GCC doesn't warn about this pattern - why?
BTW., could we please also organize such assignments vertically as well:
.phys_map = efi.memmap.phys_map,
.desc_version = efi.memmap.desc_version,
.desc_size = efi.memmap.desc_size,
.size = efi.memmap.desc_size * (efi.memmap.nr_map - n_removal),
.flags = 0,
(See the patch below.)
Had we done that earlier the weird pattern would have stuck out a lot
more:
.phys_map = efi.memmap.phys_map,
.desc_version = efi.memmap.desc_version,
.desc_size = efi.memmap.desc_size,
.size = data.desc_size * (efi.memmap.nr_map - n_removal),
.flags = 0,
BTW., is there a reason "struct efi_memory_map" doesn't embedd a "struct
efi_memory_map_data"? Or is efi_memory_map firmware ABI?
If they shared the structure then copying would be easier.
Thanks,
Ingo
Signed-off-by: Ingo Molnar <mingo@kernel.org>
arch/x86/platform/efi/efi.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index ae923ee8e2b4..293c47f9cb39 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -305,11 +305,11 @@ static void __init efi_clean_memmap(void)
if (n_removal > 0) {
struct efi_memory_map_data data = {
- .phys_map = efi.memmap.phys_map,
- .desc_version = efi.memmap.desc_version,
- .desc_size = efi.memmap.desc_size,
- .size = efi.memmap.desc_size * (efi.memmap.nr_map - n_removal),
- .flags = 0,
+ .phys_map = efi.memmap.phys_map,
+ .desc_version = efi.memmap.desc_version,
+ .desc_size = efi.memmap.desc_size,
+ .size = efi.memmap.desc_size * (efi.memmap.nr_map - n_removal),
+ .flags = 0,
};
pr_warn("Removing %d invalid memory map entries.\n", n_removal);
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [tip: efi/urgent] efi/x86: Fix boot regression on systems with invalid memmap entries
2020-02-01 23:33 [PATCH] efi/x86: fix boot regression on systems with invalid memmap entries Ard Biesheuvel
2020-02-02 9:34 ` Ingo Molnar
@ 2020-02-02 9:39 ` tip-bot2 for Ard Biesheuvel
1 sibling, 0 replies; 4+ messages in thread
From: tip-bot2 for Ard Biesheuvel @ 2020-02-02 9:39 UTC (permalink / raw)
To: linux-tip-commits
Cc: jrg.otte, Dan Williams, Ard Biesheuvel, Ingo Molnar, linux-efi,
torvalds, x86, LKML
The following commit has been merged into the efi/urgent branch of tip:
Commit-ID: 59365cadfbcd299b8cdbe0c165faf15767c5f166
Gitweb: https://git.kernel.org/tip/59365cadfbcd299b8cdbe0c165faf15767c5f166
Author: Ard Biesheuvel <ardb@kernel.org>
AuthorDate: Sun, 02 Feb 2020 00:33:04 +01:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Sun, 02 Feb 2020 10:25:43 +01:00
efi/x86: Fix boot regression on systems with invalid memmap entries
In efi_clean_memmap(), we do a pass over the EFI memory map to remove
bogus entries that may be returned on certain systems.
This recent commit:
1db91035d01aa8bf ("efi: Add tracking for dynamically allocated memmaps")
refactored this code to pass the input to efi_memmap_install() via a
temporary struct on the stack, which is populated using an initializer
which inadvertently defines the value of its size field in terms of its
desc_size field, which value cannot be relied upon yet in the initializer
itself.
Fix this by using efi.memmap.desc_size instead, which is where we get
the value for desc_size from in the first place.
Reported-by: Jörg Otte <jrg.otte@gmail.com>
Tested-by: Jörg Otte <jrg.otte@gmail.com>
Tested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: linux-efi@vger.kernel.org
Cc: jrg.otte@gmail.com
Cc: torvalds@linux-foundation.org
Cc: mingo@kernel.org
Link: https://lore.kernel.org/r/20200201233304.18322-1-ardb@kernel.org
---
arch/x86/platform/efi/efi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 59f7f6d..ae923ee 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -308,7 +308,7 @@ static void __init efi_clean_memmap(void)
.phys_map = efi.memmap.phys_map,
.desc_version = efi.memmap.desc_version,
.desc_size = efi.memmap.desc_size,
- .size = data.desc_size * (efi.memmap.nr_map - n_removal),
+ .size = efi.memmap.desc_size * (efi.memmap.nr_map - n_removal),
.flags = 0,
};
^ permalink raw reply related [flat|nested] 4+ messages in thread