* [PATCH] parisc: Ensure consistent state when switching to kernel stack at syscall entry @ 2016-10-28 20:04 Helge Deller 2016-10-29 3:00 ` John David Anglin 0 siblings, 1 reply; 3+ messages in thread From: Helge Deller @ 2016-10-28 20:04 UTC (permalink / raw) To: linux-parisc, James Bottomley, John David Anglin We have one critical section in the syscall entry path in which we switch from the userspace stack to kernel stack. In the event of an external interrupt, the interrupt code distinguishes between those two states by analyzing the value of sr7. If sr7 is zero, it uses the kernel stack. Therefore it's important, that the value of sr7 is in sync with the currently enabled stack. This patch now disables interrupts while executing the critical section. This prevents the interrupt handler to possibly see an inconsitent state which in the worst case can lead to crashes. Interestingly, in the syscall exit path interrupts were already disabled in the critical section which switches back to the userspace stack. Cc: <stable@vger.kernel.org> Signed-off-by: Helge Deller <deller@gmx.de> diff --git a/arch/parisc/kernel/syscall.S b/arch/parisc/kernel/syscall.S index d03422e..f13836b 100644 --- a/arch/parisc/kernel/syscall.S +++ b/arch/parisc/kernel/syscall.S @@ -106,8 +106,6 @@ linux_gateway_entry: mtsp %r0,%sr4 /* get kernel space into sr4 */ mtsp %r0,%sr5 /* get kernel space into sr5 */ mtsp %r0,%sr6 /* get kernel space into sr6 */ - mfsp %sr7,%r1 /* save user sr7 */ - mtsp %r1,%sr3 /* and store it in sr3 */ #ifdef CONFIG_64BIT /* for now we can *always* set the W bit on entry to the syscall @@ -134,20 +132,26 @@ linux_gateway_entry: 1: #endif mfctl %cr30,%r1 - xor %r1,%r30,%r30 /* ye olde xor trick */ - xor %r1,%r30,%r1 - xor %r1,%r30,%r30 - - ldo THREAD_SZ_ALGN+FRAME_SIZE(%r30),%r30 /* set up kernel stack */ + ldo THREAD_SZ_ALGN+FRAME_SIZE(%r1),%r1 /* set up kernel stack */ /* N.B.: It is critical that we don't set sr7 to 0 until r30 * contains a valid kernel stack pointer. It is also * critical that we don't start using the kernel stack - * until after sr7 has been set to 0. + * until after sr7 has been set to 0. To ensure this we + * use a rsm/ssm pair to make this operation atomic. + * At syscall entry %sr2 points to kernel space, otherwise + * syscalls wouldn't work. */ + rsm PSW_SM_I, %r0 /* turn interrupts off */ + STREGM %r30,FRAME_SIZE(%sr2, %r1) /* save usp on kernel stack */ + copy %r1, %r30 /* switch to kernel stack */ + + mfsp %sr7,%r1 /* get user sr7 */ + mtsp %r1,%sr3 /* store user sr7 in sr3 */ mtsp %r0,%sr7 /* get kernel space into sr7 */ - STREGM %r1,FRAME_SIZE(%r30) /* save r1 (usp) here for now */ + ssm PSW_SM_I, %r0 /* turn interrupts on */ + mfctl %cr30,%r1 /* get task ptr in %r1 */ LDREG TI_TASK(%r1),%r1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] parisc: Ensure consistent state when switching to kernel stack at syscall entry 2016-10-28 20:04 [PATCH] parisc: Ensure consistent state when switching to kernel stack at syscall entry Helge Deller @ 2016-10-29 3:00 ` John David Anglin 2016-10-29 17:33 ` Helge Deller 0 siblings, 1 reply; 3+ messages in thread From: John David Anglin @ 2016-10-29 3:00 UTC (permalink / raw) To: Helge Deller; +Cc: linux-parisc, James Bottomley [-- Attachment #1: Type: text/plain, Size: 3312 bytes --] On 2016-10-28, at 4:04 PM, Helge Deller wrote: > We have one critical section in the syscall entry path in which we switch from > the userspace stack to kernel stack. In the event of an external interrupt, the > interrupt code distinguishes between those two states by analyzing the value of > sr7. If sr7 is zero, it uses the kernel stack. Therefore it's important, that > the value of sr7 is in sync with the currently enabled stack. > > This patch now disables interrupts while executing the critical section. This > prevents the interrupt handler to possibly see an inconsitent state which in > the worst case can lead to crashes. > > Interestingly, in the syscall exit path interrupts were already disabled in the > critical section which switches back to the userspace stack. > > Cc: <stable@vger.kernel.org> > Signed-off-by: Helge Deller <deller@gmx.de> > > diff --git a/arch/parisc/kernel/syscall.S b/arch/parisc/kernel/syscall.S > index d03422e..f13836b 100644 > --- a/arch/parisc/kernel/syscall.S > +++ b/arch/parisc/kernel/syscall.S > @@ -106,8 +106,6 @@ linux_gateway_entry: > mtsp %r0,%sr4 /* get kernel space into sr4 */ > mtsp %r0,%sr5 /* get kernel space into sr5 */ > mtsp %r0,%sr6 /* get kernel space into sr6 */ > - mfsp %sr7,%r1 /* save user sr7 */ > - mtsp %r1,%sr3 /* and store it in sr3 */ > > #ifdef CONFIG_64BIT > /* for now we can *always* set the W bit on entry to the syscall > @@ -134,20 +132,26 @@ linux_gateway_entry: > 1: > #endif > mfctl %cr30,%r1 > - xor %r1,%r30,%r30 /* ye olde xor trick */ > - xor %r1,%r30,%r1 > - xor %r1,%r30,%r30 > - > - ldo THREAD_SZ_ALGN+FRAME_SIZE(%r30),%r30 /* set up kernel stack */ > + ldo THREAD_SZ_ALGN+FRAME_SIZE(%r1),%r1 /* set up kernel stack */ > > /* N.B.: It is critical that we don't set sr7 to 0 until r30 > * contains a valid kernel stack pointer. It is also > * critical that we don't start using the kernel stack > - * until after sr7 has been set to 0. > + * until after sr7 has been set to 0. To ensure this we > + * use a rsm/ssm pair to make this operation atomic. > + * At syscall entry %sr2 points to kernel space, otherwise > + * syscalls wouldn't work. > */ > + rsm PSW_SM_I, %r0 /* turn interrupts off */ > + STREGM %r30,FRAME_SIZE(%sr2, %r1) /* save usp on kernel stack */ > + copy %r1, %r30 /* switch to kernel stack */ > + > + mfsp %sr7,%r1 /* get user sr7 */ > + mtsp %r1,%sr3 /* store user sr7 in sr3 */ > > mtsp %r0,%sr7 /* get kernel space into sr7 */ > - STREGM %r1,FRAME_SIZE(%r30) /* save r1 (usp) here for now */ > + ssm PSW_SM_I, %r0 /* turn interrupts on */ > + > mfctl %cr30,%r1 /* get task ptr in %r1 */ > LDREG TI_TASK(%r1),%r1 The above doesn't assemble. The STREGM instruction doesn't allow explicit space register selection with long displacements. The attached patch does assemble and v4.7.10 boots successfully with change. I'm thinking we might now be able to remove the restriction on scheduling on the gateway page. Signed-off-by: John David Anglin <dave.anglin@bell.net> [-- Attachment #2: syscall.S.d.txt --] [-- Type: text/plain, Size: 1331 bytes --] diff --git a/arch/parisc/kernel/syscall.S b/arch/parisc/kernel/syscall.S index d03422e..deec1f8 100644 --- a/arch/parisc/kernel/syscall.S +++ b/arch/parisc/kernel/syscall.S @@ -106,8 +106,6 @@ linux_gateway_entry: mtsp %r0,%sr4 /* get kernel space into sr4 */ mtsp %r0,%sr5 /* get kernel space into sr5 */ mtsp %r0,%sr6 /* get kernel space into sr6 */ - mfsp %sr7,%r1 /* save user sr7 */ - mtsp %r1,%sr3 /* and store it in sr3 */ #ifdef CONFIG_64BIT /* for now we can *always* set the W bit on entry to the syscall @@ -133,6 +131,14 @@ linux_gateway_entry: depdi 0, 31, 32, %r21 1: #endif + + /* We use a rsm/ssm pair to prevent sr3 from being clobbered + * by external interrupts. + */ + mfsp %sr7,%r1 /* save user sr7 */ + rsm PSW_SM_I, %r0 /* disable interrupts */ + mtsp %r1,%sr3 /* and store it in sr3 */ + mfctl %cr30,%r1 xor %r1,%r30,%r30 /* ye olde xor trick */ xor %r1,%r30,%r1 @@ -147,6 +153,7 @@ linux_gateway_entry: */ mtsp %r0,%sr7 /* get kernel space into sr7 */ + ssm PSW_SM_I, %r0 /* enable interrupts */ STREGM %r1,FRAME_SIZE(%r30) /* save r1 (usp) here for now */ mfctl %cr30,%r1 /* get task ptr in %r1 */ LDREG TI_TASK(%r1),%r1 [-- Attachment #3: Type: text/plain, Size: 49 bytes --] Dave -- John David Anglin dave.anglin@bell.net ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] parisc: Ensure consistent state when switching to kernel stack at syscall entry 2016-10-29 3:00 ` John David Anglin @ 2016-10-29 17:33 ` Helge Deller 0 siblings, 0 replies; 3+ messages in thread From: Helge Deller @ 2016-10-29 17:33 UTC (permalink / raw) To: John David Anglin; +Cc: linux-parisc@vger.kernel.org, James Bottomley On 29.10.2016 05:00, John David Anglin wrote: > On 2016-10-28, at 4:04 PM, Helge Deller wrote: > >> We have one critical section in the syscall entry path in which we switch from >> the userspace stack to kernel stack. In the event of an external interrupt, the >> interrupt code distinguishes between those two states by analyzing the value of >> sr7. If sr7 is zero, it uses the kernel stack. Therefore it's important, that >> the value of sr7 is in sync with the currently enabled stack. >> >> This patch now disables interrupts while executing the critical section. This >> prevents the interrupt handler to possibly see an inconsitent state which in >> the worst case can lead to crashes. >> >> Interestingly, in the syscall exit path interrupts were already disabled in the >> critical section which switches back to the userspace stack. >> >> Cc: <stable@vger.kernel.org> >> Signed-off-by: Helge Deller <deller@gmx.de> >> >> diff --git a/arch/parisc/kernel/syscall.S b/arch/parisc/kernel/syscall.S >> index d03422e..f13836b 100644 >> --- a/arch/parisc/kernel/syscall.S >> +++ b/arch/parisc/kernel/syscall.S >> @@ -106,8 +106,6 @@ linux_gateway_entry: >> mtsp %r0,%sr4 /* get kernel space into sr4 */ >> mtsp %r0,%sr5 /* get kernel space into sr5 */ >> mtsp %r0,%sr6 /* get kernel space into sr6 */ >> - mfsp %sr7,%r1 /* save user sr7 */ >> - mtsp %r1,%sr3 /* and store it in sr3 */ >> >> #ifdef CONFIG_64BIT >> /* for now we can *always* set the W bit on entry to the syscall >> @@ -134,20 +132,26 @@ linux_gateway_entry: >> 1: >> #endif >> mfctl %cr30,%r1 >> - xor %r1,%r30,%r30 /* ye olde xor trick */ >> - xor %r1,%r30,%r1 >> - xor %r1,%r30,%r30 >> - >> - ldo THREAD_SZ_ALGN+FRAME_SIZE(%r30),%r30 /* set up kernel stack */ >> + ldo THREAD_SZ_ALGN+FRAME_SIZE(%r1),%r1 /* set up kernel stack */ >> >> /* N.B.: It is critical that we don't set sr7 to 0 until r30 >> * contains a valid kernel stack pointer. It is also >> * critical that we don't start using the kernel stack >> - * until after sr7 has been set to 0. >> + * until after sr7 has been set to 0. To ensure this we >> + * use a rsm/ssm pair to make this operation atomic. >> + * At syscall entry %sr2 points to kernel space, otherwise >> + * syscalls wouldn't work. >> */ >> + rsm PSW_SM_I, %r0 /* turn interrupts off */ >> + STREGM %r30,FRAME_SIZE(%sr2, %r1) /* save usp on kernel stack */ >> + copy %r1, %r30 /* switch to kernel stack */ >> + >> + mfsp %sr7,%r1 /* get user sr7 */ >> + mtsp %r1,%sr3 /* store user sr7 in sr3 */ >> >> mtsp %r0,%sr7 /* get kernel space into sr7 */ >> - STREGM %r1,FRAME_SIZE(%r30) /* save r1 (usp) here for now */ >> + ssm PSW_SM_I, %r0 /* turn interrupts on */ >> + >> mfctl %cr30,%r1 /* get task ptr in %r1 */ >> LDREG TI_TASK(%r1),%r1 > > > The above doesn't assemble. The STREGM instruction doesn't allow explicit space register > selection with long displacements. Argh. The assembler didn't even complained, but after looking at the generated assembly you are right. Thanks for noticing! > The attached patch does assemble and v4.7.10 boots successfully with change. > I'm thinking we might now be able to remove the restriction on scheduling on the gateway page. Yes, probably. Helge ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-10-29 17:33 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-10-28 20:04 [PATCH] parisc: Ensure consistent state when switching to kernel stack at syscall entry Helge Deller 2016-10-29 3:00 ` John David Anglin 2016-10-29 17:33 ` Helge Deller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.