Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> writes:
> 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?
Yes, definitely something for 4.11+.
> 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().
Not sure whether this would work with CONFIG_ARCH_DISCARD_MEMBLOCK=y.
This is also the reason why 2/2 is needed.
> 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
Matt's Feedback included that
"all memblock_alloc()s should probably be PAGE_SIZE aligned like the
fakemem code"
Unfortunately, I can't see why this would be needed. Furthermore, this
isn't currently done outside of fakemem and thus, aliging the memmap
allocations on PAGE_SIZE would be another, quite unrelated change?
So, are you Ok with only taking the other review comment, namely
"modify efi_fake_memmap() to use your new efi_memmap_alloc() function
for consistency"
into account for a v3?
Thanks,
Nicolai
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
