Re: [PATCH v2 1/2] x86/efi: don't allocate memmap through memblock after mm_init()

[Ard Biesheuvel]

On 5 January 2017 at 07:42, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> * Nicolai Stange <nicstange@xxxxxxxxx> wrote:
>
>> Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx> writes:
>>
>> > On Thu, 22 Dec, at 11:23:39AM, Nicolai Stange wrote:
>> >> So, after memblock is gone, allocations should be done through the "normal"
>> >> page allocator. Introduce a helper, efi_memmap_alloc() for this. Use
>> >> it from efi_arch_mem_reserve() and from efi_free_boot_services() as well.
>> >>
>> >> Fixes: 4bc9f92e64c8 ("x86/efi-bgrt: Use efi_mem_reserve() to avoid copying image data")
>> >> Signed-off-by: Nicolai Stange <nicstange@xxxxxxxxx>
>>
>> > Could you also modify efi_fake_memmap() to use your new
>> > efi_memmap_alloc() function for consistency
>>
>> Sure.
>>
>> I'm planning to submit another set of patches addressing the (bounded)
>> memmap leaking in anything calling efi_memmap_unmap() though. In the
>> course of doing so, the memmap allocation sites will get touched anyway:
>> I'll have to store some information about how the memmap's memory has
>> been obtained.
>
> Will that patch be intrusive?
>

Given that memblock_alloc() calls memblock_reserve() on its
allocations, we could simply consult the memblock_reserved table to
infer whether the allocation being freed was created with
memblock_alloc() or with alloc_pages(). So I don't think such a patch
should be that intrusive. But the normal case is that the EFI memory
map remains mapped during the lifetime of the system, and unmapping
the EFI memory map does not necessarily imply that it should be freed.
This is especially true on ARM systems, where the memory map is
allocated and populated by the stub, and never modified by the kernel
proper, and so any freeing logic in generic code should take this into
account as well (i.e., the memory map allocation is not
memblock_reserve()'d, nor is it allocated using alloc_pages())

> If yes then we'll need to keep this a separate urgent patch to fix the v4.9
> regression that Dan Williams reported. I can apply the fix to efi/urgent and get
> it to Linus straight away if you guys agree.
>

Considering the severity of the issue it solves, and the obvious
correctness of the fix, my preference would be to spin a v3 of this
patch taking Matt's feedback into account, and merging that as a fix
for v4.10 with a cc stable. The 2/2 can wait a bit longer imo