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), &buf[i], label); \ + unsafe_put_user(__t->thread.fp_state.fpscr, &buf[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), &buf[i], label); \ + unsafe_put_user(__t->thread.fp_state.fpscr, &buf[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), &buf[i], label); \ > + unsafe_put_user(__t->thread.fp_state.fpscr, &buf[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), &buf[i], label); \ + unsafe_put_user(__t->thread.fp_state.fpscr, &buf[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++) \ +