Re: [PATCH 4.4 02/96] s390/runtime instrumention: fix possible memory corruption

[Greg Kroah-Hartman]

On Wed, Dec 06, 2017 at 02:30:58PM +0100, Heiko Carstens wrote:
> On Wed, Dec 06, 2017 at 08:44:53AM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Dec 05, 2017 at 07:15:34PM +0100, Heiko Carstens wrote:
> > > On Tue, Dec 05, 2017 at 06:08:47PM +0100, Greg Kroah-Hartman wrote:
> > > > On Tue, Dec 05, 2017 at 05:02:32PM +, Ben Hutchings wrote:
> > > > > On Tue, 2017-11-28 at 11:22 +0100, Greg Kroah-Hartman wrote:
> > > > > > 4.4-stable review patch.  If anyone has any objections, please let 
> > > > > > me know.
> > > > > > 
> > > > > > --
> > > > > > 
> > > > > > From: Heiko Carstens 
> > > > > > 
> > > > > > commit d6e646ad7cfa7034d280459b2b2546288f247144 upstream.
> > > > > [...]
> > > > > > --- a/arch/s390/kernel/runtime_instr.c
> > > > > > +++ b/arch/s390/kernel/runtime_instr.c
> > > > > > @@ -47,11 +47,13 @@ void exit_thread_runtime_instr(void)
> > > > > >  {
> > > > > >     struct task_struct *task = current;
> > > > > >  
> > > > > > +   preempt_disable();
> > > > > >     if (!task->thread.ri_cb)
> > > > > >     return;
> > > > > 
> > > > > This return path now leaves preemption disabled.  This seems to have
> > > > > been fixed upstream by commit 8d9047f8b967 "s390/runtime
> > > > > instrumentation: simplify task exit handling".
> > > > 
> > > > "simplify" doesn't seem to imply "fixes a bug" :)
> > > 
> > > Indeed ;) That where two subsequent patches, but incorrectly split by 
> > > me...
> > > 
> > > > Heiko, should I also queue this patch up?
> > > 
> > > Yes, please.
> > 
> > It doesn't apply to 4.9-stable or 4.4-stable, can you provide a working
> > backport?
> 
> Below is the patch against 4.4-stable:

This and the 4.9 patch now queued up, thanks.

greg k-h

Re: [PATCH 4.4 02/96] s390/runtime instrumention: fix possible memory corruption

[Greg Kroah-Hartman]

On Wed, Dec 06, 2017 at 02:30:58PM +0100, Heiko Carstens wrote:
> On Wed, Dec 06, 2017 at 08:44:53AM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Dec 05, 2017 at 07:15:34PM +0100, Heiko Carstens wrote:
> > > On Tue, Dec 05, 2017 at 06:08:47PM +0100, Greg Kroah-Hartman wrote:
> > > > On Tue, Dec 05, 2017 at 05:02:32PM +, Ben Hutchings wrote:
> > > > > On Tue, 2017-11-28 at 11:22 +0100, Greg Kroah-Hartman wrote:
> > > > > > 4.4-stable review patch.  If anyone has any objections, please let 
> > > > > > me know.
> > > > > > 
> > > > > > --
> > > > > > 
> > > > > > From: Heiko Carstens 
> > > > > > 
> > > > > > commit d6e646ad7cfa7034d280459b2b2546288f247144 upstream.
> > > > > [...]
> > > > > > --- a/arch/s390/kernel/runtime_instr.c
> > > > > > +++ b/arch/s390/kernel/runtime_instr.c
> > > > > > @@ -47,11 +47,13 @@ void exit_thread_runtime_instr(void)
> > > > > >  {
> > > > > >     struct task_struct *task = current;
> > > > > >  
> > > > > > +   preempt_disable();
> > > > > >     if (!task->thread.ri_cb)
> > > > > >     return;
> > > > > 
> > > > > This return path now leaves preemption disabled.  This seems to have
> > > > > been fixed upstream by commit 8d9047f8b967 "s390/runtime
> > > > > instrumentation: simplify task exit handling".
> > > > 
> > > > "simplify" doesn't seem to imply "fixes a bug" :)
> > > 
> > > Indeed ;) That where two subsequent patches, but incorrectly split by 
> > > me...
> > > 
> > > > Heiko, should I also queue this patch up?
> > > 
> > > Yes, please.
> > 
> > It doesn't apply to 4.9-stable or 4.4-stable, can you provide a working
> > backport?
> 
> Below is the patch against 4.4-stable:

This and the 4.9 patch now queued up, thanks.

greg k-h

Re: [PATCH 4.4 02/96] s390/runtime instrumention: fix possible memory corruption

[Heiko Carstens]

On Wed, Dec 06, 2017 at 08:44:53AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 05, 2017 at 07:15:34PM +0100, Heiko Carstens wrote:
> > On Tue, Dec 05, 2017 at 06:08:47PM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Dec 05, 2017 at 05:02:32PM +, Ben Hutchings wrote:
> > > > On Tue, 2017-11-28 at 11:22 +0100, Greg Kroah-Hartman wrote:
> > > > > 4.4-stable review patch.  If anyone has any objections, please let me 
> > > > > know.
> > > > > 
> > > > > --
> > > > > 
> > > > > From: Heiko Carstens 
> > > > > 
> > > > > commit d6e646ad7cfa7034d280459b2b2546288f247144 upstream.
> > > > [...]
> > > > > --- a/arch/s390/kernel/runtime_instr.c
> > > > > +++ b/arch/s390/kernel/runtime_instr.c
> > > > > @@ -47,11 +47,13 @@ void exit_thread_runtime_instr(void)
> > > > >  {
> > > > >   struct task_struct *task = current;
> > > > >  
> > > > > + preempt_disable();
> > > > >   if (!task->thread.ri_cb)
> > > > >   return;
> > > > 
> > > > This return path now leaves preemption disabled.  This seems to have
> > > > been fixed upstream by commit 8d9047f8b967 "s390/runtime
> > > > instrumentation: simplify task exit handling".
> > > 
> > > "simplify" doesn't seem to imply "fixes a bug" :)
> > 
> > Indeed ;) That where two subsequent patches, but incorrectly split by me...
> > 
> > > Heiko, should I also queue this patch up?
> > 
> > Yes, please.
> 
> It doesn't apply to 4.9-stable or 4.4-stable, can you provide a working
> backport?

And here the one for 4.9-stable:

>From 5d0ccf454464a0f06c637e7c2743ae610898cd47 Mon Sep 17 00:00:00 2001
From: Heiko Carstens 
Date: Mon, 11 Sep 2017 11:24:22 +0200
Subject: [PATCH] s390/runtime instrumentation: simplify task exit handling

commit 8d9047f8b967ce6181fd824ae922978e1b055cc0 upstream.

Free data structures required for runtime instrumentation from
arch_release_task_struct(). This allows to simplify the code a bit,
and also makes the semantics a bit easier: arch_release_task_struct()
is never called from the task that is being removed.

In addition this allows to get rid of exit_thread() in a later patch.

Signed-off-by: Heiko Carstens 
Signed-off-by: Martin Schwidefsky 
---
 arch/s390/include/asm/runtime_instr.h |  4 +++-
 arch/s390/kernel/process.c|  3 +--
 arch/s390/kernel/runtime_instr.c  | 30 +++---
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/arch/s390/include/asm/runtime_instr.h 
b/arch/s390/include/asm/runtime_instr.h
index 402ad6df4897..c54a9310d814 100644
--- a/arch/s390/include/asm/runtime_instr.h
+++ b/arch/s390/include/asm/runtime_instr.h
@@ -85,6 +85,8 @@ static inline void restore_ri_cb(struct runtime_instr_cb 
*cb_next,
load_runtime_instr_cb(_instr_empty_cb);
 }
 
-void exit_thread_runtime_instr(void);
+struct task_struct;
+
+void runtime_instr_release(struct task_struct *tsk);
 
 #endif /* _RUNTIME_INSTR_H */
diff --git a/arch/s390/kernel/process.c b/arch/s390/kernel/process.c
index 172fe1121d99..8382fc62cde6 100644
--- a/arch/s390/kernel/process.c
+++ b/arch/s390/kernel/process.c
@@ -70,8 +70,6 @@ extern void kernel_thread_starter(void);
  */
 void exit_thread(struct task_struct *tsk)
 {
-   if (tsk == current)
-   exit_thread_runtime_instr();
 }
 
 void flush_thread(void)
@@ -84,6 +82,7 @@ void release_thread(struct task_struct *dead_task)
 
 void arch_release_task_struct(struct task_struct *tsk)
 {
+   runtime_instr_release(tsk);
 }
 
 int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
diff --git a/arch/s390/kernel/runtime_instr.c b/arch/s390/kernel/runtime_instr.c
index 70cdb03d4acd..fd03a7569e10 100644
--- a/arch/s390/kernel/runtime_instr.c
+++ b/arch/s390/kernel/runtime_instr.c
@@ -18,11 +18,24 @@
 /* empty control block to disable RI by loading it */
 struct runtime_instr_cb runtime_instr_empty_cb;
 
+void runtime_instr_release(struct task_struct *tsk)
+{
+   kfree(tsk->thread.ri_cb);
+}
+
 static void disable_runtime_instr(void)
 {
-   struct pt_regs *regs = task_pt_regs(current);
+   struct task_struct *task = current;
+   struct pt_regs *regs;
 
+   if (!task->thread.ri_cb)
+   return;
+   regs = task_pt_regs(task);
+   preempt_disable();
load_runtime_instr_cb(_instr_empty_cb);
+   kfree(task->thread.ri_cb);
+   task->thread.ri_cb = NULL;
+   preempt_enable();
 
/*
 * Make sure the RI bit is deleted from the PSW. If the user did not
@@ -43,19 +56,6 @@ static void init_runtime_instr_cb(struct runtime_instr_cb 
*cb)
cb->valid = 1;
 }
 
-void exit_thread_runtime_instr(void)
-{
-   struct task_struct *task = current;
-
-   preempt_disable();
-   if (!task->thread.ri_cb)
-   return;
-   disable_runtime_instr();
-   kfree(task->thread.ri_cb);
-   task->thread.ri_cb = NULL;
-  

Re: [PATCH 4.4 02/96] s390/runtime instrumention: fix possible memory corruption

[Heiko Carstens]

On Wed, Dec 06, 2017 at 08:44:53AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 05, 2017 at 07:15:34PM +0100, Heiko Carstens wrote:
> > On Tue, Dec 05, 2017 at 06:08:47PM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Dec 05, 2017 at 05:02:32PM +, Ben Hutchings wrote:
> > > > On Tue, 2017-11-28 at 11:22 +0100, Greg Kroah-Hartman wrote:
> > > > > 4.4-stable review patch.  If anyone has any objections, please let me 
> > > > > know.
> > > > > 
> > > > > --
> > > > > 
> > > > > From: Heiko Carstens 
> > > > > 
> > > > > commit d6e646ad7cfa7034d280459b2b2546288f247144 upstream.
> > > > [...]
> > > > > --- a/arch/s390/kernel/runtime_instr.c
> > > > > +++ b/arch/s390/kernel/runtime_instr.c
> > > > > @@ -47,11 +47,13 @@ void exit_thread_runtime_instr(void)
> > > > >  {
> > > > >   struct task_struct *task = current;
> > > > >  
> > > > > + preempt_disable();
> > > > >   if (!task->thread.ri_cb)
> > > > >   return;
> > > > 
> > > > This return path now leaves preemption disabled.  This seems to have
> > > > been fixed upstream by commit 8d9047f8b967 "s390/runtime
> > > > instrumentation: simplify task exit handling".
> > > 
> > > "simplify" doesn't seem to imply "fixes a bug" :)
> > 
> > Indeed ;) That where two subsequent patches, but incorrectly split by me...
> > 
> > > Heiko, should I also queue this patch up?
> > 
> > Yes, please.
> 
> It doesn't apply to 4.9-stable or 4.4-stable, can you provide a working
> backport?

And here the one for 4.9-stable:

>From 5d0ccf454464a0f06c637e7c2743ae610898cd47 Mon Sep 17 00:00:00 2001
From: Heiko Carstens 
Date: Mon, 11 Sep 2017 11:24:22 +0200
Subject: [PATCH] s390/runtime instrumentation: simplify task exit handling

commit 8d9047f8b967ce6181fd824ae922978e1b055cc0 upstream.

Free data structures required for runtime instrumentation from
arch_release_task_struct(). This allows to simplify the code a bit,
and also makes the semantics a bit easier: arch_release_task_struct()
is never called from the task that is being removed.

In addition this allows to get rid of exit_thread() in a later patch.

Signed-off-by: Heiko Carstens 
Signed-off-by: Martin Schwidefsky 
---
 arch/s390/include/asm/runtime_instr.h |  4 +++-
 arch/s390/kernel/process.c|  3 +--
 arch/s390/kernel/runtime_instr.c  | 30 +++---
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/arch/s390/include/asm/runtime_instr.h 
b/arch/s390/include/asm/runtime_instr.h
index 402ad6df4897..c54a9310d814 100644
--- a/arch/s390/include/asm/runtime_instr.h
+++ b/arch/s390/include/asm/runtime_instr.h
@@ -85,6 +85,8 @@ static inline void restore_ri_cb(struct runtime_instr_cb 
*cb_next,
load_runtime_instr_cb(_instr_empty_cb);
 }
 
-void exit_thread_runtime_instr(void);
+struct task_struct;
+
+void runtime_instr_release(struct task_struct *tsk);
 
 #endif /* _RUNTIME_INSTR_H */
diff --git a/arch/s390/kernel/process.c b/arch/s390/kernel/process.c
index 172fe1121d99..8382fc62cde6 100644
--- a/arch/s390/kernel/process.c
+++ b/arch/s390/kernel/process.c
@@ -70,8 +70,6 @@ extern void kernel_thread_starter(void);
  */
 void exit_thread(struct task_struct *tsk)
 {
-   if (tsk == current)
-   exit_thread_runtime_instr();
 }
 
 void flush_thread(void)
@@ -84,6 +82,7 @@ void release_thread(struct task_struct *dead_task)
 
 void arch_release_task_struct(struct task_struct *tsk)
 {
+   runtime_instr_release(tsk);
 }
 
 int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
diff --git a/arch/s390/kernel/runtime_instr.c b/arch/s390/kernel/runtime_instr.c
index 70cdb03d4acd..fd03a7569e10 100644
--- a/arch/s390/kernel/runtime_instr.c
+++ b/arch/s390/kernel/runtime_instr.c
@@ -18,11 +18,24 @@
 /* empty control block to disable RI by loading it */
 struct runtime_instr_cb runtime_instr_empty_cb;
 
+void runtime_instr_release(struct task_struct *tsk)
+{
+   kfree(tsk->thread.ri_cb);
+}
+
 static void disable_runtime_instr(void)
 {
-   struct pt_regs *regs = task_pt_regs(current);
+   struct task_struct *task = current;
+   struct pt_regs *regs;
 
+   if (!task->thread.ri_cb)
+   return;
+   regs = task_pt_regs(task);
+   preempt_disable();
load_runtime_instr_cb(_instr_empty_cb);
+   kfree(task->thread.ri_cb);
+   task->thread.ri_cb = NULL;
+   preempt_enable();
 
/*
 * Make sure the RI bit is deleted from the PSW. If the user did not
@@ -43,19 +56,6 @@ static void init_runtime_instr_cb(struct runtime_instr_cb 
*cb)
cb->valid = 1;
 }
 
-void exit_thread_runtime_instr(void)
-{
-   struct task_struct *task = current;
-
-   preempt_disable();
-   if (!task->thread.ri_cb)
-   return;
-   disable_runtime_instr();
-   kfree(task->thread.ri_cb);
-   task->thread.ri_cb = NULL;
-   preempt_enable();
-}
-
 SYSCALL_DEFINE1(s390_runtime_instr, int, command)
 {
struct 

Re: [PATCH 4.4 02/96] s390/runtime instrumention: fix possible memory corruption

[Heiko Carstens]

On Wed, Dec 06, 2017 at 08:44:53AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 05, 2017 at 07:15:34PM +0100, Heiko Carstens wrote:
> > On Tue, Dec 05, 2017 at 06:08:47PM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Dec 05, 2017 at 05:02:32PM +, Ben Hutchings wrote:
> > > > On Tue, 2017-11-28 at 11:22 +0100, Greg Kroah-Hartman wrote:
> > > > > 4.4-stable review patch.  If anyone has any objections, please let me 
> > > > > know.
> > > > > 
> > > > > --
> > > > > 
> > > > > From: Heiko Carstens 
> > > > > 
> > > > > commit d6e646ad7cfa7034d280459b2b2546288f247144 upstream.
> > > > [...]
> > > > > --- a/arch/s390/kernel/runtime_instr.c
> > > > > +++ b/arch/s390/kernel/runtime_instr.c
> > > > > @@ -47,11 +47,13 @@ void exit_thread_runtime_instr(void)
> > > > >  {
> > > > >   struct task_struct *task = current;
> > > > >  
> > > > > + preempt_disable();
> > > > >   if (!task->thread.ri_cb)
> > > > >   return;
> > > > 
> > > > This return path now leaves preemption disabled.  This seems to have
> > > > been fixed upstream by commit 8d9047f8b967 "s390/runtime
> > > > instrumentation: simplify task exit handling".
> > > 
> > > "simplify" doesn't seem to imply "fixes a bug" :)
> > 
> > Indeed ;) That where two subsequent patches, but incorrectly split by me...
> > 
> > > Heiko, should I also queue this patch up?
> > 
> > Yes, please.
> 
> It doesn't apply to 4.9-stable or 4.4-stable, can you provide a working
> backport?

Below is the patch against 4.4-stable:

>From e3cd188d023506d4a0045b9a2918b9fa73d4d007 Mon Sep 17 00:00:00 2001
From: Heiko Carstens 
Date: Mon, 11 Sep 2017 11:24:22 +0200
Subject: [PATCH] s390/runtime instrumentation: simplify task exit handling

commit 8d9047f8b967ce6181fd824ae922978e1b055cc0 upstream.

Free data structures required for runtime instrumentation from
arch_release_task_struct(). This allows to simplify the code a bit,
and also makes the semantics a bit easier: arch_release_task_struct()
is never called from the task that is being removed.

In addition this allows to get rid of exit_thread() in a later patch.

Signed-off-by: Heiko Carstens 
Signed-off-by: Martin Schwidefsky 
---
 arch/s390/include/asm/runtime_instr.h |  4 +++-
 arch/s390/kernel/process.c|  2 +-
 arch/s390/kernel/runtime_instr.c  | 30 +++---
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/arch/s390/include/asm/runtime_instr.h 
b/arch/s390/include/asm/runtime_instr.h
index 402ad6df4897..c54a9310d814 100644
--- a/arch/s390/include/asm/runtime_instr.h
+++ b/arch/s390/include/asm/runtime_instr.h
@@ -85,6 +85,8 @@ static inline void restore_ri_cb(struct runtime_instr_cb 
*cb_next,
load_runtime_instr_cb(_instr_empty_cb);
 }
 
-void exit_thread_runtime_instr(void);
+struct task_struct;
+
+void runtime_instr_release(struct task_struct *tsk);
 
 #endif /* _RUNTIME_INSTR_H */
diff --git a/arch/s390/kernel/process.c b/arch/s390/kernel/process.c
index efa035a31b98..7bc4e4c5d5b8 100644
--- a/arch/s390/kernel/process.c
+++ b/arch/s390/kernel/process.c
@@ -72,7 +72,6 @@ extern void kernel_thread_starter(void);
  */
 void exit_thread(void)
 {
-   exit_thread_runtime_instr();
 }
 
 void flush_thread(void)
@@ -87,6 +86,7 @@ void arch_release_task_struct(struct task_struct *tsk)
 {
/* Free either the floating-point or the vector register save area */
kfree(tsk->thread.fpu.regs);
+   runtime_instr_release(tsk);
 }
 
 int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
diff --git a/arch/s390/kernel/runtime_instr.c b/arch/s390/kernel/runtime_instr.c
index 70cdb03d4acd..fd03a7569e10 100644
--- a/arch/s390/kernel/runtime_instr.c
+++ b/arch/s390/kernel/runtime_instr.c
@@ -18,11 +18,24 @@
 /* empty control block to disable RI by loading it */
 struct runtime_instr_cb runtime_instr_empty_cb;
 
+void runtime_instr_release(struct task_struct *tsk)
+{
+   kfree(tsk->thread.ri_cb);
+}
+
 static void disable_runtime_instr(void)
 {
-   struct pt_regs *regs = task_pt_regs(current);
+   struct task_struct *task = current;
+   struct pt_regs *regs;
 
+   if (!task->thread.ri_cb)
+   return;
+   regs = task_pt_regs(task);
+   preempt_disable();
load_runtime_instr_cb(_instr_empty_cb);
+   kfree(task->thread.ri_cb);
+   task->thread.ri_cb = NULL;
+   preempt_enable();
 
/*
 * Make sure the RI bit is deleted from the PSW. If the user did not
@@ -43,19 +56,6 @@ static void init_runtime_instr_cb(struct runtime_instr_cb 
*cb)
cb->valid = 1;
 }
 
-void exit_thread_runtime_instr(void)
-{
-   struct task_struct *task = current;
-
-   preempt_disable();
-   if (!task->thread.ri_cb)
-   return;
-   disable_runtime_instr();
-   kfree(task->thread.ri_cb);
-   task->thread.ri_cb 

Re: [PATCH 4.4 02/96] s390/runtime instrumention: fix possible memory corruption

[Heiko Carstens]

On Wed, Dec 06, 2017 at 08:44:53AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 05, 2017 at 07:15:34PM +0100, Heiko Carstens wrote:
> > On Tue, Dec 05, 2017 at 06:08:47PM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Dec 05, 2017 at 05:02:32PM +, Ben Hutchings wrote:
> > > > On Tue, 2017-11-28 at 11:22 +0100, Greg Kroah-Hartman wrote:
> > > > > 4.4-stable review patch.  If anyone has any objections, please let me 
> > > > > know.
> > > > > 
> > > > > --
> > > > > 
> > > > > From: Heiko Carstens 
> > > > > 
> > > > > commit d6e646ad7cfa7034d280459b2b2546288f247144 upstream.
> > > > [...]
> > > > > --- a/arch/s390/kernel/runtime_instr.c
> > > > > +++ b/arch/s390/kernel/runtime_instr.c
> > > > > @@ -47,11 +47,13 @@ void exit_thread_runtime_instr(void)
> > > > >  {
> > > > >   struct task_struct *task = current;
> > > > >  
> > > > > + preempt_disable();
> > > > >   if (!task->thread.ri_cb)
> > > > >   return;
> > > > 
> > > > This return path now leaves preemption disabled.  This seems to have
> > > > been fixed upstream by commit 8d9047f8b967 "s390/runtime
> > > > instrumentation: simplify task exit handling".
> > > 
> > > "simplify" doesn't seem to imply "fixes a bug" :)
> > 
> > Indeed ;) That where two subsequent patches, but incorrectly split by me...
> > 
> > > Heiko, should I also queue this patch up?
> > 
> > Yes, please.
> 
> It doesn't apply to 4.9-stable or 4.4-stable, can you provide a working
> backport?

Below is the patch against 4.4-stable:

>From e3cd188d023506d4a0045b9a2918b9fa73d4d007 Mon Sep 17 00:00:00 2001
From: Heiko Carstens 
Date: Mon, 11 Sep 2017 11:24:22 +0200
Subject: [PATCH] s390/runtime instrumentation: simplify task exit handling

commit 8d9047f8b967ce6181fd824ae922978e1b055cc0 upstream.

Free data structures required for runtime instrumentation from
arch_release_task_struct(). This allows to simplify the code a bit,
and also makes the semantics a bit easier: arch_release_task_struct()
is never called from the task that is being removed.

In addition this allows to get rid of exit_thread() in a later patch.

Signed-off-by: Heiko Carstens 
Signed-off-by: Martin Schwidefsky 
---
 arch/s390/include/asm/runtime_instr.h |  4 +++-
 arch/s390/kernel/process.c|  2 +-
 arch/s390/kernel/runtime_instr.c  | 30 +++---
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/arch/s390/include/asm/runtime_instr.h 
b/arch/s390/include/asm/runtime_instr.h
index 402ad6df4897..c54a9310d814 100644
--- a/arch/s390/include/asm/runtime_instr.h
+++ b/arch/s390/include/asm/runtime_instr.h
@@ -85,6 +85,8 @@ static inline void restore_ri_cb(struct runtime_instr_cb 
*cb_next,
load_runtime_instr_cb(_instr_empty_cb);
 }
 
-void exit_thread_runtime_instr(void);
+struct task_struct;
+
+void runtime_instr_release(struct task_struct *tsk);
 
 #endif /* _RUNTIME_INSTR_H */
diff --git a/arch/s390/kernel/process.c b/arch/s390/kernel/process.c
index efa035a31b98..7bc4e4c5d5b8 100644
--- a/arch/s390/kernel/process.c
+++ b/arch/s390/kernel/process.c
@@ -72,7 +72,6 @@ extern void kernel_thread_starter(void);
  */
 void exit_thread(void)
 {
-   exit_thread_runtime_instr();
 }
 
 void flush_thread(void)
@@ -87,6 +86,7 @@ void arch_release_task_struct(struct task_struct *tsk)
 {
/* Free either the floating-point or the vector register save area */
kfree(tsk->thread.fpu.regs);
+   runtime_instr_release(tsk);
 }
 
 int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
diff --git a/arch/s390/kernel/runtime_instr.c b/arch/s390/kernel/runtime_instr.c
index 70cdb03d4acd..fd03a7569e10 100644
--- a/arch/s390/kernel/runtime_instr.c
+++ b/arch/s390/kernel/runtime_instr.c
@@ -18,11 +18,24 @@
 /* empty control block to disable RI by loading it */
 struct runtime_instr_cb runtime_instr_empty_cb;
 
+void runtime_instr_release(struct task_struct *tsk)
+{
+   kfree(tsk->thread.ri_cb);
+}
+
 static void disable_runtime_instr(void)
 {
-   struct pt_regs *regs = task_pt_regs(current);
+   struct task_struct *task = current;
+   struct pt_regs *regs;
 
+   if (!task->thread.ri_cb)
+   return;
+   regs = task_pt_regs(task);
+   preempt_disable();
load_runtime_instr_cb(_instr_empty_cb);
+   kfree(task->thread.ri_cb);
+   task->thread.ri_cb = NULL;
+   preempt_enable();
 
/*
 * Make sure the RI bit is deleted from the PSW. If the user did not
@@ -43,19 +56,6 @@ static void init_runtime_instr_cb(struct runtime_instr_cb 
*cb)
cb->valid = 1;
 }
 
-void exit_thread_runtime_instr(void)
-{
-   struct task_struct *task = current;
-
-   preempt_disable();
-   if (!task->thread.ri_cb)
-   return;
-   disable_runtime_instr();
-   kfree(task->thread.ri_cb);
-   task->thread.ri_cb = NULL;
-   preempt_enable();
-}
-
 SYSCALL_DEFINE1(s390_runtime_instr, int, command)
 {

Re: [PATCH 4.4 02/96] s390/runtime instrumention: fix possible memory corruption

[Greg Kroah-Hartman]

On Tue, Dec 05, 2017 at 07:15:34PM +0100, Heiko Carstens wrote:
> On Tue, Dec 05, 2017 at 06:08:47PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Dec 05, 2017 at 05:02:32PM +, Ben Hutchings wrote:
> > > On Tue, 2017-11-28 at 11:22 +0100, Greg Kroah-Hartman wrote:
> > > > 4.4-stable review patch.  If anyone has any objections, please let me 
> > > > know.
> > > > 
> > > > --
> > > > 
> > > > From: Heiko Carstens 
> > > > 
> > > > commit d6e646ad7cfa7034d280459b2b2546288f247144 upstream.
> > > [...]
> > > > --- a/arch/s390/kernel/runtime_instr.c
> > > > +++ b/arch/s390/kernel/runtime_instr.c
> > > > @@ -47,11 +47,13 @@ void exit_thread_runtime_instr(void)
> > > >  {
> > > >     struct task_struct *task = current;
> > > >  
> > > > +   preempt_disable();
> > > >     if (!task->thread.ri_cb)
> > > >     return;
> > > 
> > > This return path now leaves preemption disabled.  This seems to have
> > > been fixed upstream by commit 8d9047f8b967 "s390/runtime
> > > instrumentation: simplify task exit handling".
> > 
> > "simplify" doesn't seem to imply "fixes a bug" :)
> 
> Indeed ;) That where two subsequent patches, but incorrectly split by me...
> 
> > Heiko, should I also queue this patch up?
> 
> Yes, please.

It doesn't apply to 4.9-stable or 4.4-stable, can you provide a working
backport?

thanks,

greg k-h

Re: [PATCH 4.4 02/96] s390/runtime instrumention: fix possible memory corruption

[Greg Kroah-Hartman]

On Tue, Dec 05, 2017 at 07:15:34PM +0100, Heiko Carstens wrote:
> On Tue, Dec 05, 2017 at 06:08:47PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Dec 05, 2017 at 05:02:32PM +, Ben Hutchings wrote:
> > > On Tue, 2017-11-28 at 11:22 +0100, Greg Kroah-Hartman wrote:
> > > > 4.4-stable review patch.  If anyone has any objections, please let me 
> > > > know.
> > > > 
> > > > --
> > > > 
> > > > From: Heiko Carstens 
> > > > 
> > > > commit d6e646ad7cfa7034d280459b2b2546288f247144 upstream.
> > > [...]
> > > > --- a/arch/s390/kernel/runtime_instr.c
> > > > +++ b/arch/s390/kernel/runtime_instr.c
> > > > @@ -47,11 +47,13 @@ void exit_thread_runtime_instr(void)
> > > >  {
> > > >     struct task_struct *task = current;
> > > >  
> > > > +   preempt_disable();
> > > >     if (!task->thread.ri_cb)
> > > >     return;
> > > 
> > > This return path now leaves preemption disabled.  This seems to have
> > > been fixed upstream by commit 8d9047f8b967 "s390/runtime
> > > instrumentation: simplify task exit handling".
> > 
> > "simplify" doesn't seem to imply "fixes a bug" :)
> 
> Indeed ;) That where two subsequent patches, but incorrectly split by me...
> 
> > Heiko, should I also queue this patch up?
> 
> Yes, please.

It doesn't apply to 4.9-stable or 4.4-stable, can you provide a working
backport?

thanks,

greg k-h

Re: [PATCH 4.4 02/96] s390/runtime instrumention: fix possible memory corruption

[Heiko Carstens]

On Tue, Dec 05, 2017 at 06:08:47PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 05, 2017 at 05:02:32PM +, Ben Hutchings wrote:
> > On Tue, 2017-11-28 at 11:22 +0100, Greg Kroah-Hartman wrote:
> > > 4.4-stable review patch.  If anyone has any objections, please let me 
> > > know.
> > > 
> > > --
> > > 
> > > From: Heiko Carstens 
> > > 
> > > commit d6e646ad7cfa7034d280459b2b2546288f247144 upstream.
> > [...]
> > > --- a/arch/s390/kernel/runtime_instr.c
> > > +++ b/arch/s390/kernel/runtime_instr.c
> > > @@ -47,11 +47,13 @@ void exit_thread_runtime_instr(void)
> > >  {
> > >   struct task_struct *task = current;
> > >  
> > > + preempt_disable();
> > >   if (!task->thread.ri_cb)
> > >   return;
> > 
> > This return path now leaves preemption disabled.  This seems to have
> > been fixed upstream by commit 8d9047f8b967 "s390/runtime
> > instrumentation: simplify task exit handling".
> 
> "simplify" doesn't seem to imply "fixes a bug" :)

Indeed ;) That where two subsequent patches, but incorrectly split by me...

> Heiko, should I also queue this patch up?

Yes, please.

> thanks Ben for the review.

Thanks from me as well!

Re: [PATCH 4.4 02/96] s390/runtime instrumention: fix possible memory corruption

[Heiko Carstens]

On Tue, Dec 05, 2017 at 06:08:47PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 05, 2017 at 05:02:32PM +, Ben Hutchings wrote:
> > On Tue, 2017-11-28 at 11:22 +0100, Greg Kroah-Hartman wrote:
> > > 4.4-stable review patch.  If anyone has any objections, please let me 
> > > know.
> > > 
> > > --
> > > 
> > > From: Heiko Carstens 
> > > 
> > > commit d6e646ad7cfa7034d280459b2b2546288f247144 upstream.
> > [...]
> > > --- a/arch/s390/kernel/runtime_instr.c
> > > +++ b/arch/s390/kernel/runtime_instr.c
> > > @@ -47,11 +47,13 @@ void exit_thread_runtime_instr(void)
> > >  {
> > >   struct task_struct *task = current;
> > >  
> > > + preempt_disable();
> > >   if (!task->thread.ri_cb)
> > >   return;
> > 
> > This return path now leaves preemption disabled.  This seems to have
> > been fixed upstream by commit 8d9047f8b967 "s390/runtime
> > instrumentation: simplify task exit handling".
> 
> "simplify" doesn't seem to imply "fixes a bug" :)

Indeed ;) That where two subsequent patches, but incorrectly split by me...

> Heiko, should I also queue this patch up?

Yes, please.

> thanks Ben for the review.

Thanks from me as well!

Re: [PATCH 4.4 02/96] s390/runtime instrumention: fix possible memory corruption

[Greg Kroah-Hartman]

On Tue, Dec 05, 2017 at 05:02:32PM +, Ben Hutchings wrote:
> On Tue, 2017-11-28 at 11:22 +0100, Greg Kroah-Hartman wrote:
> > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > 
> > --
> > 
> > From: Heiko Carstens 
> > 
> > commit d6e646ad7cfa7034d280459b2b2546288f247144 upstream.
> [...]
> > --- a/arch/s390/kernel/runtime_instr.c
> > +++ b/arch/s390/kernel/runtime_instr.c
> > @@ -47,11 +47,13 @@ void exit_thread_runtime_instr(void)
> >  {
> >     struct task_struct *task = current;
> >  
> > +   preempt_disable();
> >     if (!task->thread.ri_cb)
> >     return;
> 
> This return path now leaves preemption disabled.  This seems to have
> been fixed upstream by commit 8d9047f8b967 "s390/runtime
> instrumentation: simplify task exit handling".

"simplify" doesn't seem to imply "fixes a bug" :)

Heiko, should I also queue this patch up?

thanks Ben for the review.

greg k-h

Re: [PATCH 4.4 02/96] s390/runtime instrumention: fix possible memory corruption

[Greg Kroah-Hartman]

On Tue, Dec 05, 2017 at 05:02:32PM +, Ben Hutchings wrote:
> On Tue, 2017-11-28 at 11:22 +0100, Greg Kroah-Hartman wrote:
> > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > 
> > --
> > 
> > From: Heiko Carstens 
> > 
> > commit d6e646ad7cfa7034d280459b2b2546288f247144 upstream.
> [...]
> > --- a/arch/s390/kernel/runtime_instr.c
> > +++ b/arch/s390/kernel/runtime_instr.c
> > @@ -47,11 +47,13 @@ void exit_thread_runtime_instr(void)
> >  {
> >     struct task_struct *task = current;
> >  
> > +   preempt_disable();
> >     if (!task->thread.ri_cb)
> >     return;
> 
> This return path now leaves preemption disabled.  This seems to have
> been fixed upstream by commit 8d9047f8b967 "s390/runtime
> instrumentation: simplify task exit handling".

"simplify" doesn't seem to imply "fixes a bug" :)

Heiko, should I also queue this patch up?

thanks Ben for the review.

greg k-h

Re: [PATCH 4.4 02/96] s390/runtime instrumention: fix possible memory corruption

[Ben Hutchings]

On Tue, 2017-11-28 at 11:22 +0100, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
> 
> --
> 
> From: Heiko Carstens 
> 
> commit d6e646ad7cfa7034d280459b2b2546288f247144 upstream.
[...]
> --- a/arch/s390/kernel/runtime_instr.c
> +++ b/arch/s390/kernel/runtime_instr.c
> @@ -47,11 +47,13 @@ void exit_thread_runtime_instr(void)
>  {
>   struct task_struct *task = current;
>  
> + preempt_disable();
>   if (!task->thread.ri_cb)
>   return;

This return path now leaves preemption disabled.  This seems to have
been fixed upstream by commit 8d9047f8b967 "s390/runtime
instrumentation: simplify task exit handling".

Ben.

>   disable_runtime_instr();
>   kfree(task->thread.ri_cb);
>   task->thread.ri_cb = NULL;
> + preempt_enable();
>  }
>  
>  SYSCALL_DEFINE1(s390_runtime_instr, int, command)
[...]

-- 
Ben Hutchings
Software Developer, Codethink Ltd.

Re: [PATCH 4.4 02/96] s390/runtime instrumention: fix possible memory corruption

[Ben Hutchings]

On Tue, 2017-11-28 at 11:22 +0100, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
> 
> --
> 
> From: Heiko Carstens 
> 
> commit d6e646ad7cfa7034d280459b2b2546288f247144 upstream.
[...]
> --- a/arch/s390/kernel/runtime_instr.c
> +++ b/arch/s390/kernel/runtime_instr.c
> @@ -47,11 +47,13 @@ void exit_thread_runtime_instr(void)
>  {
>   struct task_struct *task = current;
>  
> + preempt_disable();
>   if (!task->thread.ri_cb)
>   return;

This return path now leaves preemption disabled.  This seems to have
been fixed upstream by commit 8d9047f8b967 "s390/runtime
instrumentation: simplify task exit handling".

Ben.

>   disable_runtime_instr();
>   kfree(task->thread.ri_cb);
>   task->thread.ri_cb = NULL;
> + preempt_enable();
>  }
>  
>  SYSCALL_DEFINE1(s390_runtime_instr, int, command)
[...]

-- 
Ben Hutchings
Software Developer, Codethink Ltd.

[PATCH 4.4 02/96] s390/runtime instrumention: fix possible memory corruption

[Greg Kroah-Hartman]

4.4-stable review patch.  If anyone has any objections, please let me know.

--

From: Heiko Carstens 

commit d6e646ad7cfa7034d280459b2b2546288f247144 upstream.

For PREEMPT enabled kernels the runtime instrumentation (RI) code
contains a possible use-after-free bug. If a task that makes use of RI
exits, it will execute do_exit() while still enabled for preemption.

That function will call exit_thread_runtime_instr() via
exit_thread(). If exit_thread_runtime_instr() gets preempted after the
RI control block of the task has been freed but before the pointer to
it is set to NULL, then save_ri_cb(), called from switch_to(), will
write to already freed memory.

Avoid this and simply disable preemption while freeing the control
block and setting the pointer to NULL.

Fixes: e4b8b3f33fca ("s390: add support for runtime instrumentation")
Reviewed-by: Christian Borntraeger 
Signed-off-by: Heiko Carstens 
Signed-off-by: Martin Schwidefsky 
Signed-off-by: Greg Kroah-Hartman 

---
 arch/s390/kernel/runtime_instr.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/arch/s390/kernel/runtime_instr.c
+++ b/arch/s390/kernel/runtime_instr.c
@@ -47,11 +47,13 @@ void exit_thread_runtime_instr(void)
 {
struct task_struct *task = current;
 
+   preempt_disable();
if (!task->thread.ri_cb)
return;
disable_runtime_instr();
kfree(task->thread.ri_cb);
task->thread.ri_cb = NULL;
+   preempt_enable();
 }
 
 SYSCALL_DEFINE1(s390_runtime_instr, int, command)
@@ -62,9 +64,7 @@ SYSCALL_DEFINE1(s390_runtime_instr, int,
return -EOPNOTSUPP;
 
if (command == S390_RUNTIME_INSTR_STOP) {
-   preempt_disable();
exit_thread_runtime_instr();
-   preempt_enable();
return 0;
}
 

[PATCH 4.4 02/96] s390/runtime instrumention: fix possible memory corruption

[Greg Kroah-Hartman]

4.4-stable review patch.  If anyone has any objections, please let me know.

--

From: Heiko Carstens 

commit d6e646ad7cfa7034d280459b2b2546288f247144 upstream.

For PREEMPT enabled kernels the runtime instrumentation (RI) code
contains a possible use-after-free bug. If a task that makes use of RI
exits, it will execute do_exit() while still enabled for preemption.

That function will call exit_thread_runtime_instr() via
exit_thread(). If exit_thread_runtime_instr() gets preempted after the
RI control block of the task has been freed but before the pointer to
it is set to NULL, then save_ri_cb(), called from switch_to(), will
write to already freed memory.

Avoid this and simply disable preemption while freeing the control
block and setting the pointer to NULL.

Fixes: e4b8b3f33fca ("s390: add support for runtime instrumentation")
Reviewed-by: Christian Borntraeger 
Signed-off-by: Heiko Carstens 
Signed-off-by: Martin Schwidefsky 
Signed-off-by: Greg Kroah-Hartman 

---
 arch/s390/kernel/runtime_instr.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/arch/s390/kernel/runtime_instr.c
+++ b/arch/s390/kernel/runtime_instr.c
@@ -47,11 +47,13 @@ void exit_thread_runtime_instr(void)
 {
struct task_struct *task = current;
 
+   preempt_disable();
if (!task->thread.ri_cb)
return;
disable_runtime_instr();
kfree(task->thread.ri_cb);
task->thread.ri_cb = NULL;
+   preempt_enable();
 }
 
 SYSCALL_DEFINE1(s390_runtime_instr, int, command)
@@ -62,9 +64,7 @@ SYSCALL_DEFINE1(s390_runtime_instr, int,
return -EOPNOTSUPP;
 
if (command == S390_RUNTIME_INSTR_STOP) {
-   preempt_disable();
exit_thread_runtime_instr();
-   preempt_enable();
return 0;
}