On Wed, 12 Jun 2024 at 12:36, Dave Young <dyoung@xxxxxxxxxx> wrote: > > On Mon, 10 Jun 2024 at 22:09, Ard Biesheuvel <ardb+git@xxxxxxxxxx> wrote: > > > > From: Ard Biesheuvel <ardb@xxxxxxxxxx> > > > > The logic in __efi_memmap_init() is shared between two different > > execution flows: > > - mapping the EFI memory map early or late into the kernel VA space, so > > that its entries can be accessed; > > - cloning the EFI memory map in order to insert new entries that are > > created as a result of creating a memory reservation > > (efi_arch_mem_reserve()) > > > > In the former case, the underlying memory containing the kernel's view > > of the EFI memory map (which may be heavily modified by the kernel > > itself on x86) is not modified at all, and the only thing that changes > > is the virtual mapping of this memory, which is different between early > > and late boot. > > > > In the latter case, an entirely new allocation is created that carries a > > new, updated version of the kernel's view of the EFI memory map. When > > installing this new version, the old version will no longer be > > referenced, and if the memory was allocated by the kernel, it will leak > > unless it gets freed. > > > > The logic that implements this freeing currently lives on the code path > > that is shared between these two use cases, but it should only apply to > > the latter. So move it to the correct spot. > > > > Cc: Ashish Kalra <Ashish.Kalra@xxxxxxx> > > Cc: Dave Young <dyoung@xxxxxxxxxx> > > Cc: Mike Rapoport <rppt@xxxxxxxxxx> > > Cc: Borislav Petkov <bp@xxxxxxxxx> > > Link: https://lore.kernel.org/all/36ad5079-4326-45ed-85f6-928ff76483d3@xxxxxxx > > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx> > > --- > > arch/x86/platform/efi/memmap.c | 5 +++++ > > drivers/firmware/efi/memmap.c | 5 ----- > > 2 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/arch/x86/platform/efi/memmap.c b/arch/x86/platform/efi/memmap.c > > index 4ef20b49eb5e..4990244e5168 100644 > > --- a/arch/x86/platform/efi/memmap.c > > +++ b/arch/x86/platform/efi/memmap.c > > @@ -97,6 +97,11 @@ int __init efi_memmap_install(struct efi_memory_map_data *data) > > if (efi_enabled(EFI_PARAVIRT)) > > return 0; > > > > + if (efi.memmap.flags & (EFI_MEMMAP_MEMBLOCK | EFI_MEMMAP_SLAB)) > > + __efi_memmap_free(efi.memmap.phys_map, > > + efi.memmap.desc_size * efi.memmap.nr_map, > > + efi.memmap.flags); > > + > > return __efi_memmap_init(data); > > Nice fix! but would it be better to save the old addr/size/flag and > free them only when __efi_memmap_init succeed? > Yeah. I guess the same applies to the original code, but now is as good a time as ever to fix that as well.
