Messages in this thread | | | From | Nicolai Stange <> | Subject | Re: [PATCH v2 1/2] x86/efi: don't allocate memmap through memblock after mm_init() | Date | Thu, 05 Jan 2017 11:15:28 +0100 |
| |
Ard Biesheuvel <ard.biesheuvel@linaro.org> writes:
> On 5 January 2017 at 07:42, Ingo Molnar <mingo@kernel.org> wrote: >> >> * Nicolai Stange <nicstange@gmail.com> wrote: >> >>> Matt Fleming <matt@codeblueprint.co.uk> 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@gmail.com> >>> >>> > 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
| |