[PATCH 0/3] I-side fixes

[PATCH 0/3] I-side fixes

[Will Deacon]

Hi all,

These three patches attempt to address some issues with ISBs and I-cache
maintenance. Specifically:

  - Ensure we don't invoke dynamically modified I-cache maintenance code
    as part of the instruction patching routines
  - Remove ISBs from pte/pmd/pud setter functions
  - IPI other CPUs when invalidating the I-cache for kernel mappings

Whilst the IPI is a bit nasty, I couldn't figure out a better way to
ensure that CPUs don't retain instructions from freed pages in their
pipelines. I initially hoped to deal with this by hooking into RCU, but
that would rely on RCU being the only mechanism used to defer reclaim of
executable pages and this doesn't appear to be the case for e.g. modules.

Feedback welcome,

Will

--->8

Will Deacon (3):
  arm64: Avoid flush_icache_range() in alternatives patching code
  arm64: Remove unnecessary ISBs from set_{pte,pmd,pud}
  arm64: IPI each CPU after invalidating the I-cache for kernel mappings

 arch/arm64/include/asm/alternative.h |  7 +++++-
 arch/arm64/include/asm/cacheflush.h  | 13 +++++++++-
 arch/arm64/include/asm/pgtable.h     |  6 +----
 arch/arm64/kernel/alternative.c      | 47 ++++++++++++++++++++++++++++++------
 arch/arm64/kernel/cpu_errata.c       |  2 +-
 arch/arm64/kernel/insn.c             | 15 ++----------
 arch/arm64/kernel/module.c           |  5 ++--
 arch/arm64/mm/cache.S                |  4 +--
 8 files changed, 65 insertions(+), 34 deletions(-)

-- 
2.1.4

[PATCH 1/3] arm64: Avoid flush_icache_range() in alternatives patching code

[Will Deacon]

The implementation of flush_icache_range() includes instruction sequences
which are themselves patched at runtime, so it is not safe to call from
the patching framework.

This patch reworks the alternatives cache-flushing code so that it rolls
its own internal D-cache maintenance using DC CIVAC before invalidating
the entire I-cache after all alternatives have been applied at boot.
Modules don't cause any issues, since flush_icache_range() is safe to
call by the time they are loaded.

Reported-by: Rohit Khanna <rokhanna@nvidia.com>
Cc: Alexander Van Brunt <avanbrunt@nvidia.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/include/asm/alternative.h |  7 +++++-
 arch/arm64/kernel/alternative.c      | 46 ++++++++++++++++++++++++++++++------
 arch/arm64/kernel/module.c           |  5 ++--
 3 files changed, 47 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
index a91933b1e2e6..4b650ec1d7dd 100644
--- a/arch/arm64/include/asm/alternative.h
+++ b/arch/arm64/include/asm/alternative.h
@@ -28,7 +28,12 @@ typedef void (*alternative_cb_t)(struct alt_instr *alt,
 				 __le32 *origptr, __le32 *updptr, int nr_inst);
 
 void __init apply_alternatives_all(void);
-void apply_alternatives(void *start, size_t length);
+
+#ifdef CONFIG_MODULES
+void apply_alternatives_module(void *start, size_t length);
+#else
+static inline void apply_alternatives_module(void *start, size_t length) { }
+#endif
 
 #define ALTINSTR_ENTRY(feature,cb)					      \
 	" .word 661b - .\n"				/* label           */ \
diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index 5c4bce4ac381..4f3dcc15a5b2 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -122,7 +122,25 @@ static void patch_alternative(struct alt_instr *alt,
 	}
 }
 
-static void __apply_alternatives(void *alt_region, bool use_linear_alias)
+static void clean_dcache_range_nopatch(u64 start, u64 end)
+{
+	u64 cur, d_size, ctr_el0;
+
+	ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
+	d_size = 4 << cpuid_feature_extract_unsigned_field(ctr_el0,
+							   CTR_DMINLINE_SHIFT);
+	cur = start & ~(d_size - 1);
+	do {
+		/*
+		 * We must clean+invalidate to the PoC in order to avoid
+		 * Cortex-A53 errata 826319, 827319, 824069 and 819472
+		 * (this corresponds to ARM64_WORKAROUND_CLEAN_CACHE)
+		 */
+		asm volatile("dc civac, %0" : : "r" (cur) : "memory");
+	} while (cur += d_size, cur < end);
+}
+
+static void __apply_alternatives(void *alt_region, bool is_module)
 {
 	struct alt_instr *alt;
 	struct alt_region *region = alt_region;
@@ -145,7 +163,7 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias)
 		pr_info_once("patching kernel code\n");
 
 		origptr = ALT_ORIG_PTR(alt);
-		updptr = use_linear_alias ? lm_alias(origptr) : origptr;
+		updptr = is_module ? origptr : lm_alias(origptr);
 		nr_inst = alt->orig_len / AARCH64_INSN_SIZE;
 
 		if (alt->cpufeature < ARM64_CB_PATCH)
@@ -155,8 +173,20 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias)
 
 		alt_cb(alt, origptr, updptr, nr_inst);
 
-		flush_icache_range((uintptr_t)origptr,
-				   (uintptr_t)(origptr + nr_inst));
+		if (!is_module) {
+			clean_dcache_range_nopatch((u64)origptr,
+						   (u64)(origptr + nr_inst));
+		}
+	}
+
+	/*
+	 * The core module code takes care of cache maintenance in
+	 * flush_module_icache().
+	 */
+	if (!is_module) {
+		dsb(ish);
+		__flush_icache_all();
+		isb();
 	}
 }
 
@@ -178,7 +208,7 @@ static int __apply_alternatives_multi_stop(void *unused)
 		isb();
 	} else {
 		BUG_ON(alternatives_applied);
-		__apply_alternatives(&region, true);
+		__apply_alternatives(&region, false);
 		/* Barriers provided by the cache flushing */
 		WRITE_ONCE(alternatives_applied, 1);
 	}
@@ -192,12 +222,14 @@ void __init apply_alternatives_all(void)
 	stop_machine(__apply_alternatives_multi_stop, NULL, cpu_online_mask);
 }
 
-void apply_alternatives(void *start, size_t length)
+#ifdef CONFIG_MODULES
+void apply_alternatives_module(void *start, size_t length)
 {
 	struct alt_region region = {
 		.begin	= start,
 		.end	= start + length,
 	};
 
-	__apply_alternatives(&region, false);
+	__apply_alternatives(&region, true);
 }
+#endif
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 155fd91e78f4..f0f27aeefb73 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -448,9 +448,8 @@ int module_finalize(const Elf_Ehdr *hdr,
 	const char *secstrs = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
 
 	for (s = sechdrs, se = sechdrs + hdr->e_shnum; s < se; s++) {
-		if (strcmp(".altinstructions", secstrs + s->sh_name) == 0) {
-			apply_alternatives((void *)s->sh_addr, s->sh_size);
-		}
+		if (strcmp(".altinstructions", secstrs + s->sh_name) == 0)
+			apply_alternatives_module((void *)s->sh_addr, s->sh_size);
 #ifdef CONFIG_ARM64_MODULE_PLTS
 		if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) &&
 		    !strcmp(".text.ftrace_trampoline", secstrs + s->sh_name))
-- 
2.1.4

[PATCH 2/3] arm64: Remove unnecessary ISBs from set_{pte,pmd,pud}

[Will Deacon]

Commit 7f0b1bf04511 ("arm64: Fix barriers used for page table modifications")
fixed a reported issue with fixmap page-table entries not being visible
to the walker due to a missing DSB instruction. At the same time, it added
ISB instructions to the arm64 set_{pte,pmd,pud} functions, which are not
required by the architecture and make little sense in isolation.

Remove the redundant ISBs.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Steve Capper <steve.capper@linaro.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/include/asm/pgtable.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 9f82d6b53851..1bdeca8918a6 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -224,10 +224,8 @@ static inline void set_pte(pte_t *ptep, pte_t pte)
 	 * Only if the new pte is valid and kernel, otherwise TLB maintenance
 	 * or update_mmu_cache() have the necessary barriers.
 	 */
-	if (pte_valid_not_user(pte)) {
+	if (pte_valid_not_user(pte))
 		dsb(ishst);
-		isb();
-	}
 }
 
 extern void __sync_icache_dcache(pte_t pteval);
@@ -434,7 +432,6 @@ static inline void set_pmd(pmd_t *pmdp, pmd_t pmd)
 {
 	WRITE_ONCE(*pmdp, pmd);
 	dsb(ishst);
-	isb();
 }
 
 static inline void pmd_clear(pmd_t *pmdp)
@@ -485,7 +482,6 @@ static inline void set_pud(pud_t *pudp, pud_t pud)
 {
 	WRITE_ONCE(*pudp, pud);
 	dsb(ishst);
-	isb();
 }
 
 static inline void pud_clear(pud_t *pudp)
-- 
2.1.4

[PATCH 3/3] arm64: IPI each CPU after invalidating the I-cache for kernel mappings

[Will Deacon]

When invalidating the instruction cache for a kernel mapping via
flush_icache_range(), it is also necessary to flush the pipeline for
other CPUs so that instructions fetched into the pipeline before the
I-cache invalidation are discarded. For example, if module 'foo' is
unloaded and then module 'bar' is loaded into the same area of memory,
a CPU could end up executing instructions from 'foo' when branching into
'bar' if these instructions were fetched into the pipeline before 'foo'
was unloaded.

Whilst this is highly unlikely to occur in practice, particularly as
any exception acts as a context-synchronizing operation, following the
letter of the architecture requires us to execute an ISB on each CPU
in order for the new instruction stream to be visible.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/include/asm/cacheflush.h | 13 ++++++++++++-
 arch/arm64/kernel/alternative.c     |  1 -
 arch/arm64/kernel/cpu_errata.c      |  2 +-
 arch/arm64/kernel/insn.c            | 15 ++-------------
 arch/arm64/mm/cache.S               |  4 ++--
 5 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/include/asm/cacheflush.h b/arch/arm64/include/asm/cacheflush.h
index d264a7274811..a0ec27066e6f 100644
--- a/arch/arm64/include/asm/cacheflush.h
+++ b/arch/arm64/include/asm/cacheflush.h
@@ -71,7 +71,7 @@
  *		- kaddr  - page address
  *		- size   - region size
  */
-extern void flush_icache_range(unsigned long start, unsigned long end);
+extern void __flush_icache_range(unsigned long start, unsigned long end);
 extern int  invalidate_icache_range(unsigned long start, unsigned long end);
 extern void __flush_dcache_area(void *addr, size_t len);
 extern void __inval_dcache_area(void *addr, size_t len);
@@ -81,6 +81,17 @@ extern void __clean_dcache_area_pou(void *addr, size_t len);
 extern long __flush_cache_user_range(unsigned long start, unsigned long end);
 extern void sync_icache_aliases(void *kaddr, unsigned long len);
 
+static inline void flush_icache_range(unsigned long start, unsigned long end)
+{
+	__flush_icache_range(start, end);
+
+	/*
+	 * IPI all online CPUs so that they undergo a context synchronization
+	 * event and are forced to refetch the new instructions.
+	 */
+	kick_all_cpus_sync();
+}
+
 static inline void flush_cache_mm(struct mm_struct *mm)
 {
 }
diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index 4f3dcc15a5b2..0ac06560c166 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -205,7 +205,6 @@ static int __apply_alternatives_multi_stop(void *unused)
 	if (smp_processor_id()) {
 		while (!READ_ONCE(alternatives_applied))
 			cpu_relax();
-		isb();
 	} else {
 		BUG_ON(alternatives_applied);
 		__apply_alternatives(&region, false);
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 1d2b6d768efe..0a338a1cd2d7 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -101,7 +101,7 @@ static void __copy_hyp_vect_bpi(int slot, const char *hyp_vecs_start,
 	for (i = 0; i < SZ_2K; i += 0x80)
 		memcpy(dst + i, hyp_vecs_start, hyp_vecs_end - hyp_vecs_start);
 
-	flush_icache_range((uintptr_t)dst, (uintptr_t)dst + SZ_2K);
+	__flush_icache_range((uintptr_t)dst, (uintptr_t)dst + SZ_2K);
 }
 
 static void __install_bp_hardening_cb(bp_hardening_cb_t fn,
diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index 816d03c4c913..4cc41864f277 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -249,7 +249,6 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg)
 	} else {
 		while (atomic_read(&pp->cpu_count) <= num_online_cpus())
 			cpu_relax();
-		isb();
 	}
 
 	return ret;
@@ -283,18 +282,8 @@ int __kprobes aarch64_insn_patch_text(void *addrs[], u32 insns[], int cnt)
 		if (ret)
 			return ret;
 
-		if (aarch64_insn_hotpatch_safe(insn, insns[0])) {
-			/*
-			 * ARMv8 architecture doesn't guarantee all CPUs see
-			 * the new instruction after returning from function
-			 * aarch64_insn_patch_text_nosync(). So send IPIs to
-			 * all other CPUs to achieve instruction
-			 * synchronization.
-			 */
-			ret = aarch64_insn_patch_text_nosync(addrs[0], insns[0]);
-			kick_all_cpus_sync();
-			return ret;
-		}
+		if (aarch64_insn_hotpatch_safe(insn, insns[0]))
+			return aarch64_insn_patch_text_nosync(addrs[0], insns[0]);
 	}
 
 	return aarch64_insn_patch_text_sync(addrs, insns, cnt);
diff --git a/arch/arm64/mm/cache.S b/arch/arm64/mm/cache.S
index 30334d81b021..0c22ede52f90 100644
--- a/arch/arm64/mm/cache.S
+++ b/arch/arm64/mm/cache.S
@@ -35,7 +35,7 @@
  *	- start   - virtual start address of region
  *	- end     - virtual end address of region
  */
-ENTRY(flush_icache_range)
+ENTRY(__flush_icache_range)
 	/* FALLTHROUGH */
 
 /*
@@ -77,7 +77,7 @@ alternative_else_nop_endif
 9:
 	mov	x0, #-EFAULT
 	b	1b
-ENDPROC(flush_icache_range)
+ENDPROC(__flush_icache_range)
 ENDPROC(__flush_cache_user_range)
 
 /*
-- 
2.1.4

[PATCH 1/3] arm64: Avoid flush_icache_range() in alternatives patching code

[Mark Rutland]

On Tue, Jun 19, 2018 at 01:48:13PM +0100, Will Deacon wrote:
> The implementation of flush_icache_range() includes instruction sequences
> which are themselves patched at runtime, so it is not safe to call from
> the patching framework.
> 
> This patch reworks the alternatives cache-flushing code so that it rolls
> its own internal D-cache maintenance using DC CIVAC before invalidating
> the entire I-cache after all alternatives have been applied at boot.
> Modules don't cause any issues, since flush_icache_range() is safe to
> call by the time they are loaded.
> 
> Reported-by: Rohit Khanna <rokhanna@nvidia.com>
> Cc: Alexander Van Brunt <avanbrunt@nvidia.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/include/asm/alternative.h |  7 +++++-
>  arch/arm64/kernel/alternative.c      | 46 ++++++++++++++++++++++++++++++------
>  arch/arm64/kernel/module.c           |  5 ++--
>  3 files changed, 47 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
> index a91933b1e2e6..4b650ec1d7dd 100644
> --- a/arch/arm64/include/asm/alternative.h
> +++ b/arch/arm64/include/asm/alternative.h
> @@ -28,7 +28,12 @@ typedef void (*alternative_cb_t)(struct alt_instr *alt,
>  				 __le32 *origptr, __le32 *updptr, int nr_inst);
>  
>  void __init apply_alternatives_all(void);
> -void apply_alternatives(void *start, size_t length);
> +
> +#ifdef CONFIG_MODULES
> +void apply_alternatives_module(void *start, size_t length);
> +#else
> +static inline void apply_alternatives_module(void *start, size_t length) { }
> +#endif
>  
>  #define ALTINSTR_ENTRY(feature,cb)					      \
>  	" .word 661b - .\n"				/* label           */ \
> diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
> index 5c4bce4ac381..4f3dcc15a5b2 100644
> --- a/arch/arm64/kernel/alternative.c
> +++ b/arch/arm64/kernel/alternative.c
> @@ -122,7 +122,25 @@ static void patch_alternative(struct alt_instr *alt,
>  	}
>  }
>  

It might be worth a comment here, e.g.

/*
 * Since the regular dcache maintenance functions are patched with
 * alteratives, we use an unpatched copy to apply the alternatives
 * safely.
 */

... so as to avoid any helpful cleanup patches in future moving back to
the "optimized" the asm functions.

Regardless:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> -static void __apply_alternatives(void *alt_region, bool use_linear_alias)
> +static void clean_dcache_range_nopatch(u64 start, u64 end)
> +{
> +	u64 cur, d_size, ctr_el0;
> +
> +	ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
> +	d_size = 4 << cpuid_feature_extract_unsigned_field(ctr_el0,
> +							   CTR_DMINLINE_SHIFT);
> +	cur = start & ~(d_size - 1);
> +	do {
> +		/*
> +		 * We must clean+invalidate to the PoC in order to avoid
> +		 * Cortex-A53 errata 826319, 827319, 824069 and 819472
> +		 * (this corresponds to ARM64_WORKAROUND_CLEAN_CACHE)
> +		 */
> +		asm volatile("dc civac, %0" : : "r" (cur) : "memory");
> +	} while (cur += d_size, cur < end);
> +}
> +
> +static void __apply_alternatives(void *alt_region, bool is_module)
>  {
>  	struct alt_instr *alt;
>  	struct alt_region *region = alt_region;
> @@ -145,7 +163,7 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias)
>  		pr_info_once("patching kernel code\n");
>  
>  		origptr = ALT_ORIG_PTR(alt);
> -		updptr = use_linear_alias ? lm_alias(origptr) : origptr;
> +		updptr = is_module ? origptr : lm_alias(origptr);
>  		nr_inst = alt->orig_len / AARCH64_INSN_SIZE;
>  
>  		if (alt->cpufeature < ARM64_CB_PATCH)
> @@ -155,8 +173,20 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias)
>  
>  		alt_cb(alt, origptr, updptr, nr_inst);
>  
> -		flush_icache_range((uintptr_t)origptr,
> -				   (uintptr_t)(origptr + nr_inst));
> +		if (!is_module) {
> +			clean_dcache_range_nopatch((u64)origptr,
> +						   (u64)(origptr + nr_inst));
> +		}
> +	}
> +
> +	/*
> +	 * The core module code takes care of cache maintenance in
> +	 * flush_module_icache().
> +	 */
> +	if (!is_module) {
> +		dsb(ish);
> +		__flush_icache_all();
> +		isb();
>  	}
>  }
>  
> @@ -178,7 +208,7 @@ static int __apply_alternatives_multi_stop(void *unused)
>  		isb();
>  	} else {
>  		BUG_ON(alternatives_applied);
> -		__apply_alternatives(&region, true);
> +		__apply_alternatives(&region, false);
>  		/* Barriers provided by the cache flushing */
>  		WRITE_ONCE(alternatives_applied, 1);
>  	}
> @@ -192,12 +222,14 @@ void __init apply_alternatives_all(void)
>  	stop_machine(__apply_alternatives_multi_stop, NULL, cpu_online_mask);
>  }
>  
> -void apply_alternatives(void *start, size_t length)
> +#ifdef CONFIG_MODULES
> +void apply_alternatives_module(void *start, size_t length)
>  {
>  	struct alt_region region = {
>  		.begin	= start,
>  		.end	= start + length,
>  	};
>  
> -	__apply_alternatives(&region, false);
> +	__apply_alternatives(&region, true);
>  }
> +#endif
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index 155fd91e78f4..f0f27aeefb73 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -448,9 +448,8 @@ int module_finalize(const Elf_Ehdr *hdr,
>  	const char *secstrs = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
>  
>  	for (s = sechdrs, se = sechdrs + hdr->e_shnum; s < se; s++) {
> -		if (strcmp(".altinstructions", secstrs + s->sh_name) == 0) {
> -			apply_alternatives((void *)s->sh_addr, s->sh_size);
> -		}
> +		if (strcmp(".altinstructions", secstrs + s->sh_name) == 0)
> +			apply_alternatives_module((void *)s->sh_addr, s->sh_size);
>  #ifdef CONFIG_ARM64_MODULE_PLTS
>  		if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) &&
>  		    !strcmp(".text.ftrace_trampoline", secstrs + s->sh_name))
> -- 
> 2.1.4
> 

[PATCH 2/3] arm64: Remove unnecessary ISBs from set_{pte, pmd, pud}

[Mark Rutland]

On Tue, Jun 19, 2018 at 01:48:14PM +0100, Will Deacon wrote:
> Commit 7f0b1bf04511 ("arm64: Fix barriers used for page table modifications")
> fixed a reported issue with fixmap page-table entries not being visible
> to the walker due to a missing DSB instruction. At the same time, it added
> ISB instructions to the arm64 set_{pte,pmd,pud} functions, which are not
> required by the architecture and make little sense in isolation.
> 
> Remove the redundant ISBs.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Steve Capper <steve.capper@linaro.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/include/asm/pgtable.h | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 9f82d6b53851..1bdeca8918a6 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -224,10 +224,8 @@ static inline void set_pte(pte_t *ptep, pte_t pte)
>  	 * Only if the new pte is valid and kernel, otherwise TLB maintenance
>  	 * or update_mmu_cache() have the necessary barriers.
>  	 */
> -	if (pte_valid_not_user(pte)) {
> +	if (pte_valid_not_user(pte))
>  		dsb(ishst);
> -		isb();
> -	}
>  }
>  
>  extern void __sync_icache_dcache(pte_t pteval);
> @@ -434,7 +432,6 @@ static inline void set_pmd(pmd_t *pmdp, pmd_t pmd)
>  {
>  	WRITE_ONCE(*pmdp, pmd);
>  	dsb(ishst);
> -	isb();
>  }
>  
>  static inline void pmd_clear(pmd_t *pmdp)
> @@ -485,7 +482,6 @@ static inline void set_pud(pud_t *pudp, pud_t pud)
>  {
>  	WRITE_ONCE(*pudp, pud);
>  	dsb(ishst);
> -	isb();
>  }
>  
>  static inline void pud_clear(pud_t *pudp)
> -- 
> 2.1.4
> 

[PATCH 3/3] arm64: IPI each CPU after invalidating the I-cache for kernel mappings

[Mark Rutland]

On Tue, Jun 19, 2018 at 01:48:15PM +0100, Will Deacon wrote:
> When invalidating the instruction cache for a kernel mapping via
> flush_icache_range(), it is also necessary to flush the pipeline for
> other CPUs so that instructions fetched into the pipeline before the
> I-cache invalidation are discarded. For example, if module 'foo' is
> unloaded and then module 'bar' is loaded into the same area of memory,
> a CPU could end up executing instructions from 'foo' when branching into
> 'bar' if these instructions were fetched into the pipeline before 'foo'
> was unloaded.
> 
> Whilst this is highly unlikely to occur in practice, particularly as
> any exception acts as a context-synchronizing operation, following the
> letter of the architecture requires us to execute an ISB on each CPU
> in order for the new instruction stream to be visible.
> 
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/include/asm/cacheflush.h | 13 ++++++++++++-
>  arch/arm64/kernel/alternative.c     |  1 -
>  arch/arm64/kernel/cpu_errata.c      |  2 +-
>  arch/arm64/kernel/insn.c            | 15 ++-------------
>  arch/arm64/mm/cache.S               |  4 ++--
>  5 files changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cacheflush.h b/arch/arm64/include/asm/cacheflush.h
> index d264a7274811..a0ec27066e6f 100644
> --- a/arch/arm64/include/asm/cacheflush.h
> +++ b/arch/arm64/include/asm/cacheflush.h
> @@ -71,7 +71,7 @@
>   *		- kaddr  - page address
>   *		- size   - region size
>   */
> -extern void flush_icache_range(unsigned long start, unsigned long end);
> +extern void __flush_icache_range(unsigned long start, unsigned long end);
>  extern int  invalidate_icache_range(unsigned long start, unsigned long end);
>  extern void __flush_dcache_area(void *addr, size_t len);
>  extern void __inval_dcache_area(void *addr, size_t len);
> @@ -81,6 +81,17 @@ extern void __clean_dcache_area_pou(void *addr, size_t len);
>  extern long __flush_cache_user_range(unsigned long start, unsigned long end);
>  extern void sync_icache_aliases(void *kaddr, unsigned long len);
>  
> +static inline void flush_icache_range(unsigned long start, unsigned long end)
> +{
> +	__flush_icache_range(start, end);
> +
> +	/*
> +	 * IPI all online CPUs so that they undergo a context synchronization
> +	 * event and are forced to refetch the new instructions.
> +	 */
> +	kick_all_cpus_sync();
> +}
> +
>  static inline void flush_cache_mm(struct mm_struct *mm)
>  {
>  }
> diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
> index 4f3dcc15a5b2..0ac06560c166 100644
> --- a/arch/arm64/kernel/alternative.c
> +++ b/arch/arm64/kernel/alternative.c
> @@ -205,7 +205,6 @@ static int __apply_alternatives_multi_stop(void *unused)
>  	if (smp_processor_id()) {
>  		while (!READ_ONCE(alternatives_applied))
>  			cpu_relax();
> -		isb();
>  	} else {
>  		BUG_ON(alternatives_applied);
>  		__apply_alternatives(&region, false);
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index 1d2b6d768efe..0a338a1cd2d7 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -101,7 +101,7 @@ static void __copy_hyp_vect_bpi(int slot, const char *hyp_vecs_start,
>  	for (i = 0; i < SZ_2K; i += 0x80)
>  		memcpy(dst + i, hyp_vecs_start, hyp_vecs_end - hyp_vecs_start);
>  
> -	flush_icache_range((uintptr_t)dst, (uintptr_t)dst + SZ_2K);
> +	__flush_icache_range((uintptr_t)dst, (uintptr_t)dst + SZ_2K);
>  }
>  
>  static void __install_bp_hardening_cb(bp_hardening_cb_t fn,
> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> index 816d03c4c913..4cc41864f277 100644
> --- a/arch/arm64/kernel/insn.c
> +++ b/arch/arm64/kernel/insn.c
> @@ -249,7 +249,6 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg)
>  	} else {
>  		while (atomic_read(&pp->cpu_count) <= num_online_cpus())
>  			cpu_relax();
> -		isb();
>  	}

Something seems amiss here. 

We call __apply_alternatives_multi_stop() via stop_machine(), and I
thought that ensured that all CPUs had IRQs masked.

If so, the IPI from flush_icache_range() will deadlock.

If not, we can take IRQs, and execute potentially patched code.

I think there's also an existing problem here. Even if with have IRQs
masked, we could take SDEI events (or GICv3 psudeo-NMIs, once we have
those). I don't know how we can manage those.

Thanks,
Mark.

[PATCH 3/3] arm64: IPI each CPU after invalidating the I-cache for kernel mappings

[Mark Rutland]

On Tue, Jun 19, 2018 at 02:55:28PM +0100, Mark Rutland wrote:
> On Tue, Jun 19, 2018 at 01:48:15PM +0100, Will Deacon wrote:
> > When invalidating the instruction cache for a kernel mapping via
> > flush_icache_range(), it is also necessary to flush the pipeline for
> > other CPUs so that instructions fetched into the pipeline before the
> > I-cache invalidation are discarded. For example, if module 'foo' is
> > unloaded and then module 'bar' is loaded into the same area of memory,
> > a CPU could end up executing instructions from 'foo' when branching into
> > 'bar' if these instructions were fetched into the pipeline before 'foo'
> > was unloaded.
> > 
> > Whilst this is highly unlikely to occur in practice, particularly as
> > any exception acts as a context-synchronizing operation, following the
> > letter of the architecture requires us to execute an ISB on each CPU
> > in order for the new instruction stream to be visible.
> > 
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> >  arch/arm64/include/asm/cacheflush.h | 13 ++++++++++++-
> >  arch/arm64/kernel/alternative.c     |  1 -
> >  arch/arm64/kernel/cpu_errata.c      |  2 +-
> >  arch/arm64/kernel/insn.c            | 15 ++-------------
> >  arch/arm64/mm/cache.S               |  4 ++--
> >  5 files changed, 17 insertions(+), 18 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/cacheflush.h b/arch/arm64/include/asm/cacheflush.h
> > index d264a7274811..a0ec27066e6f 100644
> > --- a/arch/arm64/include/asm/cacheflush.h
> > +++ b/arch/arm64/include/asm/cacheflush.h
> > @@ -71,7 +71,7 @@
> >   *		- kaddr  - page address
> >   *		- size   - region size
> >   */
> > -extern void flush_icache_range(unsigned long start, unsigned long end);
> > +extern void __flush_icache_range(unsigned long start, unsigned long end);
> >  extern int  invalidate_icache_range(unsigned long start, unsigned long end);
> >  extern void __flush_dcache_area(void *addr, size_t len);
> >  extern void __inval_dcache_area(void *addr, size_t len);
> > @@ -81,6 +81,17 @@ extern void __clean_dcache_area_pou(void *addr, size_t len);
> >  extern long __flush_cache_user_range(unsigned long start, unsigned long end);
> >  extern void sync_icache_aliases(void *kaddr, unsigned long len);
> >  
> > +static inline void flush_icache_range(unsigned long start, unsigned long end)
> > +{
> > +	__flush_icache_range(start, end);
> > +
> > +	/*
> > +	 * IPI all online CPUs so that they undergo a context synchronization
> > +	 * event and are forced to refetch the new instructions.
> > +	 */
> > +	kick_all_cpus_sync();
> > +}
> > +
> >  static inline void flush_cache_mm(struct mm_struct *mm)
> >  {
> >  }
> > diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
> > index 4f3dcc15a5b2..0ac06560c166 100644
> > --- a/arch/arm64/kernel/alternative.c
> > +++ b/arch/arm64/kernel/alternative.c
> > @@ -205,7 +205,6 @@ static int __apply_alternatives_multi_stop(void *unused)
> >  	if (smp_processor_id()) {
> >  		while (!READ_ONCE(alternatives_applied))
> >  			cpu_relax();
> > -		isb();
> >  	} else {
> >  		BUG_ON(alternatives_applied);
> >  		__apply_alternatives(&region, false);
> > diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> > index 1d2b6d768efe..0a338a1cd2d7 100644
> > --- a/arch/arm64/kernel/cpu_errata.c
> > +++ b/arch/arm64/kernel/cpu_errata.c
> > @@ -101,7 +101,7 @@ static void __copy_hyp_vect_bpi(int slot, const char *hyp_vecs_start,
> >  	for (i = 0; i < SZ_2K; i += 0x80)
> >  		memcpy(dst + i, hyp_vecs_start, hyp_vecs_end - hyp_vecs_start);
> >  
> > -	flush_icache_range((uintptr_t)dst, (uintptr_t)dst + SZ_2K);
> > +	__flush_icache_range((uintptr_t)dst, (uintptr_t)dst + SZ_2K);
> >  }
> >  
> >  static void __install_bp_hardening_cb(bp_hardening_cb_t fn,
> > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> > index 816d03c4c913..4cc41864f277 100644
> > --- a/arch/arm64/kernel/insn.c
> > +++ b/arch/arm64/kernel/insn.c
> > @@ -249,7 +249,6 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg)
> >  	} else {
> >  		while (atomic_read(&pp->cpu_count) <= num_online_cpus())
> >  			cpu_relax();
> > -		isb();
> >  	}
> 
> Something seems amiss here. 
> 
> We call __apply_alternatives_multi_stop() via stop_machine(), and I
> thought that ensured that all CPUs had IRQs masked.

Whoops; that should say aarch64_insn_patch_text_cb(), not
__apply_alternatives_multi_stop()

> If so, the IPI from flush_icache_range() will deadlock.
> 
> If not, we can take IRQs, and execute potentially patched code.
> 
> I think there's also an existing problem here. Even if with have IRQs
> masked, we could take SDEI events (or GICv3 psudeo-NMIs, once we have
> those). I don't know how we can manage those.

Mark.

[PATCH 3/3] arm64: IPI each CPU after invalidating the I-cache for kernel mappings

[Will Deacon]

On Tue, Jun 19, 2018 at 02:55:28PM +0100, Mark Rutland wrote:
> On Tue, Jun 19, 2018 at 01:48:15PM +0100, Will Deacon wrote:
> > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> > index 816d03c4c913..4cc41864f277 100644
> > --- a/arch/arm64/kernel/insn.c
> > +++ b/arch/arm64/kernel/insn.c
> > @@ -249,7 +249,6 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg)
> >  	} else {
> >  		while (atomic_read(&pp->cpu_count) <= num_online_cpus())
> >  			cpu_relax();
> > -		isb();
> >  	}
> 
> Something seems amiss here. 
> 
> We call __apply_alternatives_multi_stop() via stop_machine(), and I
> thought that ensured that all CPUs had IRQs masked.
> 
> If so, the IPI from flush_icache_range() will deadlock.
> 
> If not, we can take IRQs, and execute potentially patched code.

Yes, I think you're right, and I think this only applies to kprobes (since
it patches arbitrary instructions and requires the stop_machine()). However,
I think that highlights another issue, which is that the "nosync" patching
cases as used by things like jump_labels are still going to want this IPI,
so actually the fastpath stuff can all be ripped out. ftrace can probably
be left as-is, since I doubt it's critical that new CPUs immediately see
dynamic tracepoints.

I'll cook a patch sorting this out and include it in v2.

> I think there's also an existing problem here. Even if with have IRQs
> masked, we could take SDEI events (or GICv3 psudeo-NMIs, once we have
> those). I don't know how we can manage those.

I guess there are just some places where we can't deal with an SDEI event.
That said, it's fine as long as the SDEI path is careful about what it runs
(and SDEI is masked until the worst of the patching is over during boot).

James?

Will

[PATCH 2/3] arm64: Remove unnecessary ISBs from set_{pte, pmd, pud}

[Catalin Marinas]

On Tue, Jun 19, 2018 at 01:48:14PM +0100, Will Deacon wrote:
> Commit 7f0b1bf04511 ("arm64: Fix barriers used for page table modifications")
> fixed a reported issue with fixmap page-table entries not being visible
> to the walker due to a missing DSB instruction. At the same time, it added
> ISB instructions to the arm64 set_{pte,pmd,pud} functions, which are not
> required by the architecture and make little sense in isolation.
> 
> Remove the redundant ISBs.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Steve Capper <steve.capper@linaro.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Since I no longer remember the reason I added the ISBs in the first
place:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 3/3] arm64: IPI each CPU after invalidating the I-cache for kernel mappings

[Alexander Van Brunt]

> When invalidating the instruction cache for a kernel mapping via
> flush_icache_range(), it is also necessary to flush the pipeline for
> other CPUs so that instructions fetched into the pipeline before the
> I-cache invalidation are discarded. For example, if module 'foo' is
> unloaded and then module 'bar' is loaded into the same area of memory,
> a CPU could end up executing instructions from 'foo' when branching into
> 'bar' if these instructions were fetched into the pipeline before 'foo'
> was unloaded.

I don't think this fixes the problem. If a CPU is executing 'foo', takes an IPI, and returns to find itself executing in the middle of 'bar' there is still a problem because the code changed. All this patch does is synchronize when two CPUs see 'foo' change to 'bar'.

[PATCH 3/3] arm64: IPI each CPU after invalidating the I-cache for kernel mappings

[Will Deacon]

Hi Alex,

On Wed, Jun 20, 2018 at 04:01:29PM +0000, Alexander Van Brunt wrote:
> > When invalidating the instruction cache for a kernel mapping via
> > flush_icache_range(), it is also necessary to flush the pipeline for
> > other CPUs so that instructions fetched into the pipeline before the
> > I-cache invalidation are discarded. For example, if module 'foo' is
> > unloaded and then module 'bar' is loaded into the same area of memory,
> > a CPU could end up executing instructions from 'foo' when branching into
> > 'bar' if these instructions were fetched into the pipeline before 'foo'
> > was unloaded.
> 
> I don't think this fixes the problem. If a CPU is executing 'foo', takes
> an IPI, and returns to find itself executing in the middle of 'bar' there
> is still a problem because the code changed. All this patch does is
> synchronize when two CPUs see 'foo' change to 'bar'.

Right, but that would indicate a catastophic bug in the module code. There
are two sides to this:

1. Code that manages the lifetime of executable mappings. That should all be
   present in the core code, to make sure that we don't e.g. unmap code that
   is being executed.

2. Ensuring that new instructions in an executable mapping are visible to
   the CPU I-side when that CPU decides to branch into the mapping.

This patch series is all about (2). *If* (1) was implemented exclusively
using RCU, then we could probably avoid the IPI and instead ensure that an
RCU grace period ensured all concurrent executors had gone through a context
synchronization event, but unfortunately we can't rely on that.

Will

[PATCH 3/3] arm64: IPI each CPU after invalidating the I-cache for kernel mappings

[James Morse]

Hi Mark, Will

On 19/06/18 17:50, Will Deacon wrote:
> On Tue, Jun 19, 2018 at 02:55:28PM +0100, Mark Rutland wrote:
>> On Tue, Jun 19, 2018 at 01:48:15PM +0100, Will Deacon wrote:
>>> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
>>> index 816d03c4c913..4cc41864f277 100644
>>> --- a/arch/arm64/kernel/insn.c
>>> +++ b/arch/arm64/kernel/insn.c
>>> @@ -249,7 +249,6 @@ static int __kprobes aarch64_insn_patch_text_cb(void *arg)
>>>  	} else {
>>>  		while (atomic_read(&pp->cpu_count) <= num_online_cpus())
>>>  			cpu_relax();
>>> -		isb();
>>>  	}
>>
>> Something seems amiss here. 
>>
>> We call __apply_alternatives_multi_stop() via stop_machine(), and I
>> thought that ensured that all CPUs had IRQs masked.
>>
>> If so, the IPI from flush_icache_range() will deadlock.
>>
>> If not, we can take IRQs, and execute potentially patched code.
> 
> Yes, I think you're right, and I think this only applies to kprobes (since
> it patches arbitrary instructions and requires the stop_machine()). However,
> I think that highlights another issue, which is that the "nosync" patching
> cases as used by things like jump_labels are still going to want this IPI,
> so actually the fastpath stuff can all be ripped out. ftrace can probably
> be left as-is, since I doubt it's critical that new CPUs immediately see
> dynamic tracepoints.
>
> I'll cook a patch sorting this out and include it in v2.
> 
>> I think there's also an existing problem here. Even if with have IRQs
>> masked, we could take SDEI events (or GICv3 psudeo-NMIs, once we have
>> those). I don't know how we can manage those.
> 
> I guess there are just some places where we can't deal with an SDEI event.
> That said, it's fine as long as the SDEI path is careful about what it runs
> (and SDEI is masked until the worst of the patching is over during boot).

Yup, its setup as late as possible, so cpufeature stuff should be safe.

I think break-pointing code we expect to run in_nmi() is a disaster waiting to
happen. e.g. taking an SDEI-event out of a guest, we run the handler before KVM
does it's guest exit, so if you tread on a breakpoint KVM's el2_sync is going to
bring the machine down.

The SDEI stuff has __kprobes all over it, I probably need to add some more
'notrace'.


I think forbidding code-patch on NMI code-regions is the easiest thing to do.

If this is something we needed to support, we could get stop_machine() to mask
SDEI and GIC psuedo_NMIs as part of its work. We could mask SError and disable
debug exceptions while we're at it.



Thanks,

James