[RFC,08/11] powerpc/tm: Do not reclaim on ptrace

Message ID 1536781219-13938-9-git-send-email-leitao@debian.org
State RFC
Headers show
Series
  • New TM Model
Related show

Checks

Context Check Description
snowpatch_ozlabs/checkpatch warning Test checkpatch on branch next
snowpatch_ozlabs/apply_patch success next/apply_patch Successfully applied

Commit Message

Breno Leitao Sept. 12, 2018, 7:40 p.m.
Make sure that we are not suspended on ptrace and that the registers were
already reclaimed.

Since the data was already reclaimed, there is nothing to be done here
except to restore the SPRs.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/powerpc/kernel/ptrace.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Comments

Michael Neuling Sept. 18, 2018, 5:36 a.m. | #1
On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> Make sure that we are not suspended on ptrace and that the registers were
> already reclaimed.
> 
> Since the data was already reclaimed, there is nothing to be done here
> except to restore the SPRs.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  arch/powerpc/kernel/ptrace.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 9667666eb18e..cf6ee9154b11 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -136,12 +136,10 @@ static void flush_tmregs_to_thread(struct task_struct
> *tsk)
>  	if ((!cpu_has_feature(CPU_FTR_TM)) || (tsk != current))
>  		return;
>  
> -	if (MSR_TM_SUSPENDED(mfmsr())) {
> -		tm_reclaim_current(TM_CAUSE_SIGNAL);
> -	} else {
> -		tm_enable();
> -		tm_save_sprs(&(tsk->thread));
> -	}
> +	WARN_ON(MSR_TM_SUSPENDED(mfmsr()));
> +
> +	tm_enable();
> +	tm_save_sprs(&(tsk->thread));

Do we need to check if TM was enabled in the task before saving the TM SPRs?

What happens if TM was lazily off and hence we had someone else's TM SPRs in the
CPU currently?  Wouldn't this flush the wrong values to the task_struct?

I think we need to check the processes MSR before doing this.

Mikey

>  }
>  #else
>  static inline void flush_tmregs_to_thread(struct task_struct *tsk) { }
Breno Leitao Sept. 27, 2018, 9:03 p.m. | #2
Hi Mikey,

On 09/18/2018 02:36 AM, Michael Neuling wrote:
> On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
>> Make sure that we are not suspended on ptrace and that the registers were
>> already reclaimed.
>>
>> Since the data was already reclaimed, there is nothing to be done here
>> except to restore the SPRs.
>>
>> Signed-off-by: Breno Leitao <leitao@debian.org>
>> ---
>>  arch/powerpc/kernel/ptrace.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
>> index 9667666eb18e..cf6ee9154b11 100644
>> --- a/arch/powerpc/kernel/ptrace.c
>> +++ b/arch/powerpc/kernel/ptrace.c
>> @@ -136,12 +136,10 @@ static void flush_tmregs_to_thread(struct task_struct
>> *tsk)
>>  	if ((!cpu_has_feature(CPU_FTR_TM)) || (tsk != current))
>>  		return;
>>  
>> -	if (MSR_TM_SUSPENDED(mfmsr())) {
>> -		tm_reclaim_current(TM_CAUSE_SIGNAL);
>> -	} else {
>> -		tm_enable();
>> -		tm_save_sprs(&(tsk->thread));
>> -	}
>> +	WARN_ON(MSR_TM_SUSPENDED(mfmsr()));
>> +
>> +	tm_enable();
>> +	tm_save_sprs(&(tsk->thread));
> 
> Do we need to check if TM was enabled in the task before saving the TM SPRs?
> 
> What happens if TM was lazily off and hence we had someone else's TM SPRs in the
> CPU currently?  Wouldn't this flush the wrong values to the task_struct?
> 
> I think we need to check the processes MSR before doing this.

Yes, it is a *very* good point, and I think we are vulnerable even before
this patch (in the current kernel). Take a look above, we are calling
tm_save_sprs() if MSR is not TM suspended independently if TM is lazily off.

It shouldn't be hard to create a test case for it. I can try to call
ptrace(PTRACE_GETVRREGS) on a task that sleeps until TM is lazily disabled,
compare if the TM SPR changed in this mean time. (while doing HTM in parallel
to keep HTM SPR changing). Let's see if I can read others task TM SPRs.

Thank you.
Michael Neuling Sept. 28, 2018, 5:36 a.m. | #3
On Thu, 2018-09-27 at 18:03 -0300, Breno Leitao wrote:
> Hi Mikey,
> 
> On 09/18/2018 02:36 AM, Michael Neuling wrote:
> > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> > > Make sure that we are not suspended on ptrace and that the registers were
> > > already reclaimed.
> > > 
> > > Since the data was already reclaimed, there is nothing to be done here
> > > except to restore the SPRs.
> > > 
> > > Signed-off-by: Breno Leitao <leitao@debian.org>
> > > ---
> > >  arch/powerpc/kernel/ptrace.c | 10 ++++------
> > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> > > index 9667666eb18e..cf6ee9154b11 100644
> > > --- a/arch/powerpc/kernel/ptrace.c
> > > +++ b/arch/powerpc/kernel/ptrace.c
> > > @@ -136,12 +136,10 @@ static void flush_tmregs_to_thread(struct
> > > task_struct
> > > *tsk)
> > >  	if ((!cpu_has_feature(CPU_FTR_TM)) || (tsk != current))
> > >  		return;
> > >  
> > > -	if (MSR_TM_SUSPENDED(mfmsr())) {
> > > -		tm_reclaim_current(TM_CAUSE_SIGNAL);
> > > -	} else {
> > > -		tm_enable();
> > > -		tm_save_sprs(&(tsk->thread));
> > > -	}
> > > +	WARN_ON(MSR_TM_SUSPENDED(mfmsr()));
> > > +
> > > +	tm_enable();
> > > +	tm_save_sprs(&(tsk->thread));
> > 
> > Do we need to check if TM was enabled in the task before saving the TM SPRs?
> > 
> > What happens if TM was lazily off and hence we had someone else's TM SPRs in
> > the
> > CPU currently?  Wouldn't this flush the wrong values to the task_struct?
> > 
> > I think we need to check the processes MSR before doing this.
> 
> Yes, it is a *very* good point, and I think we are vulnerable even before
> this patch (in the current kernel). Take a look above, we are calling
> tm_save_sprs() if MSR is not TM suspended independently if TM is lazily off.

I think you're right, we might already have an issue.  There are some paths in
here that don't check the userspace msr or any of the lazy tm enable. :(

Mikey


> It shouldn't be hard to create a test case for it. I can try to call
> ptrace(PTRACE_GETVRREGS) on a task that sleeps until TM is lazily disabled,
> compare if the TM SPR changed in this mean time. (while doing HTM in parallel
> to keep HTM SPR changing). Let's see if I can read others task TM SPRs.
> 
> Thank you.
>
Breno Leitao Sept. 30, 2018, 11:51 p.m. | #4
Hi Mikey,

On 09/28/2018 02:36 AM, Michael Neuling wrote:
>>>> +	WARN_ON(MSR_TM_SUSPENDED(mfmsr())); + +	tm_enable(); + 
>>>> tm_save_sprs(&(tsk->thread));
>>> 
>>> Do we need to check if TM was enabled in the task before saving the
>>> TM SPRs?
>>> 
>>> What happens if TM was lazily off and hence we had someone else's TM 
>>> SPRs in the CPU currently?  Wouldn't this flush the wrong values to 
>>> the task_struct?
>>> 
>>> I think we need to check the processes MSR before doing this.
>> 
>> Yes, it is a *very* good point, and I think we are vulnerable even 
>> before this patch (in the current kernel). Take a look above, we are 
>> calling tm_save_sprs() if MSR is not TM suspended independently if TM
>> is lazily off.
> 
> I think you're right, we might already have an issue.  There are some 
> paths in here that don't check the userspace msr or any of the lazy tm 
> enable. :(

I was able to create a test case that reproduces this bug cleanly.

The testcase basically sleeps for N cycles, and then segfaults.

If N is high enough to have load_tm overflowed, then you see a corrupted
TEXASR value in the core dump file. If load_tm != 0 during the coredump, you
see the expected TEXASR value.

I wrote a small bash that check for both cases.

  $ git clone https://github.com/leitao/texasr_corrupt.git
  $ make check

Anyway, I will propose a fix for this problem soon, since this whole patchset
may delay to get ready. Is it OK?

Thank you
Michael Neuling Oct. 1, 2018, 12:34 a.m. | #5
On Sun, 2018-09-30 at 20:51 -0300, Breno Leitao wrote:
> Hi Mikey,
> 
> On 09/28/2018 02:36 AM, Michael Neuling wrote:
> > > > > +	WARN_ON(MSR_TM_SUSPENDED(mfmsr())); + +	tm_enable(); + 
> > > > > tm_save_sprs(&(tsk->thread));
> > > > 
> > > > Do we need to check if TM was enabled in the task before saving the
> > > > TM SPRs?
> > > > 
> > > > What happens if TM was lazily off and hence we had someone else's TM 
> > > > SPRs in the CPU currently?  Wouldn't this flush the wrong values to 
> > > > the task_struct?
> > > > 
> > > > I think we need to check the processes MSR before doing this.
> > > 
> > > Yes, it is a *very* good point, and I think we are vulnerable even 
> > > before this patch (in the current kernel). Take a look above, we are 
> > > calling tm_save_sprs() if MSR is not TM suspended independently if TM
> > > is lazily off.
> > 
> > I think you're right, we might already have an issue.  There are some 
> > paths in here that don't check the userspace msr or any of the lazy tm 
> > enable. :(
> 
> I was able to create a test case that reproduces this bug cleanly.
> 
> The testcase basically sleeps for N cycles, and then segfaults.
> 
> If N is high enough to have load_tm overflowed, then you see a corrupted
> TEXASR value in the core dump file. If load_tm != 0 during the coredump, you
> see the expected TEXASR value.
> 
> I wrote a small bash that check for both cases.
> 
>   $ git clone https://github.com/leitao/texasr_corrupt.git
>   $ make check
> 
> Anyway, I will propose a fix for this problem soon, since this whole patchset
> may delay to get ready. Is it OK?

Yeah, best to get a fix for this one out soon.

Mikey

Patch

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 9667666eb18e..cf6ee9154b11 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -136,12 +136,10 @@  static void flush_tmregs_to_thread(struct task_struct *tsk)
 	if ((!cpu_has_feature(CPU_FTR_TM)) || (tsk != current))
 		return;
 
-	if (MSR_TM_SUSPENDED(mfmsr())) {
-		tm_reclaim_current(TM_CAUSE_SIGNAL);
-	} else {
-		tm_enable();
-		tm_save_sprs(&(tsk->thread));
-	}
+	WARN_ON(MSR_TM_SUSPENDED(mfmsr()));
+
+	tm_enable();
+	tm_save_sprs(&(tsk->thread));
 }
 #else
 static inline void flush_tmregs_to_thread(struct task_struct *tsk) { }