[vz8,v3,1/2] x86, cpuinfo: Fix race on parallel /proc/cpuinfo read

Submitted by Andrey Ryabinin on Nov. 3, 2020, 2:36 p.m.

Details

Message ID 20201103143606.325-1-aryabinin@virtuozzo.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Andrey Ryabinin Nov. 3, 2020, 2:36 p.m. 
If several threads read /proc/cpuinfo some can see in 'flags'
values from c->x86_capability, before __do_cpuid_fault() called
and masks applied. Fix this by forming 'flags' on stack first
and copy them in per_cpu(cpu_flags, cpu) as a last step.

https://jira.sw.ru/browse/PSBM-121823
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---

Changes since v1:
 - none

Changes since v2:
 - add spinlock, use temporary ve_flags in show_cpuinfo()
 
 arch/x86/kernel/cpu/proc.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

Patch hide | download patch | download mbox 

diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
index 4fe1577d5e6f..08fd7ff9a55b 100644
--- a/arch/x86/kernel/cpu/proc.c
+++ b/arch/x86/kernel/cpu/proc.c
@@ -65,15 +65,16 @@  struct cpu_flags {
 };
 
 static DEFINE_PER_CPU(struct cpu_flags, cpu_flags);
+static DEFINE_SPINLOCK(cpu_flags_lock);
 
 static void init_cpu_flags(void *dummy)
 {
 	int cpu = smp_processor_id();
-	struct cpu_flags *flags = &per_cpu(cpu_flags, cpu);
+	struct cpu_flags flags;
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	unsigned int eax, ebx, ecx, edx;
 
-	memcpy(flags->val, c->x86_capability, NCAPINTS * sizeof(u32));
+	memcpy(&flags, c->x86_capability, sizeof(flags));
 
 	/*
 	 * Clear feature bits masked using cpuid masking/faulting.
@@ -81,26 +82,30 @@  static void init_cpu_flags(void *dummy)
 
 	if (c->cpuid_level >= 0x00000001) {
 		__do_cpuid_fault(0x00000001, 0, &eax, &ebx, &ecx, &edx);
-		flags->val[4] &= ecx;
-		flags->val[0] &= edx;
+		flags.val[4] &= ecx;
+		flags.val[0] &= edx;
 	}
 
 	if (c->cpuid_level >= 0x00000007) {
 		__do_cpuid_fault(0x00000007, 0, &eax, &ebx, &ecx, &edx);
-		flags->val[9] &= ebx;
+		flags.val[9] &= ebx;
 	}
 
 	if ((c->extended_cpuid_level & 0xffff0000) == 0x80000000 &&
 	    c->extended_cpuid_level >= 0x80000001) {
 		__do_cpuid_fault(0x80000001, 0, &eax, &ebx, &ecx, &edx);
-		flags->val[6] &= ecx;
-		flags->val[1] &= edx;
+		flags.val[6] &= ecx;
+		flags.val[1] &= edx;
 	}
 
 	if (c->cpuid_level >= 0x0000000d) {
 		__do_cpuid_fault(0x0000000d, 1, &eax, &ebx, &ecx, &edx);
-		flags->val[10] &= eax;
+		flags.val[10] &= eax;
 	}
+
+	spin_lock(&cpu_flags_lock);
+	memcpy(&per_cpu(cpu_flags, cpu), &flags, sizeof(flags));
+	spin_unlock(&cpu_flags_lock);
 }
 
 static int show_cpuinfo(struct seq_file *m, void *v)
@@ -108,6 +113,7 @@  static int show_cpuinfo(struct seq_file *m, void *v)
 	struct cpuinfo_x86 *c = v;
 	unsigned int cpu;
 	int is_super = ve_is_super(get_exec_env());
+	struct cpu_flags ve_flags;
 	int i;
 
 	cpu = c->cpu_index;
@@ -147,12 +153,19 @@  static int show_cpuinfo(struct seq_file *m, void *v)
 	show_cpuinfo_core(m, c, cpu);
 	show_cpuinfo_misc(m, c);
 
+	if (!is_super) {
+		spin_lock_irq(&cpu_flags_lock);
+		memcpy(&ve_flags, &per_cpu(cpu_flags, cpu), sizeof(ve_flags));
+		spin_unlock_irq(&cpu_flags_lock);
+	}
+
+
 	seq_puts(m, "flags\t\t:");
 	for (i = 0; i < 32*NCAPINTS; i++)
 		if (x86_cap_flags[i] != NULL &&
 				((is_super && cpu_has(c, i)) ||
 				 (!is_super && test_bit(i, (unsigned long *)
-							&per_cpu(cpu_flags, cpu)))))
+							&ve_flags))))
 			seq_printf(m, " %s", x86_cap_flags[i]);
 
 	seq_puts(m, "\nbugs\t\t:");

Comments

Kirill Tkhai Nov. 3, 2020, 4:05 p.m. 
On 03.11.2020 17:36, Andrey Ryabinin wrote:
> If several threads read /proc/cpuinfo some can see in 'flags'
> values from c->x86_capability, before __do_cpuid_fault() called
> and masks applied. Fix this by forming 'flags' on stack first
> and copy them in per_cpu(cpu_flags, cpu) as a last step.
> 
> https://jira.sw.ru/browse/PSBM-121823
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>

Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>

> ---
> 
> Changes since v1:
>  - none
> 
> Changes since v2:
>  - add spinlock, use temporary ve_flags in show_cpuinfo()
>  
>  arch/x86/kernel/cpu/proc.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
> index 4fe1577d5e6f..08fd7ff9a55b 100644
> --- a/arch/x86/kernel/cpu/proc.c
> +++ b/arch/x86/kernel/cpu/proc.c
> @@ -65,15 +65,16 @@ struct cpu_flags {
>  };
>  
>  static DEFINE_PER_CPU(struct cpu_flags, cpu_flags);
> +static DEFINE_SPINLOCK(cpu_flags_lock);
>  
>  static void init_cpu_flags(void *dummy)
>  {
>  	int cpu = smp_processor_id();
> -	struct cpu_flags *flags = &per_cpu(cpu_flags, cpu);
> +	struct cpu_flags flags;
>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
>  	unsigned int eax, ebx, ecx, edx;
>  
> -	memcpy(flags->val, c->x86_capability, NCAPINTS * sizeof(u32));
> +	memcpy(&flags, c->x86_capability, sizeof(flags));
>  
>  	/*
>  	 * Clear feature bits masked using cpuid masking/faulting.
> @@ -81,26 +82,30 @@ static void init_cpu_flags(void *dummy)
>  
>  	if (c->cpuid_level >= 0x00000001) {
>  		__do_cpuid_fault(0x00000001, 0, &eax, &ebx, &ecx, &edx);
> -		flags->val[4] &= ecx;
> -		flags->val[0] &= edx;
> +		flags.val[4] &= ecx;
> +		flags.val[0] &= edx;
>  	}
>  
>  	if (c->cpuid_level >= 0x00000007) {
>  		__do_cpuid_fault(0x00000007, 0, &eax, &ebx, &ecx, &edx);
> -		flags->val[9] &= ebx;
> +		flags.val[9] &= ebx;
>  	}
>  
>  	if ((c->extended_cpuid_level & 0xffff0000) == 0x80000000 &&
>  	    c->extended_cpuid_level >= 0x80000001) {
>  		__do_cpuid_fault(0x80000001, 0, &eax, &ebx, &ecx, &edx);
> -		flags->val[6] &= ecx;
> -		flags->val[1] &= edx;
> +		flags.val[6] &= ecx;
> +		flags.val[1] &= edx;
>  	}
>  
>  	if (c->cpuid_level >= 0x0000000d) {
>  		__do_cpuid_fault(0x0000000d, 1, &eax, &ebx, &ecx, &edx);
> -		flags->val[10] &= eax;
> +		flags.val[10] &= eax;
>  	}
> +
> +	spin_lock(&cpu_flags_lock);
> +	memcpy(&per_cpu(cpu_flags, cpu), &flags, sizeof(flags));
> +	spin_unlock(&cpu_flags_lock);
>  }
>  
>  static int show_cpuinfo(struct seq_file *m, void *v)
> @@ -108,6 +113,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
>  	struct cpuinfo_x86 *c = v;
>  	unsigned int cpu;
>  	int is_super = ve_is_super(get_exec_env());
> +	struct cpu_flags ve_flags;
>  	int i;
>  
>  	cpu = c->cpu_index;
> @@ -147,12 +153,19 @@ static int show_cpuinfo(struct seq_file *m, void *v)
>  	show_cpuinfo_core(m, c, cpu);
>  	show_cpuinfo_misc(m, c);
>  
> +	if (!is_super) {
> +		spin_lock_irq(&cpu_flags_lock);
> +		memcpy(&ve_flags, &per_cpu(cpu_flags, cpu), sizeof(ve_flags));
> +		spin_unlock_irq(&cpu_flags_lock);
> +	}
> +
> +
>  	seq_puts(m, "flags\t\t:");
>  	for (i = 0; i < 32*NCAPINTS; i++)
>  		if (x86_cap_flags[i] != NULL &&
>  				((is_super && cpu_has(c, i)) ||
>  				 (!is_super && test_bit(i, (unsigned long *)
> -							&per_cpu(cpu_flags, cpu)))))
> +							&ve_flags))))
>  			seq_printf(m, " %s", x86_cap_flags[i]);
>  
>  	seq_puts(m, "\nbugs\t\t:");
>