From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S938369AbdAEKQS (ORCPT ); Thu, 5 Jan 2017 05:16:18 -0500 Received: from mail-wm0-f65.google.com ([74.125.82.65]:34348 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S938347AbdAEKPc (ORCPT ); Thu, 5 Jan 2017 05:15:32 -0500 From: Nicolai Stange To: Ard Biesheuvel Cc: Ingo Molnar , Nicolai Stange , Matt Fleming , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , "x86\@kernel.org" , "linux-efi\@vger.kernel.org" , "linux-kernel\@vger.kernel.org" , Mika =?utf-8?Q?Penttil=C3=A4?= , Dan Williams Subject: Re: [PATCH v2 1/2] x86/efi: don't allocate memmap through memblock after mm_init() References: <20161222102340.2689-1-nicstange@gmail.com> <20161223145206.GC16838@codeblueprint.co.uk> <878tr6jqoa.fsf@gmail.com> <20170105074221.GA1777@gmail.com> Date: Thu, 05 Jan 2017 11:15:28 +0100 In-Reply-To: (Ard Biesheuvel's message of "Thu, 5 Jan 2017 09:39:01 +0000") Message-ID: <87inpt6ce7.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Ard Biesheuvel writes: > On 5 January 2017 at 07:42, Ingo Molnar wrote: >> >> * Nicolai Stange wrote: >> >>> Matt Fleming 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 >>> >>> > 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