[PATCH 1/2] powerpc: Flush checkpointed gpr state for 32-bit processes in ptrace
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 --- 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, >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, >thread.ckpt_regs.gpr[0]); } -- 2.13.6
[PATCH 2/2] powerpc: Use helper function to flush TM state in ptrace
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 --- 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(, , , , >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(, , , , >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(, , , , >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(, , , , >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(targe
Re: [RFC PATCH 2/5] powerpc: Flush checkpointed gpr state for 32-bit processes in ptrace
Michael Ellerman 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
Re: [RFC PATCH 1/5] powerpc: Fix inverted active predicate for setting the EBB regset
Michael Ellerman 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
[RFC PATCH 2/5] powerpc: Flush checkpointed gpr state for 32-bit processes in ptrace
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, >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, >thread.ckpt_regs.gpr[0]); } -- 2.13.6
[RFC PATCH 4/5] powerpc: Add VSX regset to compat_regsets
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
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
[RFC PATCH 3/5] powerpc: Fix pmu get/set functions
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(, , , , - >thread.siar, 0, + ret = user_regset_copyout(, , , , + >thread.siar, 0, + 4 * sizeof(unsigned long)); + + if (!ret) + ret = user_regset_copyout(, , , , + , 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(, , , + , 4 * sizeof(unsigned long), + 4 * sizeof(unsigned long) + mmcr0_offset); + + if (!ret) ret = user_regset_copyin(, , , , - >thread.mmcr0, 4 * sizeof(unsigned long), - 5 * sizeof(unsigned long)); + >thread.mmcr0, + 4 * sizeof(unsigned long) + mmcr0_offset, + 4 * sizeof(unsigned long) + mmcr0_offset + + sizeof (unsigned)); return ret; } #endif -- 2.13.6
[RFC PATCH 1/5] powerpc: Fix inverted active predicate for setting the EBB regset
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(, , , , -- 2.13.6
[RFC PATCH 0/5] powerpc: Misc. ptrace regset fixes
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