| Message ID | 20240221161307.2821102-1-stli@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | S390: Do not clobber r7 in clone [BZ #31402] | expand |
On Wed, Feb 21, 2024 at 05:13:07PM +0100, Stefan Liebler wrote: > Starting with commit e57d8fc97b90127de4ed3e3a9cdf663667580935 > "S390: Always use svc 0" > clone clobbers the call-saved register r7 in error case: > function or stack is NULL. > > This patch restores the saved registers also in the error case. You could also just restore just %r7 and not both %r6 and %r7, because only %r7 has been modified. But no idea what is actually faster (and if equally fast what is smaller), especially when the saving has been done using stm/stmg of the pair. Also, wonder if there shouldn't be a testcase covering it, just call clone (NULL, NULL, ...) and verify that it returned < 0/EINVAL and try to create high register preassure in the function across the call and make sure it fails without the patch and succeeds with it? Like below, though it will need to be adjusted for the glibc ways of doing testcases (instead of abort do what is usual for test failures, etc.), plus make sure it is tested only on arches which actually have clone (i.e. Linux). #define _GNU_SOURCE #include <stdlib.h> #include <sched.h> #include <signal.h> #include <errno.h> volatile unsigned v = 0xdeadbeef; int main () { unsigned int a = v; unsigned int b = v; unsigned int c = v; unsigned int d = v; unsigned int e = v; unsigned int f = v; unsigned int g = v; unsigned int h = v; unsigned int i = v; unsigned int j = v; unsigned int k = v; unsigned int l = v; unsigned int m = v; unsigned int n = v; unsigned int o = v; if (clone (NULL, NULL, SIGCHLD, NULL) >= 0 || errno != EINVAL) abort (); if (a != v || b != v || c != v || d != v || e != v || f != v || g != v || h != v || i != v || j != v || k != v || l != v || m != v || n != v || o != v) abort (); } Jakub
On 21.02.24 17:31, Jakub Jelinek wrote: > On Wed, Feb 21, 2024 at 05:13:07PM +0100, Stefan Liebler wrote: >> Starting with commit e57d8fc97b90127de4ed3e3a9cdf663667580935 >> "S390: Always use svc 0" >> clone clobbers the call-saved register r7 in error case: >> function or stack is NULL. >> >> This patch restores the saved registers also in the error case. > > You could also just restore just %r7 and not both %r6 and %r7, > because only %r7 has been modified. But no idea what is actually > faster (and if equally fast what is smaller), especially when the > saving has been done using stm/stmg of the pair. Yes, you are right, restoring r7 is enough. Nevertheless I prefer that the error-label is equal to the remaining code. > > Also, wonder if there shouldn't be a testcase covering it, just > call clone (NULL, NULL, ...) and verify that it returned < 0/EINVAL > and try to create high register preassure in the function across the > call and make sure it fails without the patch and succeeds with it? > > Like below, though it will need to be adjusted for the glibc ways of doing > testcases (instead of abort do what is usual for test failures, etc.), > plus make sure it is tested only on arches which actually have clone (i.e. > Linux). I've extended the already existing testcase for all error cases and added your suggested register clobber test. It fails without the fix and passes with it. Here is V2: [PATCH v2] S390: Do not clobber r7 in clone [BZ #31402] https://sourceware.org/pipermail/libc-alpha/2024-February/154911.html > > #define _GNU_SOURCE > #include <stdlib.h> > #include <sched.h> > #include <signal.h> > #include <errno.h> > > volatile unsigned v = 0xdeadbeef; > > int > main () > { > unsigned int a = v; > unsigned int b = v; > unsigned int c = v; > unsigned int d = v; > unsigned int e = v; > unsigned int f = v; > unsigned int g = v; > unsigned int h = v; > unsigned int i = v; > unsigned int j = v; > unsigned int k = v; > unsigned int l = v; > unsigned int m = v; > unsigned int n = v; > unsigned int o = v; > if (clone (NULL, NULL, SIGCHLD, NULL) >= 0 || errno != EINVAL) > abort (); > if (a != v || b != v || c != v || d != v || e != v > || f != v || g != v || h != v || i != v || j != v > || k != v || l != v || m != v || n != v || o != v) > abort (); > } > > Jakub >
diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/clone.S b/sysdeps/unix/sysv/linux/s390/s390-32/clone.S index 4c882ef2ee..a7a863242c 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-32/clone.S +++ b/sysdeps/unix/sysv/linux/s390/s390-32/clone.S @@ -53,6 +53,7 @@ ENTRY(__clone) br %r14 error: lhi %r2,-EINVAL + lm %r6,%r7,24(%r15) /* Load registers. */ j SYSCALL_ERROR_LABEL PSEUDO_END (__clone) diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/clone.S b/sysdeps/unix/sysv/linux/s390/s390-64/clone.S index 4eb104be71..c552a6b8de 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-64/clone.S +++ b/sysdeps/unix/sysv/linux/s390/s390-64/clone.S @@ -54,6 +54,7 @@ ENTRY(__clone) br %r14 error: lghi %r2,-EINVAL + lmg %r6,%r7,48(%r15) /* Restore registers. */ jg SYSCALL_ERROR_LABEL PSEUDO_END (__clone)