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

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.
Furthermore the existing test misc/tst-clone is extended to check
all error cases and that clone does not clobber registers in this
error case.
---
 sysdeps/unix/sysv/linux/s390/s390-32/clone.S |  1 +
 sysdeps/unix/sysv/linux/s390/s390-64/clone.S |  1 +
 sysdeps/unix/sysv/linux/tst-clone.c          | 73 ++++++++++++++++----
 3 files changed, 63 insertions(+), 12 deletions(-)

On Thu, Feb 22, 2024 at 03:03:27PM +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.
> Furthermore the existing test misc/tst-clone is extended to check
> all error cases and that clone does not clobber registers in this
> error case.

LGTM, but given my almost complete absence from glibc development
during the last 14 years I think you want some active reviewer...

	Jakub

On 22/02/24 11:03, 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.
> Furthermore the existing test misc/tst-clone is extended to check
> all error cases and that clone does not clobber registers in this
> error case.

LGTM, thanks.

> ---
>  sysdeps/unix/sysv/linux/s390/s390-32/clone.S |  1 +
>  sysdeps/unix/sysv/linux/s390/s390-64/clone.S |  1 +
>  sysdeps/unix/sysv/linux/tst-clone.c          | 73 ++++++++++++++++----
>  3 files changed, 63 insertions(+), 12 deletions(-)
> 
> 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)
>  
> diff --git a/sysdeps/unix/sysv/linux/tst-clone.c b/sysdeps/unix/sysv/linux/tst-clone.c
> index 470676ab2b..060fdf5c66 100644
> --- a/sysdeps/unix/sysv/linux/tst-clone.c
> +++ b/sysdeps/unix/sysv/linux/tst-clone.c
> @@ -16,12 +16,16 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -/* BZ #2386 */
> +/* BZ #2386, BZ #31402 */
>  #include <errno.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <sched.h>
> +#include <stackinfo.h>  /* For _STACK_GROWS_{UP,DOWN}.  */
> +#include <support/check.h>
> +
> +volatile unsigned v = 0xdeadbeef;
>  
>  int child_fn(void *arg)
>  {
> @@ -30,22 +34,67 @@ int child_fn(void *arg)
>  }
>  
>  static int
> -do_test (void)
> +__attribute__((noinline))
> +do_clone (int (*fn)(void *_Nullable), void *stack)

Not sure why you need _Nullable here, the rest looks ok.

>  {
>    int result;
> +  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;
> +
> +  result = clone (fn, stack, 0, NULL);
> +
> +  /* Check that clone does not clobber call-saved registers.  */
> +  TEST_VERIFY (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);
> +
> +  return result;
> +}
> +
> +static void
> +__attribute__((noinline))
> +do_test_single (int (*fn)(void *_Nullable), void *stack)
> +{
> +  printf ("%s (fn=%p, stack=%p)\n", __FUNCTION__, fn, stack);
> +  errno = 0;
> +
> +  int result = do_clone (fn, stack);
> +
> +  TEST_COMPARE (errno, EINVAL);
> +  TEST_COMPARE (result, -1);
> +}
>  
> -  result = clone (child_fn, NULL, 0, NULL);
> +static int
> +do_test (void)
> +{
> +  char st[128 * 1024] __attribute__ ((aligned));
> +  void *stack = NULL;
> +#if _STACK_GROWS_DOWN
> +  stack = st + sizeof (st);
> +#elif _STACK_GROWS_UP
> +  stack = st;
> +#else
> +# error "Define either _STACK_GROWS_DOWN or _STACK_GROWS_UP"
> +#endif
>  
> -  if (errno != EINVAL || result != -1)
> -    {
> -      printf ("FAIL: clone()=%d (wanted -1) errno=%d (wanted %d)\n",
> -              result, errno, EINVAL);
> -      return 1;
> -    }
> +  do_test_single (child_fn, NULL);
> +  do_test_single (NULL, stack);
> +  do_test_single (NULL, NULL);
>  
> -  puts ("All OK");
>    return 0;
>  }
>  
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> +#include <support/test-driver.c>

On 23.02.24 17:36, Adhemerval Zanella Netto wrote:
> 
> 
> On 22/02/24 11:03, 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.
>> Furthermore the existing test misc/tst-clone is extended to check
>> all error cases and that clone does not clobber registers in this
>> error case.
> 
> LGTM, thanks.
> 
Thanks for reviewing. I've just committed it without _Nullable.
>> ---
>>  sysdeps/unix/sysv/linux/s390/s390-32/clone.S |  1 +
>>  sysdeps/unix/sysv/linux/s390/s390-64/clone.S |  1 +
>>  sysdeps/unix/sysv/linux/tst-clone.c          | 73 ++++++++++++++++----
>>  3 files changed, 63 insertions(+), 12 deletions(-)
>>
>> 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)
>>  
>> diff --git a/sysdeps/unix/sysv/linux/tst-clone.c b/sysdeps/unix/sysv/linux/tst-clone.c
>> index 470676ab2b..060fdf5c66 100644
>> --- a/sysdeps/unix/sysv/linux/tst-clone.c
>> +++ b/sysdeps/unix/sysv/linux/tst-clone.c
>> @@ -16,12 +16,16 @@
>>     License along with the GNU C Library; if not, see
>>     <https://www.gnu.org/licenses/>.  */
>>  
>> -/* BZ #2386 */
>> +/* BZ #2386, BZ #31402 */
>>  #include <errno.h>
>>  #include <stdio.h>
>>  #include <stdlib.h>
>>  #include <unistd.h>
>>  #include <sched.h>
>> +#include <stackinfo.h>  /* For _STACK_GROWS_{UP,DOWN}.  */
>> +#include <support/check.h>
>> +
>> +volatile unsigned v = 0xdeadbeef;
>>  
>>  int child_fn(void *arg)
>>  {
>> @@ -30,22 +34,67 @@ int child_fn(void *arg)
>>  }
>>  
>>  static int
>> -do_test (void)
>> +__attribute__((noinline))
>> +do_clone (int (*fn)(void *_Nullable), void *stack)
> 
> Not sure why you need _Nullable here, the rest looks ok.
Yes, you are right. It is not needed. I've removed it.
> 
>>  {
>>    int result;
>> +  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;
>> +
>> +  result = clone (fn, stack, 0, NULL);
>> +
>> +  /* Check that clone does not clobber call-saved registers.  */
>> +  TEST_VERIFY (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);
>> +
>> +  return result;
>> +}
>> +
>> +static void
>> +__attribute__((noinline))
>> +do_test_single (int (*fn)(void *_Nullable), void *stack)
>> +{
>> +  printf ("%s (fn=%p, stack=%p)\n", __FUNCTION__, fn, stack);
>> +  errno = 0;
>> +
>> +  int result = do_clone (fn, stack);
>> +
>> +  TEST_COMPARE (errno, EINVAL);
>> +  TEST_COMPARE (result, -1);
>> +}
>>  
>> -  result = clone (child_fn, NULL, 0, NULL);
>> +static int
>> +do_test (void)
>> +{
>> +  char st[128 * 1024] __attribute__ ((aligned));
>> +  void *stack = NULL;
>> +#if _STACK_GROWS_DOWN
>> +  stack = st + sizeof (st);
>> +#elif _STACK_GROWS_UP
>> +  stack = st;
>> +#else
>> +# error "Define either _STACK_GROWS_DOWN or _STACK_GROWS_UP"
>> +#endif
>>  
>> -  if (errno != EINVAL || result != -1)
>> -    {
>> -      printf ("FAIL: clone()=%d (wanted -1) errno=%d (wanted %d)\n",
>> -              result, errno, EINVAL);
>> -      return 1;
>> -    }
>> +  do_test_single (child_fn, NULL);
>> +  do_test_single (NULL, stack);
>> +  do_test_single (NULL, NULL);
>>  
>> -  puts ("All OK");
>>    return 0;
>>  }
>>  
>> -#define TEST_FUNCTION do_test ()
>> -#include "../test-skeleton.c"
>> +#include <support/test-driver.c>