Re: [PATCH v2 23/25] powerpc/signal: Create 'unsafe' versions of copy_[ck][fpr/vsx]_to_user()
Le 29/09/2020 à 07:22, Christophe Leroy a écrit :
Le 29/09/2020 à 04:04, Christopher M. Riedl a écrit :
On Tue Aug 18, 2020 at 12:19 PM CDT, Christophe Leroy wrote:
For the non VSX version, that's trivial. Just use unsafe_copy_to_user()
instead of __copy_to_user().
For the VSX version, remove the intermediate step through a buffer and
use unsafe_put_user() directly. This generates a far smaller code which
is acceptable to inline, see below:
Signed-off-by: Christophe Leroy
---
arch/powerpc/kernel/signal.h | 53
1 file changed, 53 insertions(+)
diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
index f610cfafa478..2559a681536e 100644
--- a/arch/powerpc/kernel/signal.h
+++ b/arch/powerpc/kernel/signal.h
@@ -32,7 +32,54 @@ unsigned long copy_fpr_to_user(void __user *to,
struct task_struct *task);
unsigned long copy_ckfpr_to_user(void __user *to, struct task_struct
*task);
unsigned long copy_fpr_from_user(struct task_struct *task, void __user
*from);
unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user
*from);
+
+#define unsafe_copy_fpr_to_user(to, task, label) do { \
+ struct task_struct *__t = task; \
+ u64 __user *buf = (u64 __user *)to; \
+ int i; \
+ \
+ for (i = 0; i < ELF_NFPREG - 1 ; i++) \
+ unsafe_put_user(__t->thread.TS_FPR(i), [i], label); \
+ unsafe_put_user(__t->thread.fp_state.fpscr, [i], label); \
+} while (0)
+
I've been working on the PPC64 side of this "unsafe" rework using this
series as a basis. One question here - I don't really understand what
the benefit of re-implementing this logic in macros (similarly for the
other copy_* functions below) is?
Not sure either.
The whole purpose is to not manage the error through a local var but
exclusively use labels.
However, GCC is probably smart enough to understand it and drop the local var
while inlining.
One important thing however is to make sure we won't end up with an outline function, otherwise you
completely loose the benefit of the label stuff. And you get a function call inside a user access,
which is what we want to avoid.
I am considering a "__unsafe_copy_*" implementation in signal.c for
each (just the original implementation w/ using the "unsafe_" variants
of the uaccess stuff) which gets called by the "safe" functions w/ the
appropriate "user_*_access_begin/user_*_access_end". Something like
(pseudo-ish code):
Good idea, however ...
/* signal.c */
unsigned long __unsafe_copy_fpr_to_user(...)
{
...
unsafe_copy_to_user(..., bad);
return 0;
bad:
return 1; /* -EFAULT? */
}
This __unsafe_copy_fpr_to_user() has to be in signal.h and must be tagged 'static __always_inline'
for the reasons explained above.
unsigned long copy_fpr_to_user(...)
{
unsigned long err;
if (!user_write_access_begin(...))
return 1; /* -EFAULT? */
err = __unsafe_copy_fpr_to_user(...);
user_write_access_end();
return err;
}
Also note that at the end (ie when both PPC32 and PPC64 signal code are using "unsafe" versions),
the "safe" version won't be used anymore and will be dropped.
Christophe
Re: [PATCH v2 23/25] powerpc/signal: Create 'unsafe' versions of copy_[ck][fpr/vsx]_to_user()
Le 29/09/2020 à 04:04, Christopher M. Riedl a écrit :
On Tue Aug 18, 2020 at 12:19 PM CDT, Christophe Leroy wrote:
For the non VSX version, that's trivial. Just use unsafe_copy_to_user()
instead of __copy_to_user().
For the VSX version, remove the intermediate step through a buffer and
use unsafe_put_user() directly. This generates a far smaller code which
is acceptable to inline, see below:
Signed-off-by: Christophe Leroy
---
arch/powerpc/kernel/signal.h | 53
1 file changed, 53 insertions(+)
diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
index f610cfafa478..2559a681536e 100644
--- a/arch/powerpc/kernel/signal.h
+++ b/arch/powerpc/kernel/signal.h
@@ -32,7 +32,54 @@ unsigned long copy_fpr_to_user(void __user *to,
struct task_struct *task);
unsigned long copy_ckfpr_to_user(void __user *to, struct task_struct
*task);
unsigned long copy_fpr_from_user(struct task_struct *task, void __user
*from);
unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user
*from);
+
+#define unsafe_copy_fpr_to_user(to, task, label) do { \
+ struct task_struct *__t = task; \
+ u64 __user *buf = (u64 __user *)to; \
+ int i; \
+ \
+ for (i = 0; i < ELF_NFPREG - 1 ; i++) \
+ unsafe_put_user(__t->thread.TS_FPR(i), [i], label); \
+ unsafe_put_user(__t->thread.fp_state.fpscr, [i], label); \
+} while (0)
+
I've been working on the PPC64 side of this "unsafe" rework using this
series as a basis. One question here - I don't really understand what
the benefit of re-implementing this logic in macros (similarly for the
other copy_* functions below) is?
Not sure either.
The whole purpose is to not manage the error through a local var but
exclusively use labels.
However, GCC is probably smart enough to understand it and drop the local var
while inlining.
One important thing however is to make sure we won't end up with an outline function, otherwise you
completely loose the benefit of the label stuff. And you get a function call inside a user access,
which is what we want to avoid.
I am considering a "__unsafe_copy_*" implementation in signal.c for
each (just the original implementation w/ using the "unsafe_" variants
of the uaccess stuff) which gets called by the "safe" functions w/ the
appropriate "user_*_access_begin/user_*_access_end". Something like
(pseudo-ish code):
Good idea, however ...
/* signal.c */
unsigned long __unsafe_copy_fpr_to_user(...)
{
...
unsafe_copy_to_user(..., bad);
return 0;
bad:
return 1; /* -EFAULT? */
}
This __unsafe_copy_fpr_to_user() has to be in signal.h and must be tagged 'static __always_inline'
for the reasons explained above.
unsigned long copy_fpr_to_user(...)
{
unsigned long err;
if (!user_write_access_begin(...))
return 1; /* -EFAULT? */
err = __unsafe_copy_fpr_to_user(...);
user_write_access_end();
return err;
}
/* signal.h */
unsigned long __unsafe_copy_fpr_to_user(...);
#define unsafe_copy_fpr_to_user(..., label) \
unsafe_op_wrap(__unsafe_copy_fpr_to_user(...), label)
This way there is a single implementation for each copy routine "body".
The "unsafe" wrappers then just exist as simple macros similar to what
x86 does: "unsafe_" macro wraps "__unsafe" functions for the goto label.
Yes, good point.
Christophe
Re: [PATCH v2 23/25] powerpc/signal: Create 'unsafe' versions of copy_[ck][fpr/vsx]_to_user()
On Tue Aug 18, 2020 at 12:19 PM CDT, Christophe Leroy wrote:
> For the non VSX version, that's trivial. Just use unsafe_copy_to_user()
> instead of __copy_to_user().
>
> For the VSX version, remove the intermediate step through a buffer and
> use unsafe_put_user() directly. This generates a far smaller code which
> is acceptable to inline, see below:
>
> Standard VSX version:
>
> <.copy_fpr_to_user>:
> 0: 7c 08 02 a6 mflr r0
> 4: fb e1 ff f8 std r31,-8(r1)
> 8: 39 00 00 20 li r8,32
> c: 39 24 0b 80 addi r9,r4,2944
> 10: 7d 09 03 a6 mtctr r8
> 14: f8 01 00 10 std r0,16(r1)
> 18: f8 21 fe 71 stdu r1,-400(r1)
> 1c: 39 41 00 68 addi r10,r1,104
> 20: e9 09 00 00 ld r8,0(r9)
> 24: 39 4a 00 08 addi r10,r10,8
> 28: 39 29 00 10 addi r9,r9,16
> 2c: f9 0a 00 00 std r8,0(r10)
> 30: 42 00 ff f0 bdnz 20 <.copy_fpr_to_user+0x20>
> 34: e9 24 0d 80 ld r9,3456(r4)
> 38: 3d 42 00 00 addis r10,r2,0
> 3a: R_PPC64_TOC16_HA .toc
> 3c: eb ea 00 00 ld r31,0(r10)
> 3e: R_PPC64_TOC16_LO_DS .toc
> 40: f9 21 01 70 std r9,368(r1)
> 44: e9 3f 00 00 ld r9,0(r31)
> 48: 81 29 00 20 lwz r9,32(r9)
> 4c: 2f 89 00 00 cmpwi cr7,r9,0
> 50: 40 9c 00 18 bge cr7,68 <.copy_fpr_to_user+0x68>
> 54: 4c 00 01 2c isync
> 58: 3d 20 40 00 lis r9,16384
> 5c: 79 29 07 c6 rldicr r9,r9,32,31
> 60: 7d 3d 03 a6 mtspr 29,r9
> 64: 4c 00 01 2c isync
> 68: 38 a0 01 08 li r5,264
> 6c: 38 81 00 70 addi r4,r1,112
> 70: 48 00 00 01 bl 70 <.copy_fpr_to_user+0x70>
> 70: R_PPC64_REL24 .__copy_tofrom_user
> 74: 60 00 00 00 nop
> 78: e9 3f 00 00 ld r9,0(r31)
> 7c: 81 29 00 20 lwz r9,32(r9)
> 80: 2f 89 00 00 cmpwi cr7,r9,0
> 84: 40 9c 00 18 bge cr7,9c <.copy_fpr_to_user+0x9c>
> 88: 4c 00 01 2c isync
> 8c: 39 20 ff ff li r9,-1
> 90: 79 29 00 44 rldicr r9,r9,0,1
> 94: 7d 3d 03 a6 mtspr 29,r9
> 98: 4c 00 01 2c isync
> 9c: 38 21 01 90 addi r1,r1,400
> a0: e8 01 00 10 ld r0,16(r1)
> a4: eb e1 ff f8 ld r31,-8(r1)
> a8: 7c 08 03 a6 mtlr r0
> ac: 4e 80 00 20 blr
>
> 'unsafe' simulated VSX version (The ... are only nops) using
> unsafe_copy_fpr_to_user() macro:
>
> unsigned long copy_fpr_to_user(void __user *to,
> struct task_struct *task)
> {
> unsafe_copy_fpr_to_user(to, task, failed);
> return 0;
> failed:
> return 1;
> }
>
> <.copy_fpr_to_user>:
> 0: 39 00 00 20 li r8,32
> 4: 39 44 0b 80 addi r10,r4,2944
> 8: 7d 09 03 a6 mtctr r8
> c: 7c 69 1b 78 mr r9,r3
> ...
> 20: e9 0a 00 00 ld r8,0(r10)
> 24: f9 09 00 00 std r8,0(r9)
> 28: 39 4a 00 10 addi r10,r10,16
> 2c: 39 29 00 08 addi r9,r9,8
> 30: 42 00 ff f0 bdnz 20 <.copy_fpr_to_user+0x20>
> 34: e9 24 0d 80 ld r9,3456(r4)
> 38: f9 23 01 00 std r9,256(r3)
> 3c: 38 60 00 00 li r3,0
> 40: 4e 80 00 20 blr
> ...
> 50: 38 60 00 01 li r3,1
> 54: 4e 80 00 20 blr
>
> Signed-off-by: Christophe Leroy
> ---
> arch/powerpc/kernel/signal.h | 53
> 1 file changed, 53 insertions(+)
>
> diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
> index f610cfafa478..2559a681536e 100644
> --- a/arch/powerpc/kernel/signal.h
> +++ b/arch/powerpc/kernel/signal.h
> @@ -32,7 +32,54 @@ unsigned long copy_fpr_to_user(void __user *to,
> struct task_struct *task);
> unsigned long copy_ckfpr_to_user(void __user *to, struct task_struct
> *task);
> unsigned long copy_fpr_from_user(struct task_struct *task, void __user
> *from);
> unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user
> *from);
> +
> +#define unsafe_copy_fpr_to_user(to, task, label) do { \
> + struct task_struct *__t = task; \
> + u64 __user *buf = (u64 __user *)to; \
> + int i; \
> + \
> + for (i = 0; i < ELF_NFPREG - 1 ; i++) \
> + unsafe_put_user(__t->thread.TS_FPR(i), [i], label); \
> + unsafe_put_user(__t->thread.fp_state.fpscr, [i], label); \
> +} while (0)
> +
I've been working on the PPC64 side of this "unsafe" rework using this
series as a basis. One question here - I don't really understand what
the benefit of re-implementing this logic in macros (similarly for the
other copy_* functions below) is?
I am considering a "__unsafe_copy_*" implementation in signal.c for
each (just the original implementation w/ using the "unsafe_" variants
of the uaccess stuff) which gets called by the "safe" functions w/ the
appropriate "user_*_access_begin/user_*_access_end". Something like
(pseudo-ish code):
/* signal.c */
unsigned long __unsafe_copy_fpr_to_user(...)
{
...
unsafe_copy_to_user(..., bad);
return 0;
bad:
return 1; /* -EFAULT? */
}
unsigned long copy_fpr_to_user(...)
{
unsigned long err;
if (!user_write_access_begin(...))
return 1; /* -EFAULT? */
err = __unsafe_copy_fpr_to_user(...);
user_write_access_end();
return err;
}
/* signal.h */
unsigned long __unsafe_copy_fpr_to_user(...);
#define unsafe_copy_fpr_to_user(..., label) \
[PATCH v2 23/25] powerpc/signal: Create 'unsafe' versions of copy_[ck][fpr/vsx]_to_user()
For the non VSX version, that's trivial. Just use unsafe_copy_to_user()
instead of __copy_to_user().
For the VSX version, remove the intermediate step through a buffer and
use unsafe_put_user() directly. This generates a far smaller code which
is acceptable to inline, see below:
Standard VSX version:
<.copy_fpr_to_user>:
0: 7c 08 02 a6 mflrr0
4: fb e1 ff f8 std r31,-8(r1)
8: 39 00 00 20 li r8,32
c: 39 24 0b 80 addir9,r4,2944
10: 7d 09 03 a6 mtctr r8
14: f8 01 00 10 std r0,16(r1)
18: f8 21 fe 71 stdur1,-400(r1)
1c: 39 41 00 68 addir10,r1,104
20: e9 09 00 00 ld r8,0(r9)
24: 39 4a 00 08 addir10,r10,8
28: 39 29 00 10 addir9,r9,16
2c: f9 0a 00 00 std r8,0(r10)
30: 42 00 ff f0 bdnz20 <.copy_fpr_to_user+0x20>
34: e9 24 0d 80 ld r9,3456(r4)
38: 3d 42 00 00 addis r10,r2,0
3a: R_PPC64_TOC16_HA.toc
3c: eb ea 00 00 ld r31,0(r10)
3e: R_PPC64_TOC16_LO_DS .toc
40: f9 21 01 70 std r9,368(r1)
44: e9 3f 00 00 ld r9,0(r31)
48: 81 29 00 20 lwz r9,32(r9)
4c: 2f 89 00 00 cmpwi cr7,r9,0
50: 40 9c 00 18 bge cr7,68 <.copy_fpr_to_user+0x68>
54: 4c 00 01 2c isync
58: 3d 20 40 00 lis r9,16384
5c: 79 29 07 c6 rldicr r9,r9,32,31
60: 7d 3d 03 a6 mtspr 29,r9
64: 4c 00 01 2c isync
68: 38 a0 01 08 li r5,264
6c: 38 81 00 70 addir4,r1,112
70: 48 00 00 01 bl 70 <.copy_fpr_to_user+0x70>
70: R_PPC64_REL24 .__copy_tofrom_user
74: 60 00 00 00 nop
78: e9 3f 00 00 ld r9,0(r31)
7c: 81 29 00 20 lwz r9,32(r9)
80: 2f 89 00 00 cmpwi cr7,r9,0
84: 40 9c 00 18 bge cr7,9c <.copy_fpr_to_user+0x9c>
88: 4c 00 01 2c isync
8c: 39 20 ff ff li r9,-1
90: 79 29 00 44 rldicr r9,r9,0,1
94: 7d 3d 03 a6 mtspr 29,r9
98: 4c 00 01 2c isync
9c: 38 21 01 90 addir1,r1,400
a0: e8 01 00 10 ld r0,16(r1)
a4: eb e1 ff f8 ld r31,-8(r1)
a8: 7c 08 03 a6 mtlrr0
ac: 4e 80 00 20 blr
'unsafe' simulated VSX version (The ... are only nops) using
unsafe_copy_fpr_to_user() macro:
unsigned long copy_fpr_to_user(void __user *to,
struct task_struct *task)
{
unsafe_copy_fpr_to_user(to, task, failed);
return 0;
failed:
return 1;
}
<.copy_fpr_to_user>:
0: 39 00 00 20 li r8,32
4: 39 44 0b 80 addir10,r4,2944
8: 7d 09 03 a6 mtctr r8
c: 7c 69 1b 78 mr r9,r3
...
20: e9 0a 00 00 ld r8,0(r10)
24: f9 09 00 00 std r8,0(r9)
28: 39 4a 00 10 addir10,r10,16
2c: 39 29 00 08 addir9,r9,8
30: 42 00 ff f0 bdnz20 <.copy_fpr_to_user+0x20>
34: e9 24 0d 80 ld r9,3456(r4)
38: f9 23 01 00 std r9,256(r3)
3c: 38 60 00 00 li r3,0
40: 4e 80 00 20 blr
...
50: 38 60 00 01 li r3,1
54: 4e 80 00 20 blr
Signed-off-by: Christophe Leroy
---
arch/powerpc/kernel/signal.h | 53
1 file changed, 53 insertions(+)
diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
index f610cfafa478..2559a681536e 100644
--- a/arch/powerpc/kernel/signal.h
+++ b/arch/powerpc/kernel/signal.h
@@ -32,7 +32,54 @@ unsigned long copy_fpr_to_user(void __user *to, struct
task_struct *task);
unsigned long copy_ckfpr_to_user(void __user *to, struct task_struct *task);
unsigned long copy_fpr_from_user(struct task_struct *task, void __user *from);
unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user
*from);
+
+#define unsafe_copy_fpr_to_user(to, task, label) do {\
+ struct task_struct *__t = task; \
+ u64 __user *buf = (u64 __user *)to; \
+ int i; \
+ \
+ for (i = 0; i < ELF_NFPREG - 1 ; i++) \
+ unsafe_put_user(__t->thread.TS_FPR(i), [i], label); \
+ unsafe_put_user(__t->thread.fp_state.fpscr, [i], label);\
+} while (0)
+
+#define unsafe_copy_vsx_to_user(to, task, label) do {\
+ struct task_struct *__t = task; \
+ u64 __user *buf = (u64 __user *)to; \
+ int i; \
+ \
+ for (i = 0; i < ELF_NVSRHALFREG ; i++) \
+
