Re: [Devel] [PATCH vz8 v2 1/2] x86, cpuinfo: Fix race on parallel /proc/cpuinfo read

2020-11-03 Thread Andrey Ryabinin 



On 11/3/20 2:28 PM, Kirill Tkhai wrote:
> On 02.11.2020 20:13, 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 
>> ---
>> Changes since v1:
>>  - none
>>  
>>  arch/x86/kernel/cpu/proc.c | 17 +
>>  1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
>> index 4fe1577d5e6f..4cc2951e34fb 100644
>> --- a/arch/x86/kernel/cpu/proc.c
>> +++ b/arch/x86/kernel/cpu/proc.c
>> @@ -69,11 +69,11 @@ static DEFINE_PER_CPU(struct cpu_flags, cpu_flags);
>>  static void init_cpu_flags(void *dummy)
>>  {
>>  int cpu = smp_processor_id();
>> -struct cpu_flags *flags = _cpu(cpu_flags, cpu);
>> +struct cpu_flags flags;
>>  struct cpuinfo_x86 *c = _data(cpu);
>>  unsigned int eax, ebx, ecx, edx;
>>  
>> -memcpy(flags->val, c->x86_capability, NCAPINTS * sizeof(u32));
>> +memcpy(, c->x86_capability, sizeof(flags));
>>  
>>  /*
>>   * Clear feature bits masked using cpuid masking/faulting.
>> @@ -81,26 +81,27 @@ static void init_cpu_flags(void *dummy)
>>  
>>  if (c->cpuid_level >= 0x0001) {
>>  __do_cpuid_fault(0x0001, 0, , , , );
>> -flags->val[4] &= ecx;
>> -flags->val[0] &= edx;
>> +flags.val[4] &= ecx;
>> +flags.val[0] &= edx;
>>  }
>>  
>>  if (c->cpuid_level >= 0x0007) {
>>  __do_cpuid_fault(0x0007, 0, , , , );
>> -flags->val[9] &= ebx;
>> +flags.val[9] &= ebx;
>>  }
>>  
>>  if ((c->extended_cpuid_level & 0x) == 0x8000 &&
>>  c->extended_cpuid_level >= 0x8001) {
>>  __do_cpuid_fault(0x8001, 0, , , , );
>> -flags->val[6] &= ecx;
>> -flags->val[1] &= edx;
>> +flags.val[6] &= ecx;
>> +flags.val[1] &= edx;
>>  }
>>  
>>  if (c->cpuid_level >= 0x000d) {
>>  __do_cpuid_fault(0x000d, 1, , , , );
>> -flags->val[10] &= eax;
>> +flags.val[10] &= eax;
>>  }
>> +memcpy(_cpu(cpu_flags, cpu), , sizeof(flags));
> 
> This is still racy, since memcpy() is not atomic. Maybe we should add some 
> lock on top of this?
> 

This race shouldn't be a problem since flags are not supposed to change during 
ve lifetime.
So we overwriting same values. But don't mind to add spinlock protection.
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH vz8 v2 1/2] x86, cpuinfo: Fix race on parallel /proc/cpuinfo read

2020-11-03 Thread Kirill Tkhai 
On 02.11.2020 20:13, 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 
> ---
> Changes since v1:
>  - none
>  
>  arch/x86/kernel/cpu/proc.c | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
> index 4fe1577d5e6f..4cc2951e34fb 100644
> --- a/arch/x86/kernel/cpu/proc.c
> +++ b/arch/x86/kernel/cpu/proc.c
> @@ -69,11 +69,11 @@ static DEFINE_PER_CPU(struct cpu_flags, cpu_flags);
>  static void init_cpu_flags(void *dummy)
>  {
>   int cpu = smp_processor_id();
> - struct cpu_flags *flags = _cpu(cpu_flags, cpu);
> + struct cpu_flags flags;
>   struct cpuinfo_x86 *c = _data(cpu);
>   unsigned int eax, ebx, ecx, edx;
>  
> - memcpy(flags->val, c->x86_capability, NCAPINTS * sizeof(u32));
> + memcpy(, c->x86_capability, sizeof(flags));
>  
>   /*
>* Clear feature bits masked using cpuid masking/faulting.
> @@ -81,26 +81,27 @@ static void init_cpu_flags(void *dummy)
>  
>   if (c->cpuid_level >= 0x0001) {
>   __do_cpuid_fault(0x0001, 0, , , , );
> - flags->val[4] &= ecx;
> - flags->val[0] &= edx;
> + flags.val[4] &= ecx;
> + flags.val[0] &= edx;
>   }
>  
>   if (c->cpuid_level >= 0x0007) {
>   __do_cpuid_fault(0x0007, 0, , , , );
> - flags->val[9] &= ebx;
> + flags.val[9] &= ebx;
>   }
>  
>   if ((c->extended_cpuid_level & 0x) == 0x8000 &&
>   c->extended_cpuid_level >= 0x8001) {
>   __do_cpuid_fault(0x8001, 0, , , , );
> - flags->val[6] &= ecx;
> - flags->val[1] &= edx;
> + flags.val[6] &= ecx;
> + flags.val[1] &= edx;
>   }
>  
>   if (c->cpuid_level >= 0x000d) {
>   __do_cpuid_fault(0x000d, 1, , , , );
> - flags->val[10] &= eax;
> + flags.val[10] &= eax;
>   }
> + memcpy(_cpu(cpu_flags, cpu), , sizeof(flags));

This is still racy, since memcpy() is not atomic. Maybe we should add some lock 
on top of this?

>  }
>  
>  static int show_cpuinfo(struct seq_file *m, void *v)
> 

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

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

2020-11-02 Thread Andrey Ryabinin 
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 
---
Changes since v1:
 - none
 
 arch/x86/kernel/cpu/proc.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
index 4fe1577d5e6f..4cc2951e34fb 100644
--- a/arch/x86/kernel/cpu/proc.c
+++ b/arch/x86/kernel/cpu/proc.c
@@ -69,11 +69,11 @@ static DEFINE_PER_CPU(struct cpu_flags, cpu_flags);
 static void init_cpu_flags(void *dummy)
 {
int cpu = smp_processor_id();
-   struct cpu_flags *flags = _cpu(cpu_flags, cpu);
+   struct cpu_flags flags;
struct cpuinfo_x86 *c = _data(cpu);
unsigned int eax, ebx, ecx, edx;
 
-   memcpy(flags->val, c->x86_capability, NCAPINTS * sizeof(u32));
+   memcpy(, c->x86_capability, sizeof(flags));
 
/*
 * Clear feature bits masked using cpuid masking/faulting.
@@ -81,26 +81,27 @@ static void init_cpu_flags(void *dummy)
 
if (c->cpuid_level >= 0x0001) {
__do_cpuid_fault(0x0001, 0, , , , );
-   flags->val[4] &= ecx;
-   flags->val[0] &= edx;
+   flags.val[4] &= ecx;
+   flags.val[0] &= edx;
}
 
if (c->cpuid_level >= 0x0007) {
__do_cpuid_fault(0x0007, 0, , , , );
-   flags->val[9] &= ebx;
+   flags.val[9] &= ebx;
}
 
if ((c->extended_cpuid_level & 0x) == 0x8000 &&
c->extended_cpuid_level >= 0x8001) {
__do_cpuid_fault(0x8001, 0, , , , );
-   flags->val[6] &= ecx;
-   flags->val[1] &= edx;
+   flags.val[6] &= ecx;
+   flags.val[1] &= edx;
}
 
if (c->cpuid_level >= 0x000d) {
__do_cpuid_fault(0x000d, 1, , , , );
-   flags->val[10] &= eax;
+   flags.val[10] &= eax;
}
+   memcpy(_cpu(cpu_flags, cpu), , sizeof(flags));
 }
 
 static int show_cpuinfo(struct seq_file *m, void *v)
-- 
2.26.2

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel