[PATCH] powerpc: signals: Discard transaction state from signal frames

[PATCH] powerpc: signals: Discard transaction state from signal frames

[Cyril Bur]

Userspace can begin and suspend a transaction within the signal
handler which means they might enter sys_rt_sigreturn() with the
processor in suspended state.

sys_rt_sigreturn() wants to restore process context (which may have
been in a transaction before signal delivery). To do this it must
restore TM SPRS. To achieve this, any transaction initiated within the
signal frame must be discarded in order to be able to restore TM SPRs
as TM SPRs can only be manipulated non-transactionally..
>From the PowerPC ISA:
  TM Bad Thing Exception [Category: Transactional Memory]
   An attempt is made to execute a mtspr targeting a TM register in
   other than Non-transactional state.

Not doing so results in a TM Bad Thing:
[12045.221359] Kernel BUG at c000000000050a40 [verbose debug info unavailable]
[12045.221470] Unexpected TM Bad Thing exception at c000000000050a40 (msr 0x201033)
[12045.221540] Oops: Unrecoverable exception, sig: 6 [#1]
[12045.221586] SMP NR_CPUS=2048 NUMA PowerNV
[12045.221634] Modules linked in: xt_CHECKSUM iptable_mangle ipt_MASQUERADE
 nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4
 xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp bridge stp llc ebtable_filter
 ebtables ip6table_filter ip6_tables iptable_filter ip_tables x_tables kvm_hv kvm
 uio_pdrv_genirq ipmi_powernv uio powernv_rng ipmi_msghandler autofs4 ses enclosure
 scsi_transport_sas bnx2x ipr mdio libcrc32c
[12045.222167] CPU: 68 PID: 6178 Comm: sigreturnpanic Not tainted 4.7.0 #34
[12045.222224] task: c0000000fce38600 ti: c0000000fceb4000 task.ti: c0000000fceb4000
[12045.222293] NIP: c000000000050a40 LR: c0000000000163bc CTR: 0000000000000000
[12045.222361] REGS: c0000000fceb7ac0 TRAP: 0700   Not tainted (4.7.0)
[12045.222418] MSR: 9000000300201033 <SF,HV,ME,IR,DR,RI,LE,TM[SE]> CR: 28444280  XER: 20000000
[12045.222625] CFAR: c0000000000163b8 SOFTE: 0 PACATMSCRATCH: 900000014280f033
GPR00: 01100000b8000001 c0000000fceb7d40 c00000000139c100 c0000000fce390d0
GPR04: 900000034280f033 0000000000000000 0000000000000000 0000000000000000
GPR08: 0000000000000000 b000000000001033 0000000000000001 0000000000000000
GPR12: 0000000000000000 c000000002926400 0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR24: 0000000000000000 00003ffff98cadd0 00003ffff98cb470 0000000000000000
GPR28: 900000034280f033 c0000000fceb7ea0 0000000000000001 c0000000fce390d0
[12045.223535] NIP [c000000000050a40] tm_restore_sprs+0xc/0x1c
[12045.223584] LR [c0000000000163bc] tm_recheckpoint+0x5c/0xa0
[12045.223630] Call Trace:
[12045.223655] [c0000000fceb7d80] [c000000000026e74] sys_rt_sigreturn+0x494/0x6c0
[12045.223738] [c0000000fceb7e30] [c0000000000092e0] system_call+0x38/0x108
[12045.223806] Instruction dump:
[12045.223841] 7c800164 4e800020 7c0022a6 f80304a8 7c0222a6 f80304b0 7c0122a6 f80304b8
[12045.223955] 4e800020 e80304a8 7c0023a6 e80304b0 <7c0223a6> e80304b8 7c0123a6 4e800020
[12045.224074] ---[ end trace cb8002ee240bae76 ]---

It isn't clear exactly if there is really a use case for userspace
returning with a suspended transaction, however, doing so doesn't
(on its own) constitute a bad frame. As such, this patch simply
discards the transaction state and continues.

Reported-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 arch/powerpc/kernel/signal_32.c | 13 +++++++++++++
 arch/powerpc/kernel/signal_64.c | 12 ++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index b6aa378..f892b93 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -1219,6 +1219,19 @@ long sys_rt_sigreturn(int r3, int r4, int r5, int r6, int r7, int r8,
 	unsigned long tmp;
 	int tm_restore = 0;
 #endif
+
+	/*
+	 * We always send the user to their signal handler non
+	 * transactionally. If there is a transactional/suspended state
+	 * then throw it away. The purpose of a sigreturn is to destroy
+	 * all traces of the signal frame, this includes any transactional
+	 * state created within in.
+	 * The cause is not important as there will never be a
+	 * recheckpoint so it's not user visible.
+	 */
+	if (MSR_TM_ACTIVE(mfmsr()))
+		tm_reclaim_current(0);
+
 	/* Always make any pending restarted system calls return -EINTR */
 	current->restart_block.fn = do_no_restart_syscall;
 
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 7e49984..5cbfe03d 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -667,6 +667,18 @@ int sys_rt_sigreturn(unsigned long r3, unsigned long r4, unsigned long r5,
 	unsigned long msr;
 #endif
 
+	/*
+	 * We always send the user to their signal handler non
+	 * transactionally. If there is a transactional/suspended state
+	 * then throw it away. The purpose of a sigreturn is to destroy
+	 * all traces of the signal frame, this includes any transactional
+	 * state created within in.
+	 * The cause is not important as there will never be a
+	 * recheckpoint so it's not user visible.
+	 */
+	if (MSR_TM_ACTIVE(mfmsr()))
+		tm_reclaim_current(0);
+
 	/* Always make any pending restarted system calls return -EINTR */
 	current->restart_block.fn = do_no_restart_syscall;
 
-- 
2.9.3

Re: [PATCH] powerpc: signals: Discard transaction state from signal frames

[Michael Neuling]

On Mon, 2016-08-22 at 15:15 +1000, Cyril Bur wrote:
> Userspace can begin and suspend a transaction within the signal
> handler which means they might enter sys_rt_sigreturn() with the
> processor in suspended state.
>=20
> sys_rt_sigreturn() wants to restore process context (which may have
> been in a transaction before signal delivery). To do this it must
> restore TM SPRS. To achieve this, any transaction initiated within the
> signal frame must be discarded in order to be able to restore TM SPRs
> as TM SPRs can only be manipulated non-transactionally..
> >=20
> > From the PowerPC ISA:
> =C2=A0 TM Bad Thing Exception [Category: Transactional Memory]
> =C2=A0=C2=A0=C2=A0An attempt is made to execute a mtspr targeting a TM re=
gister in
> =C2=A0=C2=A0=C2=A0other than Non-transactional state.
>=20
> Not doing so results in a TM Bad Thing:
> [12045.221359] Kernel BUG at c000000000050a40 [verbose debug info unavail=
able]
> [12045.221470] Unexpected TM Bad Thing exception at c000000000050a40 (msr=
 0x201033)
> [12045.221540] Oops: Unrecoverable exception, sig: 6 [#1]
> [12045.221586] SMP NR_CPUS=3D2048 NUMA PowerNV
> [12045.221634] Modules linked in: xt_CHECKSUM iptable_mangle ipt_MASQUERA=
DE
> =C2=A0nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_=
ipv4 nf_defrag_ipv4
> =C2=A0xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp bridg=
e stp llc ebtable_filter
> =C2=A0ebtables ip6table_filter ip6_tables iptable_filter ip_tables x_tabl=
es kvm_hv kvm
> =C2=A0uio_pdrv_genirq ipmi_powernv uio powernv_rng ipmi_msghandler autofs=
4 ses enclosure
> =C2=A0scsi_transport_sas bnx2x ipr mdio libcrc32c
> [12045.222167] CPU: 68 PID: 6178 Comm: sigreturnpanic Not tainted 4.7.0 #=
34
> [12045.222224] task: c0000000fce38600 ti: c0000000fceb4000 task.ti: c0000=
000fceb4000
> [12045.222293] NIP: c000000000050a40 LR: c0000000000163bc CTR: 0000000000=
000000
> [12045.222361] REGS: c0000000fceb7ac0 TRAP: 0700=C2=A0=C2=A0=C2=A0Not tai=
nted (4.7.0)
> [12045.222418] MSR: 9000000300201033  CR: 28444280=C2=A0=C2=A0XER: 200000=
00
> [12045.222625] CFAR: c0000000000163b8 SOFTE: 0 PACATMSCRATCH: 90000001428=
0f033
> GPR00: 01100000b8000001 c0000000fceb7d40 c00000000139c100 c0000000fce390d=
0
> GPR04: 900000034280f033 0000000000000000 0000000000000000 000000000000000=
0
> GPR08: 0000000000000000 b000000000001033 0000000000000001 000000000000000=
0
> GPR12: 0000000000000000 c000000002926400 0000000000000000 000000000000000=
0
> GPR16: 0000000000000000 0000000000000000 0000000000000000 000000000000000=
0
> GPR20: 0000000000000000 0000000000000000 0000000000000000 000000000000000=
0
> GPR24: 0000000000000000 00003ffff98cadd0 00003ffff98cb470 000000000000000=
0
> GPR28: 900000034280f033 c0000000fceb7ea0 0000000000000001 c0000000fce390d=
0
> [12045.223535] NIP [c000000000050a40] tm_restore_sprs+0xc/0x1c
> [12045.223584] LR [c0000000000163bc] tm_recheckpoint+0x5c/0xa0
> [12045.223630] Call Trace:
> [12045.223655] [c0000000fceb7d80] [c000000000026e74] sys_rt_sigreturn+0x4=
94/0x6c0
> [12045.223738] [c0000000fceb7e30] [c0000000000092e0] system_call+0x38/0x1=
08
> [12045.223806] Instruction dump:
> [12045.223841] 7c800164 4e800020 7c0022a6 f80304a8 7c0222a6 f80304b0 7c01=
22a6 f80304b8
> [12045.223955] 4e800020 e80304a8 7c0023a6 e80304b0 <7c0223a6> e80304b8 7c=
0123a6 4e800020
> [12045.224074] ---[ end trace cb8002ee240bae76 ]---
>=20
> It isn't clear exactly if there is really a use case for userspace
> returning with a suspended transaction, however, doing so doesn't
> (on its own) constitute a bad frame. As such, this patch simply
> discards the transaction state and continues.

To clarify which tm context you're talking about can we change this to.

    ... discards the transactional state of the context calling the sig
    return and continues.

Also, anyone wanna write a selftest and update
Documentation/powerpc/transactional_memory.txt?

Couple of comments below.


>=20
> Reported-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
> =C2=A0arch/powerpc/kernel/signal_32.c | 13 +++++++++++++
> =C2=A0arch/powerpc/kernel/signal_64.c | 12 ++++++++++++
> =C2=A02 files changed, 25 insertions(+)
>=20
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal=
_32.c
> index b6aa378..f892b93 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -1219,6 +1219,19 @@ long sys_rt_sigreturn(int r3, int r4, int r5, int =
r6, int r7, int r8,
> =C2=A0	unsigned long tmp;
> =C2=A0	int tm_restore =3D 0;
> =C2=A0#endif
> +
> +	/*
> +	=C2=A0* We always send the user to their signal handler non
> +	=C2=A0* transactionally.=20

This first sentence is not relevant IMHO. Same for 64bit.

> If there is a transactional/suspended state
> +	=C2=A0* then throw it away. The purpose of a sigreturn is to destroy
> +	=C2=A0* all traces of the signal frame, this includes any transactional
> +	=C2=A0* state created within in.
> +	=C2=A0* The cause is not important as there will never be a
> +	=C2=A0* recheckpoint so it's not user visible.
> +	=C2=A0*/
> +	if (MSR_TM_ACTIVE(mfmsr()))
> +		tm_reclaim_current(0);

I think this needs to be inside #ifdef CONFIG_PPC_TRANSACTIONAL_MEM. =C2=A0=
Just
move it down a bit. =C2=A0Same for 64bit.

> +
> =C2=A0	/* Always make any pending restarted system calls return -EINTR */
> =C2=A0	current->restart_block.fn =3D do_no_restart_syscall;
> =C2=A0
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal=
_64.c
> index 7e49984..5cbfe03d 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -667,6 +667,18 @@ int sys_rt_sigreturn(unsigned long r3, unsigned long=
 r4, unsigned long r5,
> =C2=A0	unsigned long msr;
> =C2=A0#endif
> =C2=A0
> +	/*
> +	=C2=A0* We always send the user to their signal handler non
> +	=C2=A0* transactionally. If there is a transactional/suspended state
> +	=C2=A0* then throw it away. The purpose of a sigreturn is to destroy
> +	=C2=A0* all traces of the signal frame, this includes any transactional
> +	=C2=A0* state created within in.
> +	=C2=A0* The cause is not important as there will never be a
> +	=C2=A0* recheckpoint so it's not user visible.
> +	=C2=A0*/
> +	if (MSR_TM_ACTIVE(mfmsr()))
> +		tm_reclaim_current(0);
> +
> =C2=A0	/* Always make any pending restarted system calls return -EINTR */
> =C2=A0	current->restart_block.fn =3D do_no_restart_syscall;
> =C2=A0

Re: [PATCH] powerpc: signals: Discard transaction state from signal frames

[Cyril Bur]

On Mon, 2016-08-22 at 17:07 +1000, Michael Neuling wrote:
> On Mon, 2016-08-22 at 15:15 +1000, Cyril Bur wrote:
> > 
> > Userspace can begin and suspend a transaction within the signal
> > handler which means they might enter sys_rt_sigreturn() with the
> > processor in suspended state.
> > 
> > sys_rt_sigreturn() wants to restore process context (which may have
> > been in a transaction before signal delivery). To do this it must
> > restore TM SPRS. To achieve this, any transaction initiated within
> > the
> > signal frame must be discarded in order to be able to restore TM
> > SPRs
> > as TM SPRs can only be manipulated non-transactionally..
> > > 
> > > 
> > > From the PowerPC ISA:
> >   TM Bad Thing Exception [Category: Transactional Memory]
> >    An attempt is made to execute a mtspr targeting a TM register in
> >    other than Non-transactional state.
> > 
> > Not doing so results in a TM Bad Thing:
> > [12045.221359] Kernel BUG at c000000000050a40 [verbose debug info
> > unavailable]
> > [12045.221470] Unexpected TM Bad Thing exception at
> > c000000000050a40 (msr 0x201033)
> > [12045.221540] Oops: Unrecoverable exception, sig: 6 [#1]
> > [12045.221586] SMP NR_CPUS=2048 NUMA PowerNV
> > [12045.221634] Modules linked in: xt_CHECKSUM iptable_mangle
> > ipt_MASQUERADE
> >  nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat
> > nf_conntrack_ipv4 nf_defrag_ipv4
> >  xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp
> > bridge stp llc ebtable_filter
> >  ebtables ip6table_filter ip6_tables iptable_filter ip_tables
> > x_tables kvm_hv kvm
> >  uio_pdrv_genirq ipmi_powernv uio powernv_rng ipmi_msghandler
> > autofs4 ses enclosure
> >  scsi_transport_sas bnx2x ipr mdio libcrc32c
> > [12045.222167] CPU: 68 PID: 6178 Comm: sigreturnpanic Not tainted
> > 4.7.0 #34
> > [12045.222224] task: c0000000fce38600 ti: c0000000fceb4000 task.ti:
> > c0000000fceb4000
> > [12045.222293] NIP: c000000000050a40 LR: c0000000000163bc CTR:
> > 0000000000000000
> > [12045.222361] REGS: c0000000fceb7ac0 TRAP: 0700   Not tainted
> > (4.7.0)
> > [12045.222418] MSR: 9000000300201033  CR: 28444280  XER: 20000000
> > [12045.222625] CFAR: c0000000000163b8 SOFTE: 0 PACATMSCRATCH:
> > 900000014280f033
> > GPR00: 01100000b8000001 c0000000fceb7d40 c00000000139c100
> > c0000000fce390d0
> > GPR04: 900000034280f033 0000000000000000 0000000000000000
> > 0000000000000000
> > GPR08: 0000000000000000 b000000000001033 0000000000000001
> > 0000000000000000
> > GPR12: 0000000000000000 c000000002926400 0000000000000000
> > 0000000000000000
> > GPR16: 0000000000000000 0000000000000000 0000000000000000
> > 0000000000000000
> > GPR20: 0000000000000000 0000000000000000 0000000000000000
> > 0000000000000000
> > GPR24: 0000000000000000 00003ffff98cadd0 00003ffff98cb470
> > 0000000000000000
> > GPR28: 900000034280f033 c0000000fceb7ea0 0000000000000001
> > c0000000fce390d0
> > [12045.223535] NIP [c000000000050a40] tm_restore_sprs+0xc/0x1c
> > [12045.223584] LR [c0000000000163bc] tm_recheckpoint+0x5c/0xa0
> > [12045.223630] Call Trace:
> > [12045.223655] [c0000000fceb7d80] [c000000000026e74]
> > sys_rt_sigreturn+0x494/0x6c0
> > [12045.223738] [c0000000fceb7e30] [c0000000000092e0]
> > system_call+0x38/0x108
> > [12045.223806] Instruction dump:
> > [12045.223841] 7c800164 4e800020 7c0022a6 f80304a8 7c0222a6
> > f80304b0 7c0122a6 f80304b8
> > [12045.223955] 4e800020 e80304a8 7c0023a6 e80304b0 <7c0223a6>
> > e80304b8 7c0123a6 4e800020
> > [12045.224074] ---[ end trace cb8002ee240bae76 ]---
> > 
> > It isn't clear exactly if there is really a use case for userspace
> > returning with a suspended transaction, however, doing so doesn't
> > (on its own) constitute a bad frame. As such, this patch simply
> > discards the transaction state and continues.
> 
> To clarify which tm context you're talking about can we change this
> to.
> 
>     ... discards the transactional state of the context calling the
> sig
>     return and continues.
> 
> Also, anyone wanna write a selftest and update
> Documentation/powerpc/transactional_memory.txt?
> 

I'll add something simple but I feel like we do need to be quite clear
about this behaviour.

> Couple of comments below.
> 
> 
> > 
> > 
> > Reported-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> >  arch/powerpc/kernel/signal_32.c | 13 +++++++++++++
> >  arch/powerpc/kernel/signal_64.c | 12 ++++++++++++
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/arch/powerpc/kernel/signal_32.c
> > b/arch/powerpc/kernel/signal_32.c
> > index b6aa378..f892b93 100644
> > --- a/arch/powerpc/kernel/signal_32.c
> > +++ b/arch/powerpc/kernel/signal_32.c
> > @@ -1219,6 +1219,19 @@ long sys_rt_sigreturn(int r3, int r4, int
> > r5, int r6, int r7, int r8,
> >  	unsigned long tmp;
> >  	int tm_restore = 0;
> >  #endif
> > +
> > +	/*
> > +	 * We always send the user to their signal handler non
> > +	 * transactionally. 
> 
> This first sentence is not relevant IMHO. Same for 64bit.
> 

Yeah, I'll remove

> > 
> > If there is a transactional/suspended state
> > +	 * then throw it away. The purpose of a sigreturn is to
> > destroy
> > +	 * all traces of the signal frame, this includes any
> > transactional
> > +	 * state created within in.
> > +	 * The cause is not important as there will never be a
> > +	 * recheckpoint so it's not user visible.
> > +	 */
> > +	if (MSR_TM_ACTIVE(mfmsr()))
> > +		tm_reclaim_current(0);
> 
> I think this needs to be inside #ifdef CONFIG_PPC_TRANSACTIONAL_MEM. 

Probably, gets me every time *sigh*

>  Just
> move it down a bit.  Same for 64bit.
> 

I was probably being paranoid about doing it early enough before we get
into the meat of sigreturning but defs fine at the start of the #ifdef,
you're right.

> > 
> > +
> >  	/* Always make any pending restarted system calls return
> > -EINTR */
> >  	current->restart_block.fn = do_no_restart_syscall;
> >  
> > diff --git a/arch/powerpc/kernel/signal_64.c
> > b/arch/powerpc/kernel/signal_64.c
> > index 7e49984..5cbfe03d 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -667,6 +667,18 @@ int sys_rt_sigreturn(unsigned long r3,
> > unsigned long r4, unsigned long r5,
> >  	unsigned long msr;
> >  #endif
> >  
> > +	/*
> > +	 * We always send the user to their signal handler non
> > +	 * transactionally. If there is a transactional/suspended
> > state
> > +	 * then throw it away. The purpose of a sigreturn is to
> > destroy
> > +	 * all traces of the signal frame, this includes any
> > transactional
> > +	 * state created within in.
> > +	 * The cause is not important as there will never be a
> > +	 * recheckpoint so it's not user visible.
> > +	 */
> > +	if (MSR_TM_ACTIVE(mfmsr()))
> > +		tm_reclaim_current(0);
> > +
> >  	/* Always make any pending restarted system calls return
> > -EINTR */
> >  	current->restart_block.fn = do_no_restart_syscall;
> >  

Re: [PATCH] powerpc: signals: Discard transaction state from signal frames

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4930 bytes --]

Hi Cyril,

[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.8-rc3 next-20160819]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Cyril-Bur/powerpc-signals-Discard-transaction-state-from-signal-frames/20160822-131843
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-c2k_defconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc 

All error/warnings (new ones prefixed by >>):

   In file included from arch/powerpc/include/asm/processor.h:13:0,
                    from arch/powerpc/include/asm/thread_info.h:33,
                    from include/linux/thread_info.h:54,
                    from include/asm-generic/preempt.h:4,
                    from ./arch/powerpc/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:59,
                    from include/linux/spinlock.h:50,
                    from include/linux/seqlock.h:35,
                    from include/linux/time.h:5,
                    from include/uapi/linux/timex.h:56,
                    from include/linux/timex.h:56,
                    from include/linux/sched.h:19,
                    from arch/powerpc/kernel/signal_32.c:20:
   arch/powerpc/kernel/signal_32.c: In function 'sys_rt_sigreturn':
>> arch/powerpc/include/asm/reg.h:64:23: error: left shift count >= width of type [-Werror=shift-count-overflow]
    #define __MASK(X) (1UL<<(X))
                          ^
   arch/powerpc/include/asm/reg.h:116:18: note: in expansion of macro '__MASK'
    #define MSR_TS_T __MASK(MSR_TS_T_LG) /*  Transaction Transactional */
                     ^
   arch/powerpc/include/asm/reg.h:117:22: note: in expansion of macro 'MSR_TS_T'
    #define MSR_TS_MASK (MSR_TS_T | MSR_TS_S)   /* Transaction State bits */
                         ^
   arch/powerpc/include/asm/reg.h:118:34: note: in expansion of macro 'MSR_TS_MASK'
    #define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */
                                     ^
>> arch/powerpc/kernel/signal_32.c:1232:6: note: in expansion of macro 'MSR_TM_ACTIVE'
     if (MSR_TM_ACTIVE(mfmsr()))
         ^
>> arch/powerpc/include/asm/reg.h:64:23: error: left shift count >= width of type [-Werror=shift-count-overflow]
    #define __MASK(X) (1UL<<(X))
                          ^
   arch/powerpc/include/asm/reg.h:115:18: note: in expansion of macro '__MASK'
    #define MSR_TS_S __MASK(MSR_TS_S_LG) /*  Transaction Suspended */
                     ^
   arch/powerpc/include/asm/reg.h:117:33: note: in expansion of macro 'MSR_TS_S'
    #define MSR_TS_MASK (MSR_TS_T | MSR_TS_S)   /* Transaction State bits */
                                    ^
   arch/powerpc/include/asm/reg.h:118:34: note: in expansion of macro 'MSR_TS_MASK'
    #define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */
                                     ^
>> arch/powerpc/kernel/signal_32.c:1232:6: note: in expansion of macro 'MSR_TM_ACTIVE'
     if (MSR_TM_ACTIVE(mfmsr()))
         ^
   cc1: all warnings being treated as errors

vim +/MSR_TM_ACTIVE +1232 arch/powerpc/kernel/signal_32.c

  1216	#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
  1217		struct ucontext __user *uc_transact;
  1218		unsigned long msr_hi;
  1219		unsigned long tmp;
  1220		int tm_restore = 0;
  1221	#endif
  1222	
  1223		/*
  1224		 * We always send the user to their signal handler non
  1225		 * transactionally. If there is a transactional/suspended state
  1226		 * then throw it away. The purpose of a sigreturn is to destroy
  1227		 * all traces of the signal frame, this includes any transactional
  1228		 * state created within in.
  1229		 * The cause is not important as there will never be a
  1230		 * recheckpoint so it's not user visible.
  1231		 */
> 1232		if (MSR_TM_ACTIVE(mfmsr()))
  1233			tm_reclaim_current(0);
  1234	
  1235		/* Always make any pending restarted system calls return -EINTR */
  1236		current->restart_block.fn = do_no_restart_syscall;
  1237	
  1238		rt_sf = (struct rt_sigframe __user *)
  1239			(regs->gpr[1] + __SIGNAL_FRAMESIZE + 16);
  1240		if (!access_ok(VERIFY_READ, rt_sf, sizeof(*rt_sf)))

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 18571 bytes --]

Re: [PATCH] powerpc: signals: Discard transaction state from signal frames

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1495 bytes --]

Hi Cyril,

[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.8-rc3 next-20160822]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Cyril-Bur/powerpc-signals-Discard-transaction-state-from-signal-frames/20160822-131843
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-g5_defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   arch/powerpc/kernel/built-in.o: In function `.compat_sys_rt_sigreturn':
>> (.text+0x5d64): undefined reference to `.tm_reclaim_current'
   arch/powerpc/kernel/built-in.o: In function `.sys_rt_sigreturn':
   (.text+0x140a4): undefined reference to `.tm_reclaim_current'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 19765 bytes --]

Re: [PATCH] powerpc: signals: Discard transaction state from signal frames

[Cyril Bur]

On Mon, 2016-08-22 at 19:21 +0800, kbuild test robot wrote:
> Hi Cyril,
> 
> [auto build test ERROR on powerpc/next]
> [also build test ERROR on v4.8-rc3 next-20160822]
> [if your patch is applied to the wrong git tree, please drop us a
> note to help improve the system]
> [Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --
> base=auto for convenience) to record what (public, well-known) commit
> your patch series was built on]
> [Check https://git-scm.com/docs/git-format-patch for more
> information]
> 
> url:    https://github.com/0day-ci/linux/commits/Cyril-Bur/powerpc-si
> gnals-Discard-transaction-state-from-signal-frames/20160822-131843
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux
> .git next
> config: powerpc-g5_defconfig (attached as .config)
> compiler: powerpc64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tes
> ts.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=powerpc 
> 
> All errors (new ones prefixed by >>):
> 
>    arch/powerpc/kernel/built-in.o: In function
> `.compat_sys_rt_sigreturn':
> > 
> > > 
> > > (.text+0x5d64): undefined reference to `.tm_reclaim_current'
>    arch/powerpc/kernel/built-in.o: In function `.sys_rt_sigreturn':
>    (.text+0x140a4): undefined reference to `.tm_reclaim_current'
> 

For the record, I don't seem to be able to reproduce this. I tried a
variety of configs including the attached one and g5_defconfig and
cannot see this.

GCC 4.9.3, 5.3.0 and 6.1.0 used with powerpc/next and v4.8.0-rc3


> ---
> 0-DAY kernel test infrastructure                Open Source
> Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel
> Corporation

[PATCH] powerpc: signals: Discard transaction state from signal frames

[Cyril Bur]

Userspace can begin and suspend a transaction within the signal
handler which means they might enter sys_rt_sigreturn() with the
processor in suspended state.

sys_rt_sigreturn() wants to restore process context (which may have
been in a transaction before signal delivery). To do this it must
restore TM SPRS. To achieve this, any transaction initiated within the
signal frame must be discarded in order to be able to restore TM SPRs
as TM SPRs can only be manipulated non-transactionally..
>From the PowerPC ISA:
  TM Bad Thing Exception [Category: Transactional Memory]
   An attempt is made to execute a mtspr targeting a TM register in
   other than Non-transactional state.

Not doing so results in a TM Bad Thing:
[12045.221359] Kernel BUG at c000000000050a40 [verbose debug info unavailable]
[12045.221470] Unexpected TM Bad Thing exception at c000000000050a40 (msr 0x201033)
[12045.221540] Oops: Unrecoverable exception, sig: 6 [#1]
[12045.221586] SMP NR_CPUS=2048 NUMA PowerNV
[12045.221634] Modules linked in: xt_CHECKSUM iptable_mangle ipt_MASQUERADE
 nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4
 xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp bridge stp llc ebtable_filter
 ebtables ip6table_filter ip6_tables iptable_filter ip_tables x_tables kvm_hv kvm
 uio_pdrv_genirq ipmi_powernv uio powernv_rng ipmi_msghandler autofs4 ses enclosure
 scsi_transport_sas bnx2x ipr mdio libcrc32c
[12045.222167] CPU: 68 PID: 6178 Comm: sigreturnpanic Not tainted 4.7.0 #34
[12045.222224] task: c0000000fce38600 ti: c0000000fceb4000 task.ti: c0000000fceb4000
[12045.222293] NIP: c000000000050a40 LR: c0000000000163bc CTR: 0000000000000000
[12045.222361] REGS: c0000000fceb7ac0 TRAP: 0700   Not tainted (4.7.0)
[12045.222418] MSR: 9000000300201033 <SF,HV,ME,IR,DR,RI,LE,TM[SE]> CR: 28444280  XER: 20000000
[12045.222625] CFAR: c0000000000163b8 SOFTE: 0 PACATMSCRATCH: 900000014280f033
GPR00: 01100000b8000001 c0000000fceb7d40 c00000000139c100 c0000000fce390d0
GPR04: 900000034280f033 0000000000000000 0000000000000000 0000000000000000
GPR08: 0000000000000000 b000000000001033 0000000000000001 0000000000000000
GPR12: 0000000000000000 c000000002926400 0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR24: 0000000000000000 00003ffff98cadd0 00003ffff98cb470 0000000000000000
GPR28: 900000034280f033 c0000000fceb7ea0 0000000000000001 c0000000fce390d0
[12045.223535] NIP [c000000000050a40] tm_restore_sprs+0xc/0x1c
[12045.223584] LR [c0000000000163bc] tm_recheckpoint+0x5c/0xa0
[12045.223630] Call Trace:
[12045.223655] [c0000000fceb7d80] [c000000000026e74] sys_rt_sigreturn+0x494/0x6c0
[12045.223738] [c0000000fceb7e30] [c0000000000092e0] system_call+0x38/0x108
[12045.223806] Instruction dump:
[12045.223841] 7c800164 4e800020 7c0022a6 f80304a8 7c0222a6 f80304b0 7c0122a6 f80304b8
[12045.223955] 4e800020 e80304a8 7c0023a6 e80304b0 <7c0223a6> e80304b8 7c0123a6 4e800020
[12045.224074] ---[ end trace cb8002ee240bae76 ]---

It isn't clear exactly if there is really a use case for userspace
returning with a suspended transaction, however, doing so doesn't (on
its own) constitute a bad frame. As such, this patch simply discards
the transactional state of the context calling the sigreturn and
continues.

Reported-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
V2: Move the code down into the #ifdef CONFIG_PPC_TRANSACTIONAL_MEM (32 and 64 bit)
	Add a small amount of text to Documentation
	Adjust the commit message for clarity 

 Documentation/powerpc/transactional_memory.txt |  2 ++
 arch/powerpc/kernel/signal_32.c                | 12 ++++++++++++
 arch/powerpc/kernel/signal_64.c                | 12 ++++++++++++
 3 files changed, 26 insertions(+)

diff --git a/Documentation/powerpc/transactional_memory.txt b/Documentation/powerpc/transactional_memory.txt
index ba0a2a4..e32fdbb 100644
--- a/Documentation/powerpc/transactional_memory.txt
+++ b/Documentation/powerpc/transactional_memory.txt
@@ -167,6 +167,8 @@ signal will be rolled back anyway.
 For signals taken in non-TM or suspended mode, we use the
 normal/non-checkpointed stack pointer.
 
+Any transaction initiated inside a sighandler and suspended on return
+from the sighandler to the kernel will get reclaimed and discarded.
 
 Failure cause codes used by kernel
 ==================================
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index b6aa378..31e4e15 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -1226,7 +1226,19 @@ long sys_rt_sigreturn(int r3, int r4, int r5, int r6, int r7, int r8,
 		(regs->gpr[1] + __SIGNAL_FRAMESIZE + 16);
 	if (!access_ok(VERIFY_READ, rt_sf, sizeof(*rt_sf)))
 		goto bad;
+
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+	/*
+	 * If there is a transactional/suspended state then throw it away.
+	 * The purpose of a sigreturn is to destroy all traces of the
+	 * signal frame, this includes any transactional state created
+	 * within in.
+	 * The cause is not important as there will never be a
+	 * recheckpoint so it's not user visible.
+	 */
+	if (MSR_TM_ACTIVE(mfmsr()))
+		tm_reclaim_current(0);
+
 	if (__get_user(tmp, &rt_sf->uc.uc_link))
 		goto bad;
 	uc_transact = (struct ucontext __user *)(uintptr_t)tmp;
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 7e49984..8425eee 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -676,7 +676,19 @@ int sys_rt_sigreturn(unsigned long r3, unsigned long r4, unsigned long r5,
 	if (__copy_from_user(&set, &uc->uc_sigmask, sizeof(set)))
 		goto badframe;
 	set_current_blocked(&set);
+
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+	/*
+	 * If there is a transactional/suspended state then throw it away.
+	 * The purpose of a sigreturn is to destroy all traces of the
+	 * signal frame, this includes any transactional state created
+	 * within in.
+	 * The cause is not important as there will never be a
+	 * recheckpoint so it's not user visible.
+	 */
+	if (MSR_TM_ACTIVE(mfmsr()))
+		tm_reclaim_current(0);
+
 	if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
 		goto badframe;
 	if (MSR_TM_ACTIVE(msr)) {
-- 
2.9.3

Re: [PATCH] powerpc: signals: Discard transaction state from signal frames

[Simon Guo]

Hi Cyril,
On Mon, Aug 22, 2016 at 05:32:06PM +1000, Cyril Bur wrote:
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index b6aa378..31e4e15 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -1226,7 +1226,19 @@ long sys_rt_sigreturn(int r3, int r4, int r5, int r6, int r7, int r8,
>  		(regs->gpr[1] + __SIGNAL_FRAMESIZE + 16);
>  	if (!access_ok(VERIFY_READ, rt_sf, sizeof(*rt_sf)))
>  		goto bad;
> +
>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +	/*
> +	 * If there is a transactional/suspended state then throw it away.
> +	 * The purpose of a sigreturn is to destroy all traces of the
> +	 * signal frame, this includes any transactional state created
> +	 * within in.
> +	 * The cause is not important as there will never be a
> +	 * recheckpoint so it's not user visible.
> +	 */
> +	if (MSR_TM_ACTIVE(mfmsr()))
> +		tm_reclaim_current(0);
> +
Maybe a little picky here:
Per my understanding, the TRANSACTIONAL state will be failed in system
call common entry. The only expected state to prevent here is SUSPEND 
state.
Should we use MSR_TM_SUSPENDED(mfmsr()) here and BUG_ON
MSR_TM_TRANSACTIONAL(mfmsr())?  -- If it is transactional state, something
 is wrong with kernel. 

Others looks good to me.

Thanks,
- Simon

Re: [PATCH] powerpc: signals: Discard transaction state from signal frames

[Cyril Bur]

On Sat, 2016-08-20 at 18:03 +0800, Simon Guo wrote:
> Hi Cyril,
> On Mon, Aug 22, 2016 at 05:32:06PM +1000, Cyril Bur wrote:
> > 
> > diff --git a/arch/powerpc/kernel/signal_32.c
> > b/arch/powerpc/kernel/signal_32.c
> > index b6aa378..31e4e15 100644
> > --- a/arch/powerpc/kernel/signal_32.c
> > +++ b/arch/powerpc/kernel/signal_32.c
> > @@ -1226,7 +1226,19 @@ long sys_rt_sigreturn(int r3, int r4, int
> > r5, int r6, int r7, int r8,
> >  		(regs->gpr[1] + __SIGNAL_FRAMESIZE + 16);
> >  	if (!access_ok(VERIFY_READ, rt_sf, sizeof(*rt_sf)))
> >  		goto bad;
> > +
> >  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > +	/*
> > +	 * If there is a transactional/suspended state then throw
> > it away.
> > +	 * The purpose of a sigreturn is to destroy all traces of
> > the
> > +	 * signal frame, this includes any transactional state
> > created
> > +	 * within in.
> > +	 * The cause is not important as there will never be a
> > +	 * recheckpoint so it's not user visible.
> > +	 */
> > +	if (MSR_TM_ACTIVE(mfmsr()))
> > +		tm_reclaim_current(0);
> > +
> Maybe a little picky here:
> Per my understanding, the TRANSACTIONAL state will be failed in
> system
> call common entry. The only expected state to prevent here is
> SUSPEND 
> state.

That is the case yes.

> Should we use MSR_TM_SUSPENDED(mfmsr()) here and BUG_ON
> MSR_TM_TRANSACTIONAL(mfmsr())?  -- If it is transactional state,
> something
>  is wrong with kernel. 
> 

I'm happy to change it to MSR_TM_SUSPENDED.

We should decide what the result of getting here with TRANSACTIONAL is.
I see two posibilities:
1. the reclaim will solve the problem and we can continue, there is
obviously a bug somewhere else but we think it doesn't affect us here
and we *hope* it will be contained elsewhere.
2. We think seeing TRANSACTIONAL here means we have a problem that is
going to lead to corruption of state such that bad data will propage,
in which case I think a BUG_ON() is a good idea.

I'm more in favour of 1, there doesn't seem to be any other way to get
here but through syscall entry, this kind of bug should probably be
caught more generally. In that case I would say that checking ACTIVE is
good since we know that we WILL blow up if ACTIVE and we don't do a
reclaim.

Maybe now that I'm thinking about it, change it to SUSPENDED but no
BUG_ON(), as I just said, we'll do a Bad Thing anyway, which will
reveal the problem.

I have to send another version anyway as it doesn't seem to be working
for 32bit.

> Others looks good to me.
> 
> Thanks,
> - Simon

Re: [PATCH] powerpc: signals: Discard transaction state from signal frames

[Cyril Bur]

On Mon, 2016-08-22 at 17:32 +1000, Cyril Bur wrote:

The subject prefix should have contained V2. Sorry.

> Userspace can begin and suspend a transaction within the signal
> handler which means they might enter sys_rt_sigreturn() with the
> processor in suspended state.
> 
> sys_rt_sigreturn() wants to restore process context (which may have
> been in a transaction before signal delivery). To do this it must
> restore TM SPRS. To achieve this, any transaction initiated within
> the
> signal frame must be discarded in order to be able to restore TM SPRs
> as TM SPRs can only be manipulated non-transactionally..
> From the PowerPC ISA:
>   TM Bad Thing Exception [Category: Transactional Memory]
>    An attempt is made to execute a mtspr targeting a TM register in
>    other than Non-transactional state.
> 
> Not doing so results in a TM Bad Thing:
> [12045.221359] Kernel BUG at c000000000050a40 [verbose debug info
> unavailable]
> [12045.221470] Unexpected TM Bad Thing exception at c000000000050a40
> (msr 0x201033)
> [12045.221540] Oops: Unrecoverable exception, sig: 6 [#1]
> [12045.221586] SMP NR_CPUS=2048 NUMA PowerNV
> [12045.221634] Modules linked in: xt_CHECKSUM iptable_mangle
> ipt_MASQUERADE
>  nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat
> nf_conntrack_ipv4 nf_defrag_ipv4
>  xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp bridge
> stp llc ebtable_filter
>  ebtables ip6table_filter ip6_tables iptable_filter ip_tables
> x_tables kvm_hv kvm
>  uio_pdrv_genirq ipmi_powernv uio powernv_rng ipmi_msghandler autofs4
> ses enclosure
>  scsi_transport_sas bnx2x ipr mdio libcrc32c
> [12045.222167] CPU: 68 PID: 6178 Comm: sigreturnpanic Not tainted
> 4.7.0 #34
> [12045.222224] task: c0000000fce38600 ti: c0000000fceb4000 task.ti:
> c0000000fceb4000
> [12045.222293] NIP: c000000000050a40 LR: c0000000000163bc CTR:
> 0000000000000000
> [12045.222361] REGS: c0000000fceb7ac0 TRAP: 0700   Not tainted
> (4.7.0)
> [12045.222418] MSR: 9000000300201033 <SF,HV,ME,IR,DR,RI,LE,TM[SE]>
> CR: 28444280  XER: 20000000
> [12045.222625] CFAR: c0000000000163b8 SOFTE: 0 PACATMSCRATCH:
> 900000014280f033
> GPR00: 01100000b8000001 c0000000fceb7d40 c00000000139c100
> c0000000fce390d0
> GPR04: 900000034280f033 0000000000000000 0000000000000000
> 0000000000000000
> GPR08: 0000000000000000 b000000000001033 0000000000000001
> 0000000000000000
> GPR12: 0000000000000000 c000000002926400 0000000000000000
> 0000000000000000
> GPR16: 0000000000000000 0000000000000000 0000000000000000
> 0000000000000000
> GPR20: 0000000000000000 0000000000000000 0000000000000000
> 0000000000000000
> GPR24: 0000000000000000 00003ffff98cadd0 00003ffff98cb470
> 0000000000000000
> GPR28: 900000034280f033 c0000000fceb7ea0 0000000000000001
> c0000000fce390d0
> [12045.223535] NIP [c000000000050a40] tm_restore_sprs+0xc/0x1c
> [12045.223584] LR [c0000000000163bc] tm_recheckpoint+0x5c/0xa0
> [12045.223630] Call Trace:
> [12045.223655] [c0000000fceb7d80] [c000000000026e74]
> sys_rt_sigreturn+0x494/0x6c0
> [12045.223738] [c0000000fceb7e30] [c0000000000092e0]
> system_call+0x38/0x108
> [12045.223806] Instruction dump:
> [12045.223841] 7c800164 4e800020 7c0022a6 f80304a8 7c0222a6 f80304b0
> 7c0122a6 f80304b8
> [12045.223955] 4e800020 e80304a8 7c0023a6 e80304b0 <7c0223a6>
> e80304b8 7c0123a6 4e800020
> [12045.224074] ---[ end trace cb8002ee240bae76 ]---
> 
> It isn't clear exactly if there is really a use case for userspace
> returning with a suspended transaction, however, doing so doesn't (on
> its own) constitute a bad frame. As such, this patch simply discards
> the transactional state of the context calling the sigreturn and
> continues.
> 
> Reported-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
> V2: Move the code down into the #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> (32 and 64 bit)
> 	Add a small amount of text to Documentation
> 	Adjust the commit message for clarity 
> 
>  Documentation/powerpc/transactional_memory.txt |  2 ++
>  arch/powerpc/kernel/signal_32.c                | 12 ++++++++++++
>  arch/powerpc/kernel/signal_64.c                | 12 ++++++++++++
>  3 files changed, 26 insertions(+)
> 
> diff --git a/Documentation/powerpc/transactional_memory.txt
> b/Documentation/powerpc/transactional_memory.txt
> index ba0a2a4..e32fdbb 100644
> --- a/Documentation/powerpc/transactional_memory.txt
> +++ b/Documentation/powerpc/transactional_memory.txt
> @@ -167,6 +167,8 @@ signal will be rolled back anyway.
>  For signals taken in non-TM or suspended mode, we use the
>  normal/non-checkpointed stack pointer.
>  
> +Any transaction initiated inside a sighandler and suspended on
> return
> +from the sighandler to the kernel will get reclaimed and discarded.
>  
>  Failure cause codes used by kernel
>  ==================================
> diff --git a/arch/powerpc/kernel/signal_32.c
> b/arch/powerpc/kernel/signal_32.c
> index b6aa378..31e4e15 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -1226,7 +1226,19 @@ long sys_rt_sigreturn(int r3, int r4, int r5,
> int r6, int r7, int r8,
>  		(regs->gpr[1] + __SIGNAL_FRAMESIZE + 16);
>  	if (!access_ok(VERIFY_READ, rt_sf, sizeof(*rt_sf)))
>  		goto bad;
> +
>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +	/*
> +	 * If there is a transactional/suspended state then throw it
> away.
> +	 * The purpose of a sigreturn is to destroy all traces of
> the
> +	 * signal frame, this includes any transactional state
> created
> +	 * within in.
> +	 * The cause is not important as there will never be a
> +	 * recheckpoint so it's not user visible.
> +	 */
> +	if (MSR_TM_ACTIVE(mfmsr()))
> +		tm_reclaim_current(0);
> +
>  	if (__get_user(tmp, &rt_sf->uc.uc_link))
>  		goto bad;
>  	uc_transact = (struct ucontext __user *)(uintptr_t)tmp;
> diff --git a/arch/powerpc/kernel/signal_64.c
> b/arch/powerpc/kernel/signal_64.c
> index 7e49984..8425eee 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -676,7 +676,19 @@ int sys_rt_sigreturn(unsigned long r3, unsigned
> long r4, unsigned long r5,
>  	if (__copy_from_user(&set, &uc->uc_sigmask, sizeof(set)))
>  		goto badframe;
>  	set_current_blocked(&set);
> +
>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +	/*
> +	 * If there is a transactional/suspended state then throw it
> away.
> +	 * The purpose of a sigreturn is to destroy all traces of
> the
> +	 * signal frame, this includes any transactional state
> created
> +	 * within in.
> +	 * The cause is not important as there will never be a
> +	 * recheckpoint so it's not user visible.
> +	 */
> +	if (MSR_TM_ACTIVE(mfmsr()))
> +		tm_reclaim_current(0);
> +
>  	if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
>  		goto badframe;
>  	if (MSR_TM_ACTIVE(msr)) {

Re: [PATCH] powerpc: signals: Discard transaction state from signal frames

[Laurent Dufour]

On 22/08/2016 09:32, Cyril Bur wrote:
> Userspace can begin and suspend a transaction within the signal
> handler which means they might enter sys_rt_sigreturn() with the
> processor in suspended state.
> 
> sys_rt_sigreturn() wants to restore process context (which may have
> been in a transaction before signal delivery). To do this it must
> restore TM SPRS. To achieve this, any transaction initiated within the
> signal frame must be discarded in order to be able to restore TM SPRs
> as TM SPRs can only be manipulated non-transactionally..
> From the PowerPC ISA:
>   TM Bad Thing Exception [Category: Transactional Memory]
>    An attempt is made to execute a mtspr targeting a TM register in
>    other than Non-transactional state.
> 
> Not doing so results in a TM Bad Thing:
> [12045.221359] Kernel BUG at c000000000050a40 [verbose debug info unavailable]
> [12045.221470] Unexpected TM Bad Thing exception at c000000000050a40 (msr 0x201033)
> [12045.221540] Oops: Unrecoverable exception, sig: 6 [#1]
> [12045.221586] SMP NR_CPUS=2048 NUMA PowerNV
> [12045.221634] Modules linked in: xt_CHECKSUM iptable_mangle ipt_MASQUERADE
>  nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4
>  xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp bridge stp llc ebtable_filter
>  ebtables ip6table_filter ip6_tables iptable_filter ip_tables x_tables kvm_hv kvm
>  uio_pdrv_genirq ipmi_powernv uio powernv_rng ipmi_msghandler autofs4 ses enclosure
>  scsi_transport_sas bnx2x ipr mdio libcrc32c
> [12045.222167] CPU: 68 PID: 6178 Comm: sigreturnpanic Not tainted 4.7.0 #34
> [12045.222224] task: c0000000fce38600 ti: c0000000fceb4000 task.ti: c0000000fceb4000
> [12045.222293] NIP: c000000000050a40 LR: c0000000000163bc CTR: 0000000000000000
> [12045.222361] REGS: c0000000fceb7ac0 TRAP: 0700   Not tainted (4.7.0)
> [12045.222418] MSR: 9000000300201033 <SF,HV,ME,IR,DR,RI,LE,TM[SE]> CR: 28444280  XER: 20000000
> [12045.222625] CFAR: c0000000000163b8 SOFTE: 0 PACATMSCRATCH: 900000014280f033
> GPR00: 01100000b8000001 c0000000fceb7d40 c00000000139c100 c0000000fce390d0
> GPR04: 900000034280f033 0000000000000000 0000000000000000 0000000000000000
> GPR08: 0000000000000000 b000000000001033 0000000000000001 0000000000000000
> GPR12: 0000000000000000 c000000002926400 0000000000000000 0000000000000000
> GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> GPR24: 0000000000000000 00003ffff98cadd0 00003ffff98cb470 0000000000000000
> GPR28: 900000034280f033 c0000000fceb7ea0 0000000000000001 c0000000fce390d0
> [12045.223535] NIP [c000000000050a40] tm_restore_sprs+0xc/0x1c
> [12045.223584] LR [c0000000000163bc] tm_recheckpoint+0x5c/0xa0
> [12045.223630] Call Trace:
> [12045.223655] [c0000000fceb7d80] [c000000000026e74] sys_rt_sigreturn+0x494/0x6c0
> [12045.223738] [c0000000fceb7e30] [c0000000000092e0] system_call+0x38/0x108
> [12045.223806] Instruction dump:
> [12045.223841] 7c800164 4e800020 7c0022a6 f80304a8 7c0222a6 f80304b0 7c0122a6 f80304b8
> [12045.223955] 4e800020 e80304a8 7c0023a6 e80304b0 <7c0223a6> e80304b8 7c0123a6 4e800020
> [12045.224074] ---[ end trace cb8002ee240bae76 ]---
> 
> It isn't clear exactly if there is really a use case for userspace
> returning with a suspended transaction, however, doing so doesn't (on
> its own) constitute a bad frame. As such, this patch simply discards
> the transactional state of the context calling the sigreturn and
> continues.
>
> Reported-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
> V2: Move the code down into the #ifdef CONFIG_PPC_TRANSACTIONAL_MEM (32 and 64 bit)
> 	Add a small amount of text to Documentation
> 	Adjust the commit message for clarity 
> 

FWIW,
Tested-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>