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 <redacted>
---
arch/powerpc/kernel/tm.S | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
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)
--
2.17.1
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 <redacted>
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,
=20
On 09/24/2018 04:27 AM, Michael Neuling wrote:
quoted
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.
=20
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.
=20
To fix this, we store r13 to the kernel stack (which can't fault)
before we access the user thread struct.
=20
Found by running P8 guest + powervm + disable_1tb_segments + TM. Seen
as a random userspace segfault with r13 looking like a kernel address.
=20
Signed-off-by: Michael Neuling <redacted>
=20
Reviewed-by: Breno Leitao <leitao@debian.org>
=20
I am wondering if the same problem could not happen with r1 as well, sinc=
e r1
is kept in the paca->tm_scratch after MSR[RI] is enabled, in a code simil=
ar
to this:
=20
std r1, PACATMSCRATCH(r13)
..
li r11, MSR_RI
mtmsrd r11, 1
....
ld r3, PACATMSCRATCH(r13) /* user r1 */
std r3, GPR1(r7)
=20
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 t=
he
SRR1, as:
=20
ld r3,_MSR(r1)
=20
...
std r3, PACATMSCRATCH(r13) /* Stash returned-to MSR */
=20
=20
It seems that slb_miss_common and instruction_access_slb does not go thro=
ugh
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.=20
I'll store it in the kernel stack and avoid PACATMSCRATCH before I turn RI =
on. =20
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)
=20
std r11, GPR11(r1) /* Temporary stash */
=20
+ /* 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:
quoted
Hi Mikey,
On 09/24/2018 04:27 AM, Michael Neuling wrote:
quoted
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 <redacted>
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 <redacted>
Reviewed-by: Breno Leitao <leitao@debian.org>
Applied to powerpc fixes, thanks.
https://git.kernel.org/powerpc/c/cf13435b730a502e814c63c84d93db
cheers