[PATCH] ARM: Fix a race in the vfp_notifier() function on SMP systems

[PATCH] ARM: Fix a race in the vfp_notifier() function on SMP systems

[Catalin Marinas]

	(patch updated following Russell's changes to vfpmodule.c)

The vfp_notifier(THREAD_NOTIFY_RELEASE) maybe be called with thread->cpu
different from the current one, causing a race condition with both the
THREAD_NOTIFY_SWITCH path and vfp_support_entry().

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm/vfp/vfpmodule.c |   23 ++++++++++++++++++++---
 1 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index aed05bc..2828767 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -69,8 +69,19 @@ static void vfp_thread_release(struct thread_info *thread)
 	union vfp_state *vfp = &thread->vfpstate;
 	unsigned int cpu = thread->cpu;
 
+#ifndef CONFIG_SMP
 	if (last_VFP_context[cpu] == vfp)
 		last_VFP_context[cpu] = NULL;
+#else
+	/*
+	 * Since release_thread() may be called from a different CPU, we use
+	 * cmpxchg() here to avoid a race with the vfp_support_entry() code
+	 * which modifies last_VFP_context[cpu]. Note that on SMP systems, a
+	 * STR instruction on a different CPU clears the global exclusive
+	 * monitor state.
+	 */
+	(void)cmpxchg(&last_VFP_context[cpu], vfp, NULL);
+#endif
 }
 
 /*
@@ -105,13 +116,19 @@ static int vfp_notifier(struct notifier_block *self, unsigned long cmd, void *v)
 		unsigned int cpu = thread->cpu;
 
 		/*
+		 * The vfpstate structure pointed to by last_VFP_context[cpu]
+		 * may be released via call_rcu(delayed_put_task_struct) but
+		 * atomic_notifier_call_chain() already holds the RCU lock.
+		 */
+		vfp = last_VFP_context[cpu];
+		/*
 		 * On SMP, if VFP is enabled, save the old state in
 		 * case the thread migrates to a different CPU. The
 		 * restoring is done lazily.
 		 */
-		if ((fpexc & FPEXC_EN) && last_VFP_context[cpu]) {
-			vfp_save_state(last_VFP_context[cpu], fpexc);
-			last_VFP_context[cpu]->hard.cpu = cpu;
+		if ((fpexc & FPEXC_EN) && vfp) {
+			vfp_save_state(vfp, fpexc);
+			vfp->hard.cpu = cpu;
 		}
 		/*
 		 * Thread migration, just force the reloading of the

[PATCH] ARM: Fix a race in the vfp_notifier() function on SMP systems

[Russell King - ARM Linux]

On Fri, Dec 18, 2009 at 01:45:09PM +0000, Catalin Marinas wrote:
> 	(patch updated following Russell's changes to vfpmodule.c)
> 
> The vfp_notifier(THREAD_NOTIFY_RELEASE) maybe be called with thread->cpu
> different from the current one, causing a race condition with both the
> THREAD_NOTIFY_SWITCH path and vfp_support_entry().

How about we provide THREAD_NOTIFY_EXIT and call these hooks from
exit_thread() - we'll be calling the notifier when the thread is
still running, and so thread->cpu will be the local CPU.

This should be much safer all round, and give much simpler semantics
if we NULL out the current CPU's last_VFP_context pointer.  It also
means that each CPUs last_VFP_context pointer is only ever accessed
from the local CPU, which can only be a good thing.

[PATCH] ARM: Fix a race in the vfp_notifier() function on SMPsystems

[Catalin Marinas]

On Fri, 2009-12-18 at 14:11 +0000, Russell King - ARM Linux wrote:
> On Fri, Dec 18, 2009 at 01:45:09PM +0000, Catalin Marinas wrote:
> >       (patch updated following Russell's changes to vfpmodule.c)
> >
> > The vfp_notifier(THREAD_NOTIFY_RELEASE) maybe be called with thread->cpu
> > different from the current one, causing a race condition with both the
> > THREAD_NOTIFY_SWITCH path and vfp_support_entry().
> 
> How about we provide THREAD_NOTIFY_EXIT and call these hooks from
> exit_thread() - we'll be calling the notifier when the thread is
> still running, and so thread->cpu will be the local CPU.
> 
> This should be much safer all round, and give much simpler semantics
> if we NULL out the current CPU's last_VFP_context pointer.  It also
> means that each CPUs last_VFP_context pointer is only ever accessed
> from the local CPU, which can only be a good thing.

It sounds like a clean workaround. I'm fine with this approach.

-- 
Catalin

[PATCH] ARM: Fix a race in the vfp_notifier() function on SMP systems

[Russell King - ARM Linux]

On Fri, Dec 18, 2009 at 02:11:00PM +0000, Russell King - ARM Linux wrote:
> On Fri, Dec 18, 2009 at 01:45:09PM +0000, Catalin Marinas wrote:
> > 	(patch updated following Russell's changes to vfpmodule.c)
> > 
> > The vfp_notifier(THREAD_NOTIFY_RELEASE) maybe be called with thread->cpu
> > different from the current one, causing a race condition with both the
> > THREAD_NOTIFY_SWITCH path and vfp_support_entry().
> 
> How about we provide THREAD_NOTIFY_EXIT and call these hooks from
> exit_thread() - we'll be calling the notifier when the thread is
> still running, and so thread->cpu will be the local CPU.
> 
> This should be much safer all round, and give much simpler semantics
> if we NULL out the current CPU's last_VFP_context pointer.  It also
> means that each CPUs last_VFP_context pointer is only ever accessed
> from the local CPU, which can only be a good thing.

Something like this:

diff --git a/arch/arm/include/asm/thread_notify.h b/arch/arm/include/asm/thread_notify.h
index f27379d..868bb39 100644
--- a/arch/arm/include/asm/thread_notify.h
+++ b/arch/arm/include/asm/thread_notify.h
@@ -43,6 +43,7 @@ static inline void thread_notify(unsigned long rc, struct thread_info *thread)
 #define THREAD_NOTIFY_FLUSH	0
 #define THREAD_NOTIFY_RELEASE	1
 #define THREAD_NOTIFY_SWITCH	2
+#define THREAD_NOTIFY_EXIT	3
 
 #endif
 #endif
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 0d96d01..c584e9f 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -274,17 +274,18 @@ void show_regs(struct pt_regs * regs)
 	__backtrace();
 }
 
+ATOMIC_NOTIFIER_HEAD(thread_notify_head);
+
+EXPORT_SYMBOL_GPL(thread_notify_head);
+
 /*
  * Free current thread data structures etc..
  */
 void exit_thread(void)
 {
+	thread_notify(THREAD_NOTIFY_EXIT, current_thread_info());
 }
 
-ATOMIC_NOTIFIER_HEAD(thread_notify_head);
-
-EXPORT_SYMBOL_GPL(thread_notify_head);
-
 void flush_thread(void)
 {
 	struct thread_info *thread = current_thread_info();
diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index aed05bc..7b95cdc 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -63,14 +63,15 @@ static void vfp_thread_flush(struct thread_info *thread)
 	put_cpu();
 }
 
-static void vfp_thread_release(struct thread_info *thread)
+static void vfp_thread_exit(struct thread_info *thread)
 {
 	/* release case: Per-thread VFP cleanup. */
 	union vfp_state *vfp = &thread->vfpstate;
-	unsigned int cpu = thread->cpu;
+	unsigned int cpu = get_cpu();
 
 	if (last_VFP_context[cpu] == vfp)
 		last_VFP_context[cpu] = NULL;
+	put_cpu();
 }
 
 /*
@@ -93,6 +94,13 @@ static void vfp_thread_release(struct thread_info *thread)
  *   - thread->cpu will be the last CPU the thread ran on, which may not
  *	be the current CPU.
  *   - we could be preempted if tree preempt rcu is enabled.
+ *  THREAD_NOTIFY_EXIT
+ *   - the thread (v) will be running on the local CPU, so
+ *	v === current_thread_info()
+ *   - thread->cpu is the local CPU number at the time it is accessed,
+ *	but may change at any time.
+ *   - we could be preempted if tree preempt rcu is enabled, so
+ *	it is unsafe to use thread->cpu.
  */
 static int vfp_notifier(struct notifier_block *self, unsigned long cmd, void *v)
 {
@@ -132,8 +140,8 @@ static int vfp_notifier(struct notifier_block *self, unsigned long cmd, void *v)
 
 	if (cmd == THREAD_NOTIFY_FLUSH)
 		vfp_thread_flush(thread);
-	else
-		vfp_thread_release(thread);
+	else if (cmd == THREAD_NOTIFY_EXIT)
+		vfp_thread_exit(thread);
 
 	return NOTIFY_DONE;
 }

[PATCH] ARM: Fix a race in the vfp_notifier() function on SMP systems

[Russell King - ARM Linux]

On Fri, Dec 18, 2009 at 02:25:03PM +0000, Russell King - ARM Linux wrote:
> On Fri, Dec 18, 2009 at 02:11:00PM +0000, Russell King - ARM Linux wrote:
> > On Fri, Dec 18, 2009 at 01:45:09PM +0000, Catalin Marinas wrote:
> > > 	(patch updated following Russell's changes to vfpmodule.c)
> > > 
> > > The vfp_notifier(THREAD_NOTIFY_RELEASE) maybe be called with thread->cpu
> > > different from the current one, causing a race condition with both the
> > > THREAD_NOTIFY_SWITCH path and vfp_support_entry().
> > 
> > How about we provide THREAD_NOTIFY_EXIT and call these hooks from
> > exit_thread() - we'll be calling the notifier when the thread is
> > still running, and so thread->cpu will be the local CPU.
> > 
> > This should be much safer all round, and give much simpler semantics
> > if we NULL out the current CPU's last_VFP_context pointer.  It also
> > means that each CPUs last_VFP_context pointer is only ever accessed
> > from the local CPU, which can only be a good thing.
> 
> Something like this:

Actually, I think we should go further and kill off THREAD_NOTIFY_RELEASE
completely.  Added a few more people because EP93xx and PXA (well,
Xscale) is now impacted by this change.

[PATCH] ARM: Convert VFP/Crunch/XscaleCP thread_release() to exit_thread()

This avoids races in the VFP code where the dead thread may have
state on another CPU.  By moving this code to exit_thread(), we
will be running as the thread, and therefore be running on the
current CPU.

This means that we can ensure that the only local state is accessed
in the thread notifiers.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/thread_notify.h |    2 +-
 arch/arm/kernel/crunch.c             |    2 +-
 arch/arm/kernel/process.c            |   12 +++++-------
 arch/arm/kernel/xscale-cp0.c         |    2 +-
 arch/arm/vfp/vfpmodule.c             |   21 ++++++++++++---------
 5 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/arch/arm/include/asm/thread_notify.h b/arch/arm/include/asm/thread_notify.h
index f27379d..c4391ba 100644
--- a/arch/arm/include/asm/thread_notify.h
+++ b/arch/arm/include/asm/thread_notify.h
@@ -41,7 +41,7 @@ static inline void thread_notify(unsigned long rc, struct thread_info *thread)
  * These are the reason codes for the thread notifier.
  */
 #define THREAD_NOTIFY_FLUSH	0
-#define THREAD_NOTIFY_RELEASE	1
+#define THREAD_NOTIFY_EXIT	1
 #define THREAD_NOTIFY_SWITCH	2
 
 #endif
diff --git a/arch/arm/kernel/crunch.c b/arch/arm/kernel/crunch.c
index 769abe1..25ef223 100644
--- a/arch/arm/kernel/crunch.c
+++ b/arch/arm/kernel/crunch.c
@@ -51,7 +51,7 @@ static int crunch_do(struct notifier_block *self, unsigned long cmd, void *t)
 		 * initialised state information on the first fault.
 		 */
 
-	case THREAD_NOTIFY_RELEASE:
+	case THREAD_NOTIFY_EXIT:
 		crunch_task_release(thread);
 		break;
 
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 0d96d01..6730413 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -274,17 +274,18 @@ void show_regs(struct pt_regs * regs)
 	__backtrace();
 }
 
+ATOMIC_NOTIFIER_HEAD(thread_notify_head);
+
+EXPORT_SYMBOL_GPL(thread_notify_head);
+
 /*
  * Free current thread data structures etc..
  */
 void exit_thread(void)
 {
+	thread_notify(THREAD_NOTIFY_EXIT, current_thread_info());
 }
 
-ATOMIC_NOTIFIER_HEAD(thread_notify_head);
-
-EXPORT_SYMBOL_GPL(thread_notify_head);
-
 void flush_thread(void)
 {
 	struct thread_info *thread = current_thread_info();
@@ -299,9 +300,6 @@ void flush_thread(void)
 
 void release_thread(struct task_struct *dead_task)
 {
-	struct thread_info *thread = task_thread_info(dead_task);
-
-	thread_notify(THREAD_NOTIFY_RELEASE, thread);
 }
 
 asmlinkage void ret_from_fork(void) __asm__("ret_from_fork");
diff --git a/arch/arm/kernel/xscale-cp0.c b/arch/arm/kernel/xscale-cp0.c
index 17127db..1796157 100644
--- a/arch/arm/kernel/xscale-cp0.c
+++ b/arch/arm/kernel/xscale-cp0.c
@@ -70,7 +70,7 @@ static int iwmmxt_do(struct notifier_block *self, unsigned long cmd, void *t)
 		 * initialised state information on the first fault.
 		 */
 
-	case THREAD_NOTIFY_RELEASE:
+	case THREAD_NOTIFY_EXIT:
 		iwmmxt_task_release(thread);
 		break;
 
diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index aed05bc..3b0a5dd 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -63,14 +63,15 @@ static void vfp_thread_flush(struct thread_info *thread)
 	put_cpu();
 }
 
-static void vfp_thread_release(struct thread_info *thread)
+static void vfp_thread_exit(struct thread_info *thread)
 {
 	/* release case: Per-thread VFP cleanup. */
 	union vfp_state *vfp = &thread->vfpstate;
-	unsigned int cpu = thread->cpu;
+	unsigned int cpu = get_cpu();
 
 	if (last_VFP_context[cpu] == vfp)
 		last_VFP_context[cpu] = NULL;
+	put_cpu();
 }
 
 /*
@@ -88,11 +89,13 @@ static void vfp_thread_release(struct thread_info *thread)
  *	but may change at any time.
  *   - we could be preempted if tree preempt rcu is enabled, so
  *	it is unsafe to use thread->cpu.
- *  THREAD_NOTIFY_RELEASE:
- *   - the thread (v) will not be running on any CPU; it is a dead thread.
- *   - thread->cpu will be the last CPU the thread ran on, which may not
- *	be the current CPU.
- *   - we could be preempted if tree preempt rcu is enabled.
+ *  THREAD_NOTIFY_EXIT
+ *   - the thread (v) will be running on the local CPU, so
+ *	v === current_thread_info()
+ *   - thread->cpu is the local CPU number at the time it is accessed,
+ *	but may change at any time.
+ *   - we could be preempted if tree preempt rcu is enabled, so
+ *	it is unsafe to use thread->cpu.
  */
 static int vfp_notifier(struct notifier_block *self, unsigned long cmd, void *v)
 {
@@ -132,8 +135,8 @@ static int vfp_notifier(struct notifier_block *self, unsigned long cmd, void *v)
 
 	if (cmd == THREAD_NOTIFY_FLUSH)
 		vfp_thread_flush(thread);
-	else
-		vfp_thread_release(thread);
+	else if (cmd == THREAD_NOTIFY_EXIT)
+		vfp_thread_exit(thread);
 
 	return NOTIFY_DONE;
 }
-- 
1.6.2.5

[PATCH] ARM: Fix a race in the vfp_notifier() function on SMPsystems

[Catalin Marinas]

On Fri, 2009-12-18 at 14:47 +0000, Russell King - ARM Linux wrote:
> On Fri, Dec 18, 2009 at 02:25:03PM +0000, Russell King - ARM Linux wrote:
> > On Fri, Dec 18, 2009 at 02:11:00PM +0000, Russell King - ARM Linux wrote:
> > > On Fri, Dec 18, 2009 at 01:45:09PM +0000, Catalin Marinas wrote:
> > > >   (patch updated following Russell's changes to vfpmodule.c)
> > > >
> > > > The vfp_notifier(THREAD_NOTIFY_RELEASE) maybe be called with thread->cpu
> > > > different from the current one, causing a race condition with both the
> > > > THREAD_NOTIFY_SWITCH path and vfp_support_entry().
> > >
> > > How about we provide THREAD_NOTIFY_EXIT and call these hooks from
> > > exit_thread() - we'll be calling the notifier when the thread is
> > > still running, and so thread->cpu will be the local CPU.
> > >
> > > This should be much safer all round, and give much simpler semantics
> > > if we NULL out the current CPU's last_VFP_context pointer.  It also
> > > means that each CPUs last_VFP_context pointer is only ever accessed
> > > from the local CPU, which can only be a good thing.
> >
> > Something like this:
> 
> Actually, I think we should go further and kill off THREAD_NOTIFY_RELEASE
> completely.  Added a few more people because EP93xx and PXA (well,
> Xscale) is now impacted by this change.
> 
> [PATCH] ARM: Convert VFP/Crunch/XscaleCP thread_release() to exit_thread()
> 
> This avoids races in the VFP code where the dead thread may have
> state on another CPU.  By moving this code to exit_thread(), we
> will be running as the thread, and therefore be running on the
> current CPU.
> 
> This means that we can ensure that the only local state is accessed
> in the thread notifiers.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
[...]
>  static int vfp_notifier(struct notifier_block *self, unsigned long cmd, void *v)
>  {
> @@ -132,8 +135,8 @@ static int vfp_notifier(struct notifier_block *self, unsigned long cmd, void *v)
> 
>         if (cmd == THREAD_NOTIFY_FLUSH)
>                 vfp_thread_flush(thread);
> -       else
> -               vfp_thread_release(thread);
> +       else if (cmd == THREAD_NOTIFY_EXIT)
> +               vfp_thread_exit(thread);

Do we still need the second "if" or "else" will do (though it's safer if
anyone adds some extra THREAD_NOTIFY_* in the future.

In any case (for the VFP code):

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

-- 
Catalin

[PATCH] ARM: Fix a race in the vfp_notifier() function on SMPsystems

[Russell King - ARM Linux]

On Fri, Dec 18, 2009 at 02:51:41PM +0000, Catalin Marinas wrote:
> On Fri, 2009-12-18 at 14:47 +0000, Russell King - ARM Linux wrote:
> > Actually, I think we should go further and kill off THREAD_NOTIFY_RELEASE
> > completely.  Added a few more people because EP93xx and PXA (well,
> > Xscale) is now impacted by this change.
> > 
> > [PATCH] ARM: Convert VFP/Crunch/XscaleCP thread_release() to exit_thread()
> > 
> > This avoids races in the VFP code where the dead thread may have
> > state on another CPU.  By moving this code to exit_thread(), we
> > will be running as the thread, and therefore be running on the
> > current CPU.
> > 
> > This means that we can ensure that the only local state is accessed
> > in the thread notifiers.
> > 
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> [...]
> >  static int vfp_notifier(struct notifier_block *self, unsigned long cmd, void *v)
> >  {
> > @@ -132,8 +135,8 @@ static int vfp_notifier(struct notifier_block *self, unsigned long cmd, void *v)
> > 
> >         if (cmd == THREAD_NOTIFY_FLUSH)
> >                 vfp_thread_flush(thread);
> > -       else
> > -               vfp_thread_release(thread);
> > +       else if (cmd == THREAD_NOTIFY_EXIT)
> > +               vfp_thread_exit(thread);
> 
> Do we still need the second "if" or "else" will do (though it's safer if
> anyone adds some extra THREAD_NOTIFY_* in the future.

The previous patch iteration did need it, but this done doesn't.  Killed.

[PATCH] ARM: Fix a race in the vfp_notifier() function on SMP systems

[Ryan Mallon]

Russell King - ARM Linux wrote:
> On Fri, Dec 18, 2009 at 02:25:03PM +0000, Russell King - ARM Linux wrote:
>> On Fri, Dec 18, 2009 at 02:11:00PM +0000, Russell King - ARM Linux wrote:
>>> On Fri, Dec 18, 2009 at 01:45:09PM +0000, Catalin Marinas wrote:
>>>> 	(patch updated following Russell's changes to vfpmodule.c)
>>>>
>>>> The vfp_notifier(THREAD_NOTIFY_RELEASE) maybe be called with thread->cpu
>>>> different from the current one, causing a race condition with both the
>>>> THREAD_NOTIFY_SWITCH path and vfp_support_entry().
>>> How about we provide THREAD_NOTIFY_EXIT and call these hooks from
>>> exit_thread() - we'll be calling the notifier when the thread is
>>> still running, and so thread->cpu will be the local CPU.
>>>
>>> This should be much safer all round, and give much simpler semantics
>>> if we NULL out the current CPU's last_VFP_context pointer.  It also
>>> means that each CPUs last_VFP_context pointer is only ever accessed
>>> from the local CPU, which can only be a good thing.
>> Something like this:
> 
> Actually, I think we should go further and kill off THREAD_NOTIFY_RELEASE
> completely.  Added a few more people because EP93xx and PXA (well,
> Xscale) is now impacted by this change.

Hi Russell,

I haven't been following this discussion. Can you explain how I can test
to see if this works correctly, and I'll try and find some time to run it.

~Ryan

[PATCH] ARM: Fix a race in the vfp_notifier() function on SMPsystems

[Dirk Behme]

On 18.12.2009 15:53, Russell King - ARM Linux wrote:
> On Fri, Dec 18, 2009 at 02:51:41PM +0000, Catalin Marinas wrote:
>> On Fri, 2009-12-18 at 14:47 +0000, Russell King - ARM Linux wrote:
>>> Actually, I think we should go further and kill off THREAD_NOTIFY_RELEASE
>>> completely.  Added a few more people because EP93xx and PXA (well,
>>> Xscale) is now impacted by this change.
>>>
>>> [PATCH] ARM: Convert VFP/Crunch/XscaleCP thread_release() to exit_thread()
>>>
>>> This avoids races in the VFP code where the dead thread may have
>>> state on another CPU.  By moving this code to exit_thread(), we
>>> will be running as the thread, and therefore be running on the
>>> current CPU.
>>>
>>> This means that we can ensure that the only local state is accessed
>>> in the thread notifiers.
>>>
>>> Signed-off-by: Russell King<rmk+kernel@arm.linux.org.uk>
>> [...]
>>>   static int vfp_notifier(struct notifier_block *self, unsigned long cmd, void *v)
>>>   {
>>> @@ -132,8 +135,8 @@ static int vfp_notifier(struct notifier_block *self, unsigned long cmd, void *v)
>>>
>>>          if (cmd == THREAD_NOTIFY_FLUSH)
>>>                  vfp_thread_flush(thread);
>>> -       else
>>> -               vfp_thread_release(thread);
>>> +       else if (cmd == THREAD_NOTIFY_EXIT)
>>> +               vfp_thread_exit(thread);
>>
>> Do we still need the second "if" or "else" will do (though it's safer if
>> anyone adds some extra THREAD_NOTIFY_* in the future.
>
> The previous patch iteration did need it, but this done doesn't.  Killed.

Sorry if I missed something, but any hint where to find the latest 
version of the patch for testing?

Many thanks and best regards

Dirk

[PATCH] ARM: Fix a race in the vfp_notifier() function on SMPsystems

[Russell King - ARM Linux]

On Sat, Dec 19, 2009 at 09:20:44AM +0100, Dirk Behme wrote:
> On 18.12.2009 15:53, Russell King - ARM Linux wrote:
>> The previous patch iteration did need it, but this done doesn't.  Killed.
>
> Sorry if I missed something, but any hint where to find the latest  
> version of the patch for testing?

Attached, can also be found in my git tree, in the master branch.

diff --git a/arch/arm/include/asm/thread_notify.h b/arch/arm/include/asm/thread_notify.h
index f27379d..c4391ba 100644
--- a/arch/arm/include/asm/thread_notify.h
+++ b/arch/arm/include/asm/thread_notify.h
@@ -41,7 +41,7 @@ static inline void thread_notify(unsigned long rc, struct thread_info *thread)
  * These are the reason codes for the thread notifier.
  */
 #define THREAD_NOTIFY_FLUSH	0
-#define THREAD_NOTIFY_RELEASE	1
+#define THREAD_NOTIFY_EXIT	1
 #define THREAD_NOTIFY_SWITCH	2
 
 #endif
diff --git a/arch/arm/kernel/crunch.c b/arch/arm/kernel/crunch.c
index 769abe1..25ef223 100644
--- a/arch/arm/kernel/crunch.c
+++ b/arch/arm/kernel/crunch.c
@@ -51,7 +51,7 @@ static int crunch_do(struct notifier_block *self, unsigned long cmd, void *t)
 		 * initialised state information on the first fault.
 		 */
 
-	case THREAD_NOTIFY_RELEASE:
+	case THREAD_NOTIFY_EXIT:
 		crunch_task_release(thread);
 		break;
 
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 0d96d01..6730413 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -274,17 +274,18 @@ void show_regs(struct pt_regs * regs)
 	__backtrace();
 }
 
+ATOMIC_NOTIFIER_HEAD(thread_notify_head);
+
+EXPORT_SYMBOL_GPL(thread_notify_head);
+
 /*
  * Free current thread data structures etc..
  */
 void exit_thread(void)
 {
+	thread_notify(THREAD_NOTIFY_EXIT, current_thread_info());
 }
 
-ATOMIC_NOTIFIER_HEAD(thread_notify_head);
-
-EXPORT_SYMBOL_GPL(thread_notify_head);
-
 void flush_thread(void)
 {
 	struct thread_info *thread = current_thread_info();
@@ -299,9 +300,6 @@ void flush_thread(void)
 
 void release_thread(struct task_struct *dead_task)
 {
-	struct thread_info *thread = task_thread_info(dead_task);
-
-	thread_notify(THREAD_NOTIFY_RELEASE, thread);
 }
 
 asmlinkage void ret_from_fork(void) __asm__("ret_from_fork");
diff --git a/arch/arm/kernel/xscale-cp0.c b/arch/arm/kernel/xscale-cp0.c
index 17127db..1796157 100644
--- a/arch/arm/kernel/xscale-cp0.c
+++ b/arch/arm/kernel/xscale-cp0.c
@@ -70,7 +70,7 @@ static int iwmmxt_do(struct notifier_block *self, unsigned long cmd, void *t)
 		 * initialised state information on the first fault.
 		 */
 
-	case THREAD_NOTIFY_RELEASE:
+	case THREAD_NOTIFY_EXIT:
 		iwmmxt_task_release(thread);
 		break;
 
diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index aed05bc..f60a540 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -63,14 +63,15 @@ static void vfp_thread_flush(struct thread_info *thread)
 	put_cpu();
 }
 
-static void vfp_thread_release(struct thread_info *thread)
+static void vfp_thread_exit(struct thread_info *thread)
 {
 	/* release case: Per-thread VFP cleanup. */
 	union vfp_state *vfp = &thread->vfpstate;
-	unsigned int cpu = thread->cpu;
+	unsigned int cpu = get_cpu();
 
 	if (last_VFP_context[cpu] == vfp)
 		last_VFP_context[cpu] = NULL;
+	put_cpu();
 }
 
 /*
@@ -88,11 +89,13 @@ static void vfp_thread_release(struct thread_info *thread)
  *	but may change at any time.
  *   - we could be preempted if tree preempt rcu is enabled, so
  *	it is unsafe to use thread->cpu.
- *  THREAD_NOTIFY_RELEASE:
- *   - the thread (v) will not be running on any CPU; it is a dead thread.
- *   - thread->cpu will be the last CPU the thread ran on, which may not
- *	be the current CPU.
- *   - we could be preempted if tree preempt rcu is enabled.
+ *  THREAD_NOTIFY_EXIT
+ *   - the thread (v) will be running on the local CPU, so
+ *	v === current_thread_info()
+ *   - thread->cpu is the local CPU number at the time it is accessed,
+ *	but may change at any time.
+ *   - we could be preempted if tree preempt rcu is enabled, so
+ *	it is unsafe to use thread->cpu.
  */
 static int vfp_notifier(struct notifier_block *self, unsigned long cmd, void *v)
 {
@@ -133,7 +136,7 @@ static int vfp_notifier(struct notifier_block *self, unsigned long cmd, void *v)
 	if (cmd == THREAD_NOTIFY_FLUSH)
 		vfp_thread_flush(thread);
 	else
-		vfp_thread_release(thread);
+		vfp_thread_exit(thread);
 
 	return NOTIFY_DONE;
 }