[RFC PATCH 0/5] powerpc: Misc. ptrace regset fixes

[RFC PATCH 0/5] powerpc: Misc. ptrace regset fixes

[Pedro Franco de Carvalho]

This series attempts to fix a few issues with ptrace regsets.

Patch 1 simply inverts the active predicate for ebb_set. I don't know
if there was a reason for having opposite predicates in
ebb_get/ebb_set, but I assumed this was a typo.

Patch 2 adds the usual HTM prologue for regsets to the tm_cgpr32
get/set functions, so that the cgprs are flushed. I don't really
understand the need for flushing the fp and altivec states, but I
copied that over since it was done in the regular tm_cgpr get/set
functions.

Patch 3 changes the pmu get/set functions so that they don't read or
write outside the bounds of thread_struct.mmcr0. The endianess of the
kernel is used to determine where the mmcr0 word should be placed (or
read from) in its corresponding 64-bit slot in the regset. I am not
sure if this is the correct way to go, or if the endianess of the
thread being traced should determine this position (can the kernel run
threads with a different endianess?). I used the kernel endianess
because that is what seems to happen for other registers smaller than
their regset fields (for instance, it seems that checkpointed CR is
saved by the kernel as a doubleword, so the the position of the word
depends on the kernel's endianess). The rest of the function assumes
that unsigned longs are doublewords, so the patch assumes that an
unsigned is a word. This patch (and the original pmu_get/set
functions) might not work if the kernel is compiled in 32 bits.

Patch 4 adds the VSX regset to compat_regsets, which could cause out
of bounds writes in fs/binfmt_elf.c.

Patch 5 adds the PMU regset to compat_regsets.

I also noticed that the regset for CGPRs for 32-bit threads has 48 * 8
bytes (same as the one for 64-bit threads), but the data only occupies
the first 48 * 4 bytes (like for the 32-bit GPR regset). I am not sure
if this was intended, or if it can be changed now that other programs
might already assume the 48 * 8 size. If the kernel is compiled in
32-bits, the size will change (because it depends on sizeof (long)),
but I don't know if HTM and the corresponding regsets are supported in
the first place for a 32-bit kernel.

I haven't added the PKEY regset to compat_regsets. Does that make
sense for 32-bit threads?

Pedro Franco de Carvalho (5):
  powerpc: Fix inverted active predicate for setting the EBB regset
  powerpc: Flush checkpointed gpr state for 32-bit processes in ptrace
  powerpc: Fix pmu get/set functions
  powerpc: Add VSX regset to compat_regsets
  powerpc: Add PMU regset to compat_regsets

 arch/powerpc/kernel/ptrace.c | 65 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 60 insertions(+), 5 deletions(-)

-- 
2.13.6

[RFC PATCH 1/5] powerpc: Fix inverted active predicate for setting the EBB regset

[Pedro Franco de Carvalho]

Currently, the ebb_set function for writing to the EBB regset returns
ENODATA when ebb is active in the thread, and copies in the data when
it is inactive. This patch inverts the condition so that it matches
ebb_get and ebb_active.
---
 arch/powerpc/kernel/ptrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index d23cf632edf0..6618570c6d56 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -1701,7 +1701,7 @@ static int ebb_set(struct task_struct *target,
 	if (!cpu_has_feature(CPU_FTR_ARCH_207S))
 		return -ENODEV;
 
-	if (target->thread.used_ebb)
+	if (!target->thread.used_ebb)
 		return -ENODATA;
 
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
-- 
2.13.6

[RFC PATCH 2/5] powerpc: Flush checkpointed gpr state for 32-bit processes in ptrace

[Pedro Franco de Carvalho]

Currently ptrace doesn't flush the register state when the
checkpointed GPRs of a 32-bit thread are accessed. This can cause core
dumps to have stale data in the checkpointed GPR note.
---
 arch/powerpc/kernel/ptrace.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 6618570c6d56..be8ca03a0bd5 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -2124,6 +2124,16 @@ static int tm_cgpr32_get(struct task_struct *target,
 		     unsigned int pos, unsigned int count,
 		     void *kbuf, void __user *ubuf)
 {
+	if (!cpu_has_feature(CPU_FTR_TM))
+		return -ENODEV;
+
+	if (!MSR_TM_ACTIVE(target->thread.regs->msr))
+		return -ENODATA;
+
+	flush_tmregs_to_thread(target);
+	flush_fp_to_thread(target);
+	flush_altivec_to_thread(target);
+
 	return gpr32_get_common(target, regset, pos, count, kbuf, ubuf,
 			&target->thread.ckpt_regs.gpr[0]);
 }
@@ -2133,6 +2143,16 @@ static int tm_cgpr32_set(struct task_struct *target,
 		     unsigned int pos, unsigned int count,
 		     const void *kbuf, const void __user *ubuf)
 {
+	if (!cpu_has_feature(CPU_FTR_TM))
+		return -ENODEV;
+
+	if (!MSR_TM_ACTIVE(target->thread.regs->msr))
+		return -ENODATA;
+
+	flush_tmregs_to_thread(target);
+	flush_fp_to_thread(target);
+	flush_altivec_to_thread(target);
+
 	return gpr32_set_common(target, regset, pos, count, kbuf, ubuf,
 			&target->thread.ckpt_regs.gpr[0]);
 }
-- 
2.13.6

[RFC PATCH 3/5] powerpc: Fix pmu get/set functions

[Pedro Franco de Carvalho]

The PMU regset exposed through ptrace has 5 64-bit words, which are
all copied in and out. However, mmcr0 in the thread_struct is an
unsigned, which causes pmu_set to clobber the next variable in the
thread_struct (used_ebb), and pmu_get to return the same variable in
one half of the mmcr0 slot.
---
 arch/powerpc/kernel/ptrace.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index be8ca03a0bd5..69123feaef9e 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -1733,6 +1733,9 @@ static int pmu_get(struct task_struct *target,
 		      unsigned int pos, unsigned int count,
 		      void *kbuf, void __user *ubuf)
 {
+	int ret = 0;
+	unsigned long mmcr0 = target->thread.mmcr0;
+
 	/* Build tests */
 	BUILD_BUG_ON(TSO(siar) + sizeof(unsigned long) != TSO(sdar));
 	BUILD_BUG_ON(TSO(sdar) + sizeof(unsigned long) != TSO(sier));
@@ -1742,9 +1745,16 @@ static int pmu_get(struct task_struct *target,
 	if (!cpu_has_feature(CPU_FTR_ARCH_207S))
 		return -ENODEV;
 
-	return user_regset_copyout(&pos, &count, &kbuf, &ubuf,
-			&target->thread.siar, 0,
+	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+		&target->thread.siar, 0,
+		4 * sizeof(unsigned long));
+
+	if (!ret)
+		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
+			&mmcr0, 4 * sizeof(unsigned long),
 			5 * sizeof(unsigned long));
+
+	return ret;
 }
 
 static int pmu_set(struct task_struct *target,
@@ -1754,6 +1764,12 @@ static int pmu_set(struct task_struct *target,
 {
 	int ret = 0;
 
+#ifdef __BIG_ENDIAN
+	int mmcr0_offset = sizeof(unsigned);
+#else
+	int mmcr0_offset = 0;
+#endif
+
 	/* Build tests */
 	BUILD_BUG_ON(TSO(siar) + sizeof(unsigned long) != TSO(sdar));
 	BUILD_BUG_ON(TSO(sdar) + sizeof(unsigned long) != TSO(sier));
@@ -1783,9 +1799,16 @@ static int pmu_set(struct task_struct *target,
 			4 * sizeof(unsigned long));
 
 	if (!ret)
+		ret = user_regset_copyin_ignore(&pos, &count, &kbuf,
+			&ubuf, 4 * sizeof(unsigned long),
+			4 * sizeof(unsigned long) + mmcr0_offset);
+
+	if (!ret)
 		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
-			&target->thread.mmcr0, 4 * sizeof(unsigned long),
-			5 * sizeof(unsigned long));
+			&target->thread.mmcr0,
+			4 * sizeof(unsigned long) + mmcr0_offset,
+			4 * sizeof(unsigned long) + mmcr0_offset
+			+ sizeof (unsigned));
 	return ret;
 }
 #endif
-- 
2.13.6

[RFC PATCH 4/5] powerpc: Add VSX regset to compat_regsets

[Pedro Franco de Carvalho]

This patch copies the the missing VSX regset to the compat_regsets
array.

Not having this regset can cause issues in fs/binfmt_elf.c in the
fill_thread_core_info function, which iterates over all the regsets
defined in compat_regsets to fill note info for a core dump of a
32-bit thread. However, the number of regset notes allocated for
writing is the number of regsets with core_note_type != 0. If the
regset array has an entry with core_note_type == 0, which is the case
for the missing VSX element, this can cause later regsets to be
written outside the bounds of the allocated notes.

The compat_regset is also missing entries for REGSET_PMR and
REGSET_PKEY, but because these are at the end of the powerpc_regset
enum, the designated initializers for the compat_regset array don't
cause implicit elements to be created, like they did for REGSET_VSX.
---
 arch/powerpc/kernel/ptrace.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 69123feaef9e..2da0668a96dc 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -2237,6 +2237,13 @@ static const struct user_regset compat_regsets[] = {
 		.active = vr_active, .get = vr_get, .set = vr_set
 	},
 #endif
+#ifdef CONFIG_VSX
+	[REGSET_VSX] = {
+		.core_note_type = NT_PPC_VSX, .n = 32,
+		.size = sizeof(double), .align = sizeof(double),
+		.active = vsr_active, .get = vsr_get, .set = vsr_set
+	},
+#endif
 #ifdef CONFIG_SPE
 	[REGSET_SPE] = {
 		.core_note_type = NT_PPC_SPE, .n = 35,
-- 
2.13.6

[RFC PATCH 5/5] powerpc: Add PMU regset to compat_regsets

[Pedro Franco de Carvalho]

This patch allows setting and getting PMU registers from 32-bit
threads.
---
 arch/powerpc/kernel/ptrace.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 2da0668a96dc..3a9c4ae65429 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -2317,6 +2317,11 @@ static const struct user_regset compat_regsets[] = {
 		.size = sizeof(u64), .align = sizeof(u64),
 		.active = ebb_active, .get = ebb_get, .set = ebb_set
 	},
+	[REGSET_PMR] = {
+		.core_note_type = NT_PPC_PMU, .n = ELF_NPMU,
+		.size = sizeof(u64), .align = sizeof(u64),
+		.active = pmu_active, .get = pmu_get, .set = pmu_set
+	},
 #endif
 };
 
-- 
2.13.6

Re: [RFC PATCH 1/5] powerpc: Fix inverted active predicate for setting the EBB regset

[Michael Ellerman]

Pedro Franco de Carvalho <pedromfc@linux.vnet.ibm.com> writes:

> Currently, the ebb_set function for writing to the EBB regset returns
> ENODATA when ebb is active in the thread, and copies in the data when
> it is inactive. This patch inverts the condition so that it matches
> ebb_get and ebb_active.
> ---
>  arch/powerpc/kernel/ptrace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Hi Pedro,

Thanks for looking into this, how did you detect this? Do you have a
test case?

I don't think Anshuman wrote it this way on purpose, but added him to Cc
in case he remembers.

But I don't think this fix is necessarily right. If we are setting the
EBB regs via ptrace then it doesn't matter if they were previously in
use or not, we should just set them. What *does* matter is that at the
end of the function we set used_ebb to true, because otherwise the
values we have set will not actually be used when the process is
rescheduled.

cheers

> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index d23cf632edf0..6618570c6d56 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -1701,7 +1701,7 @@ static int ebb_set(struct task_struct *target,
>  	if (!cpu_has_feature(CPU_FTR_ARCH_207S))
>  		return -ENODEV;
>  
> -	if (target->thread.used_ebb)
> +	if (!target->thread.used_ebb)
>  		return -ENODATA;
>  
>  	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> -- 
> 2.13.6

Re: [RFC PATCH 2/5] powerpc: Flush checkpointed gpr state for 32-bit processes in ptrace

[Michael Ellerman]

Pedro Franco de Carvalho <pedromfc@linux.vnet.ibm.com> writes:

> Currently ptrace doesn't flush the register state when the
> checkpointed GPRs of a 32-bit thread are accessed. This can cause core
> dumps to have stale data in the checkpointed GPR note.
> ---
>  arch/powerpc/kernel/ptrace.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 6618570c6d56..be8ca03a0bd5 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -2124,6 +2124,16 @@ static int tm_cgpr32_get(struct task_struct *target,
>  		     unsigned int pos, unsigned int count,
>  		     void *kbuf, void __user *ubuf)
>  {
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if (!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return -ENODATA;
> +
> +	flush_tmregs_to_thread(target);
> +	flush_fp_to_thread(target);
> +	flush_altivec_to_thread(target);

I think we already have 8 (!) copies of this logic in ptrace.c.

And you add two more, seems like it should be in a helper function.

Can you add a helper that does it and use that helper in these two
functions. Then if you can send me another patch that converts all the
other uses to use the new helper.

cheers

> @@ -2133,6 +2143,16 @@ static int tm_cgpr32_set(struct task_struct *target,
>  		     unsigned int pos, unsigned int count,
>  		     const void *kbuf, const void __user *ubuf)
>  {
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if (!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return -ENODATA;
> +
> +	flush_tmregs_to_thread(target);
> +	flush_fp_to_thread(target);
> +	flush_altivec_to_thread(target);
> +
>  	return gpr32_set_common(target, regset, pos, count, kbuf, ubuf,
>  			&target->thread.ckpt_regs.gpr[0]);
>  }
> -- 
> 2.13.6

Re: [RFC PATCH 1/5] powerpc: Fix inverted active predicate for setting the EBB regset

[Michael Ellerman]

Michael Ellerman <mpe@ellerman.id.au> writes:

> Pedro Franco de Carvalho <pedromfc@linux.vnet.ibm.com> writes:
>
>> Currently, the ebb_set function for writing to the EBB regset returns
>> ENODATA when ebb is active in the thread, and copies in the data when
>> it is inactive. This patch inverts the condition so that it matches
>> ebb_get and ebb_active.
>> ---
>>  arch/powerpc/kernel/ptrace.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Hi Pedro,
>
> Thanks for looking into this, how did you detect this? Do you have a
> test case?

I thought we had a test case for this, and it seems that we did, but it
never made it into mainline.

Anshuman wrote it originally but then passed it onto Simon who
resubmitted it and I merged it, but then I must have reverted the EBB
test because it was failing, see:

  http://patchwork.ozlabs.org/patch/676833/


So it would be good to get that test case working reliably. I'm not sure
if would have found this bug anyway, but it would be a start.

cheers

Re: [RFC PATCH 1/5] powerpc: Fix inverted active predicate for setting the EBB regset

[Pedro Franco de Carvalho]

Michael Ellerman <mpe@ellerman.id.au> writes:

> Hi Pedro,
>
> Thanks for looking into this, how did you detect this? Do you have a
> test case?

Hello,

I'm working on allowing these registers to be accessed through GDB,
which is where I saw this happen. Then I used a small program that
traces another, then reads and tries to write to the regset, but not in
linux selftest format.

> I don't think Anshuman wrote it this way on purpose, but added him to Cc
> in case he remembers.
>
> But I don't think this fix is necessarily right. If we are setting the
> EBB regs via ptrace then it doesn't matter if they were previously in
> use or not, we should just set them. What *does* matter is that at the
> end of the function we set used_ebb to true, because otherwise the
> values we have set will not actually be used when the process is
> rescheduled.

Now I'm not sure why the ptrace calls for the ebb regset are guarded
with used_ebb in the first place. The save/restore_sprs functions in
kernel/process.c seem to handle the ebb registers regardless of this
flag, and it seems to be possible for user programs to read and write to
these registers even without having first created a perf event.

The flag only appears to be used in perf/core_book3s.c in the
ebb_event_add function, and the pmu registers (mmcr0, etc) only seem to
be saved to and restored from the thread_struct through
ebb_switch_in/out. So maybe the original intent was to guard the
pmu_get/set functions with used_ebb instead?

I'm not sure about this, because I don't know if it possible for a
ptrace call to happen between ebb_event_add and the first ebb_switch_in
for a thread.

Thanks!

--
Pedro

Re: [RFC PATCH 2/5] powerpc: Flush checkpointed gpr state for 32-bit processes in ptrace

[Pedro Franco de Carvalho]

Michael Ellerman <mpe@ellerman.id.au> writes:

> Can you add a helper that does it and use that helper in these two
> functions. Then if you can send me another patch that converts all the
> other uses to use the new helper.

Yes, I'll do this.

Thanks!

--
Pedro

[PATCH 1/2] powerpc: Flush checkpointed gpr state for 32-bit processes in ptrace

[Pedro Franco de Carvalho]

Would something like this be ok?

I factored out the calls to flush_fp_to_thread and flush_altivec_to_thread,
although I don't really understand why these are necessary. The tm_cvsx_get/set
functions also calls flush_vsx_to_thread (outside of the helper function).

I also noticed that tm_ppr/dscr/tar_get/set functions don't flush the tm
state. Should they do it, and if so, should they also flush the fp and altivec
state?

Thanks!
Pedro

-- >8 --
Currently ptrace doesn't flush the register state when the
checkpointed GPRs of a 32-bit thread are accessed. This can cause core
dumps to have stale data in the checkpointed GPR note.

This patch adds a helper function to flush the TM, fpu and altivec
state and calls it from the tm_cgpr32_get/set functions.

Signed-off-by: Pedro Franco de Carvalho <pedromfc@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/ptrace.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 9667666eb18e..0d56857e1e89 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -778,6 +778,29 @@ static int evr_set(struct task_struct *target, const struct user_regset *regset,
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 /**
+ * tm_flush_if_active - flush TM, fpu and altivec state if TM active
+ * @target:	The target task.
+ *
+ * This function flushes the TM, fpu and altivec state to the target
+ * task and returns 0 if TM is available and active in the target, and
+ * returns an error code suitable for ptrace otherwise.
+ */
+static int tm_flush_if_active (struct task_struct *target)
+{
+	if (!cpu_has_feature(CPU_FTR_TM))
+		return -ENODEV;
+
+	if (!MSR_TM_ACTIVE(target->thread.regs->msr))
+		return -ENODATA;
+
+	flush_tmregs_to_thread(target);
+	flush_fp_to_thread(target);
+	flush_altivec_to_thread(target);
+
+	return 0;
+}
+
+/**
  * tm_cgpr_active - get active number of registers in CGPR
  * @target:	The target task.
  * @regset:	The user regset structure.
@@ -2124,6 +2147,11 @@ static int tm_cgpr32_get(struct task_struct *target,
 		     unsigned int pos, unsigned int count,
 		     void *kbuf, void __user *ubuf)
 {
+	int ret = tm_flush_if_active(target);
+
+	if (ret)
+		return ret;
+
 	return gpr32_get_common(target, regset, pos, count, kbuf, ubuf,
 			&target->thread.ckpt_regs.gpr[0]);
 }
@@ -2133,6 +2161,11 @@ static int tm_cgpr32_set(struct task_struct *target,
 		     unsigned int pos, unsigned int count,
 		     const void *kbuf, const void __user *ubuf)
 {
+	int ret = tm_flush_if_active(target);
+
+	if (ret)
+		return ret;
+
 	return gpr32_set_common(target, regset, pos, count, kbuf, ubuf,
 			&target->thread.ckpt_regs.gpr[0]);
 }
-- 
2.13.6

[PATCH 2/2] powerpc: Use helper function to flush TM state in ptrace

[Pedro Franco de Carvalho]

This patch updates the get/set regset functions that flush tm, fpu and
altvec state when TM is available and active to use a helper function.

Signed-off-by: Pedro Franco de Carvalho <pedromfc@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/ptrace.c | 104 ++++++++++++-------------------------------
 1 file changed, 28 insertions(+), 76 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 0d56857e1e89..84fa97587850 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -845,17 +845,10 @@ static int tm_cgpr_get(struct task_struct *target,
 			unsigned int pos, unsigned int count,
 			void *kbuf, void __user *ubuf)
 {
-	int ret;
-
-	if (!cpu_has_feature(CPU_FTR_TM))
-		return -ENODEV;
-
-	if (!MSR_TM_ACTIVE(target->thread.regs->msr))
-		return -ENODATA;
+	int ret = tm_flush_if_active(target);
 
-	flush_tmregs_to_thread(target);
-	flush_fp_to_thread(target);
-	flush_altivec_to_thread(target);
+	if (ret)
+		return ret;
 
 	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
 				  &target->thread.ckpt_regs,
@@ -910,17 +903,11 @@ static int tm_cgpr_set(struct task_struct *target,
 			const void *kbuf, const void __user *ubuf)
 {
 	unsigned long reg;
-	int ret;
-
-	if (!cpu_has_feature(CPU_FTR_TM))
-		return -ENODEV;
 
-	if (!MSR_TM_ACTIVE(target->thread.regs->msr))
-		return -ENODATA;
+	int ret = tm_flush_if_active(target);
 
-	flush_tmregs_to_thread(target);
-	flush_fp_to_thread(target);
-	flush_altivec_to_thread(target);
+	if (ret)
+		return ret;
 
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
 				 &target->thread.ckpt_regs,
@@ -1014,15 +1001,10 @@ static int tm_cfpr_get(struct task_struct *target,
 	u64 buf[33];
 	int i;
 
-	if (!cpu_has_feature(CPU_FTR_TM))
-		return -ENODEV;
-
-	if (!MSR_TM_ACTIVE(target->thread.regs->msr))
-		return -ENODATA;
+	int ret = tm_flush_if_active(target);
 
-	flush_tmregs_to_thread(target);
-	flush_fp_to_thread(target);
-	flush_altivec_to_thread(target);
+	if (ret)
+		return ret;
 
 	/* copy to local buffer then write that out */
 	for (i = 0; i < 32 ; i++)
@@ -1060,15 +1042,10 @@ static int tm_cfpr_set(struct task_struct *target,
 	u64 buf[33];
 	int i;
 
-	if (!cpu_has_feature(CPU_FTR_TM))
-		return -ENODEV;
-
-	if (!MSR_TM_ACTIVE(target->thread.regs->msr))
-		return -ENODATA;
+	int ret = tm_flush_if_active(target);
 
-	flush_tmregs_to_thread(target);
-	flush_fp_to_thread(target);
-	flush_altivec_to_thread(target);
+	if (ret)
+		return ret;
 
 	for (i = 0; i < 32; i++)
 		buf[i] = target->thread.TS_CKFPR(i);
@@ -1131,20 +1108,12 @@ static int tm_cvmx_get(struct task_struct *target,
 			unsigned int pos, unsigned int count,
 			void *kbuf, void __user *ubuf)
 {
-	int ret;
-
-	BUILD_BUG_ON(TVSO(vscr) != TVSO(vr[32]));
-
-	if (!cpu_has_feature(CPU_FTR_TM))
-		return -ENODEV;
+	int ret = tm_flush_if_active(target);
 
-	if (!MSR_TM_ACTIVE(target->thread.regs->msr))
-		return -ENODATA;
+	if (ret)
+		return ret;
 
-	/* Flush the state */
-	flush_tmregs_to_thread(target);
-	flush_fp_to_thread(target);
-	flush_altivec_to_thread(target);
+	BUILD_BUG_ON(TVSO(vscr) != TVSO(vr[32]));
 
 	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
 					&target->thread.ckvr_state, 0,
@@ -1193,19 +1162,12 @@ static int tm_cvmx_set(struct task_struct *target,
 			unsigned int pos, unsigned int count,
 			const void *kbuf, const void __user *ubuf)
 {
-	int ret;
-
-	BUILD_BUG_ON(TVSO(vscr) != TVSO(vr[32]));
-
-	if (!cpu_has_feature(CPU_FTR_TM))
-		return -ENODEV;
+	int ret = tm_flush_if_active(target);
 
-	if (!MSR_TM_ACTIVE(target->thread.regs->msr))
-		return -ENODATA;
+	if (ret)
+		return ret;
 
-	flush_tmregs_to_thread(target);
-	flush_fp_to_thread(target);
-	flush_altivec_to_thread(target);
+	BUILD_BUG_ON(TVSO(vscr) != TVSO(vr[32]));
 
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
 					&target->thread.ckvr_state, 0,
@@ -1276,18 +1238,13 @@ static int tm_cvsx_get(struct task_struct *target,
 			void *kbuf, void __user *ubuf)
 {
 	u64 buf[32];
-	int ret, i;
+	int i;
 
-	if (!cpu_has_feature(CPU_FTR_TM))
-		return -ENODEV;
+	int ret = tm_flush_if_active(target);
 
-	if (!MSR_TM_ACTIVE(target->thread.regs->msr))
-		return -ENODATA;
+	if (ret)
+		return ret;
 
-	/* Flush the state */
-	flush_tmregs_to_thread(target);
-	flush_fp_to_thread(target);
-	flush_altivec_to_thread(target);
 	flush_vsx_to_thread(target);
 
 	for (i = 0; i < 32 ; i++)
@@ -1324,18 +1281,13 @@ static int tm_cvsx_set(struct task_struct *target,
 			const void *kbuf, const void __user *ubuf)
 {
 	u64 buf[32];
-	int ret, i;
+	int i;
 
-	if (!cpu_has_feature(CPU_FTR_TM))
-		return -ENODEV;
+	int ret = tm_flush_if_active(target);
 
-	if (!MSR_TM_ACTIVE(target->thread.regs->msr))
-		return -ENODATA;
+	if (ret)
+		return ret;
 
-	/* Flush the state */
-	flush_tmregs_to_thread(target);
-	flush_fp_to_thread(target);
-	flush_altivec_to_thread(target);
 	flush_vsx_to_thread(target);
 
 	for (i = 0; i < 32 ; i++)
-- 
2.13.6

Re: [PATCH 1/2] powerpc: Flush checkpointed gpr state for 32-bit processes in ptrace

[Christophe Leroy]

Le 19/06/2018 à 21:54, Pedro Franco de Carvalho a écrit :
> Would something like this be ok?
> 
> I factored out the calls to flush_fp_to_thread and flush_altivec_to_thread,
> although I don't really understand why these are necessary. The tm_cvsx_get/set
> functions also calls flush_vsx_to_thread (outside of the helper function).
> 
> I also noticed that tm_ppr/dscr/tar_get/set functions don't flush the tm
> state. Should they do it, and if so, should they also flush the fp and altivec
> state?
> 
> Thanks!
> Pedro
> 
> -- >8 --
> Currently ptrace doesn't flush the register state when the
> checkpointed GPRs of a 32-bit thread are accessed. This can cause core
> dumps to have stale data in the checkpointed GPR note.
> 
> This patch adds a helper function to flush the TM, fpu and altivec
> state and calls it from the tm_cgpr32_get/set functions.

This patch is almost 6 yr old and doesn't apply anymore.

If someone thinks it is still relevant, please rebase and resubmit.

Thanks
Christophe

> 
> Signed-off-by: Pedro Franco de Carvalho <pedromfc@linux.vnet.ibm.com>
> ---
>   arch/powerpc/kernel/ptrace.c | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 9667666eb18e..0d56857e1e89 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -778,6 +778,29 @@ static int evr_set(struct task_struct *target, const struct user_regset *regset,
>   
>   #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>   /**
> + * tm_flush_if_active - flush TM, fpu and altivec state if TM active
> + * @target:	The target task.
> + *
> + * This function flushes the TM, fpu and altivec state to the target
> + * task and returns 0 if TM is available and active in the target, and
> + * returns an error code suitable for ptrace otherwise.
> + */
> +static int tm_flush_if_active (struct task_struct *target)
> +{
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		return -ENODEV;
> +
> +	if (!MSR_TM_ACTIVE(target->thread.regs->msr))
> +		return -ENODATA;
> +
> +	flush_tmregs_to_thread(target);
> +	flush_fp_to_thread(target);
> +	flush_altivec_to_thread(target);
> +
> +	return 0;
> +}
> +
> +/**
>    * tm_cgpr_active - get active number of registers in CGPR
>    * @target:	The target task.
>    * @regset:	The user regset structure.
> @@ -2124,6 +2147,11 @@ static int tm_cgpr32_get(struct task_struct *target,
>   		     unsigned int pos, unsigned int count,
>   		     void *kbuf, void __user *ubuf)
>   {
> +	int ret = tm_flush_if_active(target);
> +
> +	if (ret)
> +		return ret;
> +
>   	return gpr32_get_common(target, regset, pos, count, kbuf, ubuf,
>   			&target->thread.ckpt_regs.gpr[0]);
>   }
> @@ -2133,6 +2161,11 @@ static int tm_cgpr32_set(struct task_struct *target,
>   		     unsigned int pos, unsigned int count,
>   		     const void *kbuf, const void __user *ubuf)
>   {
> +	int ret = tm_flush_if_active(target);
> +
> +	if (ret)
> +		return ret;
> +
>   	return gpr32_set_common(target, regset, pos, count, kbuf, ubuf,
>   			&target->thread.ckpt_regs.gpr[0]);
>   }