On 30/11/2022 22:27, Beata Michalska wrote:
On Wed, Nov 30, 2022 at 03:49:38PM +0100, Kevin Brodsky wrote:
On 18/11/2022 01:05, Beata Michalska wrote:
Add a bunch of test-cases for clone3 syscall, focusing mainly on handling struct clone_args versioning and sizing, alongside the obvious req for respecting capabilities.
Signed-off-by: Beata Michalska beata.michalska@arm.com
tools/testing/selftests/arm64/morello/clone.c | 269 +++++++++++++++++- 1 file changed, 267 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index 8217840ba504..13c84dc7ca77 100644 --- a/tools/testing/selftests/arm64/morello/clone.c +++ b/tools/testing/selftests/arm64/morello/clone.c @@ -1,15 +1,18 @@ // SPDX-License-Identifier: GPL-2.0 // Copyright (C) 2021 Arm Limited +#include "freestanding.h" #include <linux/mman.h> #include <linux/resource.h> #include <linux/wait.h> #include <linux/signal.h> #include <linux/sched.h> #include <linux/errno.h> +#include <linux/signal.h> +#include <limits.h> #include <cheriintrin.h> -#include "freestanding.h" -#define STACK_SIZE 1024*1024 +#define STACK_SIZE 1024*1024 +#define DEFAULT_PAGE_SIZE 4096 #define in_restricted() \ (!(cheri_perms_get(cheri_pcc_get()) & ARM_CAP_PERMISSION_EXECUTIVE)) @@ -17,6 +20,10 @@ #define MAGIC_NR 0x3562 /* whatever really ..... */ +#ifndef MAX_PID_NS_LEVEL +#define MAX_PID_NS_LEVEL 32 +#endif
/* Cloned thread result */ #define CTR_SUCCESS 1 #define CTR_FAILED -1 @@ -256,6 +263,263 @@ TEST(test_wait_rusage) RUN_WITH_FIXTURE(data, CLONE_TH_RUSAGE, wait_single); }
+#define GET_ERRNO(err) (((err) + MAX_ERRNO) ^ MAX_ERRNO)
This magic vaguely rings a bell but I'm not entirely sure why we need it here, could you elaborate?
Getting the absolute value to report the errno. It's not strictly necessary, though I think I found that convenient at some point....
Why not, I'm not sure it helps that much though, in fact it feels like "ASSERT_EQ(result, data->e_result)" is enough on its own without the explicit print after.
[...]
- int result;
- pid_t pid;
- struct clone_args args = {
.flags = 0,.pidfd = (uintcap_t)&pidfd,.parent_tid = (uintcap_t)&parent_tid,.child_tid = (uintcap_t)&child_tid,.exit_signal = SIGCHLD,.stack = (uintcap_t) mmap_verified(NULL, STACK_SIZE,PROT_READ | PROT_WRITE,MAP_ANONYMOUS | MAP_PRIVATE,-1, 0, STACK_REQ_PERMS),.stack_size = STACK_SIZE,- };
- for (int i = 0; i < data->n_desc; ++i) {
unsigned char *arg;arg = (unsigned char *)&args + data->args_desc[i].offset;if (data->args_desc[i].size != sizeof(uintcap_t))*(__u64 *)arg = (__u64)data->args_desc[i].value;else*(uintcap_t *)arg = data->args_desc[i].value;- }
- /* Don't bother with the full set-up for expected failures */
- if (data->e_result) {
result = syscall(__NR_clone3, &args, data->args_size);ASSERT_EQ(result, data->e_result) {ASSERT_* causes the whole program to exit, so you probably want EXPECT_EQ() here.
That would mean all ASSERT_x should be replaced with EXPECT_x counterparts ?
Sorry I'm realising I didn't think this straight, if you exit the following munmap() is irrelevant, so there is no particular reason to use EXPECT here. I don't have much opinion on ASSERT vs EXPECT, I don't think it really matters for these tests, so no problem leaving it unchanged.
TH_LOG("Expected: %d while %d was received",GET_ERRNO(data->e_result), GET_ERRNO(result));}munmap((void *)args.stack, STACK_SIZE);return;- }
- args.flags |= CLONE_VM;
- if (args.flags & CLONE_SETTLS) {
void *addr = mmap_verified(NULL, DEFAULT_PAGE_SIZE,Not a big fan of calling this DEFAULT_PAGE_SIZE, as we shouldn't make any assumption on the page size. Fortunately the page size doesn't actually matter here as both mmap() and munmap() will align up the length to the page size. Maybe simply TLS_SIZE?
Yeah, I did hesitate on that one for a moment, but as this one is not literally expecting PAGE_SIZE I went for it. I'll change that to TLS_SIZE instead.
PROT_READ | PROT_WRITE,MAP_ANONYMOUS | MAP_PRIVATE, -1, 0,CAP_LOAD_PERMS | CAP_STORE_PERMS);args.tls = (uintcap_t)cheri_bounds_set(addr, DEFAULT_PAGE_SIZE);- }
- pid = syscall(__NR_clone3, &args, data->args_size);
- ASSERT_GE(pid, 0);
- if (!pid)
run_child(&args);- if (args.flags & CLONE_PIDFD) {
/* Signal handling is not the test target here: valid pidfd is */result = syscall(__NR_pidfd_send_signal, pidfd, SIGUSR1, NULL, 0);ASSERT_NE(result, -EBADF);Agreed that we're not testing signal handling here, in fact for that reason I'd suggest using a signal that is ignored by default like SIGCHLD, not SIGUSR1 which causes termination by default. Alternatively, you could use sigwaitinfo() in the child, which also means you wouldn't need the busy loop.
True, though I was trying to avoid using signal handling here on purpose, to not to rely on other 'functionality' to align with our initial approach to kselftests - test with minimal setup to avoid the test failing on things that are not the test subject themselves.
OTOH I think it wouldn't hurt to check that pidfd_send_signal() didn't fail (in any way), as if it does that would suggest something went wrong with the test setup.
... or smth is wrong with handling signals which relates to my comment above. I can change both if we want to relax our kselftests.
I'd go for that, assuming that fundamental subsystems like signals cannot be relied on creates a lot of overhead, so I'd only do that for the most basic tests, and I don't consider clone3 one of them.
- }
- waitid(P_PID, pid, wait_si, WEXITED, NULL);
Should check that this one doesn't fail. This also makes me think, can we now get rid of the scary wait substitute in clone_single(), since we're not bothering with it here?
We could though at this point, the wait testcases are done, which means we can safely assume wait is working as expected and it's safe to rely on it. Again - exercising limited trust.
Yep makes sense.
Kevin