Re: [PATCH] sparc64: Swap registers for fault code and address in mna trap

Re: [PATCH] sparc64: Swap registers for fault code and address in mna trap

[Xose Vazquez Perez]

Hisashi Kanda wrote:

> In fact, BUG() in do_sparc64_fault occurred in modified version
> of linux-2.6.25.8 on SPARC64VIIIfx. I'm misunderstanding that
> the same problem remains in the latest.

Fujitsu K computer, with SPARC64 VIIIfx, runs a heavily modified
2.6.25.8 kernel. And maybe also binutils, glibc, gcc, gdb, ...
Even UTS_MACHINE was changed from sparc64 to s64fx.

[PATCH] sparc64: Swap registers for fault code and address in mna trap

[神田 尚]

From: "Hisashi Kanda" <hikanda@zlab.co.jp>

This bug may occur in the following.

user_rtt_fill_64bit          <= If mna trap occurred, call do_mna
+-> do_mna                   <= Mistake storing registers for fault code and address
    +-> winfix_mna
        +-> user_rtt_fill_fixup  <= Put fault address into thread_info->flag's TI_FAULT_CODE 
            +-> do_sparc64_fault() <= If fault address has FAULT_CODE_ITLB and FAULT_CODE_DTLB bits, call BUG()
                +-> BUG()

If mna trap occured in user_rtt_fill_64bit, then do_mna is called.
So, fault address is loaded into %g4, and fault code is loaded into %g5 in do_mna.
But, %g4 is stored into thread_info->flag's TI_FAULT_CODE, and
%g5 is stored into thread_info->flag's TI_FAULT_ADDR in user_rtt_fill_fixup.
This is a mistake. If fault address has FAULT_CODE_ITLB and 
FAULT_CODE_DTLB bits, BUG() may occur in do_sparc64_fault().
Therefore, %g4, %g5 should be swapped in winfix_mna.

Signed-off-by: Hisashi Kanda <hikanda@zlab.co.jp>

---
This patch is applied to linux-4.7 rc3

diff --git a/arch/sparc/kernel/misctrap.S b/arch/sparc/kernel/misctrap.S
index 34b4933..0cfb367 100644
--- a/arch/sparc/kernel/misctrap.S
+++ b/arch/sparc/kernel/misctrap.S
@@ -35,7 +35,7 @@ do_mna:
 	ldxa		[%g3] ASI_DMMU, %g5
 	stxa		%g0, [%g3] ASI_DMMU	! Clear FaultValid bit
 	membar		#Sync
-	bgu,pn		%icc, winfix_mna
+	bgu,pn		%icc, winfix_mna_swap
 	 rdpr		%tpc, %g3
 
 1:	sethi		%hi(109f), %g7
diff --git a/arch/sparc/kernel/winfixup.S b/arch/sparc/kernel/winfixup.S
index 855019a..8359a1b 100644
--- a/arch/sparc/kernel/winfixup.S
+++ b/arch/sparc/kernel/winfixup.S
@@ -103,6 +103,11 @@ spill_fixup_dax:
 	 add	%sp, PTREGS_OFF, %o0
 	ba,a,pt	%xcc, rtrap
 
+winfix_mna_swap:
+	mov	%g4, %g3	! swapping %g4 and %g5 using %g3
+	mov	%g5, %g4	! %g4=SFSR
+	mov	%g3, %g5	! %g5=SFAR
+	rdpr	%tpc, %g3
 winfix_mna:
 	andn	%g3, 0x7f, %g3
 	add	%g3, 0x78, %g3

Re: [PATCH] sparc64: Swap registers for fault code and address in mna trap

[David Miller]

From: 神田　尚 <hikanda@zlab.co.jp>
Date: Fri, 17 Jun 2016 01:49:11 +0000

> This bug may occur in the following.
>
> user_rtt_fill_64bit          <= If mna trap occurred, call do_mna
> +-> do_mna                   <= Mistake storing registers for fault code and address
>     +-> winfix_mna
>         +-> user_rtt_fill_fixup  <= Put fault address into thread_info->flag's TI_FAULT_CODE 
>             +-> do_sparc64_fault() <= If fault address has FAULT_CODE_ITLB and FAULT_CODE_DTLB bits, call BUG()
>                 +-> BUG()

We should not be invoking do_sparc64_fault() in this case.

Instead, we call either sun4v_do_mna() or mem_address_unaligned().

Neither of which care about the values stored in the thread's fault
address and code.

If you can really trigger this code path, please post the kernel log
backtrace that happens when the BUG() triggers.  That way we can
figure out what the real problem is.

Your patch is also wrong for other reasons, it would break the
unaligned trap code paths that don't go via user_rtt_fill_64bit
fixups.

RE: [PATCH] sparc64: Swap registers for fault code and address in mna trap

[神田 尚]

Hi, David

Thank you for your reply.

> If you can really trigger this code path, please post the kernel log
> backtrace that happens when the BUG() triggers.  That way we can
> figure out what the real problem is.
I'm sorry I cannot show the information. I don't have now.
In fact, BUG() in do_sparc64_fault occurred in modified version
of linux-2.6.25.8 on SPARC64VIIIfx. I'm misunderstanding that
the same problem remains in the latest. 

> Your patch is also wrong for other reasons, it would break the
> unaligned trap code paths that don't go via user_rtt_fill_64bit
> fixups.
And from the first, I was wrong. I got it.

I have some questions.
I think that if %tl > 1, winfix_mna will be called. Is it correct?
And, the call trace is never occurred?

________________________________________
From: David Miller <davem@davemloft.net>
Date: 2016年6月18日 15:12
To: 神田　尚
CC: sparclinux@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] sparc64: Swap registers for fault code and address in mna trap

From: 神田　尚 <hikanda@zlab.co.jp>
Date: Fri, 17 Jun 2016 01:49:11 +0000

> This bug may occur in the following.
>
> user_rtt_fill_64bit          <= If mna trap occurred, call do_mna
> +-> do_mna                   <= Mistake storing registers for fault code and address
>     +-> winfix_mna
>         +-> user_rtt_fill_fixup  <= Put fault address into thread_info->flag's TI_FAULT_CODE
>             +-> do_sparc64_fault() <= If fault address has FAULT_CODE_ITLB and FAULT_CODE_DTLB bits, call BUG()
>                 +-> BUG()

We should not be invoking do_sparc64_fault() in this case.

Instead, we call either sun4v_do_mna() or mem_address_unaligned().

Neither of which care about the values stored in the thread's fault
address and code.

If you can really trigger this code path, please post the kernel log
backtrace that happens when the BUG() triggers.  That way we can
figure out what the real problem is.

Your patch is also wrong for other reasons, it would break the
unaligned trap code paths that don't go via user_rtt_fill_64bit
fixups.

Re: [PATCH] sparc64: Swap registers for fault code and address in mna trap

[David Miller]

From: 神田　尚 <hikanda@zlab.co.jp>
Date: Tue, 21 Jun 2016 10:36:42 +0000

> Hi, David
> 
> Thank you for your reply.
> 
>> If you can really trigger this code path, please post the kernel log
>> backtrace that happens when the BUG() triggers.  That way we can
>> figure out what the real problem is.
> I'm sorry I cannot show the information. I don't have now.
> In fact, BUG() in do_sparc64_fault occurred in modified version
> of linux-2.6.25.8 on SPARC64VIIIfx. I'm misunderstanding that
> the same problem remains in the latest.

You have to show me that the calltrace occurs with the latest
kernel.

[PATCH] sparc64: Swap registers for fault code and address in mna trap

[神田 尚]

From: "Hisashi Kanda" <hikanda at zlab dot co dot jp>

I found a logical bug in SPARC code.
So, I send this patch. Please check it.

This bug may occur in the following.

user_rtt_fill_64bit          <= If mna trap occurred, call do_mna
+-> do_mna                   <= Mistake storing registers for fault code and address
    +-> winfix_mna
        +-> user_rtt_fill_fixup  <= Put fault address into thread_info->flag's TI_FAULT_CODE 
            +-> do_sparc64_fault() <= If fault address has FAULT_CODE_ITLB and FAULT_CODE_DTLB bits, call BUG()
                +-> BUG()

If mna trap occured in user_rtt_fill_64bit, then do_mna is called.
So, fault address is loaded into %g4, and fault code is loaded into %g5 in do_mna.
But, %g4 is stored into thread_info->flag's TI_FAULT_CODE, and
%g5 is stored into thread_info->flag's TI_FAULT_ADDR in user_rtt_fill_fixup.
This is a mistake. If fault address has FAULT_CODE_ITLB and 
FAULT_CODE_DTLB bits, BUG() may occur in do_sparc64_fault().

The patch for this bug is the following.
Kernel version is Linux 4.7-rc3. 

Signed-off-by: Hisashi Kanda <hikanda@zlab.co.jp>

---

diff --git a/arch/sparc/kernel/misctrap.S b/arch/sparc/kernel/misctrap.S
index 34b4933..0cfb367 100644
--- a/arch/sparc/kernel/misctrap.S
+++ b/arch/sparc/kernel/misctrap.S
@@ -35,7 +35,7 @@ do_mna:
 	ldxa		[%g3] ASI_DMMU, %g5
 	stxa		%g0, [%g3] ASI_DMMU	! Clear FaultValid bit
 	membar		#Sync
-	bgu,pn		%icc, winfix_mna
+	bgu,pn		%icc, winfix_mna_swap
 	 rdpr		%tpc, %g3
 
 1:	sethi		%hi(109f), %g7
diff --git a/arch/sparc/kernel/winfixup.S b/arch/sparc/kernel/winfixup.S
index 855019a..8359a1b 100644
--- a/arch/sparc/kernel/winfixup.S
+++ b/arch/sparc/kernel/winfixup.S
@@ -103,6 +103,11 @@ spill_fixup_dax:
 	 add	%sp, PTREGS_OFF, %o0
 	ba,a,pt	%xcc, rtrap
 
+winfix_mna_swap:
+	mov	%g4, %g3	! swapping %g4 and %g5 using %g3
+	mov	%g5, %g4	! %g4=SFSR
+	mov	%g3, %g5	! %g5=SFAR
+	rdpr	%tpc, %g3
 winfix_mna:
 	andn	%g3, 0x7f, %g3
 	add	%g3, 0x78, %g3

Re: [PATCH] sparc64: Swap registers for fault code and address in mna trap

[Julian Calaby]

Hi Hisashi,

On Fri, Jun 17, 2016 at 10:05 AM, 神田　尚 <hikanda@zlab.co.jp> wrote:
> From: "Hisashi Kanda" <hikanda at zlab dot co dot jp>

Could you please put your real email address here?

> I found a logical bug in SPARC code.
> So, I send this patch. Please check it.

Leave this part out, we only need the explanation of the potential
bug, if you're not sure, put any notes like this below the "---".

> This bug may occur in the following.
>
> user_rtt_fill_64bit          <= If mna trap occurred, call do_mna
> +-> do_mna                   <= Mistake storing registers for fault code and address
>     +-> winfix_mna
>         +-> user_rtt_fill_fixup  <= Put fault address into thread_info->flag's TI_FAULT_CODE
>             +-> do_sparc64_fault() <= If fault address has FAULT_CODE_ITLB and FAULT_CODE_DTLB bits, call BUG()
>                 +-> BUG()
>
> If mna trap occured in user_rtt_fill_64bit, then do_mna is called.
> So, fault address is loaded into %g4, and fault code is loaded into %g5 in do_mna.
> But, %g4 is stored into thread_info->flag's TI_FAULT_CODE, and
> %g5 is stored into thread_info->flag's TI_FAULT_ADDR in user_rtt_fill_fixup.
> This is a mistake. If fault address has FAULT_CODE_ITLB and
> FAULT_CODE_DTLB bits, BUG() may occur in do_sparc64_fault().
>
> The patch for this bug is the following.
> Kernel version is Linux 4.7-rc3.

You should put the kernel version this applies to below the "---".

> Signed-off-by: Hisashi Kanda <hikanda@zlab.co.jp>
>
> ---

i.e. here.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH] sparc64: Swap registers for fault code and address in mna trap

[David Miller]

Your patch has been whitespace corrupted by your email client.

Also, the correct mailing list to submit Sparc patches to is
sparclinux@vger.kernel.org