| Message ID | 1370755352-29901-1-git-send-email-dongsheng.wang@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 > }
> -----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 > > } > >
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.
> -----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. > >
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
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 }
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(-)