[PATCH] powerpc: Clear branch trap (MSR.BE) before delivering SIGTRAP

[PATCH] powerpc: Clear branch trap (MSR.BE) before delivering SIGTRAP

[Matt Evans]

When using SIG_DBG_BRANCH_TRACING, MSR.BE is left enabled in the
user context when single_step_exception() prepares the SIGTRAP
delivery.  The resulting branch-trap-within-the-SIGTRAP-handler
isn't healthy.

Commit 2538c2d08f46141550a1e68819efa8fe31c6e3dc broke this, by
replacing an MSR mask operation of ~(MSR_SE | MSR_BE) with a call
to clear_single_step() which only clears MSR_SE.

This patch adds a new helper, clear_br_trace(), which clears the
debug trap before invoking the signal handler.  This helper is a
NOP for BookE as SIG_DBG_BRANCH_TRACING isn't supported on BookE.

Signed-off-by: Matt Evans <matt@ozlabs.org>
---
 arch/powerpc/kernel/traps.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 1e48d157196a..5eaab234e747 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -460,7 +460,7 @@ static inline int check_io_access(struct pt_regs *regs)
 /* single-step stuff */
 #define single_stepping(regs)	(current->thread.debug.dbcr0 & DBCR0_IC)
 #define clear_single_step(regs)	(current->thread.debug.dbcr0 &= ~DBCR0_IC)
-
+#define clear_br_trace(regs)	do {} while(0)
 #else
 /* On non-4xx, the reason for the machine check or program
    exception is in the MSR. */
@@ -473,6 +473,7 @@ static inline int check_io_access(struct pt_regs *regs)
 
 #define single_stepping(regs)	((regs)->msr & MSR_SE)
 #define clear_single_step(regs)	((regs)->msr &= ~MSR_SE)
+#define clear_br_trace(regs)	((regs)->msr &= ~MSR_BE)
 #endif
 
 #if defined(CONFIG_E500)
@@ -988,6 +989,7 @@ void single_step_exception(struct pt_regs *regs)
 	enum ctx_state prev_state = exception_enter();
 
 	clear_single_step(regs);
+	clear_br_trace(regs);
 
 	if (kprobe_post_handler(regs))
 		return;
-- 
2.14.1

Re: [PATCH] powerpc: Clear branch trap (MSR.BE) before delivering SIGTRAP

[Michael Ellerman]

Matt Evans <matt@ozlabs.org> writes:

> When using SIG_DBG_BRANCH_TRACING, MSR.BE is left enabled in the
> user context when single_step_exception() prepares the SIGTRAP
> delivery.  The resulting branch-trap-within-the-SIGTRAP-handler
> isn't healthy.
>
> Commit 2538c2d08f46141550a1e68819efa8fe31c6e3dc broke this, by
> replacing an MSR mask operation of ~(MSR_SE | MSR_BE) with a call
> to clear_single_step() which only clears MSR_SE.
>
> This patch adds a new helper, clear_br_trace(), which clears the
> debug trap before invoking the signal handler.  This helper is a
> NOP for BookE as SIG_DBG_BRANCH_TRACING isn't supported on BookE.
>
> Signed-off-by: Matt Evans <matt@ozlabs.org>

Hi Matt!

It seems we might not be regularly testing this code :}

How did you hit/find the bug? And do you have a test case by any chance?

I found the test code at the bottom of:
  https://lwn.net/Articles/114587/

But it didn't immediately work.

cheers

Re: [PATCH] powerpc: Clear branch trap (MSR.BE) before delivering SIGTRAP

[Matt Evans]

Howdy Michael,

> On 28 Mar 2018, at 06:54, Michael Ellerman <mpe@ellerman.id.au> wrote:
>=20
> Matt Evans <matt@ozlabs.org> writes:
>=20
>> When using SIG_DBG_BRANCH_TRACING, MSR.BE is left enabled in the
>> user context when single_step_exception() prepares the SIGTRAP
>> delivery.  The resulting branch-trap-within-the-SIGTRAP-handler
>> isn't healthy.
>>=20
>> Commit 2538c2d08f46141550a1e68819efa8fe31c6e3dc broke this, by
>> replacing an MSR mask operation of ~(MSR_SE | MSR_BE) with a call
>> to clear_single_step() which only clears MSR_SE.
>>=20
>> This patch adds a new helper, clear_br_trace(), which clears the
>> debug trap before invoking the signal handler.  This helper is a
>> NOP for BookE as SIG_DBG_BRANCH_TRACING isn't supported on BookE.
>>=20
>> Signed-off-by: Matt Evans <matt@ozlabs.org>
>=20
> Hi Matt!
>=20
> It seems we might not be regularly testing this code :}

I know, rite? ;-)

> How did you hit/find the bug? And do you have a test case by any =
chance?
>=20
> I found the test code at the bottom of:
>  https://lwn.net/Articles/114587/
>=20
> But it didn't immediately work.

I'm using this feature as part of a debug harness I wrote to log a =
program=E2=80=99s control flow (to create a =E2=80=9Cknown good=E2=80=9D =
pattern to compare a PPC interpreter against).  So at least the feature =
has /one/ user.  ;-)

The symptoms of the bug are that if you use single-stepping you get a =
sequence of SIGTRAPs representing each instruction completion (good), =
but if you use branch tracing the process just dies with SIGTRAP (looks =
like it=E2=80=99s never caught by the signal handler).  What=E2=80=99s =
really happening is that there /is/ a signal delivered to the handler, =
but (because branch tracing is left on) that then causes a second debug =
exception from the handler itself, i.e. whilst SIGTRAP=E2=80=99s masked.

OK, let me have a dig to reduce my program to something very basic and =
I=E2=80=99ll post something =E2=80=94 sorry, I should=E2=80=99ve got a =
PoC ready before.  (I did start out inspired by that post you linked to, =
but IIRC I don=E2=80=99t think it worked out of the box for me either.)


Cheers,


Matt

Re: [PATCH] powerpc: Clear branch trap (MSR.BE) before delivering SIGTRAP

[Matt Evans]

Hi Michael,

> On 28 Mar 2018, at 11:36, Matt Evans <matt@ozlabs.org> wrote:
>=20
> Howdy Michael,
>=20
>> On 28 Mar 2018, at 06:54, Michael Ellerman <mpe@ellerman.id.au> =
wrote:
>>=20
>> Matt Evans <matt@ozlabs.org> writes:
>>=20
>>> When using SIG_DBG_BRANCH_TRACING, MSR.BE is left enabled in the
>>> user context when single_step_exception() prepares the SIGTRAP
>>> delivery.  The resulting branch-trap-within-the-SIGTRAP-handler
>>> isn't healthy.
>>>=20
>>> Commit 2538c2d08f46141550a1e68819efa8fe31c6e3dc broke this, by
>>> replacing an MSR mask operation of ~(MSR_SE | MSR_BE) with a call
>>> to clear_single_step() which only clears MSR_SE.
>>>=20
>>> This patch adds a new helper, clear_br_trace(), which clears the
>>> debug trap before invoking the signal handler.  This helper is a
>>> NOP for BookE as SIG_DBG_BRANCH_TRACING isn't supported on BookE.
>>>=20
>>> Signed-off-by: Matt Evans <matt@ozlabs.org>
>>=20
>> Hi Matt!
>>=20
>> It seems we might not be regularly testing this code :}
>=20
> I know, rite? ;-)
>=20
>> How did you hit/find the bug? And do you have a test case by any =
chance?
>>=20
>> I found the test code at the bottom of:
>> https://lwn.net/Articles/114587/
>>=20
>> But it didn't immediately work.
>=20
> I'm using this feature as part of a debug harness I wrote to log a =
program=E2=80=99s control flow (to create a =E2=80=9Cknown good=E2=80=9D =
pattern to compare a PPC interpreter against).  So at least the feature =
has /one/ user.  ;-)
>=20
> The symptoms of the bug are that if you use single-stepping you get a =
sequence of SIGTRAPs representing each instruction completion (good), =
but if you use branch tracing the process just dies with SIGTRAP (looks =
like it=E2=80=99s never caught by the signal handler).  What=E2=80=99s =
really happening is that there /is/ a signal delivered to the handler, =
but (because branch tracing is left on) that then causes a second debug =
exception from the handler itself, i.e. whilst SIGTRAP=E2=80=99s masked.
>=20
> OK, let me have a dig to reduce my program to something very basic and =
I=E2=80=99ll post something =E2=80=94 sorry, I should=E2=80=99ve got a =
PoC ready before.  (I did start out inspired by that post you linked to, =
but IIRC I don=E2=80=99t think it worked out of the box for me either.)


I=E2=80=99ve put a simple SIG_DBG_BRANCH_TRACING test program here:

	http://ozlabs.org/~matt/files/sig_dbg_brtrace_test.c

It=E2=80=99s commented regarding expected output.  I=E2=80=99ve only =
tested this on a G4 =E2=80=94 it should work on PPC64 too but the ISA =
says support for branch tracing is optional for an implementation.  =
I=E2=80=99d be interested in what POWERx does.  :)


Cheers,


Matt

Re: [PATCH] powerpc: Clear branch trap (MSR.BE) before delivering SIGTRAP

[Michael Ellerman]

Matt Evans <matt@ozlabs.org> writes:
>> On 28 Mar 2018, at 11:36, Matt Evans <matt@ozlabs.org> wrote:
>>> On 28 Mar 2018, at 06:54, Michael Ellerman <mpe@ellerman.id.au> wrote:
>>> Matt Evans <matt@ozlabs.org> writes:
>>>> When using SIG_DBG_BRANCH_TRACING, MSR.BE is left enabled in the
>>>> user context when single_step_exception() prepares the SIGTRAP
>>>> delivery.  The resulting branch-trap-within-the-SIGTRAP-handler
>>>> isn't healthy.
>>>>=20
>>>> Commit 2538c2d08f46141550a1e68819efa8fe31c6e3dc broke this, by
>>>> replacing an MSR mask operation of ~(MSR_SE | MSR_BE) with a call
>>>> to clear_single_step() which only clears MSR_SE.
>>>>=20
>>>> This patch adds a new helper, clear_br_trace(), which clears the
>>>> debug trap before invoking the signal handler.  This helper is a
>>>> NOP for BookE as SIG_DBG_BRANCH_TRACING isn't supported on BookE.
>>>>=20
>>>> Signed-off-by: Matt Evans <matt@ozlabs.org>
>>>=20
>>> Hi Matt!
>>>=20
>>> It seems we might not be regularly testing this code :}
>>=20
>> I know, rite? ;-)
>>=20
>>> How did you hit/find the bug? And do you have a test case by any chance?
>>>=20
>>> I found the test code at the bottom of:
>>> https://lwn.net/Articles/114587/
>>>=20
>>> But it didn't immediately work.
>>=20
>> I'm using this feature as part of a debug harness I wrote to log a
>> program=E2=80=99s control flow (to create a =E2=80=9Cknown good=E2=80=9D=
 pattern to compare a
>> PPC interpreter against). So at least the feature has /one/ user. ;-)

One is enough for us :)
=20
>> The symptoms of the bug are that if you use single-stepping you get a
>> sequence of SIGTRAPs representing each instruction completion (good),
>> but if you use branch tracing the process just dies with SIGTRAP
>> (looks like it=E2=80=99s never caught by the signal handler). What=E2=80=
=99s really
>> happening is that there /is/ a signal delivered to the handler, but
>> (because branch tracing is left on) that then causes a second debug
>> exception from the handler itself, i.e. whilst SIGTRAP=E2=80=99s masked.
>>=20
>> OK, let me have a dig to reduce my program to something very basic
>> and I=E2=80=99ll post something =E2=80=94 sorry, I should=E2=80=99ve got=
 a PoC ready before.
>> (I did start out inspired by that post you linked to, but IIRC I
>> don=E2=80=99t think it worked out of the box for me either.)
>
> I=E2=80=99ve put a simple SIG_DBG_BRANCH_TRACING test program here:
>
> 	http://ozlabs.org/~matt/files/sig_dbg_brtrace_test.c

Thanks.

> It=E2=80=99s commented regarding expected output. I=E2=80=99ve only teste=
d this on a
> G4 =E2=80=94 it should work on PPC64 too but the ISA says support for bra=
nch
> tracing is optional for an implementation. I=E2=80=99d be interested in w=
hat
> POWERx does. :)

I get no traps on any 64-bit machine I tried, including 970FX, Power6,
Power7, Power8, Power9.

So I guess it's never been implemented on "server" CPUs.

I'd be happy to turn your test program into a selftest, though it won't
actually catch bugs unless someone runs it on actual 32-bit systems :)

cheers

Re: powerpc: Clear branch trap (MSR.BE) before delivering SIGTRAP

[Michael Ellerman]

On Mon, 2018-03-26 at 16:55:21 UTC, Matt Evans wrote:
> When using SIG_DBG_BRANCH_TRACING, MSR.BE is left enabled in the
> user context when single_step_exception() prepares the SIGTRAP
> delivery.  The resulting branch-trap-within-the-SIGTRAP-handler
> isn't healthy.
> 
> Commit 2538c2d08f46141550a1e68819efa8fe31c6e3dc broke this, by
> replacing an MSR mask operation of ~(MSR_SE | MSR_BE) with a call
> to clear_single_step() which only clears MSR_SE.
> 
> This patch adds a new helper, clear_br_trace(), which clears the
> debug trap before invoking the signal handler.  This helper is a
> NOP for BookE as SIG_DBG_BRANCH_TRACING isn't supported on BookE.
> 
> Signed-off-by: Matt Evans <matt@ozlabs.org>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/0e524e761fc2157f1037e0f5d616cd

cheers