Hi,
These 3 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].
Changes in v3: * Patch 1 updated for for appropriate variable name. * Patch 2 newly added to add and use waitpid() in several places. * Patch 3 updated for cleanups. * Commit log updated.
[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_v3
Thanks, Amit Daniel
Amit Daniel Kachhap (2):
Amit Daniel Kachhap (3): kselftests/arm64: morello: Fix restricted mode tests with Gcc kselftests/arm64: morello: Add a waitpid() syscall kselftests/arm64: morello: clone: Remove loop check
tools/testing/selftests/arm64/morello/clone.c | 42 +++++++++++-------- .../selftests/arm64/morello/freestanding.h | 5 +++ .../testing/selftests/arm64/morello/signal.c | 16 +++---- 3 files changed, 38 insertions(+), 25 deletions(-)
Some of the Gcc compiled clone tests in restricted mode face stack corruption. This occurs as the run-time permission settings (executive or restricted mode) in the PCC may be lost if function pointers are materialised from the GOT entries. This further leads to selecting an incorrect stack pointer(CSP instead of RCSP) and hence the crash.
As a workaround, materialise the function pointer clone_base_fn explicitly by building branch address from the PCC. Here it is assumed that the bounds and permissions of clone_base_fn are same as the PCC , which is not an issue in practice (being in the same translation unit).
Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com --- tools/testing/selftests/arm64/morello/clone.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index 69189287144a..a511054f90fa 100644 --- a/tools/testing/selftests/arm64/morello/clone.c +++ b/tools/testing/selftests/arm64/morello/clone.c @@ -109,6 +109,21 @@ void clone_single(struct test_fixture *data)
int clone_flags = CLONE_VM | CLONE_PARENT_SETTID | CLONE_CHILD_SETTID;
+ int (*clone_fn_ptr)(void *) = clone_base_fn; + + /* + * Function pointers are materialised by the compiler by either + * computing from the PCC (ADRP + ADD + SEAL) or loading from GOT. The + * run-time permission settings (executive or restrictive) in PCC may + * not match with the GOT entries. Hence, materialise the function + * pointers explicitly to avoid this mismatch issue. + */ + if (in_restricted() && + (cheri_perms_get(clone_fn_ptr) & ARM_CAP_PERMISSION_EXECUTIVE)) { + clone_fn_ptr = cheri_address_set(cheri_pcc_get(), (ptraddr_t)clone_fn_ptr); + clone_fn_ptr = cheri_sentry_create(clone_fn_ptr); + } + ASSERT_NE(new_stack, NULL); /* For stack probing .... */ data->sp = new_stack + STACK_SIZE; @@ -119,7 +134,7 @@ void clone_single(struct test_fixture *data)
EXPECT_TRUE(!(data->flags & CLONE_TH_RESTRICTED) || in_restricted());
- result = __clone(clone_base_fn, (uintcap_t)new_stack + STACK_SIZE, + result = __clone(clone_fn_ptr, (uintcap_t)new_stack + STACK_SIZE, clone_flags, data, &ppid, tls, &cpid);
EXPECT_GT(result, 0) {
In many instances waitid() syscall is used only to wait for the child process to exit and syscall out parameters struct siginfo and rusage are not used.
Introduce and utilize the lightweight waitpid() syscall which suit best in such instances.
Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com --- tools/testing/selftests/arm64/morello/clone.c | 6 +++--- .../selftests/arm64/morello/freestanding.h | 5 +++++ tools/testing/selftests/arm64/morello/signal.c | 16 ++++++++-------- 3 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index a511054f90fa..0381702ab0fc 100644 --- a/tools/testing/selftests/arm64/morello/clone.c +++ b/tools/testing/selftests/arm64/morello/clone.c @@ -398,7 +398,7 @@ void run_clone3(struct clone3_fixture *data) struct clone_args *args = &(data->args); int pidfd, parent_tid = 0, child_tid = 0; siginfo_t wait_si; - int result; + int result, wstatus; pid_t pid; void *tls = NULL;
@@ -461,8 +461,8 @@ void run_clone3(struct clone3_fixture *data) ASSERT_EQ(result, 0); }
- result = waitid(P_PID, pid, &wait_si, WEXITED, NULL); - ASSERT_EQ(result, 0); + result = waitpid(pid, &wstatus, 0); + ASSERT_EQ(result, pid);
/* child_tid set once the thread gets scheduled */ if (args->flags & CLONE_PARENT_SETTID && args->flags & CLONE_CHILD_SETTID diff --git a/tools/testing/selftests/arm64/morello/freestanding.h b/tools/testing/selftests/arm64/morello/freestanding.h index 19611b315de9..0b0de05d46bc 100644 --- a/tools/testing/selftests/arm64/morello/freestanding.h +++ b/tools/testing/selftests/arm64/morello/freestanding.h @@ -213,4 +213,9 @@ 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 diff --git a/tools/testing/selftests/arm64/morello/signal.c b/tools/testing/selftests/arm64/morello/signal.c index 3dcea434275c..3108755d9ed7 100644 --- a/tools/testing/selftests/arm64/morello/signal.c +++ b/tools/testing/selftests/arm64/morello/signal.c @@ -302,10 +302,10 @@ TEST(test_rt_sigqueueinfo)
TEST(test_rt_tgsigqueueinfo) { - siginfo_t si, wait_si; + siginfo_t si; pid_t cpid; struct sigaction sa; - int ret; + int ret, wstatus;
setup_sigusr1_handler(&sa, SIG_UNBLOCK);
@@ -324,17 +324,17 @@ TEST(test_rt_tgsigqueueinfo) ASSERT_EQ(ret, 0) { __TH_LOG_ERROR("rt_tgsigqueueinfo syscall failed"); } - ret = waitid(P_PID, cpid, &wait_si, WEXITED, NULL); - ASSERT_EQ(ret, 0) { + ret = waitpid(cpid, &wstatus, 0); + ASSERT_EQ(ret, cpid) { __TH_LOG_ERROR("test_rt_tgsigqueueinfo: Failed on wait"); } }
TEST(test_pidfd_send_signal) { - siginfo_t si, wait_si; + siginfo_t si; pid_t cpid; - int pidfd, ret; + int pidfd, ret, wstatus; struct sigaction sa;
setup_sigusr1_handler(&sa, SIG_UNBLOCK); @@ -364,8 +364,8 @@ TEST(test_pidfd_send_signal) ASSERT_EQ(ret, 0) { __TH_LOG_ERROR("pidfd_send_signal syscall failed"); } - ret = waitid(P_PID, cpid, &wait_si, WEXITED, NULL); - ASSERT_EQ(ret, 0) { + ret = waitpid(cpid, &wstatus, 0); + ASSERT_EQ(ret, cpid) { __TH_LOG_ERROR("test_pidfd_send_signal: Failed on wait"); } close(pidfd);
On 20/03/2023 11:12, Amit Daniel Kachhap wrote:
In many instances waitid() syscall is used only to wait for the child process to exit and syscall out parameters struct siginfo and rusage are not used.
Introduce and utilize the lightweight waitpid() syscall which suit best
s/suit/suits/
in such instances.
Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com
tools/testing/selftests/arm64/morello/clone.c | 6 +++--- .../selftests/arm64/morello/freestanding.h | 5 +++++ tools/testing/selftests/arm64/morello/signal.c | 16 ++++++++-------- 3 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index a511054f90fa..0381702ab0fc 100644 --- a/tools/testing/selftests/arm64/morello/clone.c +++ b/tools/testing/selftests/arm64/morello/clone.c @@ -398,7 +398,7 @@ void run_clone3(struct clone3_fixture *data) struct clone_args *args = &(data->args); int pidfd, parent_tid = 0, child_tid = 0; siginfo_t wait_si;
- int result;
- int result, wstatus; pid_t pid; void *tls = NULL;
@@ -461,8 +461,8 @@ void run_clone3(struct clone3_fixture *data) ASSERT_EQ(result, 0); }
- result = waitid(P_PID, pid, &wait_si, WEXITED, NULL);
- ASSERT_EQ(result, 0);
- result = waitpid(pid, &wstatus, 0);
The second argument can be NULL since we don't actually use wstatus.
Kevin
- ASSERT_EQ(result, pid);
/* child_tid set once the thread gets scheduled */ if (args->flags & CLONE_PARENT_SETTID && args->flags & CLONE_CHILD_SETTID diff --git a/tools/testing/selftests/arm64/morello/freestanding.h b/tools/testing/selftests/arm64/morello/freestanding.h index 19611b315de9..0b0de05d46bc 100644 --- a/tools/testing/selftests/arm64/morello/freestanding.h +++ b/tools/testing/selftests/arm64/morello/freestanding.h @@ -213,4 +213,9 @@ 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 diff --git a/tools/testing/selftests/arm64/morello/signal.c b/tools/testing/selftests/arm64/morello/signal.c index 3dcea434275c..3108755d9ed7 100644 --- a/tools/testing/selftests/arm64/morello/signal.c +++ b/tools/testing/selftests/arm64/morello/signal.c @@ -302,10 +302,10 @@ TEST(test_rt_sigqueueinfo) TEST(test_rt_tgsigqueueinfo) {
- siginfo_t si, wait_si;
- siginfo_t si; pid_t cpid; struct sigaction sa;
- int ret;
- int ret, wstatus;
setup_sigusr1_handler(&sa, SIG_UNBLOCK); @@ -324,17 +324,17 @@ TEST(test_rt_tgsigqueueinfo) ASSERT_EQ(ret, 0) { __TH_LOG_ERROR("rt_tgsigqueueinfo syscall failed"); }
- ret = waitid(P_PID, cpid, &wait_si, WEXITED, NULL);
- ASSERT_EQ(ret, 0) {
- ret = waitpid(cpid, &wstatus, 0);
- ASSERT_EQ(ret, cpid) { __TH_LOG_ERROR("test_rt_tgsigqueueinfo: Failed on wait"); }
} TEST(test_pidfd_send_signal) {
- siginfo_t si, wait_si;
- siginfo_t si; pid_t cpid;
- int pidfd, ret;
- int pidfd, ret, wstatus; struct sigaction sa;
setup_sigusr1_handler(&sa, SIG_UNBLOCK); @@ -364,8 +364,8 @@ TEST(test_pidfd_send_signal) ASSERT_EQ(ret, 0) { __TH_LOG_ERROR("pidfd_send_signal syscall failed"); }
- ret = waitid(P_PID, cpid, &wait_si, WEXITED, NULL);
- ASSERT_EQ(ret, 0) {
- ret = waitpid(cpid, &wstatus, 0);
- ASSERT_EQ(ret, cpid) { __TH_LOG_ERROR("test_pidfd_send_signal: Failed on wait"); } close(pidfd);
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 data handshake does not happen properly between parent and child due to compiler optimization as observed with Morello Gcc.
Although this can be fixed by setting the flag to volatile but here we replace the busy loop with the waitpid() syscall to wait for the child to exit before parent is executed.
Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com --- tools/testing/selftests/arm64/morello/clone.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index 0381702ab0fc..6f0ea953848c 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; @@ -78,7 +74,7 @@ static int clone_base_fn(void *data)
if (!tls) { __data->result = CTR_FAILED; - goto done; + return 0; }
probe_addr_range(tls, STACK_SIZE >> 10, 64); @@ -88,15 +84,13 @@ static int clone_base_fn(void *data) __data->result = !!(__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, @@ -107,7 +101,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;
int (*clone_fn_ptr)(void *) = clone_base_fn;
@@ -142,10 +136,9 @@ void clone_single(struct test_fixture *data) goto leave; }
- /* Wait substitute ... */ - while (data->status != MAGIC_NR) { - asm(""); - } + /* Wait for the child to exit */ + result = waitpid(cpid, &wstatus, 0); + ASSERT_EQ(result, cpid); /* * 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.
On 20/03/2023 11:12, 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 data handshake does not happen properly between parent and child due to compiler optimization as observed with Morello Gcc.
Although this can be fixed by setting the flag to volatile but here we replace the busy loop with the waitpid() syscall to wait for the child to exit before parent is executed.
Signed-off-by: Amit Daniel Kachhap amit.kachhap@arm.com
tools/testing/selftests/arm64/morello/clone.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index 0381702ab0fc..6f0ea953848c 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;
@@ -78,7 +74,7 @@ static int clone_base_fn(void *data) if (!tls) { __data->result = CTR_FAILED;
goto done;
}return 0;
probe_addr_range(tls, STACK_SIZE >> 10, 64); @@ -88,15 +84,13 @@ static int clone_base_fn(void *data) __data->result = !!(__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, @@ -107,7 +101,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;
int (*clone_fn_ptr)(void *) = clone_base_fn; @@ -142,10 +136,9 @@ void clone_single(struct test_fixture *data) goto leave; }
- /* Wait substitute ... */
- while (data->status != MAGIC_NR) {
asm("");
- }
- /* Wait for the child to exit */
- result = waitpid(cpid, &wstatus, 0);
- ASSERT_EQ(result, cpid);
This makes me realise we could also get rid of data->result. Instead we can simply return 0/1 from clone_base_fn() and here we can check that WIFEXITED(wstatus) && WEXITSTATUS(wstatus) == 0.
Kevin
/* * 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.
linux-morello@op-lists.linaro.org