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

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

[Wang Dongsheng]

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

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

[Benjamin Herrenschmidt]

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
>  }

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

[Wang Dongsheng-B40534]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQmVuamFtaW4gSGVycmVu
c2NobWlkdCBbbWFpbHRvOmJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZ10NCj4gU2VudDogU3VuZGF5
LCBKdW5lIDA5LCAyMDEzIDI6NDQgUE0NCj4gVG86IFdhbmcgRG9uZ3NoZW5nLUI0MDUzNA0KPiBD
Yzogam9oYW5uZXNAc2lwc29sdXRpb25zLm5ldDsgYW50b25AZW5vbXNnLm9yZzsgV29vZCBTY290
dC1CMDc0MjE7DQo+IGdhbGFrQGtlcm5lbC5jcmFzaGluZy5vcmc7IGxpbnV4cHBjLWRldkBsaXN0
cy5vemxhYnMub3JnDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjIgMi8yXSBwb3dlcnBjL2hpYmVy
bmF0ZTogUFBDNjQgZml4IHVzZXIgdGhyZWFkcw0KPiBhY2Nlc3MgdG8ga2VybmVsIHNwYWNlDQo+
IA0KPiBPbiBTdW4sIDIwMTMtMDYtMDkgYXQgMTM6MjIgKzA4MDAsIFdhbmcgRG9uZ3NoZW5nIHdy
b3RlOg0KPiA+IElmIFBJRCBpcyB1c2VkIGluIHRoZSBUTEIsIGFmdGVyIGhpYmVybmF0aW9uIHJl
c3VtZSwgdGhlIHVzZXIgdGhyZWFkcw0KPiA+IHdpbGwgYWNjZXNzIHRvIGtlcm5lbCBzcGFjZS4N
Cj4gDQo+ICAuLi4vLi4uDQo+IA0KPiBJIHRoaW5rIHRoZSBleHBsYW5hdGlvbiBpcyB3YXkgbW9y
ZSBjb252b2x1dGVkIGFuZCBjb25mdXNpbmcgaGVyZSB0aGFuDQo+IGFueXRoaW5nIGVsc2UuDQo+
IA0KPiBTaW1wbHkgc2F5IHRoYXQgdXBvbiByZXN1bWUgZnJvbSBoaWJlcm5hdGlvbiwgdGhlIE1N
VSBjb250ZXh0IG5lZWRzIHRvIGJlDQo+IHJlc3RvcmVkICh0aGlzIGluY2x1ZGVzIHRoZSBQSUQg
cmVnaXN0ZXIgdG9kYXkgaXQgbWlnaHQgaW5jbHVkZSBtb3JlIGlmDQo+IHdlIGRlY2lkZWQgdG8g
cHJlLXNldCBzb21lIE1BUyBmb3IgZXhhbXBsZQ0KPiBldGMuLi4pIGFuZCBiZSBkb25lIHdpdGgg
aXQuDQo+IA0KPiBOb3RlIHRoYXQgc3dpdGNoX21tdV9jb250ZXh0KCkgdXNlZCB0aGUgd2F5IHlv
dSBkbyBpcyBxdWl0ZSAiZnVsbCBvbiIsIGl0DQo+IHdpbGwgZG8gYSB3aG9sZSBwaWxlIG9mIHN0
dWZmIHRoYXQgYXJlIHByb2JhYmx5IGNvbXBsZXRlbHkgdW5uZWNlc3NhcnksDQo+IGFuZCBpbiBh
ZGRpdGlvbiBtaWdodCBzdGlsbCBtaXNzIHRoZSBuZWVkIHRvIGNvbXBsZXRlbHkgZmx1c2ggdGhl
IFRMQg0KPiBhbnl3YXkuDQo+IA0KPiBJIHdvdWxkIHN1Z2dlc3QgdGhhdCBpbnN0ZWFkLCBzb21l
Ym9keSBhZGRzIHRoZSBuZWNlc3Nhcnkgcm91dGluZSB0bw0KPiB0bGJfbm9oYXNoLmMsIHNvbWV0
aGluZyBsaWtlIHJlc3RvcmVfbW11X2NvbnRleHQoKSB3aGljaCB3aWxsIGRvIHRoYXQuDQo+IA0K
VGhhbmtzIGJlbiwgVGhpcyBpcyBhIGdvb2QgaWRlYS4NCg0KV2UgZG8gbm90IG5lZWQgdG8gZGVj
aWRlIHRoZSBjdXJyZW50IHRocmVhZCBoYXMgYSBjb250ZXh0IGluIHJlc3RvcmVfbW11X2NvbnRl
eHQoKS4NCkJlY2F1c2UgdGhlIGN1cnJlbnQgaGFzIGFscmVhZHkgZ2V0IGEgY29udGV4dCBpbiBo
aWJlcm5hdGlvbiBzdXNwZW5kIGZsb3cuDQoNClNvIHdlIGp1c3QgbmVlZCBzZXQgc2V0X2NvbnRl
eHQoKSBpbiByZXN0b3JlX21tdV9jb250ZXh0KCkuDQoNCnZvaWQgcmVzdG9yZV9tbXVfY29udGV4
dChzdHJ1Y3QgbW1fc3RydWN0ICpuZXh0KSB7DQoJc2V0X2NvbnRleHQobmV4dC0+Y29udGV4dC5p
ZCwgbmV4dC0+cGdkKTsNCn0NCg0KLWRvbmdzaGVuZw0KDQo+IENoZWVycywNCj4gQmVuLg0KPiAN
Cj4gPiBTaWduZWQtb2ZmLWJ5OiBXYW5nIERvbmdzaGVuZyA8ZG9uZ3NoZW5nLndhbmdAZnJlZXNj
YWxlLmNvbT4NCj4gPiAtLS0NCj4gPiAgYXJjaC9wb3dlcnBjL2tlcm5lbC9zd3N1c3AuYyB8IDIg
LS0NCj4gPiAgMSBmaWxlIGNoYW5nZWQsIDIgZGVsZXRpb25zKC0pDQo+ID4NCj4gPiBkaWZmIC0t
Z2l0IGEvYXJjaC9wb3dlcnBjL2tlcm5lbC9zd3N1c3AuYw0KPiA+IGIvYXJjaC9wb3dlcnBjL2tl
cm5lbC9zd3N1c3AuYyBpbmRleCBlYWUzM2UxLi4xOTMwZTQ0IDEwMDY0NA0KPiA+IC0tLSBhL2Fy
Y2gvcG93ZXJwYy9rZXJuZWwvc3dzdXNwLmMNCj4gPiArKysgYi9hcmNoL3Bvd2VycGMva2VybmVs
L3N3c3VzcC5jDQo+ID4gQEAgLTMyLDcgKzMyLDUgQEAgdm9pZCBzYXZlX3Byb2Nlc3Nvcl9zdGF0
ZSh2b2lkKQ0KPiA+DQo+ID4gIHZvaWQgcmVzdG9yZV9wcm9jZXNzb3Jfc3RhdGUodm9pZCkNCj4g
PiAgew0KPiA+IC0jaWZkZWYgQ09ORklHX1BQQzMyDQo+ID4gIAlzd2l0Y2hfbW11X2NvbnRleHQo
Y3VycmVudC0+YWN0aXZlX21tLCBjdXJyZW50LT5hY3RpdmVfbW0pOyAtI2VuZGlmDQo+ID4gfQ0K
PiANCj4gDQoNCg==

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

[Benjamin Herrenschmidt]

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.

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

[Wang Dongsheng-B40534]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQmVuamFtaW4gSGVycmVu
c2NobWlkdCBbbWFpbHRvOmJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZ10NCj4gU2VudDogU3VuZGF5
LCBKdW5lIDA5LCAyMDEzIDM6NDYgUE0NCj4gVG86IFdhbmcgRG9uZ3NoZW5nLUI0MDUzNA0KPiBD
Yzogam9oYW5uZXNAc2lwc29sdXRpb25zLm5ldDsgYW50b25AZW5vbXNnLm9yZzsgV29vZCBTY290
dC1CMDc0MjE7DQo+IGdhbGFrQGtlcm5lbC5jcmFzaGluZy5vcmc7IGxpbnV4cHBjLWRldkBsaXN0
cy5vemxhYnMub3JnDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjIgMi8yXSBwb3dlcnBjL2hpYmVy
bmF0ZTogUFBDNjQgZml4IHVzZXIgdGhyZWFkcw0KPiBhY2Nlc3MgdG8ga2VybmVsIHNwYWNlDQo+
IA0KPiBPbiBTdW4sIDIwMTMtMDYtMDkgYXQgMDc6NDQgKzAwMDAsIFdhbmcgRG9uZ3NoZW5nLUI0
MDUzNCB3cm90ZToNCj4gPiBTbyB3ZSBqdXN0IG5lZWQgc2V0IHNldF9jb250ZXh0KCkgaW4gcmVz
dG9yZV9tbXVfY29udGV4dCgpLg0KPiA+DQo+ID4gdm9pZCByZXN0b3JlX21tdV9jb250ZXh0KHN0
cnVjdCBtbV9zdHJ1Y3QgKm5leHQpIHsNCj4gPiAgICAgICAgIHNldF9jb250ZXh0KG5leHQtPmNv
bnRleHQuaWQsIG5leHQtPnBnZCk7IH0NCj4gDQo+IFdlIHByb2JhYmx5IGFsc28gd2FudCB0byBm
bHVzaCB0aGUgVExCLCBqdXN0IGluIGNhc2UgdGhlIGJvb3Qga2VybmVsIGhhcw0KPiBsZWZ0ICJz
b21ldGhpbmciIHRoZXJlICh0aG91Z2ggSSB3b3VsZG4ndCBleHBlY3QgaXQgdG8gaGF2ZSBydW4g
dXNlcnNwYWNlDQo+IGl0J3Mgbm90IGNvbXBsZXRlbHkgaW1wb3NzaWJsZSkuDQo+IA0KWWVzLCBi
dXQgVExCIGhhcyBhbHJlYWR5IGludmFsaWRhdGVkLg0KU2VlIG15IG90aGVyIHBhdGNoOg0KaHR0
cDovL3BhdGNod29yay5vemxhYnMub3JnL3BhdGNoLzI1MDAwNi8NCg0KPiBDaGVlcnMsDQo+IEJl
bi4NCj4gDQo+IA0KDQo=

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

[Wang Dongsheng-B40534]

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
>=20
> If PID is used in the TLB, after hibernation resume, the user
> threads will access to kernel space.
>=20
> 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.
>=20
> 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.
>=20
> 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)
>=20
> PPC32 Using the second method. For consistency reason, PPC64 using
> the same way.
>=20
> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> ---
>  arch/powerpc/kernel/swsusp.c | 2 --
>  1 file changed, 2 deletions(-)
>=20
> 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)
>=20
>  void restore_processor_state(void)
>  {
> -#ifdef CONFIG_PPC32
>  	switch_mmu_context(current->active_mm, current->active_mm);
> -#endif
>  }
> --
> 1.8.0