| Message ID | 20180924072704.6200-1-mikey@neuling.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cf13435b730a502e814c63c84d93db131e563f5f |
| Headers | show |
| Series | powerpc/tm: Fix userspace r13 corruption | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | success | Test checkpatch on branch next |
| snowpatch_ozlabs/build-ppc64le | success | Test build-ppc64le on branch next |
| snowpatch_ozlabs/build-ppc64be | success | Test build-ppc64be on branch next |
| snowpatch_ozlabs/build-ppc64e | success | Test build-ppc64e on branch next |
| snowpatch_ozlabs/build-ppc32 | success | Test build-ppc32 on branch next |
Hi Mikey, On 09/24/2018 04:27 AM, Michael Neuling wrote: > When we treclaim we store the userspace checkpointed r13 to a scratch > SPR and then later save the scratch SPR to the user thread struct. > > Unfortunately, this doesn't work as accessing the user thread struct > can take an SLB fault and the SLB fault handler will write the same > scratch SPRG that now contains the userspace r13. > > To fix this, we store r13 to the kernel stack (which can't fault) > before we access the user thread struct. > > Found by running P8 guest + powervm + disable_1tb_segments + TM. Seen > as a random userspace segfault with r13 looking like a kernel address. > > Signed-off-by: Michael Neuling <mikey@neuling.org> Reviewed-by: Breno Leitao <leitao@debian.org> I am wondering if the same problem could not happen with r1 as well, since r1 is kept in the paca->tm_scratch after MSR[RI] is enabled, in a code similar to this: std r1, PACATMSCRATCH(r13) .. li r11, MSR_RI mtmsrd r11, 1 .... ld r3, PACATMSCRATCH(r13) /* user r1 */ std r3, GPR1(r7) There might be a corruption if the exception that is raised just after MSR[RI] is enabled somehow exits through 'fast_exception_return' (being called by most of the exception handlers as I understand) which will overwrite paca->tm_scratch with the upcoming MSR (aka SRR1) just before the SRR1, as: ld r3,_MSR(r1) ... std r3, PACATMSCRATCH(r13) /* Stash returned-to MSR */ It seems that slb_miss_common and instruction_access_slb does not go through this path, but handle_page_fault calls ret_from_except_lite->fast_exception_return, which might cause this 'possible' issue. Anyway, I would like to investigate if this problem might happen or not for real, but I do not fully understand what are the exceptions that might be raised when just MSR[RI] is enabled.
On Mon, 2018-09-24 at 11:32 -0300, Breno Leitao wrote: > Hi Mikey, > > On 09/24/2018 04:27 AM, Michael Neuling wrote: > > When we treclaim we store the userspace checkpointed r13 to a scratch > > SPR and then later save the scratch SPR to the user thread struct. > > > > Unfortunately, this doesn't work as accessing the user thread struct > > can take an SLB fault and the SLB fault handler will write the same > > scratch SPRG that now contains the userspace r13. > > > > To fix this, we store r13 to the kernel stack (which can't fault) > > before we access the user thread struct. > > > > Found by running P8 guest + powervm + disable_1tb_segments + TM. Seen > > as a random userspace segfault with r13 looking like a kernel address. > > > > Signed-off-by: Michael Neuling <mikey@neuling.org> > > Reviewed-by: Breno Leitao <leitao@debian.org> > > I am wondering if the same problem could not happen with r1 as well, since r1 > is kept in the paca->tm_scratch after MSR[RI] is enabled, in a code similar > to this: > > std r1, PACATMSCRATCH(r13) > .. > li r11, MSR_RI > mtmsrd r11, 1 > .... > ld r3, PACATMSCRATCH(r13) /* user r1 */ > std r3, GPR1(r7) > > There might be a corruption if the exception that is raised just after > MSR[RI] is enabled somehow exits through 'fast_exception_return' (being > called by most of the exception handlers as I understand) which will > overwrite paca->tm_scratch with the upcoming MSR (aka SRR1) just before the > SRR1, as: > > ld r3,_MSR(r1) > > ... > std r3, PACATMSCRATCH(r13) /* Stash returned-to MSR */ > > > It seems that slb_miss_common and instruction_access_slb does not go through > this path, but handle_page_fault calls > ret_from_except_lite->fast_exception_return, which might cause this > 'possible' issue. Yeah, good catch! I think we are ok but it's delicate. I'll store it in the kernel stack and avoid PACATMSCRATCH before I turn RI on. This look ok? diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S index 701b0f5b09..ff781b2852 100644 --- a/arch/powerpc/kernel/tm.S +++ b/arch/powerpc/kernel/tm.S @@ -178,6 +178,12 @@ _GLOBAL(tm_reclaim) std r11, GPR11(r1) /* Temporary stash */ + /* Move r1 to kernel stack in case PACATMSCRATCH is used once + * we turn on RI + */ + ld r11, PACATMSCRATCH(r13) + std r11, GPR1(r1) + /* Store r13 away so we can free up the scratch SPR for the * SLB fault handler (needed once we start access the * thread_struct) @@ -214,7 +220,7 @@ _GLOBAL(tm_reclaim) SAVE_GPR(8, r7) /* user r8 */ SAVE_GPR(9, r7) /* user r9 */ SAVE_GPR(10, r7) /* user r10 */ - ld r3, PACATMSCRATCH(r13) /* user r1 */ + ld r3, GPR1(r1) /* user r1 */ ld r4, GPR7(r1) /* user r7 */ ld r5, GPR11(r1) /* user r11 */ ld r6, GPR12(r1) /* user r12 */
Hi Mikey, On 09/25/2018 02:24 AM, Michael Neuling wrote: > On Mon, 2018-09-24 at 11:32 -0300, Breno Leitao wrote: >> Hi Mikey, >> >> On 09/24/2018 04:27 AM, Michael Neuling wrote: >>> When we treclaim we store the userspace checkpointed r13 to a scratch >>> SPR and then later save the scratch SPR to the user thread struct. >>> >>> Unfortunately, this doesn't work as accessing the user thread struct >>> can take an SLB fault and the SLB fault handler will write the same >>> scratch SPRG that now contains the userspace r13. >>> >>> To fix this, we store r13 to the kernel stack (which can't fault) >>> before we access the user thread struct. >>> >>> Found by running P8 guest + powervm + disable_1tb_segments + TM. Seen >>> as a random userspace segfault with r13 looking like a kernel address. >>> >>> Signed-off-by: Michael Neuling <mikey@neuling.org> >> >> Reviewed-by: Breno Leitao <leitao@debian.org> >> >> I am wondering if the same problem could not happen with r1 as well, since r1 >> is kept in the paca->tm_scratch after MSR[RI] is enabled, in a code similar >> to this: >> >> std r1, PACATMSCRATCH(r13) >> .. >> li r11, MSR_RI >> mtmsrd r11, 1 >> .... >> ld r3, PACATMSCRATCH(r13) /* user r1 */ >> std r3, GPR1(r7) >> >> There might be a corruption if the exception that is raised just after >> MSR[RI] is enabled somehow exits through 'fast_exception_return' (being >> called by most of the exception handlers as I understand) which will >> overwrite paca->tm_scratch with the upcoming MSR (aka SRR1) just before the >> SRR1, as: >> >> ld r3,_MSR(r1) >> >> ... >> std r3, PACATMSCRATCH(r13) /* Stash returned-to MSR */ >> >> >> It seems that slb_miss_common and instruction_access_slb does not go through >> this path, but handle_page_fault calls >> ret_from_except_lite->fast_exception_return, which might cause this >> 'possible' issue. > > Yeah, good catch! I think we are ok but it's delicate. > > I'll store it in the kernel stack and avoid PACATMSCRATCH before I turn RI on. > > This look ok? Yes, That is exactly what I though when I looked at the code. Thanks for fixing it.
On Mon, 2018-09-24 at 07:27:04 UTC, Michael Neuling wrote: > When we treclaim we store the userspace checkpointed r13 to a scratch > SPR and then later save the scratch SPR to the user thread struct. > > Unfortunately, this doesn't work as accessing the user thread struct > can take an SLB fault and the SLB fault handler will write the same > scratch SPRG that now contains the userspace r13. > > To fix this, we store r13 to the kernel stack (which can't fault) > before we access the user thread struct. > > Found by running P8 guest + powervm + disable_1tb_segments + TM. Seen > as a random userspace segfault with r13 looking like a kernel address. > > Signed-off-by: Michael Neuling <mikey@neuling.org> > Reviewed-by: Breno Leitao <leitao@debian.org> Applied to powerpc fixes, thanks. https://git.kernel.org/powerpc/c/cf13435b730a502e814c63c84d93db cheers
diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S index 6bffbc5aff..701b0f5b09 100644 --- a/arch/powerpc/kernel/tm.S +++ b/arch/powerpc/kernel/tm.S @@ -176,13 +176,20 @@ _GLOBAL(tm_reclaim) std r1, PACATMSCRATCH(r13) ld r1, PACAR1(r13) - /* Store the PPR in r11 and reset to decent value */ std r11, GPR11(r1) /* Temporary stash */ + /* Store r13 away so we can free up the scratch SPR for the + * SLB fault handler (needed once we start access the + * thread_struct) + */ + GET_SCRATCH0(r11) + std r11, GPR13(r1) + /* Reset MSR RI so we can take SLB faults again */ li r11, MSR_RI mtmsrd r11, 1 + /* Store the PPR in r11 and reset to decent value */ mfspr r11, SPRN_PPR HMT_MEDIUM @@ -211,7 +218,7 @@ _GLOBAL(tm_reclaim) ld r4, GPR7(r1) /* user r7 */ ld r5, GPR11(r1) /* user r11 */ ld r6, GPR12(r1) /* user r12 */ - GET_SCRATCH0(8) /* user r13 */ + ld r8, GPR13(r1) /* user r13 */ std r3, GPR1(r7) std r4, GPR7(r7) std r5, GPR11(r7)
When we treclaim we store the userspace checkpointed r13 to a scratch SPR and then later save the scratch SPR to the user thread struct. Unfortunately, this doesn't work as accessing the user thread struct can take an SLB fault and the SLB fault handler will write the same scratch SPRG that now contains the userspace r13. To fix this, we store r13 to the kernel stack (which can't fault) before we access the user thread struct. Found by running P8 guest + powervm + disable_1tb_segments + TM. Seen as a random userspace segfault with r13 looking like a kernel address. Signed-off-by: Michael Neuling <mikey@neuling.org> --- arch/powerpc/kernel/tm.S | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)