Re: [PATCH 5/5] ARM: spectre-v2: per-CPU vtables to work around big.Little systems

* Re: [PATCH 5/5] ARM: spectre-v2: per-CPU vtables to work around big.Little systems
       [not found]   ` <E1g2Z6e-0003es-CI@rmk-PC.armlinux.org.uk>
@ 2018-10-05  9:09     ` Marek Szyprowski
  2018-10-05  9:35       ` Krzysztof Kozlowski
  2018-10-05  9:46       ` Russell King - ARM Linux
  0 siblings, 2 replies; 7+ messages in thread
From: Marek Szyprowski @ 2018-10-05  9:09 UTC (permalink / raw)
  To: Russell King, linux-arm-kernel
  Cc: Mark Rutland, 'Linux Samsung SOC', Julien Thierry,
	Marc Zyngier, Will Deacon, Krzysztof Kozlowski, Dietmar Eggemann,
	Morten Rasmussen, Qais Yousef

Hi All,

On 2018-09-19 11:49, Russell King wrote:
> In big.Little systems, some CPUs require the Spectre workarounds in
> paths such as the context switch, but other CPUs do not.  In order
> to handle these differences, we need per-CPU vtables.
>
> We are unable to use the kernel's per-CPU variables to support this
> as per-CPU is not initialised at times when we need access to the
> vtables, so we have to use an array indexed by logical CPU number.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

This patch causes lots of kernel 'BUG' messages on all Samsung Exynos
boards.  It started to appear since it has been merged to linux-next
on 20181002.  I wonder if this issue is Exynos specific or there are
some patches missing in linux-next, which should fix those 'BUGS'.
If this is Exynos specific, please let us know what should be changed
in Exynos platform code to avoid this issue.

Here are some examples:

BUG: using smp_processor_id() in preemptible [00000000] code: startpar/117
caller is pgd_alloc+0x54/0x160
CPU: 3 PID: 117 Comm: startpar Not tainted 4.16.0-00029-gc6eb8a2e9190 #862
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[<c0111074>] (unwind_backtrace) from [<c010d64c>] (show_stack+0x10/0x14)
[<c010d64c>] (show_stack) from [<c09b5c50>] (dump_stack+0x90/0xc8)
[<c09b5c50>] (dump_stack) from [<c044b76c>]
(check_preemption_disabled+0x118/0x130)
[<c044b76c>] (check_preemption_disabled) from [<c0119edc>]
(pgd_alloc+0x54/0x160)
[<c0119edc>] (pgd_alloc) from [<c0121570>] (mm_init+0xe4/0x174)
[<c0121570>] (mm_init) from [<c01234a4>] (copy_process.part.5+0x1480/0x1a9c)
[<c01234a4>] (copy_process.part.5) from [<c0123c24>] (_do_fork+0xc4/0x7ec)
[<c0123c24>] (_do_fork) from [<c012441c>] (SyS_clone+0x24/0x2c)
[<c012441c>] (SyS_clone) from [<c0101000>] (ret_fast_syscall+0x0/0x28)

BUG: using smp_processor_id() in preemptible [00000000] code:
checkroot-bootc/686
[<c010d64c>] (show_stack) from [<c09b5c50>] (dump_stack+0x90/0xc8)
caller is pgd_alloc+0x54/0x160
[<c09b5c50>] (dump_stack) from [<c044b76c>]
(check_preemption_disabled+0x118/0x130)
[<c044b76c>] (check_preemption_disabled) from [<c025a594>]
(vmap_page_range_noflush+0x114/0x1dc)
[<c025a594>] (vmap_page_range_noflush) from [<c025a68c>]
(map_vm_area+0x30/0x64)
[<c025a68c>] (map_vm_area) from [<c025b9bc>]
(__vmalloc_node_range+0x14c/0x220)
[<c025b9bc>] (__vmalloc_node_range) from [<c025bad8>]
(__vmalloc_node+0x48/0x50)
[<c025bad8>] (__vmalloc_node) from [<c025bb14>] (vmalloc+0x34/0x3c)
[<c025bb14>] (vmalloc) from [<c04b2960>] (n_tty_open+0x10/0xc8)
[<c04b2960>] (n_tty_open) from [<c04b66d8>] (tty_ldisc_open+0x3c/0x78)
[<c04b66d8>] (tty_ldisc_open) from [<c04b6ebc>] (tty_ldisc_setup+0x14/0x58)
[<c04b6ebc>] (tty_ldisc_setup) from [<c04b11ec>] (tty_init_dev+0xa4/0x1b0)
[<c04b11ec>] (tty_init_dev) from [<c04bab78>] (ptmx_open+0x90/0x160)
[<c04bab78>] (ptmx_open) from [<c02792d8>] (chrdev_open+0x9c/0x174)
[<c02792d8>] (chrdev_open) from [<c0271524>] (do_dentry_open+0xfc/0x30c)
[<c0271524>] (do_dentry_open) from [<c02833e4>] (path_openat+0x454/0xed0)
[<c02833e4>] (path_openat) from [<c0284968>] (do_filp_open+0x60/0xb4)
[<c0284968>] (do_filp_open) from [<c0272cb8>] (do_sys_open+0x118/0x1c4)
[<c0272cb8>] (do_sys_open) from [<c0101000>] (ret_fast_syscall+0x0/0x28)

> ---
>  arch/arm/include/asm/proc-fns.h | 14 ++++++++++++++
>  arch/arm/kernel/setup.c         |  5 +++++
>  arch/arm/kernel/smp.c           | 31 +++++++++++++++++++++++++++++++
>  arch/arm/mm/proc-v7-bugs.c      | 17 ++---------------
>  4 files changed, 52 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/include/asm/proc-fns.h b/arch/arm/include/asm/proc-fns.h
> index 571a1346245b..ddbb25cb8968 100644
> --- a/arch/arm/include/asm/proc-fns.h
> +++ b/arch/arm/include/asm/proc-fns.h
> @@ -104,11 +104,25 @@ extern void cpu_do_resume(void *);
>  #else
>  
>  extern struct processor processor;
> +#if defined(CONFIG_BIG_LITTLE) && defined(CONFIG_HARDEN_BRANCH_PREDICTOR)
> +#include <linux/smp.h>
> +/*
> + * This can't be a per-cpu variable because we need to access it before
> + * per-cpu has been initialised.
> + */
> +extern struct processor *cpu_vtable[];
> +#define PROC_VTABLE(f)			cpu_vtable[smp_processor_id()]->f
> +static inline void init_proc_vtable(const struct processor *p)
> +{
> +	*cpu_vtable[smp_processor_id()] = *p;
> +}
> +#else
>  #define PROC_VTABLE(f)			processor.f
>  static inline void init_proc_vtable(const struct processor *p)
>  {
>  	processor = *p;
>  }
> +#endif
>  
>  #define cpu_proc_init			PROC_VTABLE(_proc_init)
>  #define cpu_check_bugs			PROC_VTABLE(check_bugs)
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index c214bd14a1fe..cd46a595422c 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -115,6 +115,11 @@ EXPORT_SYMBOL(elf_hwcap2);
>  
>  #ifdef MULTI_CPU
>  struct processor processor __ro_after_init;
> +#if defined(CONFIG_BIG_LITTLE) && defined(CONFIG_HARDEN_BRANCH_PREDICTOR)
> +struct processor *cpu_vtable[NR_CPUS] = {
> +	[0] = &processor,
> +};
> +#endif
>  #endif
>  #ifdef MULTI_TLB
>  struct cpu_tlb_fns cpu_tlb __ro_after_init;
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 5ad0b67b9e33..82b879db32ee 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -42,6 +42,7 @@
>  #include <asm/mmu_context.h>
>  #include <asm/pgtable.h>
>  #include <asm/pgalloc.h>
> +#include <asm/procinfo.h>
>  #include <asm/processor.h>
>  #include <asm/sections.h>
>  #include <asm/tlbflush.h>
> @@ -102,6 +103,30 @@ static unsigned long get_arch_pgd(pgd_t *pgd)
>  #endif
>  }
>  
> +#if defined(CONFIG_BIG_LITTLE) && defined(CONFIG_HARDEN_BRANCH_PREDICTOR)
> +static int secondary_biglittle_prepare(unsigned int cpu)
> +{
> +	if (!cpu_vtable[cpu])
> +		cpu_vtable[cpu] = kzalloc(sizeof(*cpu_vtable[cpu]), GFP_KERNEL);
> +
> +	return cpu_vtable[cpu] ? 0 : -ENOMEM;
> +}
> +
> +static void secondary_biglittle_init(void)
> +{
> +	init_proc_vtable(lookup_processor(read_cpuid_id())->proc);
> +}
> +#else
> +static int secondary_biglittle_prepare(unsigned int cpu)
> +{
> +	return 0;
> +}
> +
> +static void secondary_biglittle_init(void)
> +{
> +}
> +#endif
> +
>  int __cpu_up(unsigned int cpu, struct task_struct *idle)
>  {
>  	int ret;
> @@ -109,6 +134,10 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
>  	if (!smp_ops.smp_boot_secondary)
>  		return -ENOSYS;
>  
> +	ret = secondary_biglittle_prepare(cpu);
> +	if (ret)
> +		return ret;
> +
>  	/*
>  	 * We need to tell the secondary core where to find
>  	 * its stack and the page tables.
> @@ -360,6 +389,8 @@ asmlinkage void secondary_start_kernel(void)
>  	struct mm_struct *mm = &init_mm;
>  	unsigned int cpu;
>  
> +	secondary_biglittle_init();
> +
>  	/*
>  	 * The identity mapping is uncached (strongly ordered), so
>  	 * switch away from it before attempting any exclusive accesses.
> diff --git a/arch/arm/mm/proc-v7-bugs.c b/arch/arm/mm/proc-v7-bugs.c
> index 5544b82a2e7a..9a07916af8dd 100644
> --- a/arch/arm/mm/proc-v7-bugs.c
> +++ b/arch/arm/mm/proc-v7-bugs.c
> @@ -52,8 +52,6 @@ static void cpu_v7_spectre_init(void)
>  	case ARM_CPU_PART_CORTEX_A17:
>  	case ARM_CPU_PART_CORTEX_A73:
>  	case ARM_CPU_PART_CORTEX_A75:
> -		if (processor.switch_mm != cpu_v7_bpiall_switch_mm)
> -			goto bl_error;
>  		per_cpu(harden_branch_predictor_fn, cpu) =
>  			harden_branch_predictor_bpiall;
>  		spectre_v2_method = "BPIALL";
> @@ -61,8 +59,6 @@ static void cpu_v7_spectre_init(void)
>  
>  	case ARM_CPU_PART_CORTEX_A15:
>  	case ARM_CPU_PART_BRAHMA_B15:
> -		if (processor.switch_mm != cpu_v7_iciallu_switch_mm)
> -			goto bl_error;
>  		per_cpu(harden_branch_predictor_fn, cpu) =
>  			harden_branch_predictor_iciallu;
>  		spectre_v2_method = "ICIALLU";
> @@ -88,11 +84,9 @@ static void cpu_v7_spectre_init(void)
>  					  ARM_SMCCC_ARCH_WORKAROUND_1, &res);
>  			if ((int)res.a0 != 0)
>  				break;
> -			if (processor.switch_mm != cpu_v7_hvc_switch_mm && cpu)
> -				goto bl_error;
>  			per_cpu(harden_branch_predictor_fn, cpu) =
>  				call_hvc_arch_workaround_1;
> -			processor.switch_mm = cpu_v7_hvc_switch_mm;
> +			cpu_do_switch_mm = cpu_v7_hvc_switch_mm;
>  			spectre_v2_method = "hypervisor";
>  			break;
>  
> @@ -101,11 +95,9 @@ static void cpu_v7_spectre_init(void)
>  					  ARM_SMCCC_ARCH_WORKAROUND_1, &res);
>  			if ((int)res.a0 != 0)
>  				break;
> -			if (processor.switch_mm != cpu_v7_smc_switch_mm && cpu)
> -				goto bl_error;
>  			per_cpu(harden_branch_predictor_fn, cpu) =
>  				call_smc_arch_workaround_1;
> -			processor.switch_mm = cpu_v7_smc_switch_mm;
> +			cpu_do_switch_mm = cpu_v7_smc_switch_mm;
>  			spectre_v2_method = "firmware";
>  			break;
>  
> @@ -119,11 +111,6 @@ static void cpu_v7_spectre_init(void)
>  	if (spectre_v2_method)
>  		pr_info("CPU%u: Spectre v2: using %s workaround\n",
>  			smp_processor_id(), spectre_v2_method);
> -	return;
> -
> -bl_error:
> -	pr_err("CPU%u: Spectre v2: incorrect context switching function, system vulnerable\n",
> -		cpu);
>  }
>  #else
>  static void cpu_v7_spectre_init(void)

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread