[PATCH] ARM: Fix subtle race in CPU pen_release hotplug code

[PATCH] ARM: Fix subtle race in CPU pen_release hotplug code

[Russell King - ARM Linux]

There is a subtle race in the CPU hotplug code, where a CPU which has
been offlined can online itself before being requested, which results
in things going astray on the next online/offline cycle.

What happens in the normal online/offline/online cycle is:

	CPU0			CPU3
	requests boot of CPU3
	pen_release = 3
	flush cache line
				checks pen_release, reads 3
				starts boot
				pen_release = -1
	... requests CPU3 offline ...
				... dies ...
				checks pen_release, reads -1
	requests boot of CPU3
	pen_release = 3
	flush cache line
				checks pen_release, reads 3
				starts boot
				pen_release = -1

However, as the write of -1 of pen_release is not fully flushed back to
memory, and the checking of pen_release is done with caches disabled,
this allows CPU3 the opportunity to read the old value of pen_release:

	CPU0			CPU3
	requests boot of CPU3
	pen_release = 3
	flush cache line
				checks pen_release, reads 3
				starts boot
				pen_release = -1
	... requests CPU3 offline ...
				... dies ...
				checks pen_release, reads 3
				starts boot
				pen_release = -1
	requests boot of CPU3
	pen_release = 3
	flush cache line

Fix this by grouping the write of pen_release along with its cache line
flushing code to ensure that any update to pen_release is always pushed
out to physical memory.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/mach-realview/platsmp.c |   19 +++++++++++++++----
 arch/arm/mach-s5pv310/platsmp.c  |   20 +++++++++++++++-----
 arch/arm/mach-ux500/platsmp.c    |   22 +++++++++++++++-------
 arch/arm/mach-vexpress/platsmp.c |   20 +++++++++++++++-----
 4 files changed, 60 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-realview/platsmp.c b/arch/arm/mach-realview/platsmp.c
index 226c631..bb8d6c4 100644
--- a/arch/arm/mach-realview/platsmp.c
+++ b/arch/arm/mach-realview/platsmp.c
@@ -36,6 +36,19 @@ extern void realview_secondary_startup(void);
  */
 volatile int __cpuinitdata pen_release = -1;
 
+/*
+ * Write pen_release in a way that is guaranteed to be visible to all
+ * observers, irrespective of whether they're taking part in coherency
+ * or not.  This is necessary for the hotplug code to work reliably.
+ */
+static void write_pen_release(int val)
+{
+	pen_release = val;
+	smp_wmb();
+	__cpuc_flush_dcache_area((void *)&pen_release, sizeof(pen_release));
+	outer_clean_range(__pa(&pen_release), __pa(&pen_release + 1));
+}
+
 static void __iomem *scu_base_addr(void)
 {
 	if (machine_is_realview_eb_mp())
@@ -64,8 +77,7 @@ void __cpuinit platform_secondary_init(unsigned int cpu)
 	 * let the primary processor know we're out of the
 	 * pen, then head off into the C entry point
 	 */
-	pen_release = -1;
-	smp_wmb();
+	write_pen_release(-1);
 
 	/*
 	 * Synchronise with the boot thread.
@@ -92,8 +104,7 @@ int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
 	 * Note that "pen_release" is the hardware CPU ID, whereas
 	 * "cpu" is Linux's internal ID.
 	 */
-	pen_release = cpu;
-	flush_cache_all();
+	write_pen_release(cpu);
 
 	/*
 	 * Send the secondary CPU a soft interrupt, thereby causing
diff --git a/arch/arm/mach-s5pv310/platsmp.c b/arch/arm/mach-s5pv310/platsmp.c
index 18aaf5f..98c0474 100644
--- a/arch/arm/mach-s5pv310/platsmp.c
+++ b/arch/arm/mach-s5pv310/platsmp.c
@@ -37,6 +37,19 @@ extern void s5pv310_secondary_startup(void);
 
 volatile int __cpuinitdata pen_release = -1;
 
+/*
+ * Write pen_release in a way that is guaranteed to be visible to all
+ * observers, irrespective of whether they're taking part in coherency
+ * or not.  This is necessary for the hotplug code to work reliably.
+ */
+static void write_pen_release(int val)
+{
+	pen_release = val;
+	smp_wmb();
+	__cpuc_flush_dcache_area((void *)&pen_release, sizeof(pen_release));
+	outer_clean_range(__pa(&pen_release), __pa(&pen_release + 1));
+}
+
 static void __iomem *scu_base_addr(void)
 {
 	return (void __iomem *)(S5P_VA_SCU);
@@ -57,8 +70,7 @@ void __cpuinit platform_secondary_init(unsigned int cpu)
 	 * let the primary processor know we're out of the
 	 * pen, then head off into the C entry point
 	 */
-	pen_release = -1;
-	smp_wmb();
+	write_pen_release(-1);
 
 	/*
 	 * Synchronise with the boot thread.
@@ -85,9 +97,7 @@ int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
 	 * Note that "pen_release" is the hardware CPU ID, whereas
 	 * "cpu" is Linux's internal ID.
 	 */
-	pen_release = cpu;
-	__cpuc_flush_dcache_area((void *)&pen_release, sizeof(pen_release));
-	outer_clean_range(__pa(&pen_release), __pa(&pen_release + 1));
+	write_pen_release(cpu);
 
 	/*
 	 * Send the secondary CPU a soft interrupt, thereby causing
diff --git a/arch/arm/mach-ux500/platsmp.c b/arch/arm/mach-ux500/platsmp.c
index ddedbc8..f71175a 100644
--- a/arch/arm/mach-ux500/platsmp.c
+++ b/arch/arm/mach-ux500/platsmp.c
@@ -27,6 +27,19 @@
  */
 volatile int __cpuinitdata pen_release = -1;
 
+/*
+ * Write pen_release in a way that is guaranteed to be visible to all
+ * observers, irrespective of whether they're taking part in coherency
+ * or not.  This is necessary for the hotplug code to work reliably.
+ */
+static void write_pen_release(int val)
+{
+	pen_release = val;
+	smp_wmb();
+	__cpuc_flush_dcache_area((void *)&pen_release, sizeof(pen_release));
+	outer_clean_range(__pa(&pen_release), __pa(&pen_release + 1));
+}
+
 static DEFINE_SPINLOCK(boot_lock);
 
 void __cpuinit platform_secondary_init(unsigned int cpu)
@@ -42,7 +55,7 @@ void __cpuinit platform_secondary_init(unsigned int cpu)
 	 * let the primary processor know we're out of the
 	 * pen, then head off into the C entry point
 	 */
-	pen_release = -1;
+	write_pen_release(-1);
 
 	/*
 	 * Synchronise with the boot thread.
@@ -66,9 +79,7 @@ int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
 	 * the holding pen - release it, then wait for it to flag
 	 * that it has been released by resetting pen_release.
 	 */
-	pen_release = cpu;
-	__cpuc_flush_dcache_area((void *)&pen_release, sizeof(pen_release));
-	outer_clean_range(__pa(&pen_release), __pa(&pen_release) + 1);
+	write_pen_release(cpu);
 
 	smp_cross_call(cpumask_of(cpu), 1);
 
@@ -89,9 +100,6 @@ int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
 
 static void __init wakeup_secondary(void)
 {
-	/* nobody is to be released from the pen yet */
-	pen_release = -1;
-
 	/*
 	 * write the address of secondary startup into the backup ram register
 	 * at offset 0x1FF4, then write the magic number 0xA1FEED01 to the
diff --git a/arch/arm/mach-vexpress/platsmp.c b/arch/arm/mach-vexpress/platsmp.c
index d7e0cb9..8ce9fef 100644
--- a/arch/arm/mach-vexpress/platsmp.c
+++ b/arch/arm/mach-vexpress/platsmp.c
@@ -34,6 +34,19 @@ extern void vexpress_secondary_startup(void);
  */
 volatile int __cpuinitdata pen_release = -1;
 
+/*
+ * Write pen_release in a way that is guaranteed to be visible to all
+ * observers, irrespective of whether they're taking part in coherency
+ * or not.  This is necessary for the hotplug code to work reliably.
+ */
+static void write_pen_release(int val)
+{
+	pen_release = val;
+	smp_wmb();
+	__cpuc_flush_dcache_area((void *)&pen_release, sizeof(pen_release));
+	outer_clean_range(__pa(&pen_release), __pa(&pen_release + 1));
+}
+
 static void __iomem *scu_base_addr(void)
 {
 	return MMIO_P2V(A9_MPCORE_SCU);
@@ -54,8 +67,7 @@ void __cpuinit platform_secondary_init(unsigned int cpu)
 	 * let the primary processor know we're out of the
 	 * pen, then head off into the C entry point
 	 */
-	pen_release = -1;
-	smp_wmb();
+	write_pen_release(-1);
 
 	/*
 	 * Synchronise with the boot thread.
@@ -80,9 +92,7 @@ int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
 	 * since we haven't sent them a soft interrupt, they shouldn't
 	 * be there.
 	 */
-	pen_release = cpu;
-	__cpuc_flush_dcache_area((void *)&pen_release, sizeof(pen_release));
-	outer_clean_range(__pa(&pen_release), __pa(&pen_release + 1));
+	write_pen_release(cpu);
 
 	/*
 	 * Send the secondary CPU a soft interrupt, thereby causing
-- 
1.6.2.5

[PATCH] ARM: Fix subtle race in CPU pen_release hotplug code

[Catalin Marinas]

On 18 December 2010 11:04, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> There is a subtle race in the CPU hotplug code, where a CPU which has
> been offlined can online itself before being requested, which results
> in things going astray on the next online/offline cycle.
[...]
> --- a/arch/arm/mach-realview/platsmp.c
> +++ b/arch/arm/mach-realview/platsmp.c
> @@ -36,6 +36,19 @@ extern void realview_secondary_startup(void);
> ?*/
> ?volatile int __cpuinitdata pen_release = -1;
>
> +/*
> + * Write pen_release in a way that is guaranteed to be visible to all
> + * observers, irrespective of whether they're taking part in coherency
> + * or not. ?This is necessary for the hotplug code to work reliably.
> + */
> +static void write_pen_release(int val)
> +{
> + ? ? ? pen_release = val;
> + ? ? ? smp_wmb();
> + ? ? ? __cpuc_flush_dcache_area((void *)&pen_release, sizeof(pen_release));
> + ? ? ? outer_clean_range(__pa(&pen_release), __pa(&pen_release + 1));
> +}

Just a minor thing - I don't think we need any barrier here. According
to the ARM ARM B2.2.7:

"Any data cache or unified cache maintenance operation by MVA must be
executed in program order
relative to any explicit load or store on the same processor to an
address covered by the MVA of the
cache operation."

We also have a corresponding smp_rmb() in boot_secondary(), I don't
think it has any use either.

-- 
Catalin

[PATCH] ARM: Fix subtle race in CPU pen_release hotplug code

[Russell King - ARM Linux]

On Mon, Dec 20, 2010 at 04:39:07PM +0000, Catalin Marinas wrote:
> On 18 December 2010 11:04, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > There is a subtle race in the CPU hotplug code, where a CPU which has
> > been offlined can online itself before being requested, which results
> > in things going astray on the next online/offline cycle.
> [...]
> > --- a/arch/arm/mach-realview/platsmp.c
> > +++ b/arch/arm/mach-realview/platsmp.c
> > @@ -36,6 +36,19 @@ extern void realview_secondary_startup(void);
> > ?*/
> > ?volatile int __cpuinitdata pen_release = -1;
> >
> > +/*
> > + * Write pen_release in a way that is guaranteed to be visible to all
> > + * observers, irrespective of whether they're taking part in coherency
> > + * or not. ?This is necessary for the hotplug code to work reliably.
> > + */
> > +static void write_pen_release(int val)
> > +{
> > + ? ? ? pen_release = val;
> > + ? ? ? smp_wmb();
> > + ? ? ? __cpuc_flush_dcache_area((void *)&pen_release, sizeof(pen_release));
> > + ? ? ? outer_clean_range(__pa(&pen_release), __pa(&pen_release + 1));
> > +}
> 
> Just a minor thing - I don't think we need any barrier here. According
> to the ARM ARM B2.2.7:
> 
> "Any data cache or unified cache maintenance operation by MVA must be
> executed in program order
> relative to any explicit load or store on the same processor to an
> address covered by the MVA of the
> cache operation."

Right, I had been thinking about that.  I think the barrier came from the
original ARM implementation, and I added the cache flushes.

> We also have a corresponding smp_rmb() in boot_secondary(), I don't
> think it has any use either.

I think you're right, but it looks a little unsafe without:

        timeout = jiffies + (1 * HZ);
        while (time_before(jiffies, timeout)) {
                if (pen_release == -1)
                        break;

                udelay(10);
        }

I don't think it makes much odds if the pen_release check gets
re-ordered, especially as we do a final pen_release check after we
unlock.  However, could the loop end up waiting additional 10us
without the smp_rmb() ?

[PATCH] ARM: Fix subtle race in CPU pen_release hotplug code

[Catalin Marinas]

On Mon, 2010-12-20 at 16:50 +0000, Russell King - ARM Linux wrote:
> On Mon, Dec 20, 2010 at 04:39:07PM +0000, Catalin Marinas wrote:
> > On 18 December 2010 11:04, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > > There is a subtle race in the CPU hotplug code, where a CPU which has
> > > been offlined can online itself before being requested, which results
> > > in things going astray on the next online/offline cycle.
> > [...]
> > > --- a/arch/arm/mach-realview/platsmp.c
> > > +++ b/arch/arm/mach-realview/platsmp.c
> > > @@ -36,6 +36,19 @@ extern void realview_secondary_startup(void);
> > >  */
> > >  volatile int __cpuinitdata pen_release = -1;
> > >
> > > +/*
> > > + * Write pen_release in a way that is guaranteed to be visible to all
> > > + * observers, irrespective of whether they're taking part in coherency
> > > + * or not.  This is necessary for the hotplug code to work reliably.
> > > + */
> > > +static void write_pen_release(int val)
> > > +{
> > > +       pen_release = val;
> > > +       smp_wmb();
> > > +       __cpuc_flush_dcache_area((void *)&pen_release, sizeof(pen_release));
> > > +       outer_clean_range(__pa(&pen_release), __pa(&pen_release + 1));
> > > +}
> >
> > Just a minor thing - I don't think we need any barrier here. According
> > to the ARM ARM B2.2.7:
> >
> > "Any data cache or unified cache maintenance operation by MVA must be
> > executed in program order
> > relative to any explicit load or store on the same processor to an
> > address covered by the MVA of the
> > cache operation."
> 
> Right, I had been thinking about that.  I think the barrier came from the
> original ARM implementation, and I added the cache flushes.

I think the original smp_wmb() was there to prevent the pen_release
update not being visible to the primary CPU before the spin_lock() loop
on the secondary, leading to a deadlock.

The smp_wmb() is no longer needed with your patch as we do explicit
cache flushing which contains a DSB. But for clarity, you may still want
to keep it as a separate call in platform_secondary_init() rather than
write_pen_release():

 	 * let the primary processor know we're out of the
 	 * pen, then head off into the C entry point
 	 */
-	pen_release = -1;
+	write_pen_release(-1);
 	smp_wmb();
 
 	/*
 	 * Synchronise with the boot thread.

> > We also have a corresponding smp_rmb() in boot_secondary(), I don't
> > think it has any use either.
> 
> I think you're right, but it looks a little unsafe without:
> 
>         timeout = jiffies + (1 * HZ);
>         while (time_before(jiffies, timeout)) {
>                 if (pen_release == -1)
>                         break;
> 
>                 udelay(10);
>         }
> 
> I don't think it makes much odds if the pen_release check gets
> re-ordered, especially as we do a final pen_release check after we
> unlock.  However, could the loop end up waiting additional 10us
> without the smp_rmb() ?

Looking through A3.8.2, there is only a "control dependency" between
reading the jiffies and reading the pen_release. My understanding is
that these reads could happen in any order, so to avoid an additional
10us wait the barrier is still useful.

This smp_rmb() was supposed to work in pair with the smp_wmb() above but
that's only by following the memory-barriers.txt document. From an ARM
perspective, we could simply have an smp_mb() in boot_secondary().

If we keep the smp_wmb() in platform_secondary_init() for clarity, we
could keep the smp_rmb() here as well. I think I prefer this option.

-- 
Catalin