| Message ID | 20241015082121.2484410-1-smonga@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] powerpc64: Obviate the need for ROP protection in clone/clone3 | expand |
On 15/10/24 05:21, Sachin Monga wrote: > Spilling lr in a non-volatile before scv. > The non-volatile was unused and already saved/restored. > > Removed the dead code from clone. > > Signed-off-by: Sachin Monga <smonga@linux.ibm.com> > --- > sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S | 7 ++----- > sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S | 6 ++---- > 2 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S > index 164311d2bd..e57cb6e82e 100644 > --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S > +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S > @@ -56,7 +56,6 @@ ENTRY (__clone) > > /* Save fn, args, stack across syscall. */ > mr r30,r3 /* Function in r30. */ > - mr r29,r5 /* Flags in r29. */ > mr r31,r6 /* Argument in r31. */ > > /* 'flags' argument is first parameter to clone syscall. > @@ -77,14 +76,12 @@ ENTRY (__clone) > CHECK_SCV_SUPPORT r28 0f > /* This is equivalent to DO_CALL_SCV, but we cannot use the macro here > because it uses CFI directives and we just called cfi_endproc. */ > - mflr r9 > - std r9,FRAME_LR_SAVE(r1) > + mflr r29 > .machine "push" > .machine "power9" > scv 0 > .machine "pop" > - ld r9,FRAME_LR_SAVE(r1) > - mtlr r9 > + mtlr r29 > > /* Check for child process. */ > /* When using scv, error is indicated by negative r3. */ Ok. > diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S > index 900c354c9c..a48e0115ab 100644 > --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S > +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S > @@ -39,14 +39,12 @@ > because it uses CFI directives and we just called cfi_endproc. */ > # define DO_CLONE3_SVC_CALL(jumpfalse) \ > CHECK_SCV_SUPPORT r28 jumpfalse; \ > - mflr r9; \ > - std r9, FRAME_LR_SAVE(r1); \ > + mflr r31; \ > .machine "push"; \ > .machine "power9"; \ > scv 0; \ > .machine "pop"; \ > - ld r9, FRAME_LR_SAVE(r1); \ > - mtlr r9; \ > + mtlr r31; \ > /* With scv an, an error is a value -4095 <= x < 0. */ \ > cmpdi cr1, r3, 0; \ > b 1f; Shouldn't we save 'r31' in red zone now that clone3 uses it?
On 10/15/24 8:29 AM, Adhemerval Zanella Netto wrote: > On 15/10/24 05:21, Sachin Monga wrote: >> --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S >> +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S >> @@ -39,14 +39,12 @@ >> because it uses CFI directives and we just called cfi_endproc. */ >> # define DO_CLONE3_SVC_CALL(jumpfalse) \ >> CHECK_SCV_SUPPORT r28 jumpfalse; \ >> - mflr r9; \ >> - std r9, FRAME_LR_SAVE(r1); \ >> + mflr r31; \ >> .machine "push"; \ >> .machine "power9"; \ >> scv 0; \ >> .machine "pop"; \ >> - ld r9, FRAME_LR_SAVE(r1); \ >> - mtlr r9; \ >> + mtlr r31; \ >> /* With scv an, an error is a value -4095 <= x < 0. */ \ >> cmpdi cr1, r3, 0; \ >> b 1f; > > Shouldn't we save 'r31' in red zone now that clone3 uses it? Agreed, we need to save & restore r31. I mentioned this to Sachin offline and I think he ended up sending an old version of the patch. He told me he would resubmit the updated patch that includes the save/restore of r31. Peter
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S index 164311d2bd..e57cb6e82e 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S @@ -56,7 +56,6 @@ ENTRY (__clone) /* Save fn, args, stack across syscall. */ mr r30,r3 /* Function in r30. */ - mr r29,r5 /* Flags in r29. */ mr r31,r6 /* Argument in r31. */ /* 'flags' argument is first parameter to clone syscall. @@ -77,14 +76,12 @@ ENTRY (__clone) CHECK_SCV_SUPPORT r28 0f /* This is equivalent to DO_CALL_SCV, but we cannot use the macro here because it uses CFI directives and we just called cfi_endproc. */ - mflr r9 - std r9,FRAME_LR_SAVE(r1) + mflr r29 .machine "push" .machine "power9" scv 0 .machine "pop" - ld r9,FRAME_LR_SAVE(r1) - mtlr r9 + mtlr r29 /* Check for child process. */ /* When using scv, error is indicated by negative r3. */ diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S index 900c354c9c..a48e0115ab 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S @@ -39,14 +39,12 @@ because it uses CFI directives and we just called cfi_endproc. */ # define DO_CLONE3_SVC_CALL(jumpfalse) \ CHECK_SCV_SUPPORT r28 jumpfalse; \ - mflr r9; \ - std r9, FRAME_LR_SAVE(r1); \ + mflr r31; \ .machine "push"; \ .machine "power9"; \ scv 0; \ .machine "pop"; \ - ld r9, FRAME_LR_SAVE(r1); \ - mtlr r9; \ + mtlr r31; \ /* With scv an, an error is a value -4095 <= x < 0. */ \ cmpdi cr1, r3, 0; \ b 1f;
Spilling lr in a non-volatile before scv. The non-volatile was unused and already saved/restored. Removed the dead code from clone. Signed-off-by: Sachin Monga <smonga@linux.ibm.com> --- sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S | 7 ++----- sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S | 6 ++---- 2 files changed, 4 insertions(+), 9 deletions(-)