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 <mi...@neuling.org>
>
> Reviewed-by: Breno Leitao <lei...@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 */
