RE: [PATCH V2 2/3] x86/efi: Fix EFI memory map leaks
> > -static void __init efi_clean_memmap(void)
> > +void __init efi_clean_memmap(void)
> > {
> > - efi_memory_desc_t *out = efi.memmap.map;
> > - const efi_memory_desc_t *in = out;
> > - const efi_memory_desc_t *end = efi.memmap.map_end;
> > - int i, n_removal;
> > -
> > - for (i = n_removal = 0; in < end; i++) {
> > - if (efi_memmap_entry_valid(in, i)) {
> > - if (out != in)
> > - memcpy(out, in, efi.memmap.desc_size);
> > - out = (void *)out + efi.memmap.desc_size;
> > - } else {
> > + void *out;
> > + efi_memory_desc_t *md;
> > + unsigned int i = 0, n_removal = 0;
>
> Perhaps we can use 'unsigned int i = n_removal = 0;', which appears more
> readable.
Hmm.. that actually throws an compilation error saying that "n_removal is
undeclared".
So, let's stick with unsigned int i = 0, n_removal = 0;
>
> > + struct efi_memory_map new_memmap;
> > +
> > + for_each_efi_memory_desc(md) {
> > + if (!efi_memmap_entry_valid(md, i))
> > n_removal++;
> > - }
> > - in = (void *)in + efi.memmap.desc_size;
> > }
> >
> > - if (n_removal > 0) {
> > - u64 size = efi.memmap.nr_map - n_removal;
> > + if (n_removal == 0)
>
> and 'if (!n_removal)' above, to stay in sync with the rest of the code in
> this file.
Sure! Thanks for the catch. I will fix it.
Regards,
Sai
RE: [PATCH V2 2/3] x86/efi: Fix EFI memory map leaks
> > if (efi_enabled(EFI_MEMMAP)) {
> > + /*
> > +* efi_clean_memmap() uses memblock_phys_alloc() to allocate
> > +* memory for new EFI memmap and hence will work only after
> > +* e820__memblock_setup()
> > +*/
> > + efi_clean_memmap();
> > efi_fake_memmap();
> > efi_find_mirror();
> > efi_esrt_init();
>
> I'd also suggest a namespace cleanup before we do any material
> modifications:
>
> 'efi_esrt_init()' is the proper pattern to follow, it's prefixed by efi_, then
> followed by the more generic subsystem (_esrt) and then by the functionality
> (_init). This is a good, hierarchical, top-down nomenclature that makes it
> easy to
> grep for ESRT functionality by typing 'git grep esrt'.
>
> The same is not true of the memmap functionality: 'git grep efi_memmap_'
> doesn't do the right thing.
>
> So I think this should be renamed to:
>
> efi_memmap_clean()
> efi_memmap_insert()
> efi_memmap_free()
> efi_memmap_print()
> efi_memmap_fake()
> etc.
>
> While at it, there's another area that could be improved:
>
> - efi_memmap_fake() is a bit weird naming: it's not really 'fake',
>presumably the specified table is still very much real, otherwise it
>won't result in a working kernel.
>
>What that functionality *really* is about is user-defined entries. Why
>not name it in such a fashion? efi_memmap_init_user_defined() or so?
>From the example you gave about efi_esrt_init(), the naming of efi memmap
related functions does look messy to me now.. and yes, a namespace clean up
might be good.
Regards,
Sai
Re: [PATCH V2 2/3] x86/efi: Fix EFI memory map leaks
May be the x86 maintainers would have more comments on this, but I have
a couple of nitpicks. Please see in-line:
On 12/05/2018 06:03 AM, Sai Praneeth Prakhya wrote:
Presently, in efi subsystem of kernel, every time kernel allocates memory
for a new EFI memory map, it forgets to free the memory occupied by the
existing EFI memory map. This could be fixed by unmapping and freeing the
existing EFI memory map every time before installing a new EFI memory map.
Hence, modify efi_memmap_install() accordingly since it's the only place
which installs a new EFI memory map.
Presently, efi_memmap_alloc() allocates only physical memory and every
caller of efi_memmap_alloc() should remap the newly allocated memory in
order to use it. This extra step could sometimes lead to buggy error
handling conditions where in the allocated memory isn't freed should remap
fail. So, push the remap logic into efi_memmap_alloc() so that the error
handling could be improved and it also makes the caller look simpler.
With the modified efi_memmap_alloc() and efi_memmap_install() API's, a
typical flow to install a new EFI memory map would look something like
below.
1. Get the number of entries the new EFI memory map should have (typically
through efi_memmap_split_count()).
2. Allocate memory for the new EFI memory map (efi_memmap_alloc()).
3. Populate memory descriptor entries in the new EFI memory map.
4. Install the new EFI memory map (efi_memmap_install() which also unmaps
and frees existing memory map).
Existing functions like efi_clean_memmap(), efi_arch_mem_reserve(),
efi_free_boot_services() and efi_fake_memmap() are modified to fix the
above mentioned bugs and also to follow the above recommended usage of
API's.
Note that efi_clean_memmap() could be implemented without allocating any
new memory, but since this is not fast path and hence is not a concern for
performance, readability and maintainability wins. So, change it to use
efi_memmap_alloc() and efi_memmap_install().
Signed-off-by: Sai Praneeth Prakhya
Suggested-by: Ard Biesheuvel
Cc: Ingo Molnar
Cc: Borislav Petkov
Cc: Bhupesh Sharma
Cc: Ard Biesheuvel
---
arch/x86/include/asm/efi.h | 1 +
arch/x86/kernel/setup.c | 6 ++
arch/x86/platform/efi/efi.c | 44 ++--
arch/x86/platform/efi/quirks.c | 43 +++-
drivers/firmware/efi/fake_mem.c | 21 ++
drivers/firmware/efi/memmap.c | 118 +++-
include/linux/efi.h | 7 +-
7 files changed, 132 insertions(+), 108 deletions(-)
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index d1e64ac80b9c..744f945a00e7 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -143,6 +143,7 @@ extern void efi_switch_mm(struct mm_struct *mm);
extern void efi_recover_from_page_fault(unsigned long phys_addr);
extern void efi_free_boot_services(void);
extern void efi_reserve_boot_services(void);
+extern void __init efi_clean_memmap(void);
struct efi_setup_data {
u64 fw_vendor;
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index b74e7bfed6ab..bed79b238b0d 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1102,6 +1102,12 @@ void __init setup_arch(char **cmdline_p)
reserve_bios_regions();
if (efi_enabled(EFI_MEMMAP)) {
+ /*
+* efi_clean_memmap() uses memblock_phys_alloc() to allocate
+* memory for new EFI memmap and hence will work only after
+* e820__memblock_setup()
+*/
+ efi_clean_memmap();
efi_fake_memmap();
efi_find_mirror();
efi_esrt_init();
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 715601d1c581..63885cc8e34e 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -249,30 +249,36 @@ static bool __init efi_memmap_entry_valid(const
efi_memory_desc_t *md, int i)
return false;
}
-static void __init efi_clean_memmap(void)
+void __init efi_clean_memmap(void)
{
- efi_memory_desc_t *out = efi.memmap.map;
- const efi_memory_desc_t *in = out;
- const efi_memory_desc_t *end = efi.memmap.map_end;
- int i, n_removal;
-
- for (i = n_removal = 0; in < end; i++) {
- if (efi_memmap_entry_valid(in, i)) {
- if (out != in)
- memcpy(out, in, efi.memmap.desc_size);
- out = (void *)out + efi.memmap.desc_size;
- } else {
+ void *out;
+ efi_memory_desc_t *md;
+ unsigned int i = 0, n_removal = 0;
Perhaps we can use 'unsigned int i = n_removal = 0;', which appears more
readable.
+ struct efi_memory_map new_memmap;
+
+ for_each_efi_memory_desc(md) {
+ if (!efi_memmap_entry_valid(md, i))
n_removal++;
- }
- in =
Re: [PATCH V2 2/3] x86/efi: Fix EFI memory map leaks
* Sai Praneeth Prakhya wrote:
> if (efi_enabled(EFI_MEMMAP)) {
> + /*
> + * efi_clean_memmap() uses memblock_phys_alloc() to allocate
> + * memory for new EFI memmap and hence will work only after
> + * e820__memblock_setup()
> + */
> + efi_clean_memmap();
> efi_fake_memmap();
> efi_find_mirror();
> efi_esrt_init();
I'd also suggest a namespace cleanup before we do any material
modifications:
'efi_esrt_init()' is the proper pattern to follow, it's prefixed by efi_,
then followed by the more generic subsystem (_esrt) and then by the
functionality (_init). This is a good, hierarchical, top-down
nomenclature that makes it easy to grep for ESRT functionality by typing
'git grep esrt'.
The same is not true of the memmap functionality: 'git grep efi_memmap_'
doesn't do the right thing.
So I think this should be renamed to:
efi_memmap_clean()
efi_memmap_insert()
efi_memmap_free()
efi_memmap_print()
efi_memmap_fake()
etc.
While at it, there's another area that could be improved:
- efi_memmap_fake() is a bit weird naming: it's not really 'fake',
presumably the specified table is still very much real, otherwise it
won't result in a working kernel.
What that functionality *really* is about is user-defined entries. Why
not name it in such a fashion? efi_memmap_init_user_defined() or so?
Thanks,
Ingo
[PATCH V2 2/3] x86/efi: Fix EFI memory map leaks
Presently, in efi subsystem of kernel, every time kernel allocates memory
for a new EFI memory map, it forgets to free the memory occupied by the
existing EFI memory map. This could be fixed by unmapping and freeing the
existing EFI memory map every time before installing a new EFI memory map.
Hence, modify efi_memmap_install() accordingly since it's the only place
which installs a new EFI memory map.
Presently, efi_memmap_alloc() allocates only physical memory and every
caller of efi_memmap_alloc() should remap the newly allocated memory in
order to use it. This extra step could sometimes lead to buggy error
handling conditions where in the allocated memory isn't freed should remap
fail. So, push the remap logic into efi_memmap_alloc() so that the error
handling could be improved and it also makes the caller look simpler.
With the modified efi_memmap_alloc() and efi_memmap_install() API's, a
typical flow to install a new EFI memory map would look something like
below.
1. Get the number of entries the new EFI memory map should have (typically
through efi_memmap_split_count()).
2. Allocate memory for the new EFI memory map (efi_memmap_alloc()).
3. Populate memory descriptor entries in the new EFI memory map.
4. Install the new EFI memory map (efi_memmap_install() which also unmaps
and frees existing memory map).
Existing functions like efi_clean_memmap(), efi_arch_mem_reserve(),
efi_free_boot_services() and efi_fake_memmap() are modified to fix the
above mentioned bugs and also to follow the above recommended usage of
API's.
Note that efi_clean_memmap() could be implemented without allocating any
new memory, but since this is not fast path and hence is not a concern for
performance, readability and maintainability wins. So, change it to use
efi_memmap_alloc() and efi_memmap_install().
Signed-off-by: Sai Praneeth Prakhya
Suggested-by: Ard Biesheuvel
Cc: Ingo Molnar
Cc: Borislav Petkov
Cc: Bhupesh Sharma
Cc: Ard Biesheuvel
---
arch/x86/include/asm/efi.h | 1 +
arch/x86/kernel/setup.c | 6 ++
arch/x86/platform/efi/efi.c | 44 ++--
arch/x86/platform/efi/quirks.c | 43 +++-
drivers/firmware/efi/fake_mem.c | 21 ++
drivers/firmware/efi/memmap.c | 118 +++-
include/linux/efi.h | 7 +-
7 files changed, 132 insertions(+), 108 deletions(-)
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index d1e64ac80b9c..744f945a00e7 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -143,6 +143,7 @@ extern void efi_switch_mm(struct mm_struct *mm);
extern void efi_recover_from_page_fault(unsigned long phys_addr);
extern void efi_free_boot_services(void);
extern void efi_reserve_boot_services(void);
+extern void __init efi_clean_memmap(void);
struct efi_setup_data {
u64 fw_vendor;
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index b74e7bfed6ab..bed79b238b0d 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1102,6 +1102,12 @@ void __init setup_arch(char **cmdline_p)
reserve_bios_regions();
if (efi_enabled(EFI_MEMMAP)) {
+ /*
+* efi_clean_memmap() uses memblock_phys_alloc() to allocate
+* memory for new EFI memmap and hence will work only after
+* e820__memblock_setup()
+*/
+ efi_clean_memmap();
efi_fake_memmap();
efi_find_mirror();
efi_esrt_init();
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 715601d1c581..63885cc8e34e 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -249,30 +249,36 @@ static bool __init efi_memmap_entry_valid(const
efi_memory_desc_t *md, int i)
return false;
}
-static void __init efi_clean_memmap(void)
+void __init efi_clean_memmap(void)
{
- efi_memory_desc_t *out = efi.memmap.map;
- const efi_memory_desc_t *in = out;
- const efi_memory_desc_t *end = efi.memmap.map_end;
- int i, n_removal;
-
- for (i = n_removal = 0; in < end; i++) {
- if (efi_memmap_entry_valid(in, i)) {
- if (out != in)
- memcpy(out, in, efi.memmap.desc_size);
- out = (void *)out + efi.memmap.desc_size;
- } else {
+ void *out;
+ efi_memory_desc_t *md;
+ unsigned int i = 0, n_removal = 0;
+ struct efi_memory_map new_memmap;
+
+ for_each_efi_memory_desc(md) {
+ if (!efi_memmap_entry_valid(md, i))
n_removal++;
- }
- in = (void *)in + efi.memmap.desc_size;
}
- if (n_removal > 0) {
- u64 size = efi.memmap.nr_map - n_removal;
+ if (n_removal == 0)
+ return;
- pr_warn("Removing %d invalid memory map entries.\n", n_removal);
-
