[PATCH] S390: Do not clobber r7 in clone [BZ #31402]

[PATCH] S390: Do not clobber r7 in clone [BZ #31402]

[Stefan Liebler]

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(+)

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)
 
-- 
2.43.0

Re: [PATCH] S390: Do not clobber r7 in clone [BZ #31402]

[Jakub Jelinek]

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

Re: [PATCH] S390: Do not clobber r7 in clone [BZ #31402]

[Stefan Liebler]

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
>