[v2] 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
|
linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Testing passed
|
redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
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.
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(-)
Comments
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>
@@ -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)
@@ -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)
{
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>