| Message ID | 1536781219-13938-9-git-send-email-leitao@debian.org (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | New TM Model | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | warning | Test checkpatch on branch next |
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) { }
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.
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. >
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
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
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) { }
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(-)