[v2,2/6] powerpc/pkeys: Save the pkey registers before fork

Message ID 1528936144-6696-3-git-send-email-linuxram@us.ibm.com
State Superseded
Headers show
Series
  • powerpc/pkeys: fixes to pkeys
Related show

Commit Message

Ram Pai June 14, 2018, 12:29 a.m.
When a thread forks the contents of AMR, IAMR, UAMOR registers in the
newly forked thread are not inherited.

Save the registers before forking, for content of those
registers to be automatically copied into the new thread.

CC: Michael Ellerman <mpe@ellerman.id.au>
CC: Florian Weimer <fweimer@redhat.com>
CC: Andy Lutomirski <luto@kernel.org>
CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/powerpc/kernel/process.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

Comments

Michael Ellerman June 19, 2018, 12:39 p.m. | #1
Ram Pai <linuxram@us.ibm.com> writes:

> When a thread forks the contents of AMR, IAMR, UAMOR registers in the
> newly forked thread are not inherited.
>
> Save the registers before forking, for content of those
> registers to be automatically copied into the new thread.
>
> CC: Michael Ellerman <mpe@ellerman.id.au>
> CC: Florian Weimer <fweimer@redhat.com>
> CC: Andy Lutomirski <luto@kernel.org>
> CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>

Again this is an ABI change but we'll call it a bug fix I guess.

I'll add:

  Fixes: cf43d3b26452 ("powerpc: Enable pkey subsystem")
  Cc: stable@vger.kernel.org # v4.16+


cheers

> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 9ef4aea..991d097 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -583,6 +583,7 @@ static void save_all(struct task_struct *tsk)
>  		__giveup_spe(tsk);
>  
>  	msr_check_and_clear(msr_all_available);
> +	thread_pkey_regs_save(&tsk->thread);
>  }
>  
>  void flush_all_to_thread(struct task_struct *tsk)
> -- 
> 1.7.1
Ram Pai June 19, 2018, 2:28 p.m. | #2
On Tue, Jun 19, 2018 at 10:39:56PM +1000, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > When a thread forks the contents of AMR, IAMR, UAMOR registers in the
> > newly forked thread are not inherited.
> >
> > Save the registers before forking, for content of those
> > registers to be automatically copied into the new thread.
> >
> > CC: Michael Ellerman <mpe@ellerman.id.au>
> > CC: Florian Weimer <fweimer@redhat.com>
> > CC: Andy Lutomirski <luto@kernel.org>
> > CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> 
> Again this is an ABI change but we'll call it a bug fix I guess.

yes. the same defense here too. its a behaviorial change for the better.
Single threaded applications will not see any behaviorial change.
Multithreaded apps, which were unable to consume, the behavior will now be
able to do so.

> 
> I'll add:
> 
>   Fixes: cf43d3b26452 ("powerpc: Enable pkey subsystem")
>   Cc: stable@vger.kernel.org # v4.16+

yes.  Thanks
RP
Michael Ellerman June 21, 2018, 4:13 a.m. | #3
Ram Pai <linuxram@us.ibm.com> writes:

> On Tue, Jun 19, 2018 at 10:39:56PM +1000, Michael Ellerman wrote:
>> Ram Pai <linuxram@us.ibm.com> writes:
>> 
>> > When a thread forks the contents of AMR, IAMR, UAMOR registers in the
>> > newly forked thread are not inherited.
>> >
>> > Save the registers before forking, for content of those
>> > registers to be automatically copied into the new thread.
>> >
>> > CC: Michael Ellerman <mpe@ellerman.id.au>
>> > CC: Florian Weimer <fweimer@redhat.com>
>> > CC: Andy Lutomirski <luto@kernel.org>
>> > CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>> 
>> Again this is an ABI change but we'll call it a bug fix I guess.
>
> yes. the same defense here too. its a behaviorial change for the better.
> Single threaded applications will not see any behaviorial change.
> Multithreaded apps, which were unable to consume, the behavior will now be
> able to do so.

Well threads is one thing, but this also affects processes.

And actually without this fix it's possible that a child process could
fault on a region protected in the parent, if the value in the AMR in
the thread struct happens to block access at the time of fork(). The
value in the thread struct would be whatever was in the AMR the last
time the parent was scheduled in. I think?

cheers
Ram Pai June 21, 2018, 5:35 p.m. | #4
On Thu, Jun 21, 2018 at 02:13:40PM +1000, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > On Tue, Jun 19, 2018 at 10:39:56PM +1000, Michael Ellerman wrote:
> >> Ram Pai <linuxram@us.ibm.com> writes:
> >> 
> >> > When a thread forks the contents of AMR, IAMR, UAMOR registers in the
> >> > newly forked thread are not inherited.
> >> >
> >> > Save the registers before forking, for content of those
> >> > registers to be automatically copied into the new thread.
> >> >
> >> > CC: Michael Ellerman <mpe@ellerman.id.au>
> >> > CC: Florian Weimer <fweimer@redhat.com>
> >> > CC: Andy Lutomirski <luto@kernel.org>
> >> > CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> >> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> >> 
> >> Again this is an ABI change but we'll call it a bug fix I guess.
> >
> > yes. the same defense here too. its a behaviorial change for the better.
> > Single threaded applications will not see any behaviorial change.
> > Multithreaded apps, which were unable to consume, the behavior will now be
> > able to do so.
> 
> Well threads is one thing, but this also affects processes.
> 
> And actually without this fix it's possible that a child process could
> fault on a region protected in the parent, if the value in the AMR in
> the thread struct happens to block access at the time of fork(). The
> value in the thread struct would be whatever was in the AMR the last
> time the parent was scheduled in. I think?

right. Child processes will see stale value of AMR. Technically this
behavior is a bug, since existing applications; if any,  cannot rely on
this stale AMR value.

RP

Patch

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 9ef4aea..991d097 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -583,6 +583,7 @@  static void save_all(struct task_struct *tsk)
 		__giveup_spe(tsk);
 
 	msr_check_and_clear(msr_all_available);
+	thread_pkey_regs_save(&tsk->thread);
 }
 
 void flush_all_to_thread(struct task_struct *tsk)