This series brings various cleanups and fixes for the mm (mostly pkeys) kselftests. The original goal was to make the pkeys tests work out of the box and without build warning - it turned out to be more involved than expected.
The most important change is enabling -O2 when building all mm kselftests (patch 5). This is actually needed for the pkeys tests to run successfully (see gcc command line at the top of protection_keys.c and pkey_sighandler_tests.c), and seems to have no negative impact on the other tests. It certainly can't hurt performance!
The following patches address a few obvious issues in the pkeys tests (unused code, bad scope for functions/variables, etc.) and finally make a couple of small improvements.
There is one ugliness that this series does not fix: some functions in pkey-<arch>.h call functions that are actually defined in protection_keys.c. For instance, expect_fault_on_read_execonly_key() in pkey-x86.h calls expected_pkey_fault(). This means that other test programs that use pkey-helpers.h (namely pkey_sighandler_tests) would fail to link if they called such functions defined in pkey-<arch>.h. Fixing this would require a more comprehensive reorganisation of the pkey-* headers, which doesn't seem worth it (patch 9 adds a comment to pkey-helpers.h to clarify the situation).
Some more details on the patches:
- Patch 1 is an unrelated fix that was revealed by inspecting a warning. It seems fairly harmless though, so I thought I'd just post it as part of this series.
- Patch 2-5 fix various warnings that come up by building the mm tests at -O2 and finally enable -O2.
- Patch 6-12 are various cleanups for the pkeys tests. Patch 11 in particular enables is_pkeys_supported() to be called from outside protection_keys.c (patch 13 relies on this).
- Patch 13-14 are small improvements to pkey_sighandler_tests.c.
Many thanks to Ryan Roberts for checking that the mm tests still run fine on arm64 with those patches applied. I've also checked that the pkeys tests run fine on arm64 and x86.
- Kevin
--- Cc: akpm@linux-foundation.org Cc: aruna.ramakrishna@oracle.com Cc: catalin.marinas@arm.com Cc: dave.hansen@linux.intel.com Cc: joey.gouly@arm.com Cc: keith.lucas@oracle.com Cc: ryan.roberts@arm.com Cc: shuah@kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kselftest@vger.kernel.org Cc: x86@kernel.org --- Kevin Brodsky (14): selftests/mm: Fix condition in uffd_move_test_common() selftests/mm: Fix -Wmaybe-uninitialized warnings selftests/mm: Fix strncpy() length selftests/mm: Fix -Warray-bounds warnings in pkey_sighandler_tests selftests/mm: Build with -O2 selftests/mm: Remove unused pkey helpers selftests/mm: Define types using typedef in pkey-helpers.h selftests/mm: Ensure pkey-*.h define inline functions only selftests/mm: Remove empty pkey helper definition selftests/mm: Ensure non-global pkey symbols are marked static selftests/mm: Use sys_pkey helpers consistently selftests/mm: Rename pkey register macro selftests/mm: Skip pkey_sighandler_tests if support is missing selftests/mm: Remove X permission from sigaltstack mapping
tools/testing/selftests/mm/Makefile | 6 +- tools/testing/selftests/mm/ksm_tests.c | 2 +- tools/testing/selftests/mm/mremap_test.c | 2 +- tools/testing/selftests/mm/pkey-arm64.h | 6 +- tools/testing/selftests/mm/pkey-helpers.h | 61 ++--- tools/testing/selftests/mm/pkey-powerpc.h | 4 +- tools/testing/selftests/mm/pkey-x86.h | 6 +- .../selftests/mm/pkey_sighandler_tests.c | 32 +-- tools/testing/selftests/mm/pkey_util.c | 40 ++++ tools/testing/selftests/mm/protection_keys.c | 212 +++++++----------- tools/testing/selftests/mm/soft-dirty.c | 2 +- tools/testing/selftests/mm/uffd-unit-tests.c | 4 +- .../testing/selftests/mm/write_to_hugetlbfs.c | 2 +- 13 files changed, 163 insertions(+), 216 deletions(-) create mode 100644 tools/testing/selftests/mm/pkey_util.c
area_src and area_dst are saved at the beginning of the function if chunk_size > page_size. The intention is quite clearly to restore them at the end based on the same condition, but step_size is considered instead of chunk_size. Considering that step_size is a number of pages, the condition is likely to be false.
Use the same condition as when saving so that the globals are restored as intended.
Fixes: a2bf6a9ca805 ("selftests/mm: add UFFDIO_MOVE ioctl test") Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- tools/testing/selftests/mm/uffd-unit-tests.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c index a2e71b1636e7..74c884713bf7 100644 --- a/tools/testing/selftests/mm/uffd-unit-tests.c +++ b/tools/testing/selftests/mm/uffd-unit-tests.c @@ -1190,7 +1190,7 @@ uffd_move_test_common(uffd_test_args_t *targs, unsigned long chunk_size, nr, count, count_verify[src_offs + nr + i]); } } - if (step_size > page_size) { + if (chunk_size > page_size) { area_src = orig_area_src; area_dst = orig_area_dst; }
A few -Wmaybe-uninitialized warnings show up when building the mm tests with -O2. None of them looks worrying; silence them by initialising the problematic variables.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- tools/testing/selftests/mm/ksm_tests.c | 2 +- tools/testing/selftests/mm/mremap_test.c | 2 +- tools/testing/selftests/mm/soft-dirty.c | 2 +- tools/testing/selftests/mm/uffd-unit-tests.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/mm/ksm_tests.c b/tools/testing/selftests/mm/ksm_tests.c index b748c48908d9..dcdd5bb20f3d 100644 --- a/tools/testing/selftests/mm/ksm_tests.c +++ b/tools/testing/selftests/mm/ksm_tests.c @@ -776,7 +776,7 @@ static int ksm_cow_time(int merge_type, int mapping, int prot, int timeout, size
int main(int argc, char *argv[]) { - int ret, opt; + int ret = 0, opt; int prot = 0; int ksm_scan_limit_sec = KSM_SCAN_LIMIT_SEC_DEFAULT; int merge_type = KSM_MERGE_TYPE_DEFAULT; diff --git a/tools/testing/selftests/mm/mremap_test.c b/tools/testing/selftests/mm/mremap_test.c index 5a3a9bcba640..056b227f4a30 100644 --- a/tools/testing/selftests/mm/mremap_test.c +++ b/tools/testing/selftests/mm/mremap_test.c @@ -384,7 +384,7 @@ static void mremap_move_within_range(unsigned int pattern_seed, char *rand_addr) static long long remap_region(struct config c, unsigned int threshold_mb, char *rand_addr) { - void *addr, *src_addr, *dest_addr, *dest_preamble_addr; + void *addr, *src_addr, *dest_addr, *dest_preamble_addr = NULL; unsigned long long t, d; struct timespec t_start = {0, 0}, t_end = {0, 0}; long long start_ns, end_ns, align_mask, ret, offset; diff --git a/tools/testing/selftests/mm/soft-dirty.c b/tools/testing/selftests/mm/soft-dirty.c index bdfa5d085f00..8e1462ce0532 100644 --- a/tools/testing/selftests/mm/soft-dirty.c +++ b/tools/testing/selftests/mm/soft-dirty.c @@ -128,7 +128,7 @@ static void test_mprotect(int pagemap_fd, int pagesize, bool anon) { const char *type[] = {"file", "anon"}; const char *fname = "./soft-dirty-test-file"; - int test_fd; + int test_fd = 0; char *map;
if (anon) { diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c index 74c884713bf7..9ff71fa1f9bf 100644 --- a/tools/testing/selftests/mm/uffd-unit-tests.c +++ b/tools/testing/selftests/mm/uffd-unit-tests.c @@ -1122,7 +1122,7 @@ uffd_move_test_common(uffd_test_args_t *targs, unsigned long chunk_size, char c; unsigned long long count; struct uffd_args args = { 0 }; - char *orig_area_src, *orig_area_dst; + char *orig_area_src = NULL, *orig_area_dst = NULL; unsigned long step_size, step_count; unsigned long src_offs = 0; unsigned long dst_offs = 0;
GCC complains (with -O2) that the length is equal to the destination size, which is indeed invalid. Subtract 1 from the size of the array to leave room for '\0'.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- tools/testing/selftests/mm/write_to_hugetlbfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/write_to_hugetlbfs.c b/tools/testing/selftests/mm/write_to_hugetlbfs.c index 1289d311efd7..34c91f7e6128 100644 --- a/tools/testing/selftests/mm/write_to_hugetlbfs.c +++ b/tools/testing/selftests/mm/write_to_hugetlbfs.c @@ -89,7 +89,7 @@ int main(int argc, char **argv) size = atoi(optarg); break; case 'p': - strncpy(path, optarg, sizeof(path)); + strncpy(path, optarg, sizeof(path) - 1); break; case 'm': if (atoi(optarg) >= MAX_METHOD) {
GCC doesn't like dereferencing a pointer set to 0x1 (when building at -O2):
pkey_sighandler_tests.c:166:9: warning: array subscript 0 is outside array bounds of 'int[0]' [-Warray-bounds=] 166 | *(int *) (0x1) = 1; | ^~~~~~~~~~~~~~ cc1: note: source object is likely at address zero
Using NULL instead seems to make it happy. This should make no difference in practice (SIGSEGV with SEGV_MAPERR will be the outcome regardless), we just need to update the expected si_addr.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- tools/testing/selftests/mm/pkey_sighandler_tests.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/mm/pkey_sighandler_tests.c b/tools/testing/selftests/mm/pkey_sighandler_tests.c index c593a426341c..e7b91794f184 100644 --- a/tools/testing/selftests/mm/pkey_sighandler_tests.c +++ b/tools/testing/selftests/mm/pkey_sighandler_tests.c @@ -163,7 +163,7 @@ static void *thread_segv_with_pkey0_disabled(void *ptr) __write_pkey_reg(pkey_reg_restrictive_default());
/* Segfault (with SEGV_MAPERR) */ - *(int *) (0x1) = 1; + *(int *)NULL = 1; return NULL; }
@@ -179,7 +179,6 @@ static void *thread_segv_pkuerr_stack(void *ptr) static void *thread_segv_maperr_ptr(void *ptr) { stack_t *stack = ptr; - int *bad = (int *)1; u64 pkey_reg;
/* @@ -195,7 +194,7 @@ static void *thread_segv_maperr_ptr(void *ptr) __write_pkey_reg(pkey_reg);
/* Segfault */ - *bad = 1; + *(int *)NULL = 1; syscall_raw(SYS_exit, 0, 0, 0, 0, 0, 0); return NULL; } @@ -234,7 +233,7 @@ static void test_sigsegv_handler_with_pkey0_disabled(void)
ksft_test_result(siginfo.si_signo == SIGSEGV && siginfo.si_code == SEGV_MAPERR && - siginfo.si_addr == (void *)1, + siginfo.si_addr == NULL, "%s\n", __func__); }
@@ -349,7 +348,7 @@ static void test_sigsegv_handler_with_different_pkey_for_stack(void)
ksft_test_result(siginfo.si_signo == SIGSEGV && siginfo.si_code == SEGV_MAPERR && - siginfo.si_addr == (void *)1, + siginfo.si_addr == NULL, "%s\n", __func__); }
Dereferencing a null pointer on Clang is not a good idea - it will entirely optimise out the dereference. Make the pointer volatile to force the access (and fault).
Reported-by: kernel test robot lkp@intel.com Closes: https://lore.kernel.org/oe-kbuild-all/202412140850.4TW4YBqc-lkp@intel.com/ Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com ---
Hi Andrew,
Another fixup for an issue reported by the LKP CI (building with Clang instead of GCC). This should be squashed into the patch I'm replying to: "selftests/mm: Fix -Warray-bounds warnings in pkey_sighandler_tests"
Cheers, - Kevin
Cc: aruna.ramakrishna@oracle.com Cc: catalin.marinas@arm.com Cc: dave.hansen@linux.intel.com Cc: joey.gouly@arm.com Cc: keith.lucas@oracle.com Cc: ryan.roberts@arm.com Cc: shuah@kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kselftest@vger.kernel.org Cc: linux-mm@kvack.org Cc: x86@kernel.org --- tools/testing/selftests/mm/pkey_sighandler_tests.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/mm/pkey_sighandler_tests.c b/tools/testing/selftests/mm/pkey_sighandler_tests.c index 17bbfcd552c6..1ac8c8809880 100644 --- a/tools/testing/selftests/mm/pkey_sighandler_tests.c +++ b/tools/testing/selftests/mm/pkey_sighandler_tests.c @@ -161,7 +161,7 @@ static void *thread_segv_with_pkey0_disabled(void *ptr) __write_pkey_reg(pkey_reg_restrictive_default());
/* Segfault (with SEGV_MAPERR) */ - *(int *)NULL = 1; + *(volatile int *)NULL = 1; return NULL; }
@@ -192,7 +192,7 @@ static void *thread_segv_maperr_ptr(void *ptr) __write_pkey_reg(pkey_reg);
/* Segfault */ - *(int *)NULL = 1; + *(volatile int *)NULL = 1; syscall_raw(SYS_exit, 0, 0, 0, 0, 0, 0); return NULL; }
The mm kselftests are currently built with no optimisation (-O0). It's unclear why, and besides being obviously suboptimal, this also prevents the pkeys tests from working as intended. Let's build all the tests with -O2.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- tools/testing/selftests/mm/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 3de23ea4663f..814b17a43385 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -33,7 +33,7 @@ endif # LDLIBS. MAKEFLAGS += --no-builtin-rules
-CFLAGS = -Wall -I $(top_srcdir) $(EXTRA_CFLAGS) $(KHDR_INCLUDES) $(TOOLS_INCLUDES) +CFLAGS = -Wall -O2 -I $(top_srcdir) $(EXTRA_CFLAGS) $(KHDR_INCLUDES) $(TOOLS_INCLUDES) LDLIBS = -lrt -lpthread -lm
KDIR ?= /lib/modules/$(shell uname -r)/build
Commit 5f23f6d082a9 ("x86/pkeys: Add self-tests") introduced a number of helpers and functions that don't seem to have ever been used. Let's remove them.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- tools/testing/selftests/mm/pkey-helpers.h | 34 -------------------- tools/testing/selftests/mm/protection_keys.c | 34 -------------------- 2 files changed, 68 deletions(-)
diff --git a/tools/testing/selftests/mm/pkey-helpers.h b/tools/testing/selftests/mm/pkey-helpers.h index f7cfe163b0ff..472febd992eb 100644 --- a/tools/testing/selftests/mm/pkey-helpers.h +++ b/tools/testing/selftests/mm/pkey-helpers.h @@ -26,9 +26,7 @@ #ifndef DEBUG_LEVEL #define DEBUG_LEVEL 0 #endif -#define DPRINT_IN_SIGNAL_BUF_SIZE 4096 extern int dprint_in_signal; -extern char dprint_in_signal_buffer[DPRINT_IN_SIGNAL_BUF_SIZE];
extern int test_nr; extern int iteration_nr; @@ -171,38 +169,6 @@ static inline void write_pkey_reg(u64 pkey_reg) pkey_reg, __read_pkey_reg()); }
-/* - * These are technically racy. since something could - * change PKEY register between the read and the write. - */ -static inline void __pkey_access_allow(int pkey, int do_allow) -{ - u64 pkey_reg = read_pkey_reg(); - int bit = pkey * 2; - - if (do_allow) - pkey_reg &= (1<<bit); - else - pkey_reg |= (1<<bit); - - dprintf4("pkey_reg now: %016llx\n", read_pkey_reg()); - write_pkey_reg(pkey_reg); -} - -static inline void __pkey_write_allow(int pkey, int do_allow_write) -{ - u64 pkey_reg = read_pkey_reg(); - int bit = pkey * 2 + 1; - - if (do_allow_write) - pkey_reg &= (1<<bit); - else - pkey_reg |= (1<<bit); - - write_pkey_reg(pkey_reg); - dprintf4("pkey_reg now: %016llx\n", read_pkey_reg()); -} - #define ALIGN_UP(x, align_to) (((x) + ((align_to)-1)) & ~((align_to)-1)) #define ALIGN_DOWN(x, align_to) ((x) & ~((align_to)-1)) #define ALIGN_PTR_UP(p, ptr_align_to) \ diff --git a/tools/testing/selftests/mm/protection_keys.c b/tools/testing/selftests/mm/protection_keys.c index 4990f7ab4cb7..fcbebc4490b4 100644 --- a/tools/testing/selftests/mm/protection_keys.c +++ b/tools/testing/selftests/mm/protection_keys.c @@ -53,7 +53,6 @@ int test_nr;
u64 shadow_pkey_reg; int dprint_in_signal; -char dprint_in_signal_buffer[DPRINT_IN_SIGNAL_BUF_SIZE];
void cat_into_file(char *str, char *file) { @@ -397,12 +396,6 @@ void signal_handler(int signum, siginfo_t *si, void *vucontext) dprint_in_signal = 0; }
-int wait_all_children(void) -{ - int status; - return waitpid(-1, &status, 0); -} - void sig_chld(int x) { dprint_in_signal = 1; @@ -817,39 +810,12 @@ void *malloc_pkey_hugetlb(long size, int prot, u16 pkey) return ptr; }
-void *malloc_pkey_mmap_dax(long size, int prot, u16 pkey) -{ - void *ptr; - int fd; - - dprintf1("doing %s(size=%ld, prot=0x%x, pkey=%d)\n", __func__, - size, prot, pkey); - pkey_assert(pkey < NR_PKEYS); - fd = open("/dax/foo", O_RDWR); - pkey_assert(fd >= 0); - - ptr = mmap(0, size, prot, MAP_SHARED, fd, 0); - pkey_assert(ptr != (void *)-1); - - mprotect_pkey(ptr, size, prot, pkey); - - record_pkey_malloc(ptr, size, prot); - - dprintf1("mmap()'d for pkey %d @ %p\n", pkey, ptr); - close(fd); - return ptr; -} - void *(*pkey_malloc[])(long size, int prot, u16 pkey) = {
malloc_pkey_with_mprotect, malloc_pkey_with_mprotect_subpage, malloc_pkey_anon_huge, malloc_pkey_hugetlb -/* can not do direct with the pkey_mprotect() API: - malloc_pkey_mmap_direct, - malloc_pkey_mmap_dax, -*/ };
void *malloc_pkey(long size, int prot, u16 pkey)
Using #define to define types should be avoided. Use typedef instead. Also ensure that __u* types are actually defined by including <linux/types.h>.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- tools/testing/selftests/mm/pkey-helpers.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/mm/pkey-helpers.h b/tools/testing/selftests/mm/pkey-helpers.h index 472febd992eb..84376ab09545 100644 --- a/tools/testing/selftests/mm/pkey-helpers.h +++ b/tools/testing/selftests/mm/pkey-helpers.h @@ -13,13 +13,15 @@ #include <ucontext.h> #include <sys/mman.h>
+#include <linux/types.h> + #include "../kselftest.h"
/* Define some kernel-like types */ -#define u8 __u8 -#define u16 __u16 -#define u32 __u32 -#define u64 __u64 +typedef __u8 u8; +typedef __u16 u16; +typedef __u32 u32; +typedef __u64 u64;
#define PTR_ERR_ENOTSUP ((void *)-ENOTSUP)
Headers should not define non-inline functions, as this prevents them from being included more than once in a given program. pkey-helpers.h and the arch-specific headers it includes currently define multiple such non-inline functions.
In most cases those functions can simply be made inline - this patch does just that. read_ptr() is an exception as it must not be inlined. Since it is only called from protection_keys.c, we just move it there.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- tools/testing/selftests/mm/pkey-arm64.h | 4 ++-- tools/testing/selftests/mm/pkey-helpers.h | 8 +------- tools/testing/selftests/mm/pkey-powerpc.h | 4 ++-- tools/testing/selftests/mm/pkey-x86.h | 6 +++--- tools/testing/selftests/mm/protection_keys.c | 7 +++++++ 5 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/mm/pkey-arm64.h b/tools/testing/selftests/mm/pkey-arm64.h index d9d2100eafc0..9897e31f16dd 100644 --- a/tools/testing/selftests/mm/pkey-arm64.h +++ b/tools/testing/selftests/mm/pkey-arm64.h @@ -81,11 +81,11 @@ static inline int get_arch_reserved_keys(void) return NR_RESERVED_PKEYS; }
-void expect_fault_on_read_execonly_key(void *p1, int pkey) +static inline void expect_fault_on_read_execonly_key(void *p1, int pkey) { }
-void *malloc_pkey_with_mprotect_subpage(long size, int prot, u16 pkey) +static inline void *malloc_pkey_with_mprotect_subpage(long size, int prot, u16 pkey) { return PTR_ERR_ENOTSUP; } diff --git a/tools/testing/selftests/mm/pkey-helpers.h b/tools/testing/selftests/mm/pkey-helpers.h index 84376ab09545..bc81275a89d9 100644 --- a/tools/testing/selftests/mm/pkey-helpers.h +++ b/tools/testing/selftests/mm/pkey-helpers.h @@ -84,13 +84,7 @@ extern void abort_hooks(void); # define noinline __attribute__((noinline)) #endif
-noinline int read_ptr(int *ptr) -{ - /* Keep GCC from optimizing this away somehow */ - barrier(); - return *ptr; -} - +noinline int read_ptr(int *ptr); void expected_pkey_fault(int pkey); int sys_pkey_alloc(unsigned long flags, unsigned long init_val); int sys_pkey_free(unsigned long pkey); diff --git a/tools/testing/selftests/mm/pkey-powerpc.h b/tools/testing/selftests/mm/pkey-powerpc.h index 3d0c0bdae5bc..1bad310d282a 100644 --- a/tools/testing/selftests/mm/pkey-powerpc.h +++ b/tools/testing/selftests/mm/pkey-powerpc.h @@ -91,7 +91,7 @@ static inline int get_arch_reserved_keys(void) return NR_RESERVED_PKEYS_64K_3KEYS; }
-void expect_fault_on_read_execonly_key(void *p1, int pkey) +static inline void expect_fault_on_read_execonly_key(void *p1, int pkey) { /* * powerpc does not allow userspace to change permissions of exec-only @@ -105,7 +105,7 @@ void expect_fault_on_read_execonly_key(void *p1, int pkey) /* 4-byte instructions * 16384 = 64K page */ #define __page_o_noops() asm(".rept 16384 ; nop; .endr")
-void *malloc_pkey_with_mprotect_subpage(long size, int prot, u16 pkey) +static inline void *malloc_pkey_with_mprotect_subpage(long size, int prot, u16 pkey) { void *ptr; int ret; diff --git a/tools/testing/selftests/mm/pkey-x86.h b/tools/testing/selftests/mm/pkey-x86.h index ac91777c8917..f7ecd335df1e 100644 --- a/tools/testing/selftests/mm/pkey-x86.h +++ b/tools/testing/selftests/mm/pkey-x86.h @@ -113,7 +113,7 @@ static inline u32 pkey_bit_position(int pkey) #define XSTATE_PKEY 0x200 #define XSTATE_BV_OFFSET 512
-int pkey_reg_xstate_offset(void) +static inline int pkey_reg_xstate_offset(void) { unsigned int eax; unsigned int ebx; @@ -148,7 +148,7 @@ static inline int get_arch_reserved_keys(void) return NR_RESERVED_PKEYS; }
-void expect_fault_on_read_execonly_key(void *p1, int pkey) +static inline void expect_fault_on_read_execonly_key(void *p1, int pkey) { int ptr_contents;
@@ -157,7 +157,7 @@ void expect_fault_on_read_execonly_key(void *p1, int pkey) expected_pkey_fault(pkey); }
-void *malloc_pkey_with_mprotect_subpage(long size, int prot, u16 pkey) +static inline void *malloc_pkey_with_mprotect_subpage(long size, int prot, u16 pkey) { return PTR_ERR_ENOTSUP; } diff --git a/tools/testing/selftests/mm/protection_keys.c b/tools/testing/selftests/mm/protection_keys.c index fcbebc4490b4..82ece325b70c 100644 --- a/tools/testing/selftests/mm/protection_keys.c +++ b/tools/testing/selftests/mm/protection_keys.c @@ -54,6 +54,13 @@ int test_nr; u64 shadow_pkey_reg; int dprint_in_signal;
+noinline int read_ptr(int *ptr) +{ + /* Keep GCC from optimizing this away somehow */ + barrier(); + return *ptr; +} + void cat_into_file(char *str, char *file) { int fd = open(file, O_RDWR);
Some of the functions declared in pkey-helpers.h are actually defined in protections_keys.c, meaning they can only be called from protections_keys.c. This is less than ideal, but it is hard to avoid as these helpers are themselves called from inline functions in pkey-<arch>.h. Let's at least add a comment clarifying that. We can also remove the empty definition in pkey_sighandler_tests.c: expected_pkey_fault() is not meant to be called from there.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- tools/testing/selftests/mm/pkey-helpers.h | 6 ++++-- tools/testing/selftests/mm/pkey_sighandler_tests.c | 2 -- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/mm/pkey-helpers.h b/tools/testing/selftests/mm/pkey-helpers.h index bc81275a89d9..7604cc66ef0e 100644 --- a/tools/testing/selftests/mm/pkey-helpers.h +++ b/tools/testing/selftests/mm/pkey-helpers.h @@ -84,10 +84,12 @@ extern void abort_hooks(void); # define noinline __attribute__((noinline)) #endif
-noinline int read_ptr(int *ptr); -void expected_pkey_fault(int pkey); int sys_pkey_alloc(unsigned long flags, unsigned long init_val); int sys_pkey_free(unsigned long pkey); + +/* For functions called from protection_keys.c only */ +noinline int read_ptr(int *ptr); +void expected_pkey_fault(int pkey); int mprotect_pkey(void *ptr, size_t size, unsigned long orig_prot, unsigned long pkey); void record_pkey_malloc(void *ptr, long size, int prot); diff --git a/tools/testing/selftests/mm/pkey_sighandler_tests.c b/tools/testing/selftests/mm/pkey_sighandler_tests.c index e7b91794f184..d18e38b19792 100644 --- a/tools/testing/selftests/mm/pkey_sighandler_tests.c +++ b/tools/testing/selftests/mm/pkey_sighandler_tests.c @@ -32,8 +32,6 @@
#define STACK_SIZE PTHREAD_STACK_MIN
-void expected_pkey_fault(int pkey) {} - pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER; pthread_cond_t cond = PTHREAD_COND_INITIALIZER; siginfo_t siginfo = {0};
The pkey tests define a whole lot of functions and some global variables. A few are truly global (declared in pkey-helpers.h), but the majority are file-scoped. Make sure those are labelled static.
Some of the pkey_{access,write}_{allow,deny} helpers are not called, or only called when building for some architectures. Mark them __maybe_unused to suppress compiler warnings.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- tools/testing/selftests/mm/pkey-helpers.h | 3 + .../selftests/mm/pkey_sighandler_tests.c | 6 +- tools/testing/selftests/mm/protection_keys.c | 132 +++++++++--------- 3 files changed, 72 insertions(+), 69 deletions(-)
diff --git a/tools/testing/selftests/mm/pkey-helpers.h b/tools/testing/selftests/mm/pkey-helpers.h index 7604cc66ef0e..6f0ab7b42738 100644 --- a/tools/testing/selftests/mm/pkey-helpers.h +++ b/tools/testing/selftests/mm/pkey-helpers.h @@ -83,6 +83,9 @@ extern void abort_hooks(void); #ifndef noinline # define noinline __attribute__((noinline)) #endif +#ifndef __maybe_unused +# define __maybe_unused __attribute__((__unused__)) +#endif
int sys_pkey_alloc(unsigned long flags, unsigned long init_val); int sys_pkey_free(unsigned long pkey); diff --git a/tools/testing/selftests/mm/pkey_sighandler_tests.c b/tools/testing/selftests/mm/pkey_sighandler_tests.c index d18e38b19792..e1aaeb65cfae 100644 --- a/tools/testing/selftests/mm/pkey_sighandler_tests.c +++ b/tools/testing/selftests/mm/pkey_sighandler_tests.c @@ -32,9 +32,9 @@
#define STACK_SIZE PTHREAD_STACK_MIN
-pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER; -pthread_cond_t cond = PTHREAD_COND_INITIALIZER; -siginfo_t siginfo = {0}; +static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER; +static pthread_cond_t cond = PTHREAD_COND_INITIALIZER; +static siginfo_t siginfo = {0};
/* * We need to use inline assembly instead of glibc's syscall because glibc's diff --git a/tools/testing/selftests/mm/protection_keys.c b/tools/testing/selftests/mm/protection_keys.c index 82ece325b70c..f43cf3b75d8e 100644 --- a/tools/testing/selftests/mm/protection_keys.c +++ b/tools/testing/selftests/mm/protection_keys.c @@ -61,7 +61,7 @@ noinline int read_ptr(int *ptr) return *ptr; }
-void cat_into_file(char *str, char *file) +static void cat_into_file(char *str, char *file) { int fd = open(file, O_RDWR); int ret; @@ -88,7 +88,7 @@ void cat_into_file(char *str, char *file)
#if CONTROL_TRACING > 0 static int warned_tracing; -int tracing_root_ok(void) +static int tracing_root_ok(void) { if (geteuid() != 0) { if (!warned_tracing) @@ -101,7 +101,7 @@ int tracing_root_ok(void) } #endif
-void tracing_on(void) +static void tracing_on(void) { #if CONTROL_TRACING > 0 #define TRACEDIR "/sys/kernel/tracing" @@ -125,7 +125,7 @@ void tracing_on(void) #endif }
-void tracing_off(void) +static void tracing_off(void) { #if CONTROL_TRACING > 0 if (!tracing_root_ok()) @@ -159,7 +159,7 @@ __attribute__((__aligned__(65536))) #else __attribute__((__aligned__(PAGE_SIZE))) #endif -void lots_o_noops_around_write(int *write_to_me) +static void lots_o_noops_around_write(int *write_to_me) { dprintf3("running %s()\n", __func__); __page_o_noops(); @@ -170,7 +170,7 @@ void lots_o_noops_around_write(int *write_to_me) dprintf3("%s() done\n", __func__); }
-void dump_mem(void *dumpme, int len_bytes) +static void dump_mem(void *dumpme, int len_bytes) { char *c = (void *)dumpme; int i; @@ -213,7 +213,7 @@ static int hw_pkey_set(int pkey, unsigned long rights, unsigned long flags) return 0; }
-void pkey_disable_set(int pkey, int flags) +static void pkey_disable_set(int pkey, int flags) { unsigned long syscall_flags = 0; int ret; @@ -251,7 +251,7 @@ void pkey_disable_set(int pkey, int flags) pkey, flags); }
-void pkey_disable_clear(int pkey, int flags) +static void pkey_disable_clear(int pkey, int flags) { unsigned long syscall_flags = 0; int ret; @@ -277,19 +277,19 @@ void pkey_disable_clear(int pkey, int flags) pkey, read_pkey_reg()); }
-void pkey_write_allow(int pkey) +__maybe_unused static void pkey_write_allow(int pkey) { pkey_disable_clear(pkey, PKEY_DISABLE_WRITE); } -void pkey_write_deny(int pkey) +__maybe_unused static void pkey_write_deny(int pkey) { pkey_disable_set(pkey, PKEY_DISABLE_WRITE); } -void pkey_access_allow(int pkey) +__maybe_unused static void pkey_access_allow(int pkey) { pkey_disable_clear(pkey, PKEY_DISABLE_ACCESS); } -void pkey_access_deny(int pkey) +__maybe_unused static void pkey_access_deny(int pkey) { pkey_disable_set(pkey, PKEY_DISABLE_ACCESS); } @@ -307,9 +307,9 @@ static char *si_code_str(int si_code) return "UNKNOWN"; }
-int pkey_faults; -int last_si_pkey = -1; -void signal_handler(int signum, siginfo_t *si, void *vucontext) +static int pkey_faults; +static int last_si_pkey = -1; +static void signal_handler(int signum, siginfo_t *si, void *vucontext) { ucontext_t *uctxt = vucontext; int trapno; @@ -403,14 +403,14 @@ void signal_handler(int signum, siginfo_t *si, void *vucontext) dprint_in_signal = 0; }
-void sig_chld(int x) +static void sig_chld(int x) { dprint_in_signal = 1; dprintf2("[%d] SIGCHLD: %d\n", getpid(), x); dprint_in_signal = 0; }
-void setup_sigsegv_handler(void) +static void setup_sigsegv_handler(void) { int r, rs; struct sigaction newact; @@ -436,13 +436,13 @@ void setup_sigsegv_handler(void) pkey_assert(r == 0); }
-void setup_handlers(void) +static void setup_handlers(void) { signal(SIGCHLD, &sig_chld); setup_sigsegv_handler(); }
-pid_t fork_lazy_child(void) +static pid_t fork_lazy_child(void) { pid_t forkret;
@@ -488,7 +488,7 @@ int sys_pkey_alloc(unsigned long flags, unsigned long init_val) return ret; }
-int alloc_pkey(void) +static int alloc_pkey(void) { int ret; unsigned long init_val = 0x0; @@ -546,7 +546,7 @@ int sys_pkey_free(unsigned long pkey) * not cleared. This ensures we get lots of random bit sets * and clears on the vma and pte pkey bits. */ -int alloc_random_pkey(void) +static int alloc_random_pkey(void) { int max_nr_pkey_allocs; int ret; @@ -629,7 +629,7 @@ struct pkey_malloc_record { }; struct pkey_malloc_record *pkey_malloc_records; struct pkey_malloc_record *pkey_last_malloc_record; -long nr_pkey_malloc_records; +static long nr_pkey_malloc_records; void record_pkey_malloc(void *ptr, long size, int prot) { long i; @@ -667,7 +667,7 @@ void record_pkey_malloc(void *ptr, long size, int prot) nr_pkey_malloc_records++; }
-void free_pkey_malloc(void *ptr) +static void free_pkey_malloc(void *ptr) { long i; int ret; @@ -694,8 +694,7 @@ void free_pkey_malloc(void *ptr) pkey_assert(false); }
- -void *malloc_pkey_with_mprotect(long size, int prot, u16 pkey) +static void *malloc_pkey_with_mprotect(long size, int prot, u16 pkey) { void *ptr; int ret; @@ -715,7 +714,7 @@ void *malloc_pkey_with_mprotect(long size, int prot, u16 pkey) return ptr; }
-void *malloc_pkey_anon_huge(long size, int prot, u16 pkey) +static void *malloc_pkey_anon_huge(long size, int prot, u16 pkey) { int ret; void *ptr; @@ -745,10 +744,10 @@ void *malloc_pkey_anon_huge(long size, int prot, u16 pkey) return ptr; }
-int hugetlb_setup_ok; +static int hugetlb_setup_ok; #define SYSFS_FMT_NR_HUGE_PAGES "/sys/kernel/mm/hugepages/hugepages-%ldkB/nr_hugepages" #define GET_NR_HUGE_PAGES 10 -void setup_hugetlbfs(void) +static void setup_hugetlbfs(void) { int err; int fd; @@ -796,7 +795,7 @@ void setup_hugetlbfs(void) hugetlb_setup_ok = 1; }
-void *malloc_pkey_hugetlb(long size, int prot, u16 pkey) +static void *malloc_pkey_hugetlb(long size, int prot, u16 pkey) { void *ptr; int flags = MAP_ANONYMOUS|MAP_PRIVATE|MAP_HUGETLB; @@ -817,7 +816,7 @@ void *malloc_pkey_hugetlb(long size, int prot, u16 pkey) return ptr; }
-void *(*pkey_malloc[])(long size, int prot, u16 pkey) = { +static void *(*pkey_malloc[])(long size, int prot, u16 pkey) = {
malloc_pkey_with_mprotect, malloc_pkey_with_mprotect_subpage, @@ -825,7 +824,7 @@ void *(*pkey_malloc[])(long size, int prot, u16 pkey) = { malloc_pkey_hugetlb };
-void *malloc_pkey(long size, int prot, u16 pkey) +static void *malloc_pkey(long size, int prot, u16 pkey) { void *ret; static int malloc_type; @@ -855,7 +854,7 @@ void *malloc_pkey(long size, int prot, u16 pkey) return ret; }
-int last_pkey_faults; +static int last_pkey_faults; #define UNKNOWN_PKEY -2 void expected_pkey_fault(int pkey) { @@ -897,9 +896,9 @@ void expected_pkey_fault(int pkey) pkey_assert(last_pkey_faults == pkey_faults); \ } while (0)
-int test_fds[10] = { -1 }; -int nr_test_fds; -void __save_test_fd(int fd) +static int test_fds[10] = { -1 }; +static int nr_test_fds; +static void __save_test_fd(int fd) { pkey_assert(fd >= 0); pkey_assert(nr_test_fds < ARRAY_SIZE(test_fds)); @@ -907,14 +906,14 @@ void __save_test_fd(int fd) nr_test_fds++; }
-int get_test_read_fd(void) +static int get_test_read_fd(void) { int test_fd = open("/etc/passwd", O_RDONLY); __save_test_fd(test_fd); return test_fd; }
-void close_test_fds(void) +static void close_test_fds(void) { int i;
@@ -927,7 +926,7 @@ void close_test_fds(void) nr_test_fds = 0; }
-void test_pkey_alloc_free_attach_pkey0(int *ptr, u16 pkey) +static void test_pkey_alloc_free_attach_pkey0(int *ptr, u16 pkey) { int i, err; int max_nr_pkey_allocs; @@ -979,7 +978,7 @@ void test_pkey_alloc_free_attach_pkey0(int *ptr, u16 pkey) pkey_assert(!err); }
-void test_read_of_write_disabled_region(int *ptr, u16 pkey) +static void test_read_of_write_disabled_region(int *ptr, u16 pkey) { int ptr_contents;
@@ -989,7 +988,7 @@ void test_read_of_write_disabled_region(int *ptr, u16 pkey) dprintf1("*ptr: %d\n", ptr_contents); dprintf1("\n"); } -void test_read_of_access_disabled_region(int *ptr, u16 pkey) +static void test_read_of_access_disabled_region(int *ptr, u16 pkey) { int ptr_contents;
@@ -1001,7 +1000,7 @@ void test_read_of_access_disabled_region(int *ptr, u16 pkey) expected_pkey_fault(pkey); }
-void test_read_of_access_disabled_region_with_page_already_mapped(int *ptr, +static void test_read_of_access_disabled_region_with_page_already_mapped(int *ptr, u16 pkey) { int ptr_contents; @@ -1018,7 +1017,7 @@ void test_read_of_access_disabled_region_with_page_already_mapped(int *ptr, expected_pkey_fault(pkey); }
-void test_write_of_write_disabled_region_with_page_already_mapped(int *ptr, +static void test_write_of_write_disabled_region_with_page_already_mapped(int *ptr, u16 pkey) { *ptr = __LINE__; @@ -1029,14 +1028,14 @@ void test_write_of_write_disabled_region_with_page_already_mapped(int *ptr, expected_pkey_fault(pkey); }
-void test_write_of_write_disabled_region(int *ptr, u16 pkey) +static void test_write_of_write_disabled_region(int *ptr, u16 pkey) { dprintf1("disabling write access to PKEY[%02d], doing write\n", pkey); pkey_write_deny(pkey); *ptr = __LINE__; expected_pkey_fault(pkey); } -void test_write_of_access_disabled_region(int *ptr, u16 pkey) +static void test_write_of_access_disabled_region(int *ptr, u16 pkey) { dprintf1("disabling access to PKEY[%02d], doing write\n", pkey); pkey_access_deny(pkey); @@ -1044,7 +1043,7 @@ void test_write_of_access_disabled_region(int *ptr, u16 pkey) expected_pkey_fault(pkey); }
-void test_write_of_access_disabled_region_with_page_already_mapped(int *ptr, +static void test_write_of_access_disabled_region_with_page_already_mapped(int *ptr, u16 pkey) { *ptr = __LINE__; @@ -1055,7 +1054,7 @@ void test_write_of_access_disabled_region_with_page_already_mapped(int *ptr, expected_pkey_fault(pkey); }
-void test_kernel_write_of_access_disabled_region(int *ptr, u16 pkey) +static void test_kernel_write_of_access_disabled_region(int *ptr, u16 pkey) { int ret; int test_fd = get_test_read_fd(); @@ -1067,7 +1066,8 @@ void test_kernel_write_of_access_disabled_region(int *ptr, u16 pkey) dprintf1("read ret: %d\n", ret); pkey_assert(ret); } -void test_kernel_write_of_write_disabled_region(int *ptr, u16 pkey) + +static void test_kernel_write_of_write_disabled_region(int *ptr, u16 pkey) { int ret; int test_fd = get_test_read_fd(); @@ -1080,7 +1080,7 @@ void test_kernel_write_of_write_disabled_region(int *ptr, u16 pkey) pkey_assert(ret); }
-void test_kernel_gup_of_access_disabled_region(int *ptr, u16 pkey) +static void test_kernel_gup_of_access_disabled_region(int *ptr, u16 pkey) { int pipe_ret, vmsplice_ret; struct iovec iov; @@ -1102,7 +1102,7 @@ void test_kernel_gup_of_access_disabled_region(int *ptr, u16 pkey) close(pipe_fds[1]); }
-void test_kernel_gup_write_to_write_disabled_region(int *ptr, u16 pkey) +static void test_kernel_gup_write_to_write_disabled_region(int *ptr, u16 pkey) { int ignored = 0xdada; int futex_ret; @@ -1120,7 +1120,7 @@ void test_kernel_gup_write_to_write_disabled_region(int *ptr, u16 pkey) }
/* Assumes that all pkeys other than 'pkey' are unallocated */ -void test_pkey_syscalls_on_non_allocated_pkey(int *ptr, u16 pkey) +static void test_pkey_syscalls_on_non_allocated_pkey(int *ptr, u16 pkey) { int err; int i; @@ -1143,7 +1143,7 @@ void test_pkey_syscalls_on_non_allocated_pkey(int *ptr, u16 pkey) }
/* Assumes that all pkeys other than 'pkey' are unallocated */ -void test_pkey_syscalls_bad_args(int *ptr, u16 pkey) +static void test_pkey_syscalls_bad_args(int *ptr, u16 pkey) { int err; int bad_pkey = NR_PKEYS+99; @@ -1153,7 +1153,7 @@ void test_pkey_syscalls_bad_args(int *ptr, u16 pkey) pkey_assert(err); }
-void become_child(void) +static void become_child(void) { pid_t forkret;
@@ -1169,7 +1169,7 @@ void become_child(void) }
/* Assumes that all pkeys other than 'pkey' are unallocated */ -void test_pkey_alloc_exhaust(int *ptr, u16 pkey) +static void test_pkey_alloc_exhaust(int *ptr, u16 pkey) { int err; int allocated_pkeys[NR_PKEYS] = {0}; @@ -1236,7 +1236,7 @@ void test_pkey_alloc_exhaust(int *ptr, u16 pkey) } }
-void arch_force_pkey_reg_init(void) +static void arch_force_pkey_reg_init(void) { #if defined(__i386__) || defined(__x86_64__) /* arch */ u64 *buf; @@ -1275,7 +1275,7 @@ void arch_force_pkey_reg_init(void) * a long-running test that continually checks the pkey * register. */ -void test_pkey_init_state(int *ptr, u16 pkey) +static void test_pkey_init_state(int *ptr, u16 pkey) { int err; int allocated_pkeys[NR_PKEYS] = {0}; @@ -1313,7 +1313,7 @@ void test_pkey_init_state(int *ptr, u16 pkey) * have to call pkey_alloc() to use it first. Make sure that it * is usable. */ -void test_mprotect_with_pkey_0(int *ptr, u16 pkey) +static void test_mprotect_with_pkey_0(int *ptr, u16 pkey) { long size; int prot; @@ -1337,7 +1337,7 @@ void test_mprotect_with_pkey_0(int *ptr, u16 pkey) mprotect_pkey(ptr, size, prot, pkey); }
-void test_ptrace_of_child(int *ptr, u16 pkey) +static void test_ptrace_of_child(int *ptr, u16 pkey) { __attribute__((__unused__)) int peek_result; pid_t child_pid; @@ -1413,7 +1413,7 @@ void test_ptrace_of_child(int *ptr, u16 pkey) free(plain_ptr_unaligned); }
-void *get_pointer_to_instructions(void) +static void *get_pointer_to_instructions(void) { void *p1;
@@ -1434,7 +1434,7 @@ void *get_pointer_to_instructions(void) return p1; }
-void test_executing_on_unreadable_memory(int *ptr, u16 pkey) +static void test_executing_on_unreadable_memory(int *ptr, u16 pkey) { void *p1; int scratch; @@ -1466,7 +1466,7 @@ void test_executing_on_unreadable_memory(int *ptr, u16 pkey) pkey_assert(!ret); }
-void test_implicit_mprotect_exec_only_memory(int *ptr, u16 pkey) +static void test_implicit_mprotect_exec_only_memory(int *ptr, u16 pkey) { void *p1; int scratch; @@ -1515,7 +1515,7 @@ void test_implicit_mprotect_exec_only_memory(int *ptr, u16 pkey) }
#if defined(__i386__) || defined(__x86_64__) -void test_ptrace_modifies_pkru(int *ptr, u16 pkey) +static void test_ptrace_modifies_pkru(int *ptr, u16 pkey) { u32 new_pkru; pid_t child; @@ -1638,7 +1638,7 @@ void test_ptrace_modifies_pkru(int *ptr, u16 pkey) #endif
#if defined(__aarch64__) -void test_ptrace_modifies_pkru(int *ptr, u16 pkey) +static void test_ptrace_modifies_pkru(int *ptr, u16 pkey) { pid_t child; int status, ret; @@ -1715,7 +1715,7 @@ void test_ptrace_modifies_pkru(int *ptr, u16 pkey) } #endif
-void test_mprotect_pkey_on_unsupported_cpu(int *ptr, u16 pkey) +static void test_mprotect_pkey_on_unsupported_cpu(int *ptr, u16 pkey) { int size = PAGE_SIZE; int sret; @@ -1729,7 +1729,7 @@ void test_mprotect_pkey_on_unsupported_cpu(int *ptr, u16 pkey) pkey_assert(sret < 0); }
-void (*pkey_tests[])(int *ptr, u16 pkey) = { +static void (*pkey_tests[])(int *ptr, u16 pkey) = { test_read_of_write_disabled_region, test_read_of_access_disabled_region, test_read_of_access_disabled_region_with_page_already_mapped, @@ -1755,7 +1755,7 @@ void (*pkey_tests[])(int *ptr, u16 pkey) = { #endif };
-void run_tests_once(void) +static void run_tests_once(void) { int *ptr; int prot = PROT_READ|PROT_WRITE; @@ -1789,7 +1789,7 @@ void run_tests_once(void) iteration_nr++; }
-void pkey_setup_shadow(void) +static void pkey_setup_shadow(void) { shadow_pkey_reg = __read_pkey_reg(); }
sys_pkey_alloc, sys_pkey_free and sys_mprotect_pkey are currently used in protections_keys.c, while pkey_sighandler_tests.c calls the libc wrappers directly (e.g. pkey_mprotect()). This is probably ok when using glibc (those symbols appeared a while ago), but Musl does not currently provide them. The logging in the helpers from pkey-helpers.h can also come in handy.
Make things more consistent by using the sys_pkey helpers in pkey_sighandler_tests.c too. To that end their implementation is moved to a common .c file (pkey_util.c). This also enables calling is_pkeys_supported() outside of protections_keys.c, since it relies on sys_pkey_{alloc,free}.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- tools/testing/selftests/mm/Makefile | 4 +- tools/testing/selftests/mm/pkey-helpers.h | 2 + .../selftests/mm/pkey_sighandler_tests.c | 8 ++-- tools/testing/selftests/mm/pkey_util.c | 40 +++++++++++++++++++ tools/testing/selftests/mm/protection_keys.c | 35 ---------------- 5 files changed, 48 insertions(+), 41 deletions(-) create mode 100644 tools/testing/selftests/mm/pkey_util.c
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 814b17a43385..1f0743d9459d 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -147,8 +147,8 @@ TEST_FILES += write_hugetlb_memory.sh
include ../lib.mk
-$(TEST_GEN_PROGS): vm_util.c thp_settings.c -$(TEST_GEN_FILES): vm_util.c thp_settings.c +$(TEST_GEN_PROGS): vm_util.c thp_settings.c pkey_util.c +$(TEST_GEN_FILES): vm_util.c thp_settings.c pkey_util.c
$(OUTPUT)/uffd-stress: uffd-common.c $(OUTPUT)/uffd-unit-tests: uffd-common.c diff --git a/tools/testing/selftests/mm/pkey-helpers.h b/tools/testing/selftests/mm/pkey-helpers.h index 6f0ab7b42738..f080e97b39be 100644 --- a/tools/testing/selftests/mm/pkey-helpers.h +++ b/tools/testing/selftests/mm/pkey-helpers.h @@ -89,6 +89,8 @@ extern void abort_hooks(void);
int sys_pkey_alloc(unsigned long flags, unsigned long init_val); int sys_pkey_free(unsigned long pkey); +int sys_mprotect_pkey(void *ptr, size_t size, unsigned long orig_prot, + unsigned long pkey);
/* For functions called from protection_keys.c only */ noinline int read_ptr(int *ptr); diff --git a/tools/testing/selftests/mm/pkey_sighandler_tests.c b/tools/testing/selftests/mm/pkey_sighandler_tests.c index e1aaeb65cfae..c73cee192b88 100644 --- a/tools/testing/selftests/mm/pkey_sighandler_tests.c +++ b/tools/testing/selftests/mm/pkey_sighandler_tests.c @@ -311,8 +311,8 @@ static void test_sigsegv_handler_with_different_pkey_for_stack(void) __write_pkey_reg(pkey_reg);
/* Protect the new stack with MPK 1 */ - pkey = pkey_alloc(0, 0); - pkey_mprotect(stack, STACK_SIZE, PROT_READ | PROT_WRITE, pkey); + pkey = sys_pkey_alloc(0, 0); + sys_mprotect_pkey(stack, STACK_SIZE, PROT_READ | PROT_WRITE, pkey);
/* Set up alternate signal stack that will use the default MPK */ sigstack.ss_sp = mmap(0, STACK_SIZE, PROT_READ | PROT_WRITE | PROT_EXEC, @@ -484,8 +484,8 @@ static void test_pkru_sigreturn(void) __write_pkey_reg(pkey_reg);
/* Protect the stack with MPK 2 */ - pkey = pkey_alloc(0, 0); - pkey_mprotect(stack, STACK_SIZE, PROT_READ | PROT_WRITE, pkey); + pkey = sys_pkey_alloc(0, 0); + sys_mprotect_pkey(stack, STACK_SIZE, PROT_READ | PROT_WRITE, pkey);
/* Set up alternate signal stack that will use the default MPK */ sigstack.ss_sp = mmap(0, STACK_SIZE, PROT_READ | PROT_WRITE | PROT_EXEC, diff --git a/tools/testing/selftests/mm/pkey_util.c b/tools/testing/selftests/mm/pkey_util.c new file mode 100644 index 000000000000..ca4ad0d44ab2 --- /dev/null +++ b/tools/testing/selftests/mm/pkey_util.c @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: GPL-2.0-only +#include <sys/syscall.h> +#include <unistd.h> + +#include "pkey-helpers.h" + +int sys_pkey_alloc(unsigned long flags, unsigned long init_val) +{ + int ret = syscall(SYS_pkey_alloc, flags, init_val); + dprintf1("%s(flags=%lx, init_val=%lx) syscall ret: %d errno: %d\n", + __func__, flags, init_val, ret, errno); + return ret; +} + +int sys_pkey_free(unsigned long pkey) +{ + int ret = syscall(SYS_pkey_free, pkey); + dprintf1("%s(pkey=%ld) syscall ret: %d\n", __func__, pkey, ret); + return ret; +} + +int sys_mprotect_pkey(void *ptr, size_t size, unsigned long orig_prot, + unsigned long pkey) +{ + int sret; + + dprintf2("%s(0x%p, %zx, prot=%lx, pkey=%lx)\n", __func__, + ptr, size, orig_prot, pkey); + + errno = 0; + sret = syscall(__NR_pkey_mprotect, ptr, size, orig_prot, pkey); + if (errno) { + dprintf2("SYS_mprotect_key sret: %d\n", sret); + dprintf2("SYS_mprotect_key prot: 0x%lx\n", orig_prot); + dprintf2("SYS_mprotect_key failed, errno: %d\n", errno); + if (DEBUG_LEVEL >= 2) + perror("SYS_mprotect_pkey"); + } + return sret; +} diff --git a/tools/testing/selftests/mm/protection_keys.c b/tools/testing/selftests/mm/protection_keys.c index f43cf3b75d8e..3688571e6b39 100644 --- a/tools/testing/selftests/mm/protection_keys.c +++ b/tools/testing/selftests/mm/protection_keys.c @@ -460,34 +460,6 @@ static pid_t fork_lazy_child(void) return forkret; }
-int sys_mprotect_pkey(void *ptr, size_t size, unsigned long orig_prot, - unsigned long pkey) -{ - int sret; - - dprintf2("%s(0x%p, %zx, prot=%lx, pkey=%lx)\n", __func__, - ptr, size, orig_prot, pkey); - - errno = 0; - sret = syscall(__NR_pkey_mprotect, ptr, size, orig_prot, pkey); - if (errno) { - dprintf2("SYS_mprotect_key sret: %d\n", sret); - dprintf2("SYS_mprotect_key prot: 0x%lx\n", orig_prot); - dprintf2("SYS_mprotect_key failed, errno: %d\n", errno); - if (DEBUG_LEVEL >= 2) - perror("SYS_mprotect_pkey"); - } - return sret; -} - -int sys_pkey_alloc(unsigned long flags, unsigned long init_val) -{ - int ret = syscall(SYS_pkey_alloc, flags, init_val); - dprintf1("%s(flags=%lx, init_val=%lx) syscall ret: %d errno: %d\n", - __func__, flags, init_val, ret, errno); - return ret; -} - static int alloc_pkey(void) { int ret; @@ -534,13 +506,6 @@ static int alloc_pkey(void) return ret; }
-int sys_pkey_free(unsigned long pkey) -{ - int ret = syscall(SYS_pkey_free, pkey); - dprintf1("%s(pkey=%ld) syscall ret: %d\n", __func__, pkey, ret); - return ret; -} - /* * I had a bug where pkey bits could be set by mprotect() but * not cleared. This ensures we get lots of random bit sets
On Mon, Dec 09, 2024 at 09:50:16AM +0000, Kevin Brodsky wrote:
Hi Kevin, Andrew,
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 814b17a43385..1f0743d9459d 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -147,8 +147,8 @@ TEST_FILES += write_hugetlb_memory.sh include ../lib.mk -$(TEST_GEN_PROGS): vm_util.c thp_settings.c -$(TEST_GEN_FILES): vm_util.c thp_settings.c +$(TEST_GEN_PROGS): vm_util.c thp_settings.c pkey_util.c +$(TEST_GEN_FILES): vm_util.c thp_settings.c pkey_util.c
This patch breaks s390 in -next with the below error messages:
In file included from pkey_util.c:5: pkey-helpers.h:109:2: error: #error Architecture not supported 109 | #error Architecture not supported | ^~~~~ pkey-helpers.h: In function ‘set_pkey_bits’: pkey-helpers.h:126:21: error: implicit declaration of function ‘pkey_bit_position’ [-Wimplicit-function-declaration] 126 | u32 shift = pkey_bit_position(pkey); | ^~~~~~~~~~~~~~~~~ pkey-helpers.h: In function ‘_read_pkey_reg’: pkey-helpers.h:151:24: error: implicit declaration of function ‘__read_pkey_reg’; did you mean ‘_read_pkey_reg’? [-Wimplicit-function-declaration] 151 | u64 pkey_reg = __read_pkey_reg(); | ^~~~~~~~~~~~~~~ | _read_pkey_reg pkey-helpers.h: In function ‘write_pkey_reg’: pkey-helpers.h:165:18: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 3 has type ‘int’ [-Wformat=] 165 | dprintf4("%s() changing %016llx to %016llx\n", __func__, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 166 | __read_pkey_reg(), pkey_reg); | ~~~~~~~~~~~~~~~~~ | | | int pkey-helpers.h:62:32: note: in definition of macro ‘dprintf_level’ 62 | sigsafe_printf(args); \ | ^~~~ pkey-helpers.h:165:9: note: in expansion of macro ‘dprintf4’ 165 | dprintf4("%s() changing %016llx to %016llx\n", __func__, | ^~~~~~~~ pkey-helpers.h:165:39: note: format string is defined here 165 | dprintf4("%s() changing %016llx to %016llx\n", __func__, | ~~~~~~^ | | | long long unsigned int | %016x pkey-helpers.h:169:9: error: implicit declaration of function ‘__write_pkey_reg’; did you mean ‘write_pkey_reg’? [-Wimplicit-function-declaration] 169 | __write_pkey_reg(pkey_reg); | ^~~~~~~~~~~~~~~~ | write_pkey_reg pkey-helpers.h:171:18: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘int’ [-Wformat=] 171 | dprintf4("%s(%016llx) pkey_reg: %016llx\n", __func__, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 172 | pkey_reg, __read_pkey_reg()); | ~~~~~~~~~~~~~~~~~ | | | int pkey-helpers.h:62:32: note: in definition of macro ‘dprintf_level’ 62 | sigsafe_printf(args); \ | ^~~~ pkey-helpers.h:171:9: note: in expansion of macro ‘dprintf4’ 171 | dprintf4("%s(%016llx) pkey_reg: %016llx\n", __func__, | ^~~~~~~~ pkey-helpers.h:171:47: note: format string is defined here 171 | dprintf4("%s(%016llx) pkey_reg: %016llx\n", __func__, | ~~~~~~^ | | | long long unsigned int | %016x pkey-helpers.h: In function ‘is_pkeys_supported’: pkey-helpers.h:207:14: error: implicit declaration of function ‘cpu_has_pkeys’; did you mean ‘kernel_has_pkeys’? [-Wimplicit-function-declaration] 207 | if (!cpu_has_pkeys()) { | ^~~~~~~~~~~~~ | kernel_has_pkeys
Please, let me know if I am missing something.
$(OUTPUT)/uffd-stress: uffd-common.c $(OUTPUT)/uffd-unit-tests: uffd-common.c
Thanks!
The pkey* files can only be built on architectures that support pkeys (pkey-helpers.h #error's otherwise). Adding pkey_util.c as dependency to all $(TEST_GEN_FILES) is therefore a bad idea. Make it a dependency of the pkeys tests only.
Those tests are built in 32/64-bit variants on x86_64 so we need to add an explicit dependency there as well.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com ---
Hi Alexander,
Thank you for the bug report, that patch indeed breaks all architectures that don't support pkeys, I should have realised that! This patch should fix it.
Andrew, it would make sense to squash this patch into the original ("selftests/mm: Use sys_pkey helpers consistently").
Cheers, - Kevin
Cc: akpm@linux-foundation.org Cc: aruna.ramakrishna@oracle.com Cc: catalin.marinas@arm.com Cc: dave.hansen@linux.intel.com Cc: joey.gouly@arm.com Cc: keith.lucas@oracle.com Cc: ryan.roberts@arm.com Cc: shuah@kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kselftest@vger.kernel.org Cc: linux-mm@kvack.org Cc: x86@kernel.org Cc: linux-s390@vger.kernel.org --- tools/testing/selftests/mm/Makefile | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 1f0743d9459d..18041de1aebf 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -147,16 +147,20 @@ TEST_FILES += write_hugetlb_memory.sh
include ../lib.mk
-$(TEST_GEN_PROGS): vm_util.c thp_settings.c pkey_util.c -$(TEST_GEN_FILES): vm_util.c thp_settings.c pkey_util.c +$(TEST_GEN_PROGS): vm_util.c thp_settings.c +$(TEST_GEN_FILES): vm_util.c thp_settings.c
$(OUTPUT)/uffd-stress: uffd-common.c $(OUTPUT)/uffd-unit-tests: uffd-common.c +$(OUTPUT)/protection_keys: pkey_util.c +$(OUTPUT)/pkey_sighandler_tests: pkey_util.c
ifeq ($(ARCH),x86_64) BINARIES_32 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_32)) BINARIES_64 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_64))
+$(BINARIES_32) $(BINARIES_64): pkey_util.c + define gen-target-rule-32 $(1) $(1)_32: $(OUTPUT)/$(1)_32 .PHONY: $(1) $(1)_32
PKEY_ALLOW_ALL is meant to represent the pkey register value that allows all accesses (enables all pkeys). However its current naming suggests that the value applies to *one* key only (like PKEY_DISABLE_ACCESS for instance).
Rename PKEY_ALLOW_ALL to PKEY_REG_ALLOW_ALL to avoid such misunderstanding. This is consistent with the PKEY_REG_ALLOW_NONE macro introduced by commit 6e182dc9f268 ("selftests/mm: Use generic pkey register manipulation").
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- tools/testing/selftests/mm/pkey-arm64.h | 2 +- tools/testing/selftests/mm/protection_keys.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/mm/pkey-arm64.h b/tools/testing/selftests/mm/pkey-arm64.h index 9897e31f16dd..8e9685e03c44 100644 --- a/tools/testing/selftests/mm/pkey-arm64.h +++ b/tools/testing/selftests/mm/pkey-arm64.h @@ -30,7 +30,7 @@ #define NR_PKEYS 8 #define NR_RESERVED_PKEYS 1 /* pkey-0 */
-#define PKEY_ALLOW_ALL 0x77777777 +#define PKEY_REG_ALLOW_ALL 0x77777777 #define PKEY_REG_ALLOW_NONE 0x0
#define PKEY_BITS_PER_PKEY 4 diff --git a/tools/testing/selftests/mm/protection_keys.c b/tools/testing/selftests/mm/protection_keys.c index 3688571e6b39..a4683f2476f2 100644 --- a/tools/testing/selftests/mm/protection_keys.c +++ b/tools/testing/selftests/mm/protection_keys.c @@ -396,7 +396,7 @@ static void signal_handler(int signum, siginfo_t *si, void *vucontext) /* restore access and let the faulting instruction continue */ pkey_access_allow(siginfo_pkey); #elif defined(__aarch64__) - aarch64_write_signal_pkey(uctxt, PKEY_ALLOW_ALL); + aarch64_write_signal_pkey(uctxt, PKEY_REG_ALLOW_ALL); #endif /* arch */ pkey_faults++; dprintf1("<<<<==================================================\n"); @@ -842,7 +842,7 @@ void expected_pkey_fault(int pkey) */ if (__read_pkey_reg() != 0) #elif defined(__aarch64__) - if (__read_pkey_reg() != PKEY_ALLOW_ALL) + if (__read_pkey_reg() != PKEY_REG_ALLOW_ALL) #else if (__read_pkey_reg() != shadow_pkey_reg) #endif /* arch */
The pkey_sighandler_tests are bound to fail if either the kernel or CPU doesn't support pkeys. Skip the tests if pkeys support is missing.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- tools/testing/selftests/mm/pkey_sighandler_tests.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/mm/pkey_sighandler_tests.c b/tools/testing/selftests/mm/pkey_sighandler_tests.c index c73cee192b88..449ec5acec75 100644 --- a/tools/testing/selftests/mm/pkey_sighandler_tests.c +++ b/tools/testing/selftests/mm/pkey_sighandler_tests.c @@ -535,6 +535,9 @@ int main(int argc, char *argv[]) ksft_print_header(); ksft_set_plan(ARRAY_SIZE(pkey_tests));
+ if (!is_pkeys_supported()) + ksft_exit_skip("pkeys not supported\n"); + for (i = 0; i < ARRAY_SIZE(pkey_tests); i++) (*pkey_tests[i])();
There is no reason why the alternate signal stack should be mapped as RWX. Map it as RW instead.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- tools/testing/selftests/mm/pkey_sighandler_tests.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/mm/pkey_sighandler_tests.c b/tools/testing/selftests/mm/pkey_sighandler_tests.c index 449ec5acec75..17bbfcd552c6 100644 --- a/tools/testing/selftests/mm/pkey_sighandler_tests.c +++ b/tools/testing/selftests/mm/pkey_sighandler_tests.c @@ -315,7 +315,7 @@ static void test_sigsegv_handler_with_different_pkey_for_stack(void) sys_mprotect_pkey(stack, STACK_SIZE, PROT_READ | PROT_WRITE, pkey);
/* Set up alternate signal stack that will use the default MPK */ - sigstack.ss_sp = mmap(0, STACK_SIZE, PROT_READ | PROT_WRITE | PROT_EXEC, + sigstack.ss_sp = mmap(0, STACK_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); sigstack.ss_flags = 0; sigstack.ss_size = STACK_SIZE; @@ -488,7 +488,7 @@ static void test_pkru_sigreturn(void) sys_mprotect_pkey(stack, STACK_SIZE, PROT_READ | PROT_WRITE, pkey);
/* Set up alternate signal stack that will use the default MPK */ - sigstack.ss_sp = mmap(0, STACK_SIZE, PROT_READ | PROT_WRITE | PROT_EXEC, + sigstack.ss_sp = mmap(0, STACK_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); sigstack.ss_flags = 0; sigstack.ss_size = STACK_SIZE;
linux-kselftest-mirror@lists.linaro.org