[PATCH] x86: avoid invalid phys_proc_id reference

[PATCH] x86: avoid invalid phys_proc_id reference

[Chao Peng]

phys_proc_id is invalidated in remove_siblinginfo() which gets called
before cpu_smpboot_free(). This means calling cpu_to_socket(cpu) in
cpu_smpboot_free() is not possible to be correct.

This patch invokes remove_siblinginfo() in cpu_smpboot_free(),
immediately after the use for cpu_to_socket(cpu).

The clearing of cpu_{core,sibling}_mask in remove_siblinginfo() is also
removed as now both masks will get freed afterwards so clearing is
useless.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
Reported-by: Dario Faggioli <dario.faggioli@citrix.com>
---
 xen/arch/x86/smpboot.c | 55 +++++++++++++++++++++++++-------------------------
 1 file changed, 27 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 0f03364..ededa1c 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -320,6 +320,29 @@ static void set_cpu_sibling_map(int cpu)
     }
 }
 
+static void
+remove_siblinginfo(int cpu)
+{
+    int sibling;
+    struct cpuinfo_x86 *c = cpu_data;
+
+    for_each_cpu ( sibling, per_cpu(cpu_core_mask, cpu) )
+    {
+        cpumask_clear_cpu(cpu, per_cpu(cpu_core_mask, sibling));
+        /* Last thread sibling in this cpu core going down. */
+        if ( cpumask_weight(per_cpu(cpu_sibling_mask, cpu)) == 1 )
+            c[sibling].booted_cores--;
+    }
+
+    for_each_cpu(sibling, per_cpu(cpu_sibling_mask, cpu))
+        cpumask_clear_cpu(cpu, per_cpu(cpu_sibling_mask, sibling));
+    c[cpu].phys_proc_id = XEN_INVALID_SOCKET_ID;
+    c[cpu].cpu_core_id = XEN_INVALID_CORE_ID;
+    c[cpu].compute_unit_id = INVALID_CUID;
+    cpumask_clear_cpu(cpu, &cpu_sibling_setup_map);
+}
+
+
 void start_secondary(void *unused)
 {
     /*
@@ -667,12 +690,16 @@ static void cpu_smpboot_free(unsigned int cpu)
 {
     unsigned int order, socket = cpu_to_socket(cpu);
 
+    cpumask_clear_cpu(cpu, socket_cpumask[socket]);
+
     if ( cpumask_empty(socket_cpumask[socket]) )
     {
         xfree(socket_cpumask[socket]);
         socket_cpumask[socket] = NULL;
     }
 
+    remove_siblinginfo(cpu);
+
     free_cpumask_var(per_cpu(cpu_sibling_mask, cpu));
     free_cpumask_var(per_cpu(cpu_core_mask, cpu));
 
@@ -878,32 +905,6 @@ void __init smp_prepare_boot_cpu(void)
     cpumask_set_cpu(smp_processor_id(), &cpu_present_map);
 }
 
-static void
-remove_siblinginfo(int cpu)
-{
-    int sibling;
-    struct cpuinfo_x86 *c = cpu_data;
-
-    cpumask_clear_cpu(cpu, socket_cpumask[cpu_to_socket(cpu)]);
-
-    for_each_cpu ( sibling, per_cpu(cpu_core_mask, cpu) )
-    {
-        cpumask_clear_cpu(cpu, per_cpu(cpu_core_mask, sibling));
-        /* Last thread sibling in this cpu core going down. */
-        if ( cpumask_weight(per_cpu(cpu_sibling_mask, cpu)) == 1 )
-            c[sibling].booted_cores--;
-    }
-   
-    for_each_cpu(sibling, per_cpu(cpu_sibling_mask, cpu))
-        cpumask_clear_cpu(cpu, per_cpu(cpu_sibling_mask, sibling));
-    cpumask_clear(per_cpu(cpu_sibling_mask, cpu));
-    cpumask_clear(per_cpu(cpu_core_mask, cpu));
-    c[cpu].phys_proc_id = XEN_INVALID_SOCKET_ID;
-    c[cpu].cpu_core_id = XEN_INVALID_CORE_ID;
-    c[cpu].compute_unit_id = INVALID_CUID;
-    cpumask_clear_cpu(cpu, &cpu_sibling_setup_map);
-}
-
 void __cpu_disable(void)
 {
     int cpu = smp_processor_id();
@@ -919,8 +920,6 @@ void __cpu_disable(void)
 
     time_suspend();
 
-    remove_siblinginfo(cpu);
-
     /* It's now safe to remove this processor from the online map */
     cpumask_clear_cpu(cpu, &cpu_online_map);
     fixup_irqs();
-- 
1.9.1

Re: [PATCH] x86: avoid invalid phys_proc_id reference

[Jan Beulich]

>>> On 13.07.15 at 05:36, <chao.p.peng@linux.intel.com> wrote:
> phys_proc_id is invalidated in remove_siblinginfo() which gets called
> before cpu_smpboot_free(). This means calling cpu_to_socket(cpu) in
> cpu_smpboot_free() is not possible to be correct.
> 
> This patch invokes remove_siblinginfo() in cpu_smpboot_free(),
> immediately after the use for cpu_to_socket(cpu).

You having picked that variant of the two I proposed, did you verify
that (as I said when talking about the alternative) there are no
hidden dependencies? If you didn't, or if for whatever else reason
there is any doubt, the less intrusive variant should be chosen at
least for now.

Jan

Re: [PATCH] x86: avoid invalid phys_proc_id reference

[Chao Peng]

On Mon, Jul 13, 2015 at 09:55:39AM +0100, Jan Beulich wrote:
> >>> On 13.07.15 at 05:36, <chao.p.peng@linux.intel.com> wrote:
> > phys_proc_id is invalidated in remove_siblinginfo() which gets called
> > before cpu_smpboot_free(). This means calling cpu_to_socket(cpu) in
> > cpu_smpboot_free() is not possible to be correct.
> > 
> > This patch invokes remove_siblinginfo() in cpu_smpboot_free(),
> > immediately after the use for cpu_to_socket(cpu).
> 
> You having picked that variant of the two I proposed, did you verify
> that (as I said when talking about the alternative) there are no
> hidden dependencies? If you didn't, or if for whatever else reason
> there is any doubt, the less intrusive variant should be chosen at
> least for now.

I just did some basic tests but I don't think I can conclude that I
verified all the cases.

Because of this, I'm glad to follow your advice to have a gentle fix.

Chao