linux-kernel - [PATCH v1] x86/smpboot: broken calibration path during cpu bringup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 27 Oct 2017 20:11:00 -0400
From:   Pavel Tatashin <pasha.tatashin@...cle.com>
To:     steven.sistare@...cle.com, daniel.m.jordan@...cle.com,
        bob.picco@...cle.com, tglx@...utronix.de, peterz@...radead.org,
        linux-kernel@...r.kernel.org, x86@...nel.org, hpa@...or.com
Subject: [PATCH v1] x86/smpboot: broken calibration path during cpu bringup

While studying why it takes 0.06s to bring up every cpu, which accounts to
15.36s on 256 cpu system, I determined that it is all because of
calibrate_delay() call.

After, studying code further I found that there are bugs in the current
code:

If tsc is enabled, and cpu has TSC_CONSTANT feature, and a cpu is in the
same core has already been calibrated, we do not need to calibrate again:

This check is done here:

calibrate_delay()
	calibrate_delay_is_known()

But, calibrate_delay() is called before topology for new cpu is updated,
so we never actually take the optimized path.

The second bug, is that inside calibrate_delay_is_known() there is branch
like this:

if (!tsc_disabled && !cpu_has(&cpu_data(cpu), X86_FEATURE_CONSTANT_TSC))
	return 0;

But the logic is broken, it should be:

if (tsc_disabled || !cpu_has(&cpu_data(cpu), X86_FEATURE_CONSTANT_TSC))
	return 0;

Fixes: c25323c07345 ("x86/tsc: Use topology functions")

Signed-off-by: Pavel Tatashin <pasha.tatashin@...cle.com>
---
 arch/x86/kernel/smpboot.c | 13 ++++++++-----
 arch/x86/kernel/tsc.c     |  6 ++----
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index ad59edd84de7..e7a3bab6818b 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -193,6 +193,14 @@ static void smp_callin(void)
 	 */
 	smp_store_cpu_info(cpuid);
 
+	/*
+	 * This must be done before setting cpu_online_mask
+	 * or calling notify_cpu_starting.
+	 * And also before calibrate_delay(), as the information about topology
+	 * is used to determine if calibration is needed.
+	 */
+	set_cpu_sibling_map(raw_smp_processor_id());
+
 	/*
 	 * Get our bogomips.
 	 * Update loops_per_jiffy in cpu_data. Previous call to
@@ -203,11 +211,6 @@ static void smp_callin(void)
 	cpu_data(cpuid).loops_per_jiffy = loops_per_jiffy;
 	pr_debug("Stack at about %p\n", &cpuid);
 
-	/*
-	 * This must be done before setting cpu_online_mask
-	 * or calling notify_cpu_starting.
-	 */
-	set_cpu_sibling_map(raw_smp_processor_id());
 	wmb();
 
 	notify_cpu_starting(cpuid);
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 796d96bb0821..a99cde96201f 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -1346,12 +1346,10 @@ void __init tsc_init(void)
 unsigned long calibrate_delay_is_known(void)
 {
 	int sibling, cpu = smp_processor_id();
+	int constant_tsc = cpu_has(&cpu_data(cpu), X86_FEATURE_CONSTANT_TSC);
 	struct cpumask *mask = topology_core_cpumask(cpu);
 
-	if (!tsc_disabled && !cpu_has(&cpu_data(cpu), X86_FEATURE_CONSTANT_TSC))
-		return 0;
-
-	if (!mask)
+	if (tsc_disabled || !constant_tsc || !mask)
 		return 0;
 
 	sibling = cpumask_any_but(mask, cpu);
-- 
2.14.3