S390: Do not clobber r7 in clone [BZ #31402]
Checks
| Context |
Check |
Description |
| redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
| linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Testing passed
|
Commit Message
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.
---
sysdeps/unix/sysv/linux/s390/s390-32/clone.S | 1 +
sysdeps/unix/sysv/linux/s390/s390-64/clone.S | 1 +
2 files changed, 2 insertions(+)
Comments
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
>
@@ -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)
@@ -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)