Re: [PATCH V2 0/3] Fix EFI memory map leaks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



* Sai Praneeth Prakhya <sai.praneeth.prakhya@xxxxxxxxx> 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 old EFI memory map.
> It does clear the mappings though (using efi_memmap_unmap()), but forgets to
> free up the memory. Also, there is another minor issue, where in the newly
> allocated memory isn't freed, should remap fail.
> 
> The first issue is addressed by adding efi_memmap_free() to efi_memmap_install()
> and the second issue is addressed by pushing the remap code into
> efi_memmap_alloc() and there by handling the failure condition.
> 
> Memory allocated to EFI memory map is leaked in below functions and hence they
> are modified to fix the issue. Functions that modify EFI memmap are:
> 1. efi_clean_memmap(),
> 2. efi_fake_memmap(),
> 3. efi_arch_mem_reserve(),
> 4. efi_free_boot_services(),
> 5. and __efi_enter_virtual_mode()
> 
> More detailed explanation:
> --------------------------
> A typical boot flow on EFI supported x86_64 machines might look something like
> below
> 1. EFI memory map is passed by firmware to kernel.
> 2. Kernel does a memblock_reserve() on this memory
>    (see efi_memblock_x86_reserve_range()).
> 3. This memory map is checked for invalid entries in efi_clean_memmap(). If any
>    invalid entries are found, they are omitted from EFI memory map but the
>    memory occupied by these invalid EFI memory descriptors isn't freed.
> 3. To further process this memory map (see efi_fake_memmap(), efi_bgrt_init()
>    and efi_esrt_init()), kernel allocates memory using efi_memmap_alloc() and
>    copies the processed memory map to newly allocated memory but it forgets to
>    free memory occupied by old EFI memory map.
> 4. Further, in efi_map_regions() the EFI memory map is processed again to
>    include only EFI memory descriptors of type Runtime Code/Data and Boot
>    Code/Data. Again, memory is allocated for this new memory map through
>    realloc_pages() and the old EFI memory map is not freed.
> 5. After SetVirtualAddressMap() is done, the EFI memory map is processed again
>    to have only EFI memory descriptors of type Runtime Code/Data. Again, memory
>    is allocated for this new memory map through efi_memmap_alloc() and the old
>    EFI memory map is not freed.

So it was only halfway through the changelog that I realized that by 
'free the memory occupied by old EFI memory map' you didn't mean the EFI 
memory map itself (the system RAM described), but the EFI memory map 
*table*, this relatively small array of descriptors that we allocate and 
reallocate every now and then according to the limitations of the 
(largely non-existent yet) early boot memory management system...

Could we please use much more precise terminology when referring to these 
entities, in changelogs and code alike? I.e.:

 - 'EFI memory map' is the whole map and the contents

 - 'EFI memory map table' is the table structure itself, without regard 
   of its contents

 - 'EFI memory map table entry/descriptor' is an entry in the table.

This kind of clarity is what we are using on the e820 memory map side as 
well: we have struct e820_table and struct e820_entry.

Ard, would you object to standardizing the EFI code in a similar fashion 
as well: efi_mem_table and efi_mem_entry?

Parameters and local variables should be named unambiguously following 
these concepts as well. There should be no opaque 'new' variables 
*especially* where the primary role of the efi_memmap_insert() API call 
is to add a new *entry* - it should be 'new_efi_map_table' (or 
'new_table' where it's unambiguous) instead, and new entries added should 
be 'new_entry' - etc.

Once this is reorganized and clarified it should be a lot more easy to 
review 'table freeing' patches. I can help out with patches if there's no 
conceptual objections.

Thanks,

	Ingo



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux