A few tests currently create temporary files in / to obtain an fd for testing further syscalls. This is suboptimal as these files may be used for other purposes, they are not deleted after the tests are run, and we should generally avoid creating files at the root.
We could create files in /tmp instead, with some naming pattern à la mkstemp(), but it turns out there is an even simpler solution. open() can be passed O_TMPFILE to create an unnamed file under a given directory. The file disappears once the fd is closed, so there can be no leftover. This approach is used to implement tmpfile() in glibc, if available.
On the model of tmpfile(), add a new helper tmpfd() in freestanding.h and make use of it in the tests that need a temporary fd, replacing the hardcoded files.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com ---
This will also help for Akram's uaccess kselftests.
Review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/morello/kselfte...
.../selftests/arm64/morello/freestanding.h | 15 +++++++++++++++ tools/testing/selftests/arm64/morello/mmap.c | 4 +--- .../testing/selftests/arm64/morello/read_write.c | 7 ++----- 3 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/arm64/morello/freestanding.h b/tools/testing/selftests/arm64/morello/freestanding.h index 8dab905d2b54..5ab02dee1270 100644 --- a/tools/testing/selftests/arm64/morello/freestanding.h +++ b/tools/testing/selftests/arm64/morello/freestanding.h @@ -8,6 +8,7 @@ #include <stdint.h> #include <stddef.h> #include <asm/unistd.h> +#include <linux/fcntl.h> #include <linux/posix_types.h> #include <linux/resource.h> #include <linux/signal.h> @@ -229,4 +230,18 @@ static inline int waitpid(pid_t pid, int *wstatus, int options) return syscall(__NR_wait4, pid, wstatus, options, 0); }
+/* + * Creates a new temporary file and returns an fd to it. The file has no name + * (see open(2) regarding O_TMPFILE) and is deleted when the fd is closed. + */ +static inline int tmpfd(void) +{ + int fd; + + fd = syscall(__NR_openat, 0, "/", O_TMPFILE | O_RDWR, 0666); + ASSERT_GE(fd, 0); + + return fd; +} + #endif diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 62fc14d14d46..618eb221d0ae 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -7,7 +7,6 @@ #include <linux/mman.h> #include <linux/errno.h> #include <linux/fs.h> -#include <linux/fcntl.h> #include <cheriintrin.h> #include "freestanding.h"
@@ -65,13 +64,12 @@ void syscall_mmap2(void) { const char msg[] = "foo"; unsigned int msg_len = sizeof(msg); /* No need for the terminator */ - const char *sample_file = "/limbo.dat"; void *addr; int fd; int retval;
/* create a sample file to map onto with mmap */ - fd = syscall(__NR_openat, 0, sample_file, O_RDWR | O_CREAT, FILE_PERM); + fd = tmpfd();
ASSERT_GE(fd, 0);
diff --git a/tools/testing/selftests/arm64/morello/read_write.c b/tools/testing/selftests/arm64/morello/read_write.c index a68b5efeb5ab..6425c8449baa 100644 --- a/tools/testing/selftests/arm64/morello/read_write.c +++ b/tools/testing/selftests/arm64/morello/read_write.c @@ -7,7 +7,6 @@ #include <linux/fs.h> #include <linux/stat.h> #include <linux/uio.h> -#include <asm/fcntl.h> #include <asm/unistd.h>
#include "freestanding.h" @@ -26,8 +25,6 @@ static const char w_vec_msg2[VEC_MSG_LEN] = " is vector\n"; static char r_vec_msg1[VEC_MSG_LEN]; static char r_vec_msg2[VEC_MSG_LEN];
-static const char file[] = "/check_cap.txt"; - static const struct iovec w_iovec[VEC_MSG_NUM] = { {.iov_base = (char *) w_vec_msg1, .iov_len = VEC_MSG_LEN}, {.iov_base = (char *) w_vec_msg2, .iov_len = VEC_MSG_LEN}, @@ -58,8 +55,8 @@ TEST(test_writev)
TEST(test_open) { - fd = syscall(__NR_openat, 0, file, O_RDWR | O_CREAT, 0666); - ASSERT_LE(1, fd) TH_LOG("open failed"); + /* tmpfd() asserts that the openat syscall succeeds */ + fd = tmpfd(); }
TEST(test_read)
On 01/02/2024 15:45, Kevin Brodsky wrote:
[...] +/*
- Creates a new temporary file and returns an fd to it. The file has no name
- (see open(2) regarding O_TMPFILE) and is deleted when the fd is closed.
- */
+static inline int tmpfd(void) +{
- int fd;
- fd = syscall(__NR_openat, 0, "/", O_TMPFILE | O_RDWR, 0666);
- ASSERT_GE(fd, 0);
The man pages [1] discuss using O_EXCL with O_TMPFILE to prevent fd from being linked into the filesystem with linkat [2]; is this something worth considering here if we want to discourage files opened with tmpfd() from being linked at any time? Just curious, I imagine it isn't a big deal at all since it would be hard to accidentally call tmpfd and link the result in a test, where this is meant to be used.
Akram
[1] https://man7.org/linux/man-pages/man2/open.2.html#DESCRIPTION
[2] https://man7.org/linux/man-pages/man2/linkat.2.html
**
On 14/02/2024 14:19, Akram Ahmad wrote:
On 01/02/2024 15:45, Kevin Brodsky wrote:
[...] +/*
- Creates a new temporary file and returns an fd to it. The file has no name
- (see open(2) regarding O_TMPFILE) and is deleted when the fd is closed.
- */
+static inline int tmpfd(void) +{
- int fd;
- fd = syscall(__NR_openat, 0, "/", O_TMPFILE | O_RDWR, 0666);
- ASSERT_GE(fd, 0);
The man pages [1] discuss using O_EXCL with O_TMPFILE to prevent fd from being linked into the filesystem with linkat [2]; is this something worth considering here if we want to discourage files opened with tmpfd() from being linked at any time? Just curious, I imagine it isn't a big deal at all since it would be hard to accidentally call tmpfd and link the result in a test, where this is meant to be used.
I don't think that's a concern. You can only use linkat() if you have the fd around, so that pretty much means the test itself using linkat(), and I don't see that happening by accident.
Kevin
Akram
[1] https://man7.org/linux/man-pages/man2/open.2.html#DESCRIPTION
On 01/02/2024 15:45, Kevin Brodsky wrote:
A few tests currently create temporary files in / to obtain an fd for testing further syscalls. This is suboptimal as these files may be used for other purposes, they are not deleted after the tests are run, and we should generally avoid creating files at the root.
We could create files in /tmp instead, with some naming pattern à la mkstemp(), but it turns out there is an even simpler solution. open() can be passed O_TMPFILE to create an unnamed file under a given directory. The file disappears once the fd is closed, so there can be no leftover. This approach is used to implement tmpfile() in glibc, if available.
On the model of tmpfile(), add a new helper tmpfd() in freestanding.h and make use of it in the tests that need a temporary fd, replacing the hardcoded files.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
This will also help for Akram's uaccess kselftests.
Review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/morello/kselftests_tmpfile
.../selftests/arm64/morello/freestanding.h | 15 +++++++++++++++ tools/testing/selftests/arm64/morello/mmap.c | 4 +--- .../testing/selftests/arm64/morello/read_write.c | 7 ++----- 3 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/arm64/morello/freestanding.h b/tools/testing/selftests/arm64/morello/freestanding.h index 8dab905d2b54..5ab02dee1270 100644 --- a/tools/testing/selftests/arm64/morello/freestanding.h +++ b/tools/testing/selftests/arm64/morello/freestanding.h @@ -8,6 +8,7 @@ #include <stdint.h> #include <stddef.h> #include <asm/unistd.h> +#include <linux/fcntl.h> #include <linux/posix_types.h> #include <linux/resource.h> #include <linux/signal.h> @@ -229,4 +230,18 @@ static inline int waitpid(pid_t pid, int *wstatus, int options) return syscall(__NR_wait4, pid, wstatus, options, 0); } +/*
- Creates a new temporary file and returns an fd to it. The file has no name
- (see open(2) regarding O_TMPFILE) and is deleted when the fd is closed.
- */
+static inline int tmpfd(void) +{
- int fd;
- fd = syscall(__NR_openat, 0, "/", O_TMPFILE | O_RDWR, 0666);
- ASSERT_GE(fd, 0);
- return fd;
+}
- #endif
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 62fc14d14d46..618eb221d0ae 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -7,7 +7,6 @@ #include <linux/mman.h> #include <linux/errno.h> #include <linux/fs.h> -#include <linux/fcntl.h> #include <cheriintrin.h> #include "freestanding.h" @@ -65,13 +64,12 @@ void syscall_mmap2(void) { const char msg[] = "foo"; unsigned int msg_len = sizeof(msg); /* No need for the terminator */
- const char *sample_file = "/limbo.dat"; void *addr; int fd; int retval;
/* create a sample file to map onto with mmap */
- fd = syscall(__NR_openat, 0, sample_file, O_RDWR | O_CREAT, FILE_PERM);
- fd = tmpfd();
ASSERT_GE(fd, 0); diff --git a/tools/testing/selftests/arm64/morello/read_write.c b/tools/testing/selftests/arm64/morello/read_write.c index a68b5efeb5ab..6425c8449baa 100644 --- a/tools/testing/selftests/arm64/morello/read_write.c +++ b/tools/testing/selftests/arm64/morello/read_write.c @@ -7,7 +7,6 @@ #include <linux/fs.h> #include <linux/stat.h> #include <linux/uio.h> -#include <asm/fcntl.h> #include <asm/unistd.h> #include "freestanding.h" @@ -26,8 +25,6 @@ static const char w_vec_msg2[VEC_MSG_LEN] = " is vector\n"; static char r_vec_msg1[VEC_MSG_LEN]; static char r_vec_msg2[VEC_MSG_LEN]; -static const char file[] = "/check_cap.txt";
- static const struct iovec w_iovec[VEC_MSG_NUM] = { {.iov_base = (char *) w_vec_msg1, .iov_len = VEC_MSG_LEN}, {.iov_base = (char *) w_vec_msg2, .iov_len = VEC_MSG_LEN},
@@ -58,8 +55,8 @@ TEST(test_writev) TEST(test_open) {
- fd = syscall(__NR_openat, 0, file, O_RDWR | O_CREAT, 0666);
- ASSERT_LE(1, fd) TH_LOG("open failed");
- /* tmpfd() asserts that the openat syscall succeeds */
- fd = tmpfd(); }
TEST(test_read)
I rebased my v6 tests onto the patch and attempted to perform preadv before writev, and I saw the same failures as before (i.e. expected EFAULT, got 0 for manual out-of-bounds check). I'm not sure whether this is expected when using tmpfd() instead of the normal open()?
Akram
On 14/02/2024 16:03, Akram Ahmad wrote:
I rebased my v6 tests onto the patch and attempted to perform preadv before writev, and I saw the same failures as before (i.e. expected EFAULT, got 0 for manual out-of-bounds check). I'm not sure whether this is expected when using tmpfd() instead of the normal open()?
Sorry I misunderstood your question at first - yes this is expected. When using readv/preadv on an empty file, no capability check will occur as there is nothing to read. The uaccess tests as they were merged write to the file first, so this is not a concern.
Kevin
linux-morello@op-lists.linaro.org