From patchwork Fri Jul 22 16:45:24 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ard Biesheuvel X-Patchwork-Id: 72633 Delivered-To: patch@linaro.org Received: by 10.140.29.52 with SMTP id a49csp1092733qga; Fri, 22 Jul 2016 09:47:00 -0700 (PDT) X-Received: by 10.66.193.163 with SMTP id hp3mr8052005pac.73.1469206020161; Fri, 22 Jul 2016 09:47:00 -0700 (PDT) Return-Path: Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id f9si16657345pfa.18.2016.07.22.09.47.00 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 22 Jul 2016 09:47:00 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org designates 2001:1868:205::9 as permitted sender) client-ip=2001:1868:205::9; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@linaro.org; spf=pass (google.com: best guess record for domain of linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org designates 2001:1868:205::9 as permitted sender) smtp.mailfrom=linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1bQdaB-0001Et-Lq; Fri, 22 Jul 2016 16:45:51 +0000 Received: from mail-io0-x22a.google.com ([2607:f8b0:4001:c06::22a]) by bombadil.infradead.org with esmtps (Exim 4.85_2 #1 (Red Hat Linux)) id 1bQda7-0001D0-Jc for linux-arm-kernel@lists.infradead.org; Fri, 22 Jul 2016 16:45:48 +0000 Received: by mail-io0-x22a.google.com with SMTP id m101so110036497ioi.2 for ; Fri, 22 Jul 2016 09:45:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=1ox5knkw+gPlDIp/YAxv2N5DI/iTASRa6AduPdvCUtQ=; b=fDMylBmWPgGWPkVKvrKIHGq1wlChVE4TIPv7fFMbzXaqLUA1mT32vzJYb9cNBubMt0 v6rJ10s0wyspOdapgOrEocnAtxB248kW5BUih2ZX0x3fhvNklVWgohnau6kxeWfIrd/g QixtS3aawFhbaBl6aHzh/FbJbKV0/9d3HPn0o= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=1ox5knkw+gPlDIp/YAxv2N5DI/iTASRa6AduPdvCUtQ=; b=Ef2C4tJiqOkYphc4FYL98c9cuxDXPEa0bXuwkRUGuNva1o2KkipBvSujt2Sr+ThowG FMdNmdGV2t27t7ODSOHUmRDBGpNVVC3/GSYqYU3GyAtQbE8l66ifRAf8IzZMJbPeSCTr EI2cjuA5OOOA7wWQVV4EM5wO+oP6znM8Z6O5l6KgBph/EPzdrvFFe1feOz4QqIj1ryKU +OB2CwSI1C3DCBCBWIeYPttWj3TrmUekrT2NOqXmGLW/6R1ioH0eD1FKL91rCsx9J0j/ JdvLD8vVapSWLEXouSICt1+0K6yk9Eat48ZvoBU9k6Xa9YMCrztJWYTqFXfCxeY76euK /0+g== X-Gm-Message-State: AEkoouvKxboi902+CzzoahGkU57aKA4hFAqWtVxaR+Mo7uNOS0bjYypo2YtIncPSwA3uaQ2PvQXL7lk6h2erf0DG X-Received: by 10.107.135.22 with SMTP id j22mr6371457iod.56.1469205925338; Fri, 22 Jul 2016 09:45:25 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.214.6 with HTTP; Fri, 22 Jul 2016 09:45:24 -0700 (PDT) In-Reply-To: References: <1467204690-10790-1-git-send-email-ard.biesheuvel@linaro.org> <1467204690-10790-3-git-send-email-ard.biesheuvel@linaro.org> <64a731b2-1ddb-78f5-fd02-204f19f0a432@arm.com> From: Ard Biesheuvel Date: Fri, 22 Jul 2016 18:45:24 +0200 Message-ID: Subject: Re: [PATCH 2/5] arm64: efi: always map runtime services code and data regions down to pages To: Suzuki K Poulose X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160722_094547_879535_710F9755 X-CRM114-Status: GOOD ( 26.03 ) X-Spam-Score: -2.7 (--) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-2.7 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [2607:f8b0:4001:c06:0:0:0:22a listed in] [list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , "linux-efi@vger.kernel.org" , Matt Fleming , Catalin Marinas , Leif Lindholm , Sudeep Holla , "linux-arm-kernel@lists.infradead.org" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org On 22 July 2016 at 18:36, Suzuki K Poulose wrote: > On 22/07/16 17:27, Ard Biesheuvel wrote: >> >> On 22 July 2016 at 16:30, Sudeep Holla wrote: >>> >>> Hi Ard, >>> >>> On 29/06/16 13:51, Ard Biesheuvel wrote: >>>> >>>> >>>> To avoid triggering diagnostics in the MMU code that are finicky about >>>> splitting block mappings into more granular mappings, ensure that >>>> regions >>>> that are likely to appear in the Memory Attributes table as well as the >>>> UEFI memory map are always mapped down to pages. This way, we can use >>>> apply_to_page_range() instead of create_pgd_mapping() for the second >>>> pass, >>>> which cannot split or merge block entries, and operates strictly on >>>> PTEs. >>>> >>>> Note that this aligns the arm64 Memory Attributes table handling code >>>> with >>>> the ARM code, which already uses apply_to_page_range() to set the strict >>>> permissions. >>>> >>> >>> This patch is merged in arm64/for-next/core now and when I try that >>> branch with defconfig + CONFIG_PROVE_LOCKING, I get the following splat >>> on boot and it fails to boot further on Juno. >>> >>> I could bisect that to this patch(Commit bd264d046aad ("arm64: efi: >>> always map runtime services code and data regions down to pages") in >>> that branch) >>> >> >> Hi Sudeep, >> >> I can reproduce this on QEMU as well. It appears that >> apply_to_page_range() expects pages containing translation tables to >> have their per-page spinlock initialized if they are not part of >> init_mm. >> >> This >> >> --- a/arch/arm64/mm/mmu.c >> +++ b/arch/arm64/mm/mmu.c >> @@ -272,6 +272,7 @@ static phys_addr_t late_pgtable_alloc(void) >> { >> void *ptr = (void *)__get_free_page(PGALLOC_GFP); >> BUG_ON(!ptr); >> + BUG_ON(!pgtable_page_ctor(virt_to_page(ptr))); >> >> /* Ensure the zeroed page is visible to the page table walker */ >> dsb(ishst); >> >> makes the problem go away for me (just as a temporary hack) but I will >> try to come up with something more appropriate, and check if ARM has >> the same issue (since it uses apply_to_page_range() as well) >> > > Ard, > > I took a quick look at it. Looks like we don't initialise the page-table > pages allocated via late_pgtable_alloc. Since we allocate it for an mm != > init_mm, > the lock validator comes into picture and finds a lock which is not > initialised. > The following patch fixes the issue. But is not a perfect one. May need to > polish it > a little bit. > > ----8>---- > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > index a96a241..d312667 100644 > --- a/arch/arm64/mm/mmu.c > +++ b/arch/arm64/mm/mmu.c > @@ -270,12 +270,12 @@ static void __create_pgd_mapping(pgd_t *pgdir, > phys_addr_t phys, > static phys_addr_t late_pgtable_alloc(void) > { > - void *ptr = (void *)__get_free_page(PGALLOC_GFP); > - BUG_ON(!ptr); > + struct page *page = pte_alloc_one(NULL, 0); > + BUG_ON(!page); > /* Ensure the zeroed page is visible to the page table walker */ > dsb(ishst); > - return __pa(ptr); > + return __pa(page_address(page)); > } > Actually, I just sent a response to the same effect. Alternatively, we could educate apply_to_page_range() to treat the EFI page tables specially (or simply all statically allocated mm_struct instances), e.g., if (!pte) @@ -1873,7 +1876,7 @@ static int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd, arch_leave_lazy_mmu_mode(); - if (mm != &init_mm) + if (!is_kernel_mm) pte_unmap_unlock(pte-1, ptl); return err; } but it seems more appropriate to initialize the struct pages correctly, to preemptively deal with other code that may make similar assumptions. I also noticed that create_mapping_late() and create_mapping_noalloc() are essentially the same, since the only two invocations of the former should not split block entries, and simply remaps regions that have already been mapped with stricter permissions. This means late_pgtable_alloc is only used by create_pgd_mapping, which is only used by the EFI code. So that allows for some shortcuts to be taken, I would think. The only downside is that I will need to fix it in two places (arm64 and ARM) Anyway, thanks for the suggestion. -- Ard. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel diff --git a/mm/memory.c b/mm/memory.c index 15322b73636b..dc6145129170 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -1852,8 +1852,11 @@ static int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd, int err; pgtable_t token; spinlock_t *uninitialized_var(ptl); + bool is_kernel_mm; - pte = (mm == &init_mm) ? + is_kernel_mm = (mm == &init_mm || core_kernel_data((unsigned long)mm)); + + pte = is_kernel_mm ? pte_alloc_kernel(pmd, addr) : pte_alloc_map_lock(mm, pmd, addr, &ptl);