From patchwork Fri Mar 17 16:10:02 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: "Jon Medhurst (Tixy)" X-Patchwork-Id: 9631039 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id B26B360132 for ; Fri, 17 Mar 2017 16:10:42 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B465F286DA for ; Fri, 17 Mar 2017 16:10:42 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id A93E6286DE; Fri, 17 Mar 2017 16:10:42 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 0BA23286DA for ; Fri, 17 Mar 2017 16:10:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Mime-Version:References:In-Reply-To: Date:To:From:Subject:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=UCRQNTh7XLipcVl7DQ81Tg7UMgWV+UOHdudZ+ORnPf0=; b=pubm1h4c0K9r0z go995BiZR1Vc1AqJnHJS/QOKAqTcRd2/n9PDU+0aFyHoE8ku6kywewgMmjbj4gXaR+BI6GItUO4EI JPT6+aSJpG4dSBjSmcWC/xA1uatl4vy1u0ptTcc9KPKhYoWKJm/j+V1UwSYwHbonfiMm8zix9YaTp UjUuRunLagBuEKF73uMPT62O0lxRAaQuDqSuZPn8BPZpoygRt292cK/XGnO77LBoAxznrCyn++IDN SjrEzawfsiN7TtYnS5dzazEsGGz+bHxzivfsIhT4U2CdwQU3DqsCx0F2rwllxMpxp2+PwWg38PKej 1tbYlWeo4/sSmtKb/3xA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1couSX-0003fD-3L; Fri, 17 Mar 2017 16:10:33 +0000 Received: from mail-wr0-x230.google.com ([2a00:1450:400c:c0c::230]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1couSR-0002cY-NQ for linux-arm-kernel@lists.infradead.org; Fri, 17 Mar 2017 16:10:31 +0000 Received: by mail-wr0-x230.google.com with SMTP id u48so55034527wrc.0 for ; Fri, 17 Mar 2017 09:10:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=message-id:subject:from:to:cc:date:in-reply-to:references :mime-version:content-transfer-encoding; bh=4tBLtNS8L1NkJoIjBieiGLTslV+C3JKP+JE4qZvHGCY=; b=bjiPdeL9jYaoqnkA6Rz8bCKcaPEIjGl/Vm+jLNnpPq6C6nEcAzTp2KzoGVeNYPGSk2 p/PzQC5BxO9dypZI0SeSah74g7X6vatXjvy9aYHNCElEj+SbMEB9DRuPxndDcz6dXylC B7m2qCDJwKb15X2YfSM65RQK06ADUIFhYlDZE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:mime-version:content-transfer-encoding; bh=4tBLtNS8L1NkJoIjBieiGLTslV+C3JKP+JE4qZvHGCY=; b=TDeqrwt0kZsb7Bse0px5X2LWWSjlf46Bgi1lgkM/szBRFykFrnnNk9PGwpo/fPwJMT p7jAiyQWD+SzffNvhZdXrqtcwHO3D6J5NjND8/j0i7U3nJzxRch3qZ9X0c7/7v5F0r4B vfuSpJpEZvtwuP6QkqhFLyqhaOrtT722PMp9KQCHF/pgb/6Plzd5bkgmHMsqia1Rm0X/ NjSGNyRsahKK77TwcW23ibxy29dscRk4vTMXHZHspu3jGJIUog9u8wWuv8f2iIed1ytV C2VwrE/iZPCp12DIZzbZy7vrD4SprRa3haYkObDvlAkKHbrJFZgTpFxTV7rujjl7sDJj mqyw== X-Gm-Message-State: AFeK/H3MURvgzmIPxbCIYl3Dl7IsCfN8pv2Ku9SSbQVaG4Cqd6wms6ELtyp43Mi6a0Tfc6uE X-Received: by 10.223.138.250 with SMTP id z55mr14924617wrz.130.1489767004696; Fri, 17 Mar 2017 09:10:04 -0700 (PDT) Received: from linaro1 (82-69-122-217.dsl.in-addr.zen.co.uk. [82.69.122.217]) by smtp.gmail.com with ESMTPSA id s18sm3145806wmb.18.2017.03.17.09.10.03 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 17 Mar 2017 09:10:03 -0700 (PDT) Message-ID: <1489767002.3063.10.camel@linaro.org> Subject: Re: [PATCH 1/2] arm: Fix cache inconsistency when using fixmap From: "Jon Medhurst (Tixy)" To: Russell King - ARM Linux Date: Fri, 17 Mar 2017 16:10:02 +0000 In-Reply-To: <20170317115321.GB21222@n2100.armlinux.org.uk> References: <20170316132958.22227-1-tixy@linaro.org> <20170317115321.GB21222@n2100.armlinux.org.uk> X-Mailer: Evolution 3.22.5-1 Mime-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170317_091028_251257_35B47450 X-CRM114-Status: GOOD ( 25.39 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Stefan Agner , linux-arm-kernel@lists.infradead.org, Ard Biesheuvel Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On Fri, 2017-03-17 at 11:53 +0000, Russell King - ARM Linux wrote: > On Thu, Mar 16, 2017 at 01:29:57PM +0000, Jon Medhurst wrote: > > To cope with the variety in ARM architectures and configurations, the > > pagetable attributes for kernel memory are generated at runtime to match > > the system the kernel finds itself on. This calculated value is stored > > in pgprot_kernel. > > > > However, when early fixmap support was added for arm (commit > > a5f4c561b3b1) the attributes used for mappings were hard coded because > > pgprot_kernel is not set up early enough. Unfortunately, the values used > > didn't include the 'shareable' attribute which means that for later > > non-early fixmap use, when multiple CPUs are running, any cache entries > > allocated for fixmap memory aren't kept consistent between CPUs. This > > can result in different CPUs seeing different memory contents. > > This also likely causes unpredictable behaviour (aliased attributes). > > > This issue was discovered on a dual cluster system by failures with > > kprobes, which uses fixmap to modify the kernel image if > > CONFIG_DEBUG_RODATA is enabled. It will also affect kgdb and jump_labels > > which also make use of the same code to modify the kernel, and any other > > uses of fixmap after secondary CPUs are brought online. > > > > To fix this issue, and to help avoid other potential problems where > > pagetable attributes are incorrect, we change the fixmap code to use the > > same generated value in pgprot_kernel that the rest of the kernel uses, > > and only fall back to a hard coded value if this isn't set - which will > > be early on in boot before other CPUs are brought online. > > I'm not happy with this - if we need to create early fixmaps, then > we need to know the correct attributes to use, so let's move the > attribute initialisation earlier. This solution feels too much like > hacking around the problem. I must admit to having similar thoughts and plead guilty to ignoring them. Not knowing how early fixmaps are being used I let myself think 'it must have been originally done this way for a reason'. It looks to me that build_mem_type_table doesn't have much in the way of dependencies so can probably be just called earlier. So, is the correct solution to a) call build_mem_type_table from setup_arch before early_fixmap_init b) move call to build_mem_type_table into early_fixmap_init c) call build_mem_type_table from early_fixmap_init as well as paging_init and have a test in build_mem_type_table so it only exectutes once d) something else a) seems simplest, b) seems wrong, c) seems over the top Anyway, below is an alternative to $subject patch that works for my kprobes test cases. Note, the removal of FIXMAP_PAGE_{NORMAL,RO} means the generic fixmap code will define these from PAGE_KERNEL{,_RO}. Not knowing how early fixmap is used, I hope Stefan and/or Ard have testcases they could run.  I'm also wondering if the existing definition of FIXMAP_PAGE_IO is correc t and should not also be based on some platform specific value calculated  in build_mem_type_table?   unsigned int cr = get_cr(); @@ -1616,7 +1616,6 @@ void __init paging_init(const struct machine_desc *mdesc)  {   void *zero_page;   - build_mem_type_table();   prepare_page_table();   map_lowmem();   memblock_set_current_limit(arm_lowmem_limit); --  2.11.0 diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h index 5c17d2dec777..30871fb269f0 100644 --- a/arch/arm/include/asm/fixmap.h +++ b/arch/arm/include/asm/fixmap.h @@ -41,9 +41,6 @@ static const enum fixed_addresses __end_of_fixed_addresses =    #define FIXMAP_PAGE_COMMON (L_PTE_YOUNG | L_PTE_PRESENT | L_PTE_XN | L_PTE_DIRTY)   -#define FIXMAP_PAGE_NORMAL (FIXMAP_PAGE_COMMON | L_PTE_MT_WRITEBACK) -#define FIXMAP_PAGE_RO (FIXMAP_PAGE_NORMAL | L_PTE_RDONLY) -  /* Used by set_fixmap_(io|nocache), both meant for mapping a device */  #define FIXMAP_PAGE_IO (FIXMAP_PAGE_COMMON | L_PTE_MT_DEV_SHARED | L_PTE_SHARED)  #define FIXMAP_PAGE_NOCACHE FIXMAP_PAGE_IO diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h index 1c462381c225..6a98856a8fa9 100644 --- a/arch/arm/include/asm/pgtable.h +++ b/arch/arm/include/asm/pgtable.h @@ -98,6 +98,7 @@ extern pgprot_t pgprot_s2_device;  #define PAGE_READONLY_EXEC _MOD_PROT(pgprot_user, L_PTE_USER | L_PTE_RDONLY)  #define PAGE_KERNEL _MOD_PROT(pgprot_kernel, L_PTE_XN)  #define PAGE_KERNEL_EXEC pgprot_kernel +#define PAGE_KERNEL_RO _MOD_PROT(pgprot_kernel, L_PTE_XN | L_PTE_RDONLY)  #define PAGE_HYP _MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_XN)  #define PAGE_HYP_EXEC _MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_RDONLY)  #define PAGE_HYP_RO _MOD_PROT(pgprot_kernel, L_PTE_HYP | L_PTE_RDONLY | L_PTE_XN) diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c index f4e54503afa9..fc4782fa5071 100644 --- a/arch/arm/kernel/setup.c +++ b/arch/arm/kernel/setup.c @@ -79,6 +79,7 @@ __setup("fpe=", fpe_setup);  #endif    extern void init_default_cache_policy(unsigned long); +extern void build_mem_type_table(void);  extern void paging_init(const struct machine_desc *desc);  extern void early_paging_init(const struct machine_desc *);  extern void adjust_lowmem_bounds(void); @@ -1082,6 +1083,8 @@ void __init setup_arch(char **cmdline_p)   strlcpy(cmd_line, boot_command_line, COMMAND_LINE_SIZE);   *cmdline_p = cmd_line;   + build_mem_type_table(); +   early_fixmap_init();   early_ioremap_init();   diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c index 4e016d7f37b3..c8e1de3ceb02 100644 --- a/arch/arm/mm/mmu.c +++ b/arch/arm/mm/mmu.c @@ -425,7 +425,7 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)  /*   * Adjust the PMD section entries according to the CPU in use.   */ -static void __init build_mem_type_table(void) +void __init build_mem_type_table(void)  {   struct cachepolicy *cp;