Hi,
These 2 patch fixes the failure in clone test with Gcc toolchain. They need the recent v4 version of Gcc support patches posted earlier [1]. The full patch series can be found here [2].
[1]: git@git.morello-project.org:amitdaniel/linux.git gcc_kselftests_support_v4 [2]: git@git.morello-project.org:amitdaniel/linux.git gcc_kselftests_clone_fixes_v1
Thanks, Amit Daniel
Amit Daniel Kachhap (2): kselftests/arm64: morello: Fix restricted mode tests with Gcc kselftests/arm64: morello: clone: Remove loop check
tools/testing/selftests/arm64/morello/clone.c | 19 +++++++------------ .../selftests/arm64/morello/freestanding.h | 8 ++++++++ .../arm64/morello/freestanding_start.S | 18 ++++++++++++++++++ 3 files changed, 33 insertions(+), 12 deletions(-)
Some of the Gcc compiled clone tests in restricted mode face stack corruption. This occurs as the dynamic relocations by Gcc do not get input from compiler about clearing the executive mode in function pointers. On the other hand, Clang provides input about this permission in the structure cap_reloc (may be with hint from instruction blrr) to the capability relocation code.
As a workaround, clear this permission bit explicitly if required before branching to this function pointer. Also, keep any other function called from this function as inline.
Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com --- tools/testing/selftests/arm64/morello/clone.c | 3 ++- .../arm64/morello/freestanding_start.S | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index 69189287144a..67468cb524bc 100644 --- a/tools/testing/selftests/arm64/morello/clone.c +++ b/tools/testing/selftests/arm64/morello/clone.c @@ -44,7 +44,8 @@ struct test_fixture {
#define PROBE_INTERVAL (1 << 12)
-static void probe_addr_range(uintcap_t start_addr, size_t size, int interval) +static inline __attribute__((always_inline)) +void probe_addr_range(uintcap_t start_addr, size_t size, int interval) { size_t i;
diff --git a/tools/testing/selftests/arm64/morello/freestanding_start.S b/tools/testing/selftests/arm64/morello/freestanding_start.S index 92260fdc384d..365a8d16606a 100644 --- a/tools/testing/selftests/arm64/morello/freestanding_start.S +++ b/tools/testing/selftests/arm64/morello/freestanding_start.S @@ -199,6 +199,24 @@ FUNCTION_START(__clone) 1: mov c0, c10 mov c1, c9 + + /* + * Function pointers executed in restrictive mode have the executive + * mode permission cleared off by the Clang capability relocation code. + * However, dynamic relocations by GCC do not manage this, so clear this + * permission bit explicitly if required. + */ + adr c2, #0 + gcperm x3, c2 + tbnz x3, #__ARM_CAP_PERMISSION_EXECUTIVE__, 2f + mov x4, #__ARM_CAP_PERMISSION_EXECUTIVE__ + clrperm c1, c1, x4 + /* + * Function pointers are sealed so clrperm untagged c1 - rebuild a valid + * capability from PCC + */ + build c1, c1, c2 +2: blr c1 mov x8, #__NR_exit svc #0
Hi Amit,
On Fri, Mar 03, 2023 at 06:34:41PM +0530, Amit Daniel Kachhap wrote:
Some of the Gcc compiled clone tests in restricted mode face stack corruption. This occurs as the dynamic relocations by Gcc do not get input from compiler about clearing the executive mode in function pointers. On the other hand, Clang provides input about this permission in the structure cap_reloc (may be with hint from instruction blrr) to the capability relocation code.
So I must admit I've been struggling a bit to connect the dots here. I'm not entirely sure how relocations would deal with executive vs restricted: two entries for the same symbol with different permissions ? Furthermore, the symbol at hand, in the case of clang is not actually being relocated. The branch capability is being build based on PCC, which for restricted renders proper capability permissions (as PCC will already have the executive bit cleared). Now with GCC, the symbol is relocated with executive permission which means the branch instruction will switch from restrictive back to executive which is were the problem comes from: that switch will also switch the SP (from RCSP to CSP) and as that one is not the one expected, it will result in number of issues. Note that the CSP will point to the stack from before switching to restricted. And that is the main problem here. Your change, even though it is actually fixing the mess we end up with GCC, it brings more generic question on how to actually handle restricted. Note that any branch to relocated symbol, without proper handling of related registers or permissions will blow up. I guess though, that's the best we can do at this point.
--- BR B.
As a workaround, clear this permission bit explicitly if required before branching to this function pointer. Also, keep any other function called from this function as inline.
Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com
tools/testing/selftests/arm64/morello/clone.c | 3 ++- .../arm64/morello/freestanding_start.S | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index 69189287144a..67468cb524bc 100644 --- a/tools/testing/selftests/arm64/morello/clone.c +++ b/tools/testing/selftests/arm64/morello/clone.c @@ -44,7 +44,8 @@ struct test_fixture { #define PROBE_INTERVAL (1 << 12) -static void probe_addr_range(uintcap_t start_addr, size_t size, int interval) +static inline __attribute__((always_inline)) +void probe_addr_range(uintcap_t start_addr, size_t size, int interval) { size_t i; diff --git a/tools/testing/selftests/arm64/morello/freestanding_start.S b/tools/testing/selftests/arm64/morello/freestanding_start.S index 92260fdc384d..365a8d16606a 100644 --- a/tools/testing/selftests/arm64/morello/freestanding_start.S +++ b/tools/testing/selftests/arm64/morello/freestanding_start.S @@ -199,6 +199,24 @@ FUNCTION_START(__clone) 1: mov c0, c10 mov c1, c9
- /*
* Function pointers executed in restrictive mode have the executive* mode permission cleared off by the Clang capability relocation code.* However, dynamic relocations by GCC do not manage this, so clear this* permission bit explicitly if required.*/- adr c2, #0
- gcperm x3, c2
- tbnz x3, #__ARM_CAP_PERMISSION_EXECUTIVE__, 2f
- mov x4, #__ARM_CAP_PERMISSION_EXECUTIVE__
- clrperm c1, c1, x4
- /*
* Function pointers are sealed so clrperm untagged c1 - rebuild a valid* capability from PCC*/- build c1, c1, c2
+2: blr c1 mov x8, #__NR_exit svc #0 -- 2.25.1
On 3/8/23 14:45, Beata Michalska wrote:
Hi Amit,
On Fri, Mar 03, 2023 at 06:34:41PM +0530, Amit Daniel Kachhap wrote:
Some of the Gcc compiled clone tests in restricted mode face stack corruption. This occurs as the dynamic relocations by Gcc do not get input from compiler about clearing the executive mode in function pointers. On the other hand, Clang provides input about this permission in the structure cap_reloc (may be with hint from instruction blrr) to the capability relocation code.
So I must admit I've been struggling a bit to connect the dots here. I'm not entirely sure how relocations would deal with executive vs restricted: two entries for the same symbol with different permissions ? Furthermore, the symbol at hand, in the case of clang is not actually being relocated. The branch capability is being build based on PCC, which for restricted renders proper capability permissions (as PCC will already have the executive bit cleared). Now with GCC, the symbol is relocated with executive permission which means the branch instruction will switch from restrictive back to executive which is were the problem comes from: that switch will also switch the SP (from RCSP to CSP) and as that one is not the one expected, it will result in number of issues. Note that the CSP will point to the stack from before switching to restricted. And that is the main problem here. Your change, even though it is actually fixing the mess we end up with GCC, it brings more generic question on how to actually handle restricted. Note that any branch to relocated symbol, without proper handling of related registers or permissions will blow up. I guess though, that's the best we can do at this point.
Thanks for the detailed analysis. I will update the commit logs to describe the actual issue here.
On the side note, I think mixing executive and restricted mode in the same binary is problematic. Either run the binary completely in executive or restrictive.
Amit
BR B.
As a workaround, clear this permission bit explicitly if required before branching to this function pointer. Also, keep any other function called from this function as inline.
Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com
tools/testing/selftests/arm64/morello/clone.c | 3 ++- .../arm64/morello/freestanding_start.S | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index 69189287144a..67468cb524bc 100644 --- a/tools/testing/selftests/arm64/morello/clone.c +++ b/tools/testing/selftests/arm64/morello/clone.c @@ -44,7 +44,8 @@ struct test_fixture { #define PROBE_INTERVAL (1 << 12) -static void probe_addr_range(uintcap_t start_addr, size_t size, int interval) +static inline __attribute__((always_inline)) +void probe_addr_range(uintcap_t start_addr, size_t size, int interval) { size_t i; diff --git a/tools/testing/selftests/arm64/morello/freestanding_start.S b/tools/testing/selftests/arm64/morello/freestanding_start.S index 92260fdc384d..365a8d16606a 100644 --- a/tools/testing/selftests/arm64/morello/freestanding_start.S +++ b/tools/testing/selftests/arm64/morello/freestanding_start.S @@ -199,6 +199,24 @@ FUNCTION_START(__clone) 1: mov c0, c10 mov c1, c9
- /*
* Function pointers executed in restrictive mode have the executive* mode permission cleared off by the Clang capability relocation code.* However, dynamic relocations by GCC do not manage this, so clear this* permission bit explicitly if required.*/- adr c2, #0
- gcperm x3, c2
- tbnz x3, #__ARM_CAP_PERMISSION_EXECUTIVE__, 2f
- mov x4, #__ARM_CAP_PERMISSION_EXECUTIVE__
- clrperm c1, c1, x4
- /*
* Function pointers are sealed so clrperm untagged c1 - rebuild a valid* capability from PCC*/- build c1, c1, c2
+2: blr c1 mov x8, #__NR_exit svc #0 -- 2.25.1
On 08/03/2023 10:15, Beata Michalska wrote:
Hi Amit,
On Fri, Mar 03, 2023 at 06:34:41PM +0530, Amit Daniel Kachhap wrote:
Some of the Gcc compiled clone tests in restricted mode face stack corruption. This occurs as the dynamic relocations by Gcc do not get input from compiler about clearing the executive mode in function pointers. On the other hand, Clang provides input about this permission in the structure cap_reloc (may be with hint from instruction blrr) to the capability relocation code.
So I must admit I've been struggling a bit to connect the dots here. I'm not entirely sure how relocations would deal with executive vs restricted: two entries for the same symbol with different permissions ? Furthermore, the symbol at hand, in the case of clang is not actually being relocated. The branch capability is being build based on PCC, which for restricted renders proper capability permissions (as PCC will already have the executive bit cleared). Now with GCC, the symbol is relocated with executive permission which means the branch instruction will switch from restrictive back to executive which is were the problem comes from: that switch will also switch the SP (from RCSP to CSP) and as that one is not the one expected, it will result in number of issues. Note that the CSP will point to the stack from before switching to restricted. And that is the main problem here.
This is also my understanding. The commit message and comment should be updated accordingly.
Your change, even though it is actually fixing the mess we end up with GCC, it brings more generic question on how to actually handle restricted. Note that any branch to relocated symbol, without proper handling of related registers or permissions will blow up.
Agreed, this is pretty fragile and we just got lucky so far. That said I don't think we can make the test fundamentally more robust without changing the approach completely, so we'll have to live with workarounds.
I guess though, that's the best we can do at this point.
This approach is one option, though I think we can avoiding adding extra logic to __clone(), which IMHO should be as thin a wrapper as possible.
An alternative is essentially to materialise the function pointer ourselves, instead of relying on on Clang to do it for us, something like:
ptr = cheri_address_set(cheri_pcc_get(), (ptraddr_t)clone_base_fn); ptr = cheri_sentry_create(ptr);
This avoids adding logic in assembly, keeps __clone() untouched and doesn't rely on any particular behaviour from the compiler. It makes the assumption that the bounds and permissions of clone_base_fn are the same as PCC, which is not an issue in practice (being in the same translation unit).
BR B.
As a workaround, clear this permission bit explicitly if required before branching to this function pointer. Also, keep any other function called from this function as inline.
I don't think that's necessary. Normal function calls are not capability branches, just a BL with an offset. We only have a problem when a full function pointer needs to be materialised.
Kevin
Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com
tools/testing/selftests/arm64/morello/clone.c | 3 ++- .../arm64/morello/freestanding_start.S | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index 69189287144a..67468cb524bc 100644 --- a/tools/testing/selftests/arm64/morello/clone.c +++ b/tools/testing/selftests/arm64/morello/clone.c @@ -44,7 +44,8 @@ struct test_fixture { #define PROBE_INTERVAL (1 << 12) -static void probe_addr_range(uintcap_t start_addr, size_t size, int interval) +static inline __attribute__((always_inline)) +void probe_addr_range(uintcap_t start_addr, size_t size, int interval) { size_t i; diff --git a/tools/testing/selftests/arm64/morello/freestanding_start.S b/tools/testing/selftests/arm64/morello/freestanding_start.S index 92260fdc384d..365a8d16606a 100644 --- a/tools/testing/selftests/arm64/morello/freestanding_start.S +++ b/tools/testing/selftests/arm64/morello/freestanding_start.S @@ -199,6 +199,24 @@ FUNCTION_START(__clone) 1: mov c0, c10 mov c1, c9
- /*
* Function pointers executed in restrictive mode have the executive* mode permission cleared off by the Clang capability relocation code.* However, dynamic relocations by GCC do not manage this, so clear this* permission bit explicitly if required.*/- adr c2, #0
- gcperm x3, c2
- tbnz x3, #__ARM_CAP_PERMISSION_EXECUTIVE__, 2f
- mov x4, #__ARM_CAP_PERMISSION_EXECUTIVE__
- clrperm c1, c1, x4
- /*
* Function pointers are sealed so clrperm untagged c1 - rebuild a valid* capability from PCC*/- build c1, c1, c2
+2: blr c1 mov x8, #__NR_exit svc #0 -- 2.25.1
On Wed, Mar 15, 2023 at 10:30:26AM +0100, Kevin Brodsky wrote:
On 08/03/2023 10:15, Beata Michalska wrote:
Hi Amit,
On Fri, Mar 03, 2023 at 06:34:41PM +0530, Amit Daniel Kachhap wrote:
Some of the Gcc compiled clone tests in restricted mode face stack corruption. This occurs as the dynamic relocations by Gcc do not get input from compiler about clearing the executive mode in function pointers. On the other hand, Clang provides input about this permission in the structure cap_reloc (may be with hint from instruction blrr) to the capability relocation code.
So I must admit I've been struggling a bit to connect the dots here. I'm not entirely sure how relocations would deal with executive vs restricted: two entries for the same symbol with different permissions ? Furthermore, the symbol at hand, in the case of clang is not actually being relocated. The branch capability is being build based on PCC, which for restricted renders proper capability permissions (as PCC will already have the executive bit cleared). Now with GCC, the symbol is relocated with executive permission which means the branch instruction will switch from restrictive back to executive which is were the problem comes from: that switch will also switch the SP (from RCSP to CSP) and as that one is not the one expected, it will result in number of issues. Note that the CSP will point to the stack from before switching to restricted. And that is the main problem here.
This is also my understanding. The commit message and comment should be updated accordingly.
Your change, even though it is actually fixing the mess we end up with GCC, it brings more generic question on how to actually handle restricted. Note that any branch to relocated symbol, without proper handling of related registers or permissions will blow up.
Agreed, this is pretty fragile and we just got lucky so far. That said I don't think we can make the test fundamentally more robust without changing the approach completely, so we'll have to live with workarounds.
I guess though, that's the best we can do at this point.
This approach is one option, though I think we can avoiding adding extra logic to __clone(), which IMHO should be as thin a wrapper as possible.
An alternative is essentially to materialise the function pointer ourselves, instead of relying on on Clang to do it for us, something like:
ptr = cheri_address_set(cheri_pcc_get(), (ptraddr_t)clone_base_fn); ptr = cheri_sentry_create(ptr);
This avoids adding logic in assembly, keeps __clone() untouched and doesn't rely on any particular behaviour from the compiler. It makes the assumption that the bounds and permissions of clone_base_fn are the same as PCC, which is not an issue in practice (being in the same translation unit).
+1 for this one.
--- BR B.
BR B.
As a workaround, clear this permission bit explicitly if required before branching to this function pointer. Also, keep any other function called from this function as inline.
I don't think that's necessary. Normal function calls are not capability branches, just a BL with an offset. We only have a problem when a full function pointer needs to be materialised.
Kevin
Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com
tools/testing/selftests/arm64/morello/clone.c | 3 ++- .../arm64/morello/freestanding_start.S | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index 69189287144a..67468cb524bc 100644 --- a/tools/testing/selftests/arm64/morello/clone.c +++ b/tools/testing/selftests/arm64/morello/clone.c @@ -44,7 +44,8 @@ struct test_fixture { #define PROBE_INTERVAL (1 << 12) -static void probe_addr_range(uintcap_t start_addr, size_t size, int interval) +static inline __attribute__((always_inline)) +void probe_addr_range(uintcap_t start_addr, size_t size, int interval) { size_t i; diff --git a/tools/testing/selftests/arm64/morello/freestanding_start.S b/tools/testing/selftests/arm64/morello/freestanding_start.S index 92260fdc384d..365a8d16606a 100644 --- a/tools/testing/selftests/arm64/morello/freestanding_start.S +++ b/tools/testing/selftests/arm64/morello/freestanding_start.S @@ -199,6 +199,24 @@ FUNCTION_START(__clone) 1: mov c0, c10 mov c1, c9
- /*
* Function pointers executed in restrictive mode have the executive* mode permission cleared off by the Clang capability relocation code.* However, dynamic relocations by GCC do not manage this, so clear this* permission bit explicitly if required.*/- adr c2, #0
- gcperm x3, c2
- tbnz x3, #__ARM_CAP_PERMISSION_EXECUTIVE__, 2f
- mov x4, #__ARM_CAP_PERMISSION_EXECUTIVE__
- clrperm c1, c1, x4
- /*
* Function pointers are sealed so clrperm untagged c1 - rebuild a valid* capability from PCC*/- build c1, c1, c2
+2: blr c1 mov x8, #__NR_exit svc #0 -- 2.25.1
Some of the clone tests use loop check on status flag to determine if the child exited normally. However, this may cause infinite looping if the child crashes due to any error scenario.
Replace the busy loop with the lightweight waitpid() syscall (Instead of existing waitid()) to wait for the child to exit normally.
Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com --- tools/testing/selftests/arm64/morello/clone.c | 16 +++++----------- .../selftests/arm64/morello/freestanding.h | 8 ++++++++ 2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index 67468cb524bc..045d2cca4ad2 100644 --- a/tools/testing/selftests/arm64/morello/clone.c +++ b/tools/testing/selftests/arm64/morello/clone.c @@ -18,9 +18,6 @@ #define in_restricted() \ (!(cheri_perms_get(cheri_pcc_get()) & ARM_CAP_PERMISSION_EXECUTIVE))
- -#define MAGIC_NR 0x3562 /* whatever really ..... */ - #ifndef MAX_PID_NS_LEVEL #define MAX_PID_NS_LEVEL 32 #endif @@ -36,7 +33,6 @@ #define CLONE_TH_RUSAGE BIT(3)
struct test_fixture { - int status; int flags; void *sp; int result; @@ -90,14 +86,13 @@ static int clone_base_fn(void *data) !!(__data->flags & CLONE_TH_RESTRICTED) != in_restricted() ? CTR_FAILED : CTR_SUCCESS; done: - __data->status = MAGIC_NR; return 0; }
static inline __attribute__((always_inline)) void clone_single(struct test_fixture *data) { - int ppid = 0, cpid = 0; + int ppid = 0, cpid = 0, wstatus; int result = -EINVAL;
void *new_stack = mmap_verified(NULL, STACK_SIZE, PROT_READ | PROT_WRITE, @@ -108,7 +103,7 @@ void clone_single(struct test_fixture *data) CAP_LOAD_PERMS | CAP_STORE_PERMS) : NULL;
- int clone_flags = CLONE_VM | CLONE_PARENT_SETTID | CLONE_CHILD_SETTID; + int clone_flags = CLONE_VM | CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | SIGCHLD;
ASSERT_NE(new_stack, NULL); /* For stack probing .... */ @@ -128,10 +123,9 @@ void clone_single(struct test_fixture *data) goto leave; }
- /* Wait substitute ... */ - while (data->status != MAGIC_NR) { - asm(""); - } + /* Wait for the child */ + waitpid(cpid, &wstatus, 0); + ASSERT_TRUE(WIFEXITED(wstatus)); /* * CLONE_CHILD_SETTID sets child's thread ID to provided child's * memory but as VM is being shared, it's all good at this point. diff --git a/tools/testing/selftests/arm64/morello/freestanding.h b/tools/testing/selftests/arm64/morello/freestanding.h index 19611b315de9..135150701b0d 100644 --- a/tools/testing/selftests/arm64/morello/freestanding.h +++ b/tools/testing/selftests/arm64/morello/freestanding.h @@ -31,6 +31,10 @@ typedef __uintcap_t uintcap_t;
#define EXIT_SUCCESS 0
+#ifndef WIFEXITED +#define WIFEXITED(status) (((status) & 0x7f) == 0) +#endif + struct __test_meta { int message; }; @@ -213,4 +217,8 @@ static inline int waitid(int id_type, pid_t id, siginfo_t *info, int options, st return syscall(__NR_waitid, id_type, id, info, options, ru); }
+static inline int waitpid(pid_t pid, int *wstatus, int options) +{ + return syscall(__NR_wait4, pid, wstatus, options, 0); +} #endif
On Fri, Mar 03, 2023 at 06:34:42PM +0530, Amit Daniel Kachhap wrote:
Some of the clone tests use loop check on status flag to determine if the child exited normally. However, this may cause infinite looping if the child crashes due to any error scenario.
This has been introduced very early when the signal support was still a work in progress, and the infinite looping was, in a way, an indication that smth went wrong on the way. The change though looks sensible and it gives more adequate report on failures.
--- BR B.
Replace the busy loop with the lightweight waitpid() syscall (Instead of existing waitid()) to wait for the child to exit normally.
Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com
tools/testing/selftests/arm64/morello/clone.c | 16 +++++----------- .../selftests/arm64/morello/freestanding.h | 8 ++++++++ 2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index 67468cb524bc..045d2cca4ad2 100644 --- a/tools/testing/selftests/arm64/morello/clone.c +++ b/tools/testing/selftests/arm64/morello/clone.c @@ -18,9 +18,6 @@ #define in_restricted() \ (!(cheri_perms_get(cheri_pcc_get()) & ARM_CAP_PERMISSION_EXECUTIVE))
-#define MAGIC_NR 0x3562 /* whatever really ..... */
#ifndef MAX_PID_NS_LEVEL #define MAX_PID_NS_LEVEL 32 #endif @@ -36,7 +33,6 @@ #define CLONE_TH_RUSAGE BIT(3) struct test_fixture {
- int status; int flags; void *sp; int result;
@@ -90,14 +86,13 @@ static int clone_base_fn(void *data) !!(__data->flags & CLONE_TH_RESTRICTED) != in_restricted() ? CTR_FAILED : CTR_SUCCESS; done:
- __data->status = MAGIC_NR; return 0;
} static inline __attribute__((always_inline)) void clone_single(struct test_fixture *data) {
- int ppid = 0, cpid = 0;
- int ppid = 0, cpid = 0, wstatus; int result = -EINVAL;
void *new_stack = mmap_verified(NULL, STACK_SIZE, PROT_READ | PROT_WRITE, @@ -108,7 +103,7 @@ void clone_single(struct test_fixture *data) CAP_LOAD_PERMS | CAP_STORE_PERMS) : NULL;
- int clone_flags = CLONE_VM | CLONE_PARENT_SETTID | CLONE_CHILD_SETTID;
- int clone_flags = CLONE_VM | CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | SIGCHLD;
ASSERT_NE(new_stack, NULL); /* For stack probing .... */ @@ -128,10 +123,9 @@ void clone_single(struct test_fixture *data) goto leave; }
- /* Wait substitute ... */
- while (data->status != MAGIC_NR) {
asm("");- }
- /* Wait for the child */
- waitpid(cpid, &wstatus, 0);
- ASSERT_TRUE(WIFEXITED(wstatus)); /*
- CLONE_CHILD_SETTID sets child's thread ID to provided child's
- memory but as VM is being shared, it's all good at this point.
diff --git a/tools/testing/selftests/arm64/morello/freestanding.h b/tools/testing/selftests/arm64/morello/freestanding.h index 19611b315de9..135150701b0d 100644 --- a/tools/testing/selftests/arm64/morello/freestanding.h +++ b/tools/testing/selftests/arm64/morello/freestanding.h @@ -31,6 +31,10 @@ typedef __uintcap_t uintcap_t; #define EXIT_SUCCESS 0 +#ifndef WIFEXITED +#define WIFEXITED(status) (((status) & 0x7f) == 0) +#endif
struct __test_meta { int message; }; @@ -213,4 +217,8 @@ static inline int waitid(int id_type, pid_t id, siginfo_t *info, int options, st return syscall(__NR_waitid, id_type, id, info, options, ru); } +static inline int waitpid(pid_t pid, int *wstatus, int options) +{
- return syscall(__NR_wait4, pid, wstatus, options, 0);
+}
#endif
2.25.1
On 03/03/2023 14:04, Amit Daniel Kachhap wrote:
Some of the clone tests use loop check on status flag to determine if the child exited normally. However, this may cause infinite looping if the child crashes due to any error scenario.
We should also say that the compiler is free to optimise out the loop entirely since the flag is not volatile.
Replace the busy loop with the lightweight waitpid() syscall (Instead of existing waitid()) to wait for the child to exit normally.
I don't have anything against using waitid or waitpid, but I would rather we use the same one everywhere, unless there's a specific reason to use one or the other.
Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com
tools/testing/selftests/arm64/morello/clone.c | 16 +++++----------- .../selftests/arm64/morello/freestanding.h | 8 ++++++++ 2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index 67468cb524bc..045d2cca4ad2 100644 --- a/tools/testing/selftests/arm64/morello/clone.c +++ b/tools/testing/selftests/arm64/morello/clone.c @@ -18,9 +18,6 @@ #define in_restricted() \ (!(cheri_perms_get(cheri_pcc_get()) & ARM_CAP_PERMISSION_EXECUTIVE))
-#define MAGIC_NR 0x3562 /* whatever really ..... */
#ifndef MAX_PID_NS_LEVEL #define MAX_PID_NS_LEVEL 32 #endif @@ -36,7 +33,6 @@ #define CLONE_TH_RUSAGE BIT(3) struct test_fixture {
- int status; int flags; void *sp; int result;
@@ -90,14 +86,13 @@ static int clone_base_fn(void *data) !!(__data->flags & CLONE_TH_RESTRICTED) != in_restricted() ? CTR_FAILED : CTR_SUCCESS; done:
We don't really need the label any more so might as well replace the goto with a return too.
Kevin
- __data->status = MAGIC_NR; return 0;
} static inline __attribute__((always_inline)) void clone_single(struct test_fixture *data) {
- int ppid = 0, cpid = 0;
- int ppid = 0, cpid = 0, wstatus; int result = -EINVAL;
void *new_stack = mmap_verified(NULL, STACK_SIZE, PROT_READ | PROT_WRITE, @@ -108,7 +103,7 @@ void clone_single(struct test_fixture *data) CAP_LOAD_PERMS | CAP_STORE_PERMS) : NULL;
- int clone_flags = CLONE_VM | CLONE_PARENT_SETTID | CLONE_CHILD_SETTID;
- int clone_flags = CLONE_VM | CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | SIGCHLD;
ASSERT_NE(new_stack, NULL); /* For stack probing .... */ @@ -128,10 +123,9 @@ void clone_single(struct test_fixture *data) goto leave; }
- /* Wait substitute ... */
- while (data->status != MAGIC_NR) {
asm("");- }
- /* Wait for the child */
- waitpid(cpid, &wstatus, 0);
- ASSERT_TRUE(WIFEXITED(wstatus)); /*
- CLONE_CHILD_SETTID sets child's thread ID to provided child's
- memory but as VM is being shared, it's all good at this point.
diff --git a/tools/testing/selftests/arm64/morello/freestanding.h b/tools/testing/selftests/arm64/morello/freestanding.h index 19611b315de9..135150701b0d 100644 --- a/tools/testing/selftests/arm64/morello/freestanding.h +++ b/tools/testing/selftests/arm64/morello/freestanding.h @@ -31,6 +31,10 @@ typedef __uintcap_t uintcap_t; #define EXIT_SUCCESS 0 +#ifndef WIFEXITED +#define WIFEXITED(status) (((status) & 0x7f) == 0) +#endif
struct __test_meta { int message; }; @@ -213,4 +217,8 @@ static inline int waitid(int id_type, pid_t id, siginfo_t *info, int options, st return syscall(__NR_waitid, id_type, id, info, options, ru); } +static inline int waitpid(pid_t pid, int *wstatus, int options) +{
- return syscall(__NR_wait4, pid, wstatus, options, 0);
+} #endif
Hi,
On 3/15/23 15:00, Kevin Brodsky wrote:
On 03/03/2023 14:04, Amit Daniel Kachhap wrote:
Some of the clone tests use loop check on status flag to determine if the child exited normally. However, this may cause infinite looping if the child crashes due to any error scenario.
We should also say that the compiler is free to optimise out the loop entirely since the flag is not volatile.
Yes makes sense.
Replace the busy loop with the lightweight waitpid() syscall (Instead of existing waitid()) to wait for the child to exit normally.
I don't have anything against using waitid or waitpid, but I would rather we use the same one everywhere, unless there's a specific reason to use one or the other.
I wanted to use waitpid() as waitid() is used for advanced clone tests below and they further makes use of the the parameters siginfo, wait_ru.
As it may also make the tests repetitive so thought of using lightweight wait syscall for these initial clone tests.
Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com
tools/testing/selftests/arm64/morello/clone.c | 16 +++++----------- .../selftests/arm64/morello/freestanding.h | 8 ++++++++ 2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index 67468cb524bc..045d2cca4ad2 100644 --- a/tools/testing/selftests/arm64/morello/clone.c +++ b/tools/testing/selftests/arm64/morello/clone.c @@ -18,9 +18,6 @@ #define in_restricted() \ (!(cheri_perms_get(cheri_pcc_get()) & ARM_CAP_PERMISSION_EXECUTIVE))
-#define MAGIC_NR 0x3562 /* whatever really ..... */
- #ifndef MAX_PID_NS_LEVEL #define MAX_PID_NS_LEVEL 32 #endif
@@ -36,7 +33,6 @@ #define CLONE_TH_RUSAGE BIT(3) struct test_fixture {
- int status; int flags; void *sp; int result;
@@ -90,14 +86,13 @@ static int clone_base_fn(void *data) !!(__data->flags & CLONE_TH_RESTRICTED) != in_restricted() ? CTR_FAILED : CTR_SUCCESS; done:
We don't really need the label any more so might as well replace the goto with a return too.
Good catch.
Amit
Kevin
- __data->status = MAGIC_NR; return 0; }
static inline __attribute__((always_inline)) void clone_single(struct test_fixture *data) {
- int ppid = 0, cpid = 0;
- int ppid = 0, cpid = 0, wstatus; int result = -EINVAL;
void *new_stack = mmap_verified(NULL, STACK_SIZE, PROT_READ | PROT_WRITE, @@ -108,7 +103,7 @@ void clone_single(struct test_fixture *data) CAP_LOAD_PERMS | CAP_STORE_PERMS) : NULL;
- int clone_flags = CLONE_VM | CLONE_PARENT_SETTID | CLONE_CHILD_SETTID;
- int clone_flags = CLONE_VM | CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | SIGCHLD;
ASSERT_NE(new_stack, NULL); /* For stack probing .... */ @@ -128,10 +123,9 @@ void clone_single(struct test_fixture *data) goto leave; }
- /* Wait substitute ... */
- while (data->status != MAGIC_NR) {
asm("");- }
- /* Wait for the child */
- waitpid(cpid, &wstatus, 0);
- ASSERT_TRUE(WIFEXITED(wstatus)); /*
- CLONE_CHILD_SETTID sets child's thread ID to provided child's
- memory but as VM is being shared, it's all good at this point.
diff --git a/tools/testing/selftests/arm64/morello/freestanding.h b/tools/testing/selftests/arm64/morello/freestanding.h index 19611b315de9..135150701b0d 100644 --- a/tools/testing/selftests/arm64/morello/freestanding.h +++ b/tools/testing/selftests/arm64/morello/freestanding.h @@ -31,6 +31,10 @@ typedef __uintcap_t uintcap_t; #define EXIT_SUCCESS 0 +#ifndef WIFEXITED +#define WIFEXITED(status) (((status) & 0x7f) == 0) +#endif
- struct __test_meta { int message; };
@@ -213,4 +217,8 @@ static inline int waitid(int id_type, pid_t id, siginfo_t *info, int options, st return syscall(__NR_waitid, id_type, id, info, options, ru); } +static inline int waitpid(pid_t pid, int *wstatus, int options) +{
- return syscall(__NR_wait4, pid, wstatus, options, 0);
+} #endif
On 17/03/2023 10:57, Amit Daniel Kachhap wrote:
Replace the busy loop with the lightweight waitpid() syscall (Instead of existing waitid()) to wait for the child to exit normally.
I don't have anything against using waitid or waitpid, but I would rather we use the same one everywhere, unless there's a specific reason to use one or the other.
I wanted to use waitpid() as waitid() is used for advanced clone tests below and they further makes use of the the parameters siginfo, wait_ru.
As it may also make the tests repetitive so thought of using lightweight wait syscall for these initial clone tests.
Sure, I understand the rationale for using the simpler waitpid() when we just want to wait for a process to exit.
My issue is rather that our use of waitid() becomes inconsistent. The tests in signal.c could use waitpid() in the same way, and arguably run_clone3() too when waiting for the child to exit. None of these actually use either wait_si or wait_ru. Maybe we could have a separate patch that introduces waitpid() in freestanding.h and converts these calls to using waitpid(), and then this patch that replaces the busy loop with (the now already available) waitpid()?
Kevin
On 3/17/23 17:17, Kevin Brodsky wrote:
On 17/03/2023 10:57, Amit Daniel Kachhap wrote:
Replace the busy loop with the lightweight waitpid() syscall (Instead of existing waitid()) to wait for the child to exit normally.
I don't have anything against using waitid or waitpid, but I would rather we use the same one everywhere, unless there's a specific reason to use one or the other.
I wanted to use waitpid() as waitid() is used for advanced clone tests below and they further makes use of the the parameters siginfo, wait_ru.
As it may also make the tests repetitive so thought of using lightweight wait syscall for these initial clone tests.
Sure, I understand the rationale for using the simpler waitpid() when we just want to wait for a process to exit.
My issue is rather that our use of waitid() becomes inconsistent. The tests in signal.c could use waitpid() in the same way, and arguably run_clone3() too when waiting for the child to exit. None of these actually use either wait_si or wait_ru. Maybe we could have a separate patch that introduces waitpid() in freestanding.h and converts these calls to using waitpid(), and then this patch that replaces the busy loop with (the now already available) waitpid()?
This seems to be a nice cleanup. I am incorporating them in v3 version.
Amit
Kevin
linux-morello@op-lists.linaro.org
-
Amit Daniel Kachhap -
Beata Michalska -
Kevin Brodsky