All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] cpu/hotplug: Reset node state after operation
@ 2017-10-21 14:06 Thomas Gleixner
  2017-10-21 14:16 ` [tip:smp/urgent] " tip-bot for Thomas Gleixner
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Thomas Gleixner @ 2017-10-21 14:06 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: linux-kernel, Ingo Molnar, Peter Zijlstra,
	Sebastian Andrzej Siewior, Paul E. McKenney, Boris Ostrovsky

The recent rework of the cpu hotplug internals changed the usage of the per
cpu state->node field, but missed to clean it up after usage.

So subsequent hotplug operations use the stale pointer from a previous
operation and hand it into the callback functions. The callbacks then
dereference a pointer which either belongs to a different facility or
points to freed and potentially reused memory. In either case data
corruption and crashes are the obvious consequence.

Reset the node and the last pointers in the per cpu state to NULL after the
operation which set them has completed.

Fixes: 96abb968549c ("smp/hotplug: Allow external multi-instance rollback")
Reported-by: Tvrtko Ursulin <tursulin@ursulin.net>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/cpu.c |    5 +++++
 1 file changed, 5 insertions(+)

--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -632,6 +632,11 @@ cpuhp_invoke_ap_callback(int cpu, enum c
 		__cpuhp_kick_ap(st);
 	}
 
+	/*
+	 * Clean up the leftovers so the next hotplug operation wont use stale
+	 * data.
+	 */
+	st->node = st->last = NULL;
 	return ret;
 }
 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [tip:smp/urgent] cpu/hotplug: Reset node state after operation
  2017-10-21 14:06 [PATCH] cpu/hotplug: Reset node state after operation Thomas Gleixner
@ 2017-10-21 14:16 ` tip-bot for Thomas Gleixner
  2017-10-22  2:49 ` [PATCH] " Paul E. McKenney
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: tip-bot for Thomas Gleixner @ 2017-10-21 14:16 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: mingo, linux-kernel, tglx, peterz, paulmck, tursulin, bigeasy,
	boris.ostrovsky, hpa

Commit-ID:  1f7c70d6b2bc5de301f30456621e1161fddf4242
Gitweb:     https://git.kernel.org/tip/1f7c70d6b2bc5de301f30456621e1161fddf4242
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 21 Oct 2017 16:06:52 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sat, 21 Oct 2017 16:11:30 +0200

cpu/hotplug: Reset node state after operation

The recent rework of the cpu hotplug internals changed the usage of the per
cpu state->node field, but missed to clean it up after usage.

So subsequent hotplug operations use the stale pointer from a previous
operation and hand it into the callback functions. The callbacks then
dereference a pointer which either belongs to a different facility or
points to freed and potentially reused memory. In either case data
corruption and crashes are the obvious consequence.

Reset the node and the last pointers in the per cpu state to NULL after the
operation which set them has completed.

Fixes: 96abb968549c ("smp/hotplug: Allow external multi-instance rollback")
Reported-by: Tvrtko Ursulin <tursulin@ursulin.net>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Link: https://lkml.kernel.org/r/alpine.DEB.2.20.1710211606130.3213@nanos

---
 kernel/cpu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index d851df2..04892a8 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -632,6 +632,11 @@ cpuhp_invoke_ap_callback(int cpu, enum cpuhp_state state, bool bringup,
 		__cpuhp_kick_ap(st);
 	}
 
+	/*
+	 * Clean up the leftovers so the next hotplug operation wont use stale
+	 * data.
+	 */
+	st->node = st->last = NULL;
 	return ret;
 }
 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] cpu/hotplug: Reset node state after operation
  2017-10-21 14:06 [PATCH] cpu/hotplug: Reset node state after operation Thomas Gleixner
  2017-10-21 14:16 ` [tip:smp/urgent] " tip-bot for Thomas Gleixner
@ 2017-10-22  2:49 ` Paul E. McKenney
  2017-10-22  9:22 ` Peter Zijlstra
  2017-10-23  8:01 ` Tvrtko Ursulin
  3 siblings, 0 replies; 5+ messages in thread
From: Paul E. McKenney @ 2017-10-22  2:49 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Tvrtko Ursulin, linux-kernel, Ingo Molnar, Peter Zijlstra,
	Sebastian Andrzej Siewior, Boris Ostrovsky

On Sat, Oct 21, 2017 at 04:06:52PM +0200, Thomas Gleixner wrote:
> The recent rework of the cpu hotplug internals changed the usage of the per
> cpu state->node field, but missed to clean it up after usage.
> 
> So subsequent hotplug operations use the stale pointer from a previous
> operation and hand it into the callback functions. The callbacks then
> dereference a pointer which either belongs to a different facility or
> points to freed and potentially reused memory. In either case data
> corruption and crashes are the obvious consequence.
> 
> Reset the node and the last pointers in the per cpu state to NULL after the
> operation which set them has completed.
> 
> Fixes: 96abb968549c ("smp/hotplug: Allow external multi-instance rollback")
> Reported-by: Tvrtko Ursulin <tursulin@ursulin.net>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

This does fine with rcutorture, though it does not fix the lost
(or egregiously delayed) timer problem that is still eluding me.
Nevertheless, for whatever it is worth:

Tested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  kernel/cpu.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -632,6 +632,11 @@ cpuhp_invoke_ap_callback(int cpu, enum c
>  		__cpuhp_kick_ap(st);
>  	}
> 
> +	/*
> +	 * Clean up the leftovers so the next hotplug operation wont use stale
> +	 * data.
> +	 */
> +	st->node = st->last = NULL;
>  	return ret;
>  }
> 
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] cpu/hotplug: Reset node state after operation
  2017-10-21 14:06 [PATCH] cpu/hotplug: Reset node state after operation Thomas Gleixner
  2017-10-21 14:16 ` [tip:smp/urgent] " tip-bot for Thomas Gleixner
  2017-10-22  2:49 ` [PATCH] " Paul E. McKenney
@ 2017-10-22  9:22 ` Peter Zijlstra
  2017-10-23  8:01 ` Tvrtko Ursulin
  3 siblings, 0 replies; 5+ messages in thread
From: Peter Zijlstra @ 2017-10-22  9:22 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Tvrtko Ursulin, linux-kernel, Ingo Molnar,
	Sebastian Andrzej Siewior, Paul E. McKenney, Boris Ostrovsky

On Sat, Oct 21, 2017 at 04:06:52PM +0200, Thomas Gleixner wrote:
> The recent rework of the cpu hotplug internals changed the usage of the per
> cpu state->node field, but missed to clean it up after usage.
> 
> So subsequent hotplug operations use the stale pointer from a previous
> operation and hand it into the callback functions. The callbacks then
> dereference a pointer which either belongs to a different facility or
> points to freed and potentially reused memory. In either case data
> corruption and crashes are the obvious consequence.
> 
> Reset the node and the last pointers in the per cpu state to NULL after the
> operation which set them has completed.
> 
> Fixes: 96abb968549c ("smp/hotplug: Allow external multi-instance rollback")
> Reported-by: Tvrtko Ursulin <tursulin@ursulin.net>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Thanks for digging into that during the conf :-) And sorry for wrecking
that.

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> ---
>  kernel/cpu.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -632,6 +632,11 @@ cpuhp_invoke_ap_callback(int cpu, enum c
>  		__cpuhp_kick_ap(st);
>  	}
>  
> +	/*
> +	 * Clean up the leftovers so the next hotplug operation wont use stale
> +	 * data.
> +	 */
> +	st->node = st->last = NULL;
>  	return ret;
>  }
>  

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] cpu/hotplug: Reset node state after operation
  2017-10-21 14:06 [PATCH] cpu/hotplug: Reset node state after operation Thomas Gleixner
                   ` (2 preceding siblings ...)
  2017-10-22  9:22 ` Peter Zijlstra
@ 2017-10-23  8:01 ` Tvrtko Ursulin
  3 siblings, 0 replies; 5+ messages in thread
From: Tvrtko Ursulin @ 2017-10-23  8:01 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-kernel, Ingo Molnar, Peter Zijlstra,
	Sebastian Andrzej Siewior, Paul E. McKenney, Boris Ostrovsky


On 21/10/17 15:06, Thomas Gleixner wrote:
> The recent rework of the cpu hotplug internals changed the usage of the per
> cpu state->node field, but missed to clean it up after usage.
> 
> So subsequent hotplug operations use the stale pointer from a previous
> operation and hand it into the callback functions. The callbacks then
> dereference a pointer which either belongs to a different facility or
> points to freed and potentially reused memory. In either case data
> corruption and crashes are the obvious consequence.
> 
> Reset the node and the last pointers in the per cpu state to NULL after the
> operation which set them has completed.
> 
> Fixes: 96abb968549c ("smp/hotplug: Allow external multi-instance rollback")
> Reported-by: Tvrtko Ursulin <tursulin@ursulin.net>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>   kernel/cpu.c |    5 +++++
>   1 file changed, 5 insertions(+)
> 
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -632,6 +632,11 @@ cpuhp_invoke_ap_callback(int cpu, enum c
>   		__cpuhp_kick_ap(st);
>   	}
>   
> +	/*
> +	 * Clean up the leftovers so the next hotplug operation wont use stale
> +	 * data.
> +	 */
> +	st->node = st->last = NULL;
>   	return ret;
>   }
>   
> 

Our CI farm is happy with this fix, so:

Tested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks for looking into is so quickly!

Regards,

Tvrtko

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-10-23  8:01 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-21 14:06 [PATCH] cpu/hotplug: Reset node state after operation Thomas Gleixner
2017-10-21 14:16 ` [tip:smp/urgent] " tip-bot for Thomas Gleixner
2017-10-22  2:49 ` [PATCH] " Paul E. McKenney
2017-10-22  9:22 ` Peter Zijlstra
2017-10-23  8:01 ` Tvrtko Ursulin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.