Patchwork [v2,2/2] powerpc/hibernate: PPC64 fix user threads access to kernel space

login
register
mail settings
Submitter Dongsheng Wang
Date June 9, 2013, 5:22 a.m.
Message ID <1370755352-29901-1-git-send-email-dongsheng.wang@freescale.com>
Download mbox | patch
Permalink /patch/250007/
State Superseded
Headers show

Comments

Dongsheng Wang - June 9, 2013, 5:22 a.m.
If PID is used in the TLB, after hibernation resume, the user
threads will access to kernel space.

We must restore PID register, because TLB will use PID. The
hibernation suspend flow is trapped from user space to kernel
space, the PID register is user thread pid.

The hibernation resume is begin in kernel start flow, the PID
alway 0. After the kernel thread back to user thread, there is
not have context switch and the pid can not update, because the
kernel thread is trapped form user space. So if we did't restore
PID the user space of thread will be addressing in the kernel
space.

There are two ways to restore PID:
1/ In swsusp_arch_suspend/swsusp_arch_resume, save/resotre PID register.
2/ Form restore_processor_state to restore. this function will
   do context switch.
   switch_mmu_context(current->active_mm, current->active_mm)

PPC32 Using the second method. For consistency reason, PPC64 using
the same way.

Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
---
 arch/powerpc/kernel/swsusp.c | 2 --
 1 file changed, 2 deletions(-)
Benjamin Herrenschmidt - June 9, 2013, 6:43 a.m.
On Sun, 2013-06-09 at 13:22 +0800, Wang Dongsheng wrote:
> If PID is used in the TLB, after hibernation resume, the user
> threads will access to kernel space.

 .../...

I think the explanation is way more convoluted and confusing
here than anything else.

Simply say that upon resume from hibernation, the MMU context
needs to be restored (this includes the PID register today
it might include more if we decided to pre-set some MAS for example
etc...) and be done with it.

Note that switch_mmu_context() used the way you do is quite "full on",
it will do a whole pile of stuff that are probably completely
unnecessary, and in addition might still miss the need to completely
flush the TLB anyway.

I would suggest that instead, somebody adds the necessary routine
to tlb_nohash.c, something like restore_mmu_context() which will
do that.

Cheers,
Ben.

> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> ---
>  arch/powerpc/kernel/swsusp.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/swsusp.c b/arch/powerpc/kernel/swsusp.c
> index eae33e1..1930e44 100644
> --- a/arch/powerpc/kernel/swsusp.c
> +++ b/arch/powerpc/kernel/swsusp.c
> @@ -32,7 +32,5 @@ void save_processor_state(void)
>  
>  void restore_processor_state(void)
>  {
> -#ifdef CONFIG_PPC32
>  	switch_mmu_context(current->active_mm, current->active_mm);
> -#endif
>  }
Wang Dongsheng-B40534 - June 9, 2013, 7:44 a.m.
> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]
> Sent: Sunday, June 09, 2013 2:44 PM
> To: Wang Dongsheng-B40534
> Cc: johannes@sipsolutions.net; anton@enomsg.org; Wood Scott-B07421;
> galak@kernel.crashing.org; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH v2 2/2] powerpc/hibernate: PPC64 fix user threads
> access to kernel space
> 
> On Sun, 2013-06-09 at 13:22 +0800, Wang Dongsheng wrote:
> > If PID is used in the TLB, after hibernation resume, the user threads
> > will access to kernel space.
> 
>  .../...
> 
> I think the explanation is way more convoluted and confusing here than
> anything else.
> 
> Simply say that upon resume from hibernation, the MMU context needs to be
> restored (this includes the PID register today it might include more if
> we decided to pre-set some MAS for example
> etc...) and be done with it.
> 
> Note that switch_mmu_context() used the way you do is quite "full on", it
> will do a whole pile of stuff that are probably completely unnecessary,
> and in addition might still miss the need to completely flush the TLB
> anyway.
> 
> I would suggest that instead, somebody adds the necessary routine to
> tlb_nohash.c, something like restore_mmu_context() which will do that.
> 
Thanks ben, This is a good idea.

We do not need to decide the current thread has a context in restore_mmu_context().
Because the current has already get a context in hibernation suspend flow.

So we just need set set_context() in restore_mmu_context().

void restore_mmu_context(struct mm_struct *next) {
	set_context(next->context.id, next->pgd);
}

-dongsheng

> Cheers,
> Ben.
> 
> > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> > ---
> >  arch/powerpc/kernel/swsusp.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/swsusp.c
> > b/arch/powerpc/kernel/swsusp.c index eae33e1..1930e44 100644
> > --- a/arch/powerpc/kernel/swsusp.c
> > +++ b/arch/powerpc/kernel/swsusp.c
> > @@ -32,7 +32,5 @@ void save_processor_state(void)
> >
> >  void restore_processor_state(void)
> >  {
> > -#ifdef CONFIG_PPC32
> >  	switch_mmu_context(current->active_mm, current->active_mm); -#endif
> > }
> 
>
Benjamin Herrenschmidt - June 9, 2013, 7:45 a.m.
On Sun, 2013-06-09 at 07:44 +0000, Wang Dongsheng-B40534 wrote:
> So we just need set set_context() in restore_mmu_context().
> 
> void restore_mmu_context(struct mm_struct *next) {
>         set_context(next->context.id, next->pgd);
> }

We probably also want to flush the TLB, just in case the boot kernel has
left "something" there (though I wouldn't expect it to have run
userspace it's not completely impossible).

Cheers,
Ben.
Wang Dongsheng-B40534 - June 9, 2013, 7:57 a.m.
> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]
> Sent: Sunday, June 09, 2013 3:46 PM
> To: Wang Dongsheng-B40534
> Cc: johannes@sipsolutions.net; anton@enomsg.org; Wood Scott-B07421;
> galak@kernel.crashing.org; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH v2 2/2] powerpc/hibernate: PPC64 fix user threads
> access to kernel space
> 
> On Sun, 2013-06-09 at 07:44 +0000, Wang Dongsheng-B40534 wrote:
> > So we just need set set_context() in restore_mmu_context().
> >
> > void restore_mmu_context(struct mm_struct *next) {
> >         set_context(next->context.id, next->pgd); }
> 
> We probably also want to flush the TLB, just in case the boot kernel has
> left "something" there (though I wouldn't expect it to have run userspace
> it's not completely impossible).
> 
Yes, but TLB has already invalidated.
See my other patch:
http://patchwork.ozlabs.org/patch/250006/

> Cheers,
> Ben.
> 
>
Wang Dongsheng-B40534 - June 9, 2013, 11:31 a.m.
Sorry, Please ignore this patch.
This is replaced.
Replace by: http://patchwork.ozlabs.org/patch/250033/

- dongsheng

> -----Original Message-----
> From: Wang Dongsheng-B40534
> Sent: Sunday, June 09, 2013 1:23 PM
> To: benh@kernel.crashing.org; johannes@sipsolutions.net; anton@enomsg.org
> Cc: Wood Scott-B07421; galak@kernel.crashing.org; linuxppc-
> dev@lists.ozlabs.org; Wang Dongsheng-B40534
> Subject: [PATCH v2 2/2] powerpc/hibernate: PPC64 fix user threads access
> to kernel space
> 
> If PID is used in the TLB, after hibernation resume, the user
> threads will access to kernel space.
> 
> We must restore PID register, because TLB will use PID. The
> hibernation suspend flow is trapped from user space to kernel
> space, the PID register is user thread pid.
> 
> The hibernation resume is begin in kernel start flow, the PID
> alway 0. After the kernel thread back to user thread, there is
> not have context switch and the pid can not update, because the
> kernel thread is trapped form user space. So if we did't restore
> PID the user space of thread will be addressing in the kernel
> space.
> 
> There are two ways to restore PID:
> 1/ In swsusp_arch_suspend/swsusp_arch_resume, save/resotre PID register.
> 2/ Form restore_processor_state to restore. this function will
>    do context switch.
>    switch_mmu_context(current->active_mm, current->active_mm)
> 
> PPC32 Using the second method. For consistency reason, PPC64 using
> the same way.
> 
> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> ---
>  arch/powerpc/kernel/swsusp.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/swsusp.c b/arch/powerpc/kernel/swsusp.c
> index eae33e1..1930e44 100644
> --- a/arch/powerpc/kernel/swsusp.c
> +++ b/arch/powerpc/kernel/swsusp.c
> @@ -32,7 +32,5 @@ void save_processor_state(void)
> 
>  void restore_processor_state(void)
>  {
> -#ifdef CONFIG_PPC32
>  	switch_mmu_context(current->active_mm, current->active_mm);
> -#endif
>  }
> --
> 1.8.0

Patch

diff --git a/arch/powerpc/kernel/swsusp.c b/arch/powerpc/kernel/swsusp.c
index eae33e1..1930e44 100644
--- a/arch/powerpc/kernel/swsusp.c
+++ b/arch/powerpc/kernel/swsusp.c
@@ -32,7 +32,5 @@  void save_processor_state(void)
 
 void restore_processor_state(void)
 {
-#ifdef CONFIG_PPC32
 	switch_mmu_context(current->active_mm, current->active_mm);
-#endif
 }