From patchwork Mon Nov 23 16:52:15 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Catalin Marinas X-Patchwork-Id: 57168 Delivered-To: patch@linaro.org Received: by 10.112.155.196 with SMTP id vy4csp1549295lbb; Mon, 23 Nov 2015 08:54:15 -0800 (PST) X-Received: by 10.98.1.140 with SMTP id 134mr16531781pfb.134.1448297655583; Mon, 23 Nov 2015 08:54:15 -0800 (PST) Return-Path: Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id u66si20684746pfa.243.2015.11.23.08.54.15 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 23 Nov 2015 08:54:15 -0800 (PST) Received-SPF: pass (google.com: 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; spf=pass (google.com: 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 Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1a0uM7-0005XE-FE; Mon, 23 Nov 2015 16:52:43 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1a0uM4-0005Rd-57 for linux-arm-kernel@lists.infradead.org; Mon, 23 Nov 2015 16:52:40 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 214EF49; Mon, 23 Nov 2015 08:52:02 -0800 (PST) Received: from e104818-lin.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B31613F51B; Mon, 23 Nov 2015 08:52:17 -0800 (PST) Date: Mon, 23 Nov 2015 16:52:15 +0000 From: Catalin Marinas To: Jeremy Linton Subject: Re: [PATCH] [PATCH] arm64: Boot failure on m400 with new cont PTEs Message-ID: <20151123165214.GD32300@e104818-lin.cambridge.arm.com> References: <1447858999-26665-1-git-send-email-jeremy.linton@arm.com> <20151118152044.GD10644@leverpostej> <564CA29A.9050905@arm.com> <20151118162932.GA13355@leverpostej> <20151123155132.GC32300@e104818-lin.cambridge.arm.com> <5653387A.2000101@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <5653387A.2000101@redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20151123_085240_215045_70A95FD6 X-CRM114-Status: GOOD ( 25.06 ) X-Spam-Score: -7.5 (-------) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-7.5 points) pts rule name description ---- ---------------------- -------------------------------------------------- -5.0 RCVD_IN_DNSWL_HI RBL: Sender listed at http://www.dnswl.org/, high trust [217.140.101.70 listed in list.dnswl.org] -0.6 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] 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 , lauraa@codeaurora.org, suzuki.poulose@arm.com, ard.biesheuvel@linaro.org, will.deacon@arm.com, Jeremy Linton , linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org On Mon, Nov 23, 2015 at 10:02:02AM -0600, Jeremy Linton wrote: > On 11/23/2015 09:51 AM, Catalin Marinas wrote: > >Call trace: > >[] __create_mapping.isra.5+0x360/0x530 > >[] fixup_init+0x64/0x80 > >[] free_initmem+0xc/0x38 > >[] kernel_init+0x20/0xe0 > >[] ret_from_fork+0x10/0x40 > > > >What I don't get is why we have fixup_init() even when > >!CONFIG_DEBUG_RODATA. It probably breaks the initial mapping just to get > >a non-executable init section. However, the other sections are left > >executable when this config option is disabled. The patch below fixes > >the warnings above: > > Well the kernel permissions are a bit of a mess, and not at all "secure" in > their current state. But I guess the thought must have been that turning off > execute on the init sections was a good way to find functions incorrectly > marked _init(). Which is different from RO. fixup_executable() is non-empty only with DEBUG_RODATA, even though it is done early. I was assuming we should have done the same with fixup_init() but I've seen Laura's reply that it was deliberate. > Frankly, I expect someone will push to cleanup the kernel permissions > at some point (I've got it on my "spare time todo, list"), but this > will of course make the create_mapping_late a lot more popular as I > see it being called during module load/unload. > Anyway, I've been saying the problem is create_mapping_late() > all along, as you notice there isn't any TLB flushes in fixup_init() > and that is the core of the problem, not all this other discussion. The problem here is not changing permissions, we do this all the time with fork() and a single TLB at the end. While the __create_mapping() path seems to have TLB invalidation when changing a non-empty entry on higher levels (though without break-before-make), it assumes that the PTE was always none and no further TLB done. Your patch in this thread indeed fixes this part (though without a check for pte_none(old_pte) but that's an optimisation). However, the problem you are seeing is not some permission change being ignored (like a stale entry) but a TLB conflict which means that the same VA has two entries in the TLB. We need better clarification in the ARM ARM (a ticket about to be raised) but what makes this very likely is going from a small block mapping to a bigger one. In this case, a non-contiguous PTE overlapping with a contiguous one loaded via a different address in the same contiguous range. Adding the TLB flush in __populate_init_pte() hides this by reducing the window. We have other cases where we go for smaller to larger block like the 1GB section. I think until MarkR finishes his code to go via a temporary TTBR1 + idmap, we should prevent all those. We can hope that going the other direction (from bigger to smaller block mapping) is fine but we don't have a clear answer yet. Your original patch with a better description of the use cases and potentially a check for pte_none() is still useful, though not a fix for the TLB conflict. Something like: ----------------8<-------------------- ----------------8<-------------------- -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index abb66f84d4ac..d110313e58e9 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -113,14 +113,20 @@ static void __populate_init_pte(pte_t *pte, unsigned long addr, pgprot_t prot) { unsigned long pfn = __phys_to_pfn(phys); + bool need_flush = false; do { + if (!pte_none(*pte)) + need_flush = true; /* clear all the bits except the pfn, then apply the prot */ set_pte(pte, pfn_pte(pfn, prot)); pte++; pfn++; addr += PAGE_SIZE; } while (addr != end); + + if (need_flush) + flush_tlb_all(); } static void alloc_init_pte(pmd_t *pmd, unsigned long addr,