Message ID | 20201102171329.4312-1-aryabinin@virtuozzo.com |
---|---|
State | New |
Series | "Series without cover letter" |
Headers | show |
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 = &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 +81,27 @@ 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; } + memcpy(&per_cpu(cpu_flags, cpu), &flags, sizeof(flags)); } static int show_cpuinfo(struct seq_file *m, void *v)
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 <aryabinin@virtuozzo.com> > --- > 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 = &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 +81,27 @@ 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; > } > + memcpy(&per_cpu(cpu_flags, cpu), &flags, 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) >
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 <aryabinin@virtuozzo.com> >> --- >> 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 = &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 +81,27 @@ 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; >> } >> + memcpy(&per_cpu(cpu_flags, cpu), &flags, 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.
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 arch/x86/kernel/cpu/proc.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)