From: Sai Praneeth <[email protected]>
efi_memmap_alloc(), as the name suggests, allocates memory for a new efi
memory map. It's referenced from couple of places, namely,
efi_arch_mem_reserve() and efi_free_boot_services(). These callers,
after allocating memory, remap it for further use. As usual, a routine
check is performed to confirm successful remap. If the remap fails,
ideally, the allocated memory should be freed but presently we just
return without freeing it up. Hence, fix this bug by freeing up the
memory appropriately.
As efi_memmap_alloc() allocates memory depending on whether mm_init()
has already been invoked or not, similarly, while freeing use
memblock_free() to free memory allocated before invoking mm_init() and
__free_pages() to free memory allocated after invoking mm_init().
It's a fact that memremap() and early_memremap() might never fail and
this code might never get a chance to run but to maintain good kernel
programming semantics, we might need this patch.
Signed-off-by: Sai Praneeth Prakhya <[email protected]>
Cc: Lee Chun-Yi <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Ricardo Neri <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ravi Shankar <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
---
I found this bug when working on a different patch set which uses
efi_memmap_alloc() and then noticed that I never freed the allocated
memory. I found it weird, in the sense that, memory is allocated but is
not freed (upon returning from an error). So, wasn't sure if that should
be treated as a bug or should I just leave it as is because everything
works fine even without this patch. Since the effort for the patch is
very minimal, I just went ahead and posted one, so that I could know
your thoughts on it.
Changes from V1 to V2:
----------------------
1. Fix the bug of freeing memory map that was just installed by correctly
calling free_pages().
2. Call memblock_free() and __free_pages() directly from the appropriate
places instead of efi_memmap_free().
Note: Patch based on Linus's mainline tree V4.18-rc1
arch/x86/platform/efi/quirks.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 36c1f8b9f7e0..cfa93af97def 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -285,6 +285,7 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
new = early_memremap(new_phys, new_size);
if (!new) {
pr_err("Failed to map new boot services memmap\n");
+ memblock_free(new_phys, new_size);
return;
}
@@ -429,6 +430,7 @@ void __init efi_free_boot_services(void)
new = memremap(new_phys, new_size, MEMREMAP_WB);
if (!new) {
pr_err("Failed to map new EFI memmap\n");
+ __free_pages(pfn_to_page(PHYS_PFN(new_phys)), get_order(new_size));
return;
}
@@ -452,6 +454,7 @@ void __init efi_free_boot_services(void)
if (efi_memmap_install(new_phys, num_entries)) {
pr_err("Could not install new EFI memmap\n");
+ __free_pages(pfn_to_page(PHYS_PFN(new_phys)), get_order(new_size));
return;
}
}
--
2.7.4
On 18 June 2018 at 21:36, Sai Praneeth Prakhya
<[email protected]> wrote:
> From: Sai Praneeth <[email protected]>
>
> efi_memmap_alloc(), as the name suggests, allocates memory for a new efi
> memory map. It's referenced from couple of places, namely,
> efi_arch_mem_reserve() and efi_free_boot_services(). These callers,
> after allocating memory, remap it for further use. As usual, a routine
> check is performed to confirm successful remap. If the remap fails,
> ideally, the allocated memory should be freed but presently we just
> return without freeing it up. Hence, fix this bug by freeing up the
> memory appropriately.
>
> As efi_memmap_alloc() allocates memory depending on whether mm_init()
> has already been invoked or not, similarly, while freeing use
> memblock_free() to free memory allocated before invoking mm_init() and
> __free_pages() to free memory allocated after invoking mm_init().
>
> It's a fact that memremap() and early_memremap() might never fail and
> this code might never get a chance to run but to maintain good kernel
> programming semantics, we might need this patch.
>
> Signed-off-by: Sai Praneeth Prakhya <[email protected]>
> Cc: Lee Chun-Yi <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Tony Luck <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: Bhupesh Sharma <[email protected]>
> Cc: Ricardo Neri <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ravi Shankar <[email protected]>
> Cc: Matt Fleming <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> ---
>
> I found this bug when working on a different patch set which uses
> efi_memmap_alloc() and then noticed that I never freed the allocated
> memory. I found it weird, in the sense that, memory is allocated but is
> not freed (upon returning from an error). So, wasn't sure if that should
> be treated as a bug or should I just leave it as is because everything
> works fine even without this patch. Since the effort for the patch is
> very minimal, I just went ahead and posted one, so that I could know
> your thoughts on it.
>
> Changes from V1 to V2:
> ----------------------
> 1. Fix the bug of freeing memory map that was just installed by correctly
> calling free_pages().
> 2. Call memblock_free() and __free_pages() directly from the appropriate
> places instead of efi_memmap_free().
>
> Note: Patch based on Linus's mainline tree V4.18-rc1
>
> arch/x86/platform/efi/quirks.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 36c1f8b9f7e0..cfa93af97def 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -285,6 +285,7 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
> new = early_memremap(new_phys, new_size);
> if (!new) {
> pr_err("Failed to map new boot services memmap\n");
> + memblock_free(new_phys, new_size);
> return;
> }
>
> @@ -429,6 +430,7 @@ void __init efi_free_boot_services(void)
> new = memremap(new_phys, new_size, MEMREMAP_WB);
> if (!new) {
> pr_err("Failed to map new EFI memmap\n");
> + __free_pages(pfn_to_page(PHYS_PFN(new_phys)), get_order(new_size));
> return;
> }
>
> @@ -452,6 +454,7 @@ void __init efi_free_boot_services(void)
>
> if (efi_memmap_install(new_phys, num_entries)) {
> pr_err("Could not install new EFI memmap\n");
> + __free_pages(pfn_to_page(PHYS_PFN(new_phys)), get_order(new_size));
> return;
> }
> }
> --
> 2.7.4
>
Thank you Sai.
But this is not really what I meant.
How about we modify efi_memmap_alloc() like this
@@ -39,10 +39,12 @@ static phys_addr_t __init
__efi_memmap_alloc_late(unsigned long size)
* Returns the physical address of the allocated memory map on
* success, zero on failure.
*/
-phys_addr_t __init efi_memmap_alloc(unsigned int num_entries)
+phys_addr_t __init efi_memmap_alloc(unsigned int num_entries, bool *late)
{
unsigned long size = num_entries * efi.memmap.desc_size;
+ if (late)
+ *late = slab_is_available();
if (slab_is_available())
return __efi_memmap_alloc_late(size);
and introduce efi_memmap_free() as before, but pass it the 'late'
parameter you received from efi_memmap_alloc(). That way, it is the
caller's job to take care of this.
Also, it seems to me that efi_arch_mem_reserve() leaks the old memory
map every time you create a new one, no? That is a separate issue that
you may want to look into, but it affects the design of this API as
well.
> >
> Thank you Sai.
>
> But this is not really what I meant.
>
Ya.. sorry! about that. I had a hunch that you might be suggesting something
like below but I went ahead with this implementation as it looked very simple
(just 3 insertions and no deletions)
> How about we modify efi_memmap_alloc() like this
>
It sounds like a good idea to me. Leaving aside the pros (which are obvious),
the only con I could see is few extra checks and some code but I don't think
it's an issue at all because this code is not in fast path and it dosen't
impact performance. So, I will post a V3 with suggested changes.
> @@ -39,10 +39,12 @@ static phys_addr_t __init
> __efi_memmap_alloc_late(unsigned long size)
> * Returns the physical address of the allocated memory map on
> * success, zero on failure.
> */
> -phys_addr_t __init efi_memmap_alloc(unsigned int num_entries)
> +phys_addr_t __init efi_memmap_alloc(unsigned int num_entries, bool *late)
> {
> unsigned long size = num_entries * efi.memmap.desc_size;
>
> + if (late)
> + *late = slab_is_available();
> if (slab_is_available())
> return __efi_memmap_alloc_late(size);
>
> and introduce efi_memmap_free() as before, but pass it the 'late'
> parameter you received from efi_memmap_alloc(). That way, it is the
> caller's job to take care of this.
>
Sure! makes sense.
> Also, it seems to me that efi_arch_mem_reserve() leaks the old memory
> map every time you create a new one, no?
I think you are right. The issue I see is (please let me know if you think
otherwise):
1. efi_arch_mem_reserve() comes up with a new memory map and then tries to
install it via efi_memmap_install().
2. efi_memmap_install(), unmaps the existing memory map and installs the new
memory map but doesn't free the memory used by the existing memory map.
Hence, as you said, leaks the old memory map.
If this you what you meant, I think, the issue is not just limited to
efi_arch_mem_reserve() but to all the places that call efi_memmap_install().
I think, we could solve it as below
diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index 678e85704054..50ae4ffbf058 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -228,6 +228,7 @@ int __init efi_memmap_install(phys_addr_t addr, unsigned
int nr_map)
struct efi_memory_map_data data;
efi_memmap_unmap();
+ efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map,
efi.memmap.late);
data.phys_map = addr;
data.size = efi.memmap.desc_size * nr_map;
Please let me know your thoughts on it.
> That is a separate issue that
> you may want to look into, but it affects the design of this API as
> well.
Probably, I could have misunderstood you here.. but I think the
efi_memmap_free() API in V3 should work (without changes). Don't you think so?
Regards,
Sai