selftests, powerpc, x86 : Memory Protection Keys
Subsystems (4)
v14 cover selftests, powerpc, x86 : Memory Protection Keys
Memory protection keys enables an application to protect its address space from inadvertent access by its own code.
This feature is now enabled on powerpc architecture and integrated in 4.16-rc1. The patches move the selftests to arch neutral directory and enhance their test coverage.
Test
----
Verified for correctness on powerpc and on x86.
History:
-------
version 14:
(1) incorporated another round of comments from Dave Hansen.
version 13:
(1) Incorporated comments for Dave Hansen.
(2) Added one more test for correct pkey-0 behavior.
version 12:
(1) fixed the offset of pkey field in the siginfo structure for x86_64 and powerpc. And tries to use the actual field if the headers have it defined.
version 11:
(1) fixed a deadlock in the ptrace testcase.
version 10 and prior:
(1) moved the testcase to arch neutral directory
(2) split the changes into incremental patches.
Ram Pai (20):
selftests/x86: Move protecton key selftest to arch neutral directory selftests/vm: rename all references to pkru to a generic name selftests/vm: move generic definitions to header file selftests/vm: typecast the pkey register
selftests/vm: generic function to handle shadow key register selftests/vm: fix the wrong assert in pkey_disable_set() selftests/vm: fixed bugs in pkey_disable_clear() selftests/vm: fix alloc_random_pkey() to make it really random selftests/vm: introduce two arch independent abstraction selftests/vm: pkey register should match shadow pkey selftests/vm: generic cleanup
selftests/vm: Introduce generic abstractions selftests/vm: powerpc implementation to check support for pkey selftests/vm: fix an assertion in test_pkey_alloc_exhaust() selftests/vm: associate key on a mapped page and detect access
violation
selftests/vm: associate key on a mapped page and detect write
violation
selftests/vm: detect write violation on a mapped access-denied-key
page
selftests/vm: testcases must restore pkey-permissions selftests/vm: sub-page allocator
selftests/vm: test correct behavior of pkey-0
Thiago Jung Bauermann (2):
selftests/vm: move arch-specific definitions to arch-specific header selftests/vm: Make gcc check arguments of sigsafe_printf()
tools/testing/selftests/vm/.gitignore | 1 + tools/testing/selftests/vm/Makefile | 1 + tools/testing/selftests/vm/pkey-helpers.h | 214 ++++ tools/testing/selftests/vm/pkey-powerpc.h | 128 ++ tools/testing/selftests/vm/pkey-x86.h | 184 +++ tools/testing/selftests/vm/protection_keys.c | 1593 +++++++++++++++++++++++++ tools/testing/selftests/x86/.gitignore | 1 - tools/testing/selftests/x86/pkey-helpers.h | 219 ---- tools/testing/selftests/x86/protection_keys.c | 1485 ----------------------- 9 files changed, 2121 insertions(+), 1705 deletions(-) create mode 100644 tools/testing/selftests/vm/pkey-helpers.h create mode 100644 tools/testing/selftests/vm/pkey-powerpc.h create mode 100644 tools/testing/selftests/vm/pkey-x86.h create mode 100644 tools/testing/selftests/vm/protection_keys.c delete mode 100644 tools/testing/selftests/x86/pkey-helpers.h delete mode 100644 tools/testing/selftests/x86/protection_keys.c
PATCH v14 1/22 selftests/x86: Move protecton key selftest to arch neutral directory
--- tools/testing/selftests/vm/.gitignore | 1 + tools/testing/selftests/vm/Makefile | 1 + tools/testing/selftests/vm/pkey-helpers.h | 219 ++++ tools/testing/selftests/vm/protection_keys.c | 1485 +++++++++++++++++++++++++ tools/testing/selftests/x86/.gitignore | 1 - tools/testing/selftests/x86/pkey-helpers.h | 219 ---- tools/testing/selftests/x86/protection_keys.c | 1485 ------------------------- 7 files changed, 1706 insertions(+), 1705 deletions(-) create mode 100644 tools/testing/selftests/vm/pkey-helpers.h create mode 100644 tools/testing/selftests/vm/protection_keys.c delete mode 100644 tools/testing/selftests/x86/pkey-helpers.h delete mode 100644 tools/testing/selftests/x86/protection_keys.c
tools/testing/selftests/vm/.gitignore +1 -0
@@ -12,3 +12,4 @@ mlock-random-test virtual_address_range gup_benchmark va_128TBswitch +protection_keys
tools/testing/selftests/vm/Makefile +1 -0
@@ -20,6 +20,7 @@ TEST_GEN_FILES += transhuge-stress TEST_GEN_FILES += userfaultfd TEST_GEN_FILES += va_128TBswitch TEST_GEN_FILES += virtual_address_range +TEST_GEN_FILES += protection_keys TEST_PROGS := run_vmtests
tools/testing/selftests/vm/pkey-helpers.h new file +219 -0
219 lines not on this page · raw message
tools/testing/selftests/vm/protection_keys.c new file +1485 -0
1,485 lines not on this page · raw message
tools/testing/selftests/x86/.gitignore +0 -1
@@ -11,5 +11,4 @@ ldt_gdt iopl mpx-mini-test ioperm -protection_keys test_vdso
tools/testing/selftests/x86/pkey-helpers.h deleted +0 -219
219 lines not on this page · raw message
tools/testing/selftests/x86/protection_keys.c deleted +0 -1485
1,061 lines not on this page · raw message
The rest of this diff is not shown. View the raw message.
Signature · 1 line
1.7.1
Part of the diff is not shown here. View the raw message.
Re: [PATCH v14 01/22] selftests/x86: Move protecton key selftest to arch neutral directory
PATCH v14 2/22 selftests/vm: rename all references to pkru to a generic name
In reply to Ram Paisome pkru references are named to pkey_reg
and some prku references are renamed to pkey
--- tools/testing/selftests/vm/pkey-helpers.h | 85 +++++----- tools/testing/selftests/vm/protection_keys.c | 238 ++++++++++++++------------ 2 files changed, 169 insertions(+), 154 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +44 -41
@@ -14,7 +14,7 @@#include <sys/mman.h> #define NR_PKEYS 16 -#define PKRU_BITS_PER_PKEY 2 +#define PKEY_BITS_PER_PKEY 2#ifndef DEBUG_LEVEL #define DEBUG_LEVEL 0
@@ -53,85 +53,88 @@ static inline void sigsafe_printf(const char *format, ...) #define dprintf3(args...) dprintf_level(3, args) #define dprintf4(args...) dprintf_level(4, args) -extern unsigned int shadow_pkru; -static inline unsigned int __rdpkru (void ) +extern unsigned int shadow_pkey_reg; +static inline unsigned int __read_pkey_reg (void ) {unsigned int eax, edx;unsigned int ecx =0 ; -unsigned int pkru; +unsigned int pkey_reg; asmvolatile (".byte 0x0f,0x01,0xee\n\t" :"=a" (eax),"=d" (edx) :"c" (ecx)); - pkru = eax; -return pkru; + pkey_reg = eax; +return pkey_reg; } -static inline unsigned int _rdpkru (int line) +static inline unsigned int _read_pkey_reg (int line) { -unsigned int pkru =__rdpkru (); +unsigned int pkey_reg =__read_pkey_reg (); -dprintf4 ("rdpkru(line=%d) pkru: %x shadow: %x\n" , - line, pkru, shadow_pkru); -assert (pkru == shadow_pkru); +dprintf4 ("read_pkey_reg(line=%d) pkey_reg: %x shadow: %x\n" , + line, pkey_reg, shadow_pkey_reg); +assert (pkey_reg == shadow_pkey_reg); -return pkru; +return pkey_reg; } -#define rdpkru () _rdpkru(__LINE__) +#define read_pkey_reg () _read_pkey_reg(__LINE__) -static inline void __wrpkru (unsigned int pkru) +static inline void __write_pkey_reg (unsigned int pkey_reg) { -unsigned int eax = pkru; +unsigned int eax = pkey_reg;unsigned int ecx =0 ;unsigned int edx =0 ; -dprintf4 ("%s() changing %08x to %08x\n" , __func__,__rdpkru (), pkru); +dprintf4 ("%s() changing %08x to %08x\n" , __func__, +__read_pkey_reg (), pkey_reg); asmvolatile (".byte 0x0f,0x01,0xef\n\t" : :"a" (eax),"c" (ecx),"d" (edx)); -assert (pkru ==__rdpkru ()); +assert (pkey_reg ==__read_pkey_reg ()); } -static inline void wrpkru (unsigned int pkru) +static inline void write_pkey_reg (unsigned int pkey_reg) { -dprintf4 ("%s() changing %08x to %08x\n" , __func__,__rdpkru (), pkru); +dprintf4 ("%s() changing %08x to %08x\n" , __func__, +__read_pkey_reg (), pkey_reg);/* will do the shadow check for us: */ -rdpkru (); -__wrpkru (pkru); - shadow_pkru = pkru; -dprintf4 ("%s(%08x) pkru: %08x\n" , __func__, pkru,__rdpkru ()); +read_pkey_reg (); +__write_pkey_reg (pkey_reg); + shadow_pkey_reg = pkey_reg; +dprintf4 ("%s(%08x) pkey_reg: %08x\n" , __func__, + pkey_reg,__read_pkey_reg ()); }/* * These are technically racy. since something could -* change PKRU between the read and the write. +* change PKEY register between the read and the write. */ static inline void __pkey_access_allow (int pkey,int do_allow) { -unsigned int pkru =rdpkru (); +unsigned int pkey_reg =read_pkey_reg ();int bit = pkey *2 ;if (do_allow) - pkru &= (1 <<bit); + pkey_reg &= (1 <<bit);else - pkru |= (1 <<bit); + pkey_reg |= (1 <<bit); -dprintf4 ("pkru now: %08x\n" ,rdpkru ()); -wrpkru (pkru); +dprintf4 ("pkey_reg now: %08x\n" ,read_pkey_reg ()); +write_pkey_reg (pkey_reg); }static inline void __pkey_write_allow (int pkey,int do_allow_write) { -long pkru =rdpkru (); +long pkey_reg =read_pkey_reg ();int bit = pkey *2 +1 ;if (do_allow_write) - pkru &= (1 <<bit); + pkey_reg &= (1 <<bit);else - pkru |= (1 <<bit); + pkey_reg |= (1 <<bit); -wrpkru (pkru); -dprintf4 ("pkru now: %08x\n" ,rdpkru ()); +write_pkey_reg (pkey_reg); +dprintf4 ("pkey_reg now: %08x\n" ,read_pkey_reg ()); }#define PROT_PKEY0 0x10/* protection key value (bit 0) */
@@ -181,10 +184,10 @@ static inline int cpu_has_pku(void)return 1 ; } -#define XSTATE_PKRU_BIT (9) -#define XSTATE_PKRU 0x200 +#define XSTATE_PKEY_BIT (9) +#define XSTATE_PKEY 0x200 -int pkru_xstate_offset (void ) +int pkey_reg_xstate_offset (void ) {unsigned int eax;unsigned int ebx;
@@ -195,21 +198,21 @@ int pkru_xstate_offset(void)unsigned long XSTATE_CPUID =0xd ;int leaf; -/* assume that XSTATE_PKRU is set in XCR0 */ - leaf =XSTATE_PKRU_BIT ; +/* assume that XSTATE_PKEY is set in XCR0 */ + leaf =XSTATE_PKEY_BIT ; { eax =XSTATE_CPUID ; ecx = leaf;__cpuid (&eax, &ebx, &ecx, &edx); -if (leaf ==XSTATE_PKRU_BIT ) { +if (leaf ==XSTATE_PKEY_BIT ) { xstate_offset = ebx; xstate_size = eax; } }if (xstate_size ==0 ) { -printf ("could not find size/offset of PKRU in xsave state\n" ); +printf ("could not find size/offset of PKEY in xsave state\n" );return 0 ; }
tools/testing/selftests/vm/protection_keys.c +125 -113
527 lines not on this page · raw message
Signature · 1 line
1.7.1
PATCH v14 3/22 selftests/vm: move generic definitions to header file
In reply to Ram PaiMoved all the generic definition and helper functions to the header file.
--- tools/testing/selftests/vm/pkey-helpers.h | 35 ++++++++++++++++++++++--- tools/testing/selftests/vm/protection_keys.c | 27 -------------------- 2 files changed, 30 insertions(+), 32 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +30 -5
@@ -13,6 +13,14 @@#include <ucontext.h> #include <sys/mman.h> +/* Define some kernel-like types */ +#define u8 uint8_t +#define u16 uint16_t +#define u32 uint32_t +#define u64 uint64_t + +#define PTR_ERR_ENOTSUP ((void *)-ENOTSUP) +#define NR_PKEYS 16#define PKEY_BITS_PER_PKEY 2
@@ -53,6 +61,18 @@ static inline void sigsafe_printf(const char *format, ...) #define dprintf3(args...) dprintf_level(3, args) #define dprintf4(args...) dprintf_level(4, args) +extern void abort_hooks (void ); +#define pkey_assert (condition) do { \ + if (!(condition)) { \ + dprintf0("assert() at %s::%d test_nr: %d iteration: %d\n", \ + __FILE__, __LINE__, \ + test_nr, iteration_nr); \ + dprintf0("errno at assert: %d", errno); \ + abort_hooks(); \ + exit(__LINE__); \ + } \ +} while (0) +extern unsigned int shadow_pkey_reg;static inline unsigned int __read_pkey_reg (void ) {
@@ -137,11 +157,6 @@ static inline void __pkey_write_allow(int pkey, int do_allow_write)dprintf4 ("pkey_reg now: %08x\n" ,read_pkey_reg ()); } -#define PROT_PKEY0 0x10/* protection key value (bit 0) */ -#define PROT_PKEY1 0x20/* protection key value (bit 1) */ -#define PROT_PKEY2 0x40/* protection key value (bit 2) */ -#define PROT_PKEY3 0x80/* protection key value (bit 3) */ -#define PAGE_SIZE 4096#define MB (1<<20)
@@ -219,4 +234,14 @@ int pkey_reg_xstate_offset(void)return xstate_offset; } +#define ARRAY_SIZE (x) (sizeof(x) / sizeof(*(x))) +#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) \ + ((typeof(p))ALIGN_UP((unsigned long)(p), ptr_align_to)) +#define ALIGN_PTR_DOWN (p, ptr_align_to) \ + ((typeof(p))ALIGN_DOWN((unsigned long)(p), ptr_align_to)) +#define __stringify_1(x...) #x +#define __stringify(x...) __stringify_1(x) +#endif /* _PKEYS_HELPER_H */
tools/testing/selftests/vm/protection_keys.c +0 -27
@@ -51,31 +51,10 @@unsigned int shadow_pkey_reg;#define HPAGE_SIZE (1UL<<21) -#define ARRAY_SIZE (x) (sizeof(x) / sizeof(*(x))) -#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) ((typeof(p))ALIGN_UP((unsigned long)(p), ptr_align_to)) -#define ALIGN_PTR_DOWN (p, ptr_align_to) ((typeof(p))ALIGN_DOWN((unsigned long)(p), ptr_align_to)) -#define __stringify_1(x...) #x -#define __stringify(x...) __stringify_1(x) - -#define PTR_ERR_ENOTSUP ((void *)-ENOTSUP)int dprint_in_signal;char dprint_in_signal_buffer[DPRINT_IN_SIGNAL_BUF_SIZE ]; -extern void abort_hooks (void ); -#define pkey_assert (condition) do { \ - if (!(condition)) { \ - dprintf0("assert() at %s::%d test_nr: %d iteration: %d\n", \ - __FILE__, __LINE__, \ - test_nr, iteration_nr); \ - dprintf0("errno at assert: %d", errno); \ - abort_hooks(); \ - exit(__LINE__); \ - } \ -} while (0) -void cat_into_file (char *str,char *file) {int fd =open (file,O_RDWR );
@@ -186,12 +165,6 @@ void lots_o_noops_around_write(int *write_to_me)dprintf3 ("%s() done\n" , __func__); } -/* Define some kernel-like types */ -#define u8 uint8_t -#define u16 uint16_t -#define u32 uint32_t -#define u64 uint64_t -#ifdef __i386__#ifndef SYS_mprotect_key
Signature · 1 line
1.7.1
Re: [PATCH v14 03/22] selftests/vm: move generic definitions to header file
PATCH v14 4/22 selftests/vm: move arch-specific definitions to arch-specific header
In reply to Ram PaiFrom: Thiago Jung Bauermann <[email protected]>
In preparation for multi-arch support, move definitions which have arch-specific values to x86-specific header.
--- tools/testing/selftests/vm/pkey-helpers.h | 111 +----------------- tools/testing/selftests/vm/pkey-x86.h | 156 ++++++++++++++++++++++++++ tools/testing/selftests/vm/protection_keys.c | 47 -------- 3 files changed, 162 insertions(+), 152 deletions(-) create mode 100644 tools/testing/selftests/vm/pkey-x86.h
tools/testing/selftests/vm/pkey-helpers.h +6 -105
@@ -21,9 +21,6 @@#define PTR_ERR_ENOTSUP ((void *)-ENOTSUP) -#define NR_PKEYS 16 -#define PKEY_BITS_PER_PKEY 2 -#ifndef DEBUG_LEVEL #define DEBUG_LEVEL 0#endif
@@ -73,19 +70,13 @@ static inline void sigsafe_printf(const char *format, ...) } \ } while (0) +#if defined(__i386__) || defined(__x86_64__)/* arch */ +#include "pkey-x86.h" +#else /* arch */ +#error Architecture not supported +#endif /* arch */ +extern unsigned int shadow_pkey_reg; -static inline unsigned int __read_pkey_reg (void ) -{ -unsigned int eax, edx; -unsigned int ecx =0 ; -unsigned int pkey_reg; - - asmvolatile (".byte 0x0f,0x01,0xee\n\t" - :"=a" (eax),"=d" (edx) - :"c" (ecx)); - pkey_reg = eax; -return pkey_reg; -}static inline unsigned int _read_pkey_reg (int line) {
@@ -100,19 +91,6 @@ static inline unsigned int _read_pkey_reg(int line)#define read_pkey_reg () _read_pkey_reg(__LINE__) -static inline void __write_pkey_reg (unsigned int pkey_reg) -{ -unsigned int eax = pkey_reg; -unsigned int ecx =0 ; -unsigned int edx =0 ; - -dprintf4 ("%s() changing %08x to %08x\n" , __func__, -__read_pkey_reg (), pkey_reg); - asmvolatile (".byte 0x0f,0x01,0xef\n\t" - : :"a" (eax),"c" (ecx),"d" (edx)); -assert (pkey_reg ==__read_pkey_reg ()); -} -static inline void write_pkey_reg (unsigned int pkey_reg) { dprintf4("%s() changing %08x to %08x\n", __func__,
@@ -157,83 +135,6 @@ static inline void __pkey_write_allow(int pkey, int do_allow_write)dprintf4 ("pkey_reg now: %08x\n" ,read_pkey_reg ()); } -#define PAGE_SIZE 4096 -#define MB (1<<20) - -static inline void __cpuid (unsigned int *eax,unsigned int *ebx, -unsigned int *ecx,unsigned int *edx) -{ -/* ecx is often an input as well as an output. */ - asmvolatile ( -"cpuid;" - :"=a" (*eax), -"=b" (*ebx), -"=c" (*ecx), -"=d" (*edx) - :"0" (*eax),"2" (*ecx)); -} - -/* Intel-defined CPU features, CPUID level 0x00000007:0 (ecx) */ -#define X86_FEATURE_PKU (1<<3)/* Protection Keys for Userspace */ -#define X86_FEATURE_OSPKE (1<<4)/* OS Protection Keys Enable */ - -static inline int cpu_has_pku (void ) -{ -unsigned int eax; -unsigned int ebx; -unsigned int ecx; -unsigned int edx; - - eax =0x7 ; - ecx =0x0 ; -__cpuid (&eax, &ebx, &ecx, &edx); - -if (!(ecx &X86_FEATURE_PKU )) { -dprintf2 ("cpu does not have PKU\n" ); -return 0 ; - } -if (!(ecx &X86_FEATURE_OSPKE )) { -dprintf2 ("cpu does not have OSPKE\n" ); -return 0 ; - } -return 1 ; -} - -#define XSTATE_PKEY_BIT (9) -#define XSTATE_PKEY 0x200 - -int pkey_reg_xstate_offset (void ) -{ -unsigned int eax; -unsigned int ebx; -unsigned int ecx; -unsigned int edx; -int xstate_offset; -int xstate_size; -unsigned long XSTATE_CPUID =0xd ; -int leaf; - -/* assume that XSTATE_PKEY is set in XCR0 */ - leaf =XSTATE_PKEY_BIT ; - { - eax =XSTATE_CPUID ; - ecx = leaf; -__cpuid (&eax, &ebx, &ecx, &edx); - -if (leaf ==XSTATE_PKEY_BIT ) { - xstate_offset = ebx; - xstate_size = eax; - } - } - -if (xstate_size ==0 ) { -printf ("could not find size/offset of PKEY in xsave state\n" ); -return 0 ; - } - -return xstate_offset; -} -#define ARRAY_SIZE (x) (sizeof(x) / sizeof(*(x)))#define ALIGN_UP (x, align_to) (((x) + ((align_to)-1)) & ~((align_to)-1))#define ALIGN_DOWN (x, align_to) ((x) & ~((align_to)-1))
tools/testing/selftests/vm/pkey-x86.h new file +156 -0
156 lines not on this page · raw message
tools/testing/selftests/vm/protection_keys.c +0 -47
71 lines not on this page · raw message
Signature · 1 line
1.7.1
Re: [PATCH v14 04/22] selftests/vm: move arch-specific definitions to arch-specific header
Quoted 2 lines · Ram Pai
In preparation for multi-arch support, move definitions which have arch-specific values to x86-specific header.
PATCH v14 5/22 selftests/vm: Make gcc check arguments of sigsafe_printf()
In reply to Ram PaiFrom: Thiago Jung Bauermann <[email protected]>
This will help us ensure we print pkey_reg_t values correctly in different architectures.
--- tools/testing/selftests/vm/pkey-helpers.h | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +4 -0
@@ -27,6 +27,10 @@#define DPRINT_IN_SIGNAL_BUF_SIZE 4096extern int dprint_in_signal;extern char dprint_in_signal_buffer[DPRINT_IN_SIGNAL_BUF_SIZE ]; + +#ifdef __GNUC__ +__attribute__((format(printf, 1, 2))) +#endif static inline void sigsafe_printf (const char *format, ...) {va_list ap;
Signature · 1 line
1.7.1
PATCH v14 6/22 selftests/vm: typecast the pkey register
In reply to Ram PaiThis is in preparation to accomadate a differing size register across architectures.
--- tools/testing/selftests/vm/pkey-helpers.h | 23 ++++--- tools/testing/selftests/vm/pkey-x86.h | 16 +++-- tools/testing/selftests/vm/protection_keys.c | 87 +++++++++++++++---------- 3 files changed, 73 insertions(+), 53 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +12 -11
@@ -80,13 +80,14 @@ static inline void sigsafe_printf(const char *format, ...)#error Architecture not supported#endif /* arch */ -extern unsigned int shadow_pkey_reg; +extern pkey_reg_t shadow_pkey_reg; -static inline unsigned int _read_pkey_reg (int line) +static inline pkey_reg_t _read_pkey_reg (int line) { -unsigned int pkey_reg =__read_pkey_reg (); +pkey_reg_t pkey_reg =__read_pkey_reg (); -dprintf4 ("read_pkey_reg(line=%d) pkey_reg: %x shadow: %x\n" , +dprintf4 ("read_pkey_reg(line=%d) pkey_reg: " PKEY_REG_FMT +" shadow: " PKEY_REG_FMT "\n" , line, pkey_reg, shadow_pkey_reg);assert (pkey_reg == shadow_pkey_reg);
@@ -95,15 +96,15 @@ static inline unsigned int _read_pkey_reg(int line)#define read_pkey_reg () _read_pkey_reg(__LINE__) -static inline void write_pkey_reg (unsigned int pkey_reg) +static inline void write_pkey_reg (pkey_reg_t pkey_reg) { -dprintf4 ("%s() changing %08x to %08x\n" , __func__, +dprintf4 ("%s() changing " PKEY_REG_FMT " to " PKEY_REG_FMT "\n" , __func__,__read_pkey_reg (), pkey_reg);/* will do the shadow check for us: */ read_pkey_reg ();__write_pkey_reg (pkey_reg); shadow_pkey_reg = pkey_reg; -dprintf4 ("%s(%08x) pkey_reg: %08x\n" , __func__, +dprintf4 ("%s(" PKEY_REG_FMT ") pkey_reg: " PKEY_REG_FMT "\n" , __func__, pkey_reg,__read_pkey_reg ()); }
@@ -113,7 +114,7 @@ static inline void write_pkey_reg(unsigned int pkey_reg)*/ static inline void __pkey_access_allow (int pkey,int do_allow) { -unsigned int pkey_reg =read_pkey_reg (); +pkey_reg_t pkey_reg =read_pkey_reg ();int bit = pkey *2 ;if (do_allow)
@@ -121,13 +122,13 @@ static inline void __pkey_access_allow(int pkey, int do_allow)else pkey_reg |= (1 <<bit); -dprintf4 ("pkey_reg now: %08x\n" ,read_pkey_reg ()); +dprintf4 ("pkey_reg now: " PKEY_REG_FMT "\n" ,read_pkey_reg ());write_pkey_reg (pkey_reg); }static inline void __pkey_write_allow (int pkey,int do_allow_write) { -long pkey_reg =read_pkey_reg (); +pkey_reg_t pkey_reg =read_pkey_reg ();int bit = pkey *2 +1 ;if (do_allow_write)
@@ -136,7 +137,7 @@ static inline void __pkey_write_allow(int pkey, int do_allow_write) pkey_reg |= (1 <<bit);write_pkey_reg (pkey_reg); -dprintf4 ("pkey_reg now: %08x\n" ,read_pkey_reg ()); +dprintf4 ("pkey_reg now: " PKEY_REG_FMT "\n" ,read_pkey_reg ()); }#define ARRAY_SIZE (x) (sizeof(x) / sizeof(*(x)))
tools/testing/selftests/vm/pkey-x86.h +9 -7
@@ -46,6 +46,8 @@#define HPAGE_SIZE (1UL<<21)#define PAGE_SIZE 4096#define MB (1<<20) +#define pkey_reg_t u32 +#define PKEY_REG_FMT "%016x"static inline void __page_o_noops (void ) {
@@ -53,11 +55,11 @@ static inline void __page_o_noops(void) asm(".rept 512 ; nopl 0x7eeeeeee(%eax) ; .endr" ); } -static inline unsigned int __read_pkey_reg (void ) +static inline pkey_reg_t __read_pkey_reg (void ) {unsigned int eax, edx;unsigned int ecx =0 ; -unsigned int pkey_reg; +pkey_reg_t pkey_reg; asmvolatile (".byte 0x0f,0x01,0xee\n\t" : "=a" (eax), "=d" (edx)
@@ -66,13 +68,13 @@ static inline unsigned int __read_pkey_reg(void)return pkey_reg; } -static inline void __write_pkey_reg (unsigned int pkey_reg) +static inline void __write_pkey_reg (pkey_reg_t pkey_reg) { -unsigned int eax = pkey_reg; -unsigned int ecx =0 ; -unsigned int edx =0 ; +pkey_reg_t eax = pkey_reg; +pkey_reg_t ecx =0 ; +pkey_reg_t edx =0 ; -dprintf4 ("%s() changing %08x to %08x\n" , __func__, +dprintf4 ("%s() changing " PKEY_REG_FMT " to " PKEY_REG_FMT "\n" , __func__,__read_pkey_reg (), pkey_reg); asmvolatile (".byte 0x0f,0x01,0xef\n\t" : :"a" (eax),"c" (ecx),"d" (edx));
tools/testing/selftests/vm/protection_keys.c +52 -35
265 lines not on this page · raw message
Signature · 1 line
1.7.1
Re: [PATCH v14 06/22] selftests/vm: typecast the pkey register
Quoted 2 lines · Ram Pai
This is in preparation to accomadate a differing size register across architectures.
This is pretty fugly, and reading it again, I wonder whether we should have just made it 64-bit, at least in all the printk's. Or even
prink("pk reg: %*llx\n", PKEY_FMT_LEN, pkey_reg);But, I don't _really_ care in the end.
PATCH v14 7/22 selftests/vm: generic function to handle shadow key register
In reply to Ram Paihelper functions to handler shadow pkey register
--- tools/testing/selftests/vm/pkey-helpers.h | 29 ++++++++++++++++++++++ tools/testing/selftests/vm/pkey-x86.h | 5 ++++ tools/testing/selftests/vm/protection_keys.c | 34 ++++++++++++++++--------- 3 files changed, 56 insertions(+), 12 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +29 -0
@@ -80,6 +80,35 @@ static inline void sigsafe_printf(const char *format, ...)#error Architecture not supported#endif /* arch */ +static inline pkey_reg_t clear_pkey_flags (int pkey,pkey_reg_t flags) +{ +u32 shift =pkey_bit_position (pkey); + +return ~(flags << shift); +} + +/* +* Takes pkey flags and puts them at the right bit position for the given key so +* that the result can be ORed into the register. +*/ +static inline pkey_reg_t left_shift_bits (int pkey,pkey_reg_t bits) +{ +u32 shift =pkey_bit_position (pkey); + +return (bits << shift); +} + +/* +* Takes pkey register values and puts the flags for the given pkey at the least +* significant bits of the returned value. +*/ +static inline pkey_reg_t right_shift_bits (int pkey,pkey_reg_t bits) +{ +u32 shift =pkey_bit_position (pkey); + +return (bits >> shift); +} +extern pkey_reg_t shadow_pkey_reg;static inline pkey_reg_t _read_pkey_reg (int line)
tools/testing/selftests/vm/pkey-x86.h +5 -0
@@ -49,6 +49,11 @@#define pkey_reg_t u32#define PKEY_REG_FMT "%016x" +static inline u32 pkey_bit_position (int pkey) +{ +return pkey *PKEY_BITS_PER_PKEY ; +} +static inline void __page_o_noops (void ) {/* 8-bytes of instruction * 512 bytes = 1 page */
tools/testing/selftests/vm/protection_keys.c +22 -12
@@ -342,7 +342,7 @@ static u32 hw_pkey_get(int pkey, unsigned long flags) __func__, pkey, flags, 0, 0);dprintf2 ("%s() raw pkey_reg: " PKEY_REG_FMT "\n" , __func__, pkey_reg); - shifted_pkey_reg = (pkey_reg >> (pkey *PKEY_BITS_PER_PKEY )); + shifted_pkey_reg =right_shift_bits (pkey, pkey_reg);dprintf2 ("%s() shifted_pkey_reg: " PKEY_REG_FMT "\n" , __func__, shifted_pkey_reg); masked_pkey_reg = shifted_pkey_reg & mask;
@@ -366,9 +366,9 @@ static int hw_pkey_set(int pkey, unsigned long rights, unsigned long flags)/* copy old pkey_reg */ new_pkey_reg = old_pkey_reg;/* mask out bits from pkey in old value: */ - new_pkey_reg &= ~(mask << (pkey *PKEY_BITS_PER_PKEY )); + new_pkey_reg &=clear_pkey_flags (pkey, mask);/* OR in new bits for pkey: */ - new_pkey_reg |= (rights << (pkey *PKEY_BITS_PER_PKEY )); + new_pkey_reg |=left_shift_bits (pkey, rights);__write_pkey_reg (new_pkey_reg);
@@ -402,7 +402,7 @@ void pkey_disable_set(int pkey, int flags) ret =hw_pkey_set (pkey, pkey_rights, syscall_flags);assert (!ret);/*pkey_reg and flags have the same format */ - shadow_pkey_reg |= flags << (pkey *2 ); + shadow_pkey_reg |=left_shift_bits (pkey, flags);dprintf1 ("%s(%d) shadow: 0x" PKEY_REG_FMT "\n" , __func__, pkey, shadow_pkey_reg);
@@ -436,7 +436,7 @@ void pkey_disable_clear(int pkey, int flags) pkey_rights |= flags; ret =hw_pkey_set (pkey, pkey_rights,0 ); - shadow_pkey_reg &= ~(flags << (pkey *2 )); + shadow_pkey_reg &=clear_pkey_flags (pkey, flags);pkey_assert (ret >=0 ); pkey_rights =hw_pkey_get (pkey, syscall_flags);
@@ -494,6 +494,21 @@ int sys_pkey_alloc(unsigned long flags, unsigned long init_val)return ret; } +void pkey_setup_shadow (void ) +{ + shadow_pkey_reg =__read_pkey_reg (); +} + +void pkey_reset_shadow (u32 key) +{ + shadow_pkey_reg &=clear_pkey_flags (key,0x3 ); +} + +void pkey_set_shadow (u32 key,u64 init_val) +{ + shadow_pkey_reg |=left_shift_bits (key, init_val); +} +int alloc_pkey (void ) {int ret;
@@ -512,7 +527,7 @@ int alloc_pkey(void) shadow_pkey_reg);if (ret) {/* clear both the bits: */ - shadow_pkey_reg &= ~(0x3 << (ret *2 )); +pkey_reset_shadow (ret); dprintf4("%s()::%d, ret: %d pkey_reg: 0x"PKEY_REG_FMT " shadow: 0x"PKEY_REG_FMT"\n", __func__,
@@ -522,7 +537,7 @@ int alloc_pkey(void)* move the new state in from init_val * (remember, we cheated and init_val == pkey_reg format) */ - shadow_pkey_reg |= (init_val << (ret *2 )); +pkey_set_shadow (ret, init_val); } dprintf4("%s()::%d, ret: %d pkey_reg: 0x"PKEY_REG_FMT " shadow: 0x"PKEY_REG_FMT"\n",
@@ -1403,11 +1418,6 @@ void run_tests_once(void) iteration_nr++; } -void pkey_setup_shadow (void ) -{ - shadow_pkey_reg =__read_pkey_reg (); -} -int main (void ) {int nr_iterations =22 ;
Signature · 1 line
1.7.1
Re: [PATCH v14 07/22] selftests/vm: generic function to handle shadow key register
tools/testing/selftests/vm/protection_keys.c static u32 hw_pkey_get(int pkey, unsigned long flags) · line 348
Quoted 5 lines · Ram Pai
tools/testing/selftests/vm/protection_keys.c +1
-1- shifted_pkey_reg = (pkey_reg >> (pkey *PKEY_BITS_PER_PKEY )); + shifted_pkey_reg =right_shift_bits (pkey, pkey_reg);dprintf2 ("%s() shifted_pkey_reg: " PKEY_REG_FMT "\n" , __func__, shifted_pkey_reg); masked_pkey_reg = shifted_pkey_reg & mask;
I'm not a fan of how this looks. This is almost certainly going to get the argument order mixed up at some point.
Why do we need this? The description doesn't say.
PATCH v14 8/22 selftests/vm: fix the wrong assert in pkey_disable_set()
In reply to Ram PaiIf the flag is 0, no bits will be set. Hence we cant expect the resulting bitmap to have a higher value than what it was earlier.
--- tools/testing/selftests/vm/protection_keys.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
tools/testing/selftests/vm/protection_keys.c +1 -1
@@ -415,7 +415,7 @@ void pkey_disable_set(int pkey, int flags)dprintf1 ("%s(%d) pkey_reg: 0x" PKEY_REG_FMT "\n" , __func__, pkey,read_pkey_reg ());if (flags) -pkey_assert (read_pkey_reg () > orig_pkey_reg); +pkey_assert (read_pkey_reg () >= orig_pkey_reg);dprintf1 ("END<---%s(%d, 0x%x)\n" , __func__, pkey, flags); }
Signature · 1 line
1.7.1
Re: [PATCH v14 08/22] selftests/vm: fix the wrong assert in pkey_disable_set()
Quoted 3 lines · Ram Pai
If the flag is 0, no bits will be set. Hence we cant expect the resulting bitmap to have a higher value than what it was earlier.
...
tools/testing/selftests/vm/protection_keys.c void pkey_disable_set(int pkey, int flags) · line 421
Quoted 11 lines
tools/testing/selftests/vm/protection_keys.c +1
-1@@ -415,7 +415,7 @@ void pkey_disable_set(int pkey, int flags)dprintf1 ("%s(%d) pkey_reg: 0x" PKEY_REG_FMT "\n" , __func__, pkey,read_pkey_reg ());if (flags) -pkey_assert (read_pkey_reg () > orig_pkey_reg); +pkey_assert (read_pkey_reg () >= orig_pkey_reg);dprintf1 ("END<---%s(%d, 0x%x)\n" , __func__, pkey, flags); }
I know these are just selftests, but this change makes zero sense without the context from how powerpc works. It's also totally non-obvious from the patch itself what is going on, even though I specifically called this out in a previous review.
Please add a comment here that either specifically calls out powerpc or talks about "an architecture that does this ..."
PATCH v14 9/22 selftests/vm: fixed bugs in pkey_disable_clear()
In reply to Ram Paiinstead of clearing the bits, pkey_disable_clear() was setting the bits. Fixed it.
Also fixed a wrong assertion in that function. When bits are cleared, the resulting bit value will be less than the original.
This hasn't been a problem so far because this code isn't currently used.
--- tools/testing/selftests/vm/protection_keys.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
tools/testing/selftests/vm/protection_keys.c +2 -2
@@ -433,7 +433,7 @@ void pkey_disable_clear(int pkey, int flags) pkey, pkey, pkey_rights);pkey_assert (pkey_rights >=0 ); - pkey_rights |= flags; + pkey_rights &= ~flags; ret =hw_pkey_set (pkey, pkey_rights,0 ); shadow_pkey_reg &=clear_pkey_flags (pkey, flags);
@@ -446,7 +446,7 @@ void pkey_disable_clear(int pkey, int flags)dprintf1 ("%s(%d) pkey_reg: 0x" PKEY_REG_FMT "\n" , __func__, pkey,read_pkey_reg ());if (flags) -assert (read_pkey_reg () > orig_pkey_reg); +assert (read_pkey_reg () <= orig_pkey_reg); }void pkey_write_allow (int pkey)
Signature · 1 line
1.7.1
Re: [PATCH v14 09/22] selftests/vm: fixed bugs in pkey_disable_clear()
Quoted 2 lines · Ram Pai
instead of clearing the bits,
pkey_disable_clear()was setting the bits. Fixed it.
Again, I know these are just selftests, but I'm seeing a real lack of attention to detail with these. Please at least go to the trouble of writing actual changelogs with full sentences and actual capitalization.
I think it's the least you can do if I'm going to spend time reviewing
these.
I'll go one better. Can you just use this as a changelog?
pkey_disable_clear() does not work. Instead of clearing bits, it sets them, probably because it originated as a copy-and-paste of pkey_disable_set().
This has been dead code up to now because the only callers are pkey_access/write_allow() and _those_ are currently unused.
Quoted 27 lines
Also fixed a wrong assertion in that function. When bits are cleared, the resulting bit value will be less than the original.
This hasn't been a problem so far because this code isn't currently used.
cc Dave Hansen <[email protected]>cc Florian Weimer <[email protected]>Signed-off-by Ram Pai <[email protected]>--- tools/testing/selftests/vm/protection_keys.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)tools/testing/selftests/vm/protection_keys.c +1
-1@@ -433,7 +433,7 @@ void pkey_disable_clear(int pkey, int flags) pkey, pkey, pkey_rights);pkey_assert (pkey_rights >=0 ); - pkey_rights |= flags; + pkey_rights &= ~flags; ret =hw_pkey_set (pkey, pkey_rights,0 ); shadow_pkey_reg &=clear_pkey_flags (pkey, flags);
tools/testing/selftests/vm/protection_keys.c void pkey_disable_clear(int pkey, int flags) · line 447
Quoted 2 lines
tools/testing/selftests/vm/protection_keys.c +0
-0@@ -446,7 +446,7 @@ void pkey_disable_clear(int pkey, int flags)dprintf1 ("%s(%d) pkey_reg: 0x" PKEY_REG_FMT "\n" , __func__, pkey,read_pkey_reg ());
If you add a better changelog:
tools/testing/selftests/vm/protection_keys.c void pkey_disable_clear(int pkey, int flags) · line 450
Quoted 4 lines
tools/testing/selftests/vm/protection_keys.c +1
-1if (flags) -assert (read_pkey_reg () > orig_pkey_reg); +assert (read_pkey_reg () <= orig_pkey_reg); }
This really is an orthogonal change that would have been better placed with the other patch that did the exact same thing. But I'd be OK with it here, as long as it has an actual comment.
PATCH v14 10/22 selftests/vm: fix alloc_random_pkey() to make it really random
In reply to Ram Paialloc_random_pkey() was allocating the same pkey every time. Not all pkeys were geting tested. fixed it.
--- tools/testing/selftests/vm/protection_keys.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
tools/testing/selftests/vm/protection_keys.c +5 -1
@@ -24,6 +24,7 @@#define _GNU_SOURCE#include <errno.h> #include <linux/futex.h> +#include <time.h> #include <sys/time.h> #include <sys/syscall.h> #include <string.h>
@@ -573,13 +574,15 @@ int alloc_random_pkey(void)int alloced_pkeys[NR_PKEYS ];int nr_alloced =0 ;int random_index; +memset (alloced_pkeys,0 ,sizeof (alloced_pkeys)); +srand ((unsigned int )time (NULL ));/* allocate every possible key and make a note of which ones we got */ max_nr_pkey_allocs =NR_PKEYS ; - max_nr_pkey_allocs =1 ;for (i =0 ; i < max_nr_pkey_allocs; i++) {int new_pkey =alloc_pkey (); +if (new_pkey <0 )break ; alloced_pkeys[nr_alloced++] = new_pkey;
@@ -595,6 +598,7 @@ int alloc_random_pkey(void)/* go through the allocated ones that we did not want and free them */ for (i =0 ; i < nr_alloced; i++) {int free_ret; +if (!alloced_pkeys[i])continue ; free_ret =sys_pkey_free (alloced_pkeys[i]);
Signature · 1 line
1.7.1
Re: [PATCH v14 10/22] selftests/vm: fix alloc_random_pkey() to make it really random
Quoted 2 lines · Ram Pai
alloc_random_pkey()was allocating the same pkey every time. Not all pkeys were geting tested. fixed it.
This fixes a real issue but also unnecessarily munges whitespace. If you rev these again, please fix the munging. Otherwise:
PATCH v14 11/22 selftests/vm: introduce two arch independent abstraction
In reply to Ram Paiopen_hugepage_file() <- opens the huge page file get_start_key() <-- provides the first non-reserved key.
--- tools/testing/selftests/vm/pkey-helpers.h | 10 ++++++++++ tools/testing/selftests/vm/pkey-x86.h | 1 + tools/testing/selftests/vm/protection_keys.c | 6 +++--- 3 files changed, 14 insertions(+), 3 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +10 -0
@@ -179,4 +179,14 @@ static inline void __pkey_write_allow(int pkey, int do_allow_write) #define __stringify_1(x...) #x #define __stringify(x...) __stringify_1(x) +static inline int open_hugepage_file (int flag) +{ +return open (HUGEPAGE_FILE , flag); +} + +static inline int get_start_key (void ) +{ +return 1 ; +} +#endif /* _PKEYS_HELPER_H */
tools/testing/selftests/vm/pkey-x86.h +1 -0
@@ -48,6 +48,7 @@#define MB (1<<20)#define pkey_reg_t u32#define PKEY_REG_FMT "%016x" +#define HUGEPAGE_FILE "/sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages"static inline u32 pkey_bit_position (int pkey) {
tools/testing/selftests/vm/protection_keys.c +3 -3
@@ -788,7 +788,7 @@ void setup_hugetlbfs(void)* Now go make sure that we got the pages and that they * are 2M pages. Someone might have made 1G the default. */ - fd =open ("/sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages" ,O_RDONLY ); + fd =open_hugepage_file (O_RDONLY );if (fd <0 ) {perror ("opening sysfs 2M hugetlb config" );return ;
@@ -1075,10 +1075,10 @@ void test_kernel_gup_write_to_write_disabled_region(int *ptr, u16 pkey)void test_pkey_syscalls_on_non_allocated_pkey (int *ptr,u16 pkey) {int err; -int i; +int i =get_start_key ();/* Note: 0 is the default pkey, so don't mess with it */ -for (i =1 ; i <NR_PKEYS ; i++) { +for (; i <NR_PKEYS ; i++) {if (pkey == i)continue ;
Signature · 1 line
1.7.1
Re: [PATCH v14 11/22] selftests/vm: introduce two arch independent abstraction
Quoted 1 line · Ram Pai
open_hugepage_file()<- opens the huge page file
Folks, a sentence here would be nice:
Different architectures have different huge page sizes and thus have different sysfs filees to manipulate when allocating huge pages.
Quoted 1 line
get_start_key()<-- provides the first non-reserved key.
Does powerpc not start on key 0? Why do you need this?
Quoted 16 lines
cc Dave Hansen <[email protected]>cc Florian Weimer <[email protected]>Signed-off-by Ram Pai <[email protected]>Signed-off-by Thiago Jung Bauermann <[email protected]>Reviewed-by Dave Hansen <[email protected]>--- tools/testing/selftests/vm/pkey-helpers.h | 10 ++++++++++ tools/testing/selftests/vm/pkey-x86.h | 1 + tools/testing/selftests/vm/protection_keys.c | 6 +++--- 3 files changed, 14 insertions(+), 3 deletions(-)tools/testing/selftests/vm/pkey-helpers.h +0
-0
tools/testing/selftests/vm/pkey-helpers.h static inline void __pkey_write_allow(int pkey, int do_allow_write) · line 185
Quoted 7 lines
tools/testing/selftests/vm/pkey-helpers.h +4
-0@@ -179,4 +179,14 @@ static inline void __pkey_write_allow(int pkey, int do_allow_write) #define __stringify_1(x...) #x #define __stringify(x...) __stringify_1(x) +static inline int open_hugepage_file (int flag) +{ +return open (HUGEPAGE_FILE , flag); +}
open_nr_hugepages_file() if you revise this, please
tools/testing/selftests/vm/pkey-helpers.h static inline void __pkey_write_allow(int pkey, int do_allow_write) · line 190
Quoted 5 lines
tools/testing/selftests/vm/pkey-helpers.h +5
-0+ +static inline int get_start_key (void ) +{ +return 1 ; +}
get_first_user_pkey(), please.
Quoted 18 lines
tools/testing/selftests/vm/pkey-helpers.h +0
-0#endif /* _PKEYS_HELPER_H */ tools/testing/selftests/vm/pkey-x86.h +1
-0@@ -48,6 +48,7 @@#define MB (1<<20)#define pkey_reg_t u32#define PKEY_REG_FMT "%016x" +#define HUGEPAGE_FILE "/sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages"static inline u32 pkey_bit_position (int pkey) {tools/testing/selftests/vm/protection_keys.c +0
-0
tools/testing/selftests/vm/protection_keys.c void setup_hugetlbfs(void) · line 794
Quoted 8 lines
tools/testing/selftests/vm/protection_keys.c +1
-1@@ -788,7 +788,7 @@ void setup_hugetlbfs(void)* Now go make sure that we got the pages and that they * are 2M pages. Someone might have made 1G the default. */ - fd =open ("/sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages" ,O_RDONLY ); + fd =open_hugepage_file (O_RDONLY );if (fd <0 ) {perror ("opening sysfs 2M hugetlb config" );return ;
This is fine, and obviously necessary.
tools/testing/selftests/vm/protection_keys.c void test_kernel_gup_write_to_write_disabled_region(int *ptr, u16 pkey) · line 1083
Quoted 12 lines
tools/testing/selftests/vm/protection_keys.c +2
-2@@ -1075,10 +1075,10 @@ void test_kernel_gup_write_to_write_disabled_region(int *ptr, u16 pkey)void test_pkey_syscalls_on_non_allocated_pkey (int *ptr,u16 pkey) {int err; -int i; +int i =get_start_key ();/* Note: 0 is the default pkey, so don't mess with it */ -for (i =1 ; i <NR_PKEYS ; i++) { +for (; i <NR_PKEYS ; i++) {if (pkey == i)continue ;
Grumble, grumble, you moved the code away from the comment connected to it.
PATCH v14 12/22 selftests/vm: pkey register should match shadow pkey
In reply to Ram Paiexpected_pkey_fault() is comparing the contents of pkey register with 0. This may not be true all the time. There could be bits set by default by the architecture which can never be changed. Hence compare the value against shadow pkey register, which is supposed to track the bits accurately all throughout
--- tools/testing/selftests/vm/protection_keys.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
tools/testing/selftests/vm/protection_keys.c +2 -2
@@ -913,10 +913,10 @@ void expected_pkey_fault(int pkey)pkey_assert (last_si_pkey == pkey);/* -* The signal handler shold have cleared out PKEY register to let the +* The signal handler should have cleared out pkey-register to let the * test program continue. We now have to restore it. */ -if (__read_pkey_reg () !=0 ) +if (__read_pkey_reg () != shadow_pkey_reg)pkey_assert (0 );__write_pkey_reg (shadow_pkey_reg);
Signature · 1 line
1.7.1
Re: [PATCH v14 12/22] selftests/vm: pkey register should match shadow pkey
Quoted 6 lines · Ram Pai
expected_pkey_fault()is comparing the contents of pkey register with 0. This may not be true all the time. There could be bits set by default by the architecture which can never be changed. Hence compare the value against shadow pkey register, which is supposed to track the bits accurately all throughout
This is getting dangerously close to full sentences that actually describe the patch. You forgot a period, but much this is a substantial improvement over earlier parts of the series. Thanks for writing this, seriously.
Quoted 12 lines
cc Dave Hansen <[email protected]>cc Florian Weimer <[email protected]>Signed-off-by Ram Pai <[email protected]>--- tools/testing/selftests/vm/protection_keys.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)tools/testing/selftests/vm/protection_keys.c +0
-0
tools/testing/selftests/vm/protection_keys.c void expected_pkey_fault(int pkey) · line 918
Quoted 7 lines
tools/testing/selftests/vm/protection_keys.c +1
-1@@ -913,10 +913,10 @@ void expected_pkey_fault(int pkey)pkey_assert (last_si_pkey == pkey);/* -* The signal handler shold have cleared out PKEY register to let the +* The signal handler should have cleared out pkey-register to let the * test program continue. We now have to restore it. */
... while I appreciate the spelling corrections, and I would totally ack a patch that fixed them in one fell swoop, could we please segregate the random spelling corrections from code fixes unless you touch those lines otherwise?
tools/testing/selftests/vm/protection_keys.c void expected_pkey_fault(int pkey) · line 922
Quoted 5 lines
tools/testing/selftests/vm/protection_keys.c +1
-1-if (__read_pkey_reg () !=0 ) +if (__read_pkey_reg () != shadow_pkey_reg)pkey_assert (0 );__write_pkey_reg (shadow_pkey_reg);
I know this is a one-line change, but I don't fully understand it.
On x86, if we take a pkey fault, we clear PKRU entirely (via the on-stack XSAVE state that is restored at sigreturn) which allows the faulting instruction to resume and execute normally. That's what this check is looking for: Did the signal handler clear PKRU?
Now, you're saying that powerpc might not clear it. That makes sense.
While PKRU's state here is obvious, it isn't patently obvious to me what shadow_pkey_reg's state is. In fact, looking at it, I don't see the signal handler manipulating the shadow. So, how can this patch work?
PATCH v14 13/22 selftests/vm: generic cleanup
In reply to Ram Paicleanup the code to satisfy coding styles.
--- tools/testing/selftests/vm/protection_keys.c | 64 +++++++++++++++++-------- 1 files changed, 43 insertions(+), 21 deletions(-)
tools/testing/selftests/vm/protection_keys.c +43 -21
@@ -4,7 +4,7 @@* * There are examples in here of: * * how to set protection keys on memory -* * how to set/clear bits in pkey registers (the rights register) +* * how to set/clear bits in Protection Key registers (the rights register) * * how to handle SEGV_PKUERR signals and extract pkey-relevant * information from the siginfo *
@@ -13,13 +13,18 @@* prefault pages in at malloc, or not * protect MPX bounds tables with protection keys? * make sure VMA splitting/merging is working correctly -* OOMs can destroy mm->mmap (see exit_mmap()), so make sure it is immune to pkeys -* look for pkey "leaks" where it is still set on a VMA but "freed" back to the kernel -* do a plain mprotect() to a mprotect_pkey() area and make sure the pkey sticks +* OOMs can destroy mm->mmap (see exit_mmap()), +* so make sure it is immune to pkeys +* look for pkey "leaks" where it is still set on a VMA +* but "freed" back to the kernel +* do a plain mprotect() to a mprotect_pkey() area and make +* sure the pkey sticks * * Compile like this: -* gcc -o protection_keys -O2 -g -std=gnu99 -pthread -Wall protection_keys.c -lrt -ldl -lm -* gcc -m32 -o protection_keys_32 -O2 -g -std=gnu99 -pthread -Wall protection_keys.c -lrt -ldl -lm +* gcc -o protection_keys -O2 -g -std=gnu99 +* -pthread -Wall protection_keys.c -lrt -ldl -lm +* gcc -m32 -o protection_keys_32 -O2 -g -std=gnu99 +* -pthread -Wall protection_keys.c -lrt -ldl -lm */ #define _GNU_SOURCE#include <errno.h>
@@ -263,10 +268,12 @@ void signal_handler(int signum, siginfo_t *si, void *vucontext) __read_pkey_reg());dprintf1 ("pkey from siginfo: %jx\n" , siginfo_pkey); *(u64 *)pkey_reg_ptr =0x00000000 ; -dprintf1 ("WARNING: set PRKU=0 to allow faulting instruction to continue\n" ); +dprintf1 ("WARNING: set PKEY_REG=0 to allow faulting instruction " +"to continue\n" ); pkey_faults++;dprintf1 ("<<<<==================================================\n" ); dprint_in_signal =0 ; +return ; }int wait_all_children (void )
@@ -384,7 +391,7 @@ void pkey_disable_set(int pkey, int flags) {unsigned long syscall_flags =0 ;int ret; -int pkey_rights; +u32 pkey_rights;pkey_reg_t orig_pkey_reg =read_pkey_reg (); dprintf1("START->%s(%d, 0x%x)\n", __func__,
@@ -487,9 +494,10 @@ int sys_mprotect_pkey(void *ptr, size_t size, unsigned long orig_prot,return sret; } -int sys_pkey_alloc (unsigned long flags,unsigned long init_val) +int sys_pkey_alloc (unsigned long flags,u64 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;
@@ -513,7 +521,7 @@ void pkey_set_shadow(u32 key, u64 init_val)int alloc_pkey (void ) {int ret; -unsigned long init_val =0x0 ; +u64 init_val =0x0 ;dprintf1 ("%s()::%d, pkey_reg: " PKEY_REG_FMT " shadow: " PKEY_REG_FMT "\n" , __func__, __LINE__,__read_pkey_reg (), shadow_pkey_reg);
@@ -669,7 +677,9 @@ void record_pkey_malloc(void *ptr, long size, int prot)/* every record is full */ size_t old_nr_records = nr_pkey_malloc_records;size_t new_nr_records = (nr_pkey_malloc_records *2 +1 ); -size_t new_size = new_nr_records *sizeof (struct pkey_malloc_record ); +size_t new_size = new_nr_records * +sizeof (struct pkey_malloc_record ); +dprintf2 ("new_nr_records: %zd\n" , new_nr_records);dprintf2 ("new_size: %zd\n" , new_size); pkey_malloc_records =realloc (pkey_malloc_records, new_size);
@@ -695,9 +705,11 @@ void free_pkey_malloc(void *ptr) {long i;int ret; +dprintf3 ("%s(%p)\n" , __func__, ptr);for (i =0 ; i < nr_pkey_malloc_records; i++) {struct pkey_malloc_record *rec = &pkey_malloc_records[i]; +dprintf4 ("looking for ptr %p at record[%ld/%p]: {%p, %ld}\n" , ptr, i, rec, rec->ptr, rec->size); if ((ptr < rec->ptr) ||
@@ -778,11 +790,13 @@ void setup_hugetlbfs(void)char buf[] ="123" ;if (geteuid () !=0 ) { -fprintf (stderr,"WARNING: not run as root, can not do hugetlb test\n" ); +fprintf (stderr, +"WARNING: not run as root, can not do hugetlb test\n" );return ; } -cat_into_file (__stringify (GET_NR_HUGE_PAGES ),"/proc/sys/vm/nr_hugepages" ); +cat_into_file (__stringify (GET_NR_HUGE_PAGES ), +"/proc/sys/vm/nr_hugepages" );/* * Now go make sure that we got the pages and that they
@@ -803,7 +817,8 @@ void setup_hugetlbfs(void) }if (atoi (buf) !=GET_NR_HUGE_PAGES ) { -fprintf (stderr,"could not confirm 2M pages, got: '%s' expected %d\n" , +fprintf (stderr,"could not confirm 2M pages, got:" +" '%s' expected %d\n" , buf,GET_NR_HUGE_PAGES );return ; }
@@ -945,6 +960,7 @@ void __save_test_fd(int fd)int get_test_read_fd (void ) {int test_fd =open ("/etc/passwd" ,O_RDONLY ); +__save_test_fd (test_fd);return test_fd; }
@@ -986,7 +1002,8 @@ void test_read_of_access_disabled_region(int *ptr, u16 pkey) {int ptr_contents; -dprintf1 ("disabling access to PKEY[%02d], doing read @ %p\n" , pkey, ptr); +dprintf1 ("disabling access to PKEY[%02d], doing read @ %p\n" , + pkey, ptr);read_pkey_reg ();pkey_access_deny (pkey); ptr_contents =read_ptr (ptr);
@@ -1108,13 +1125,14 @@ void test_pkey_syscalls_bad_args(int *ptr, u16 pkey)/* Assumes that all pkeys other than 'pkey' are unallocated */ void test_pkey_alloc_exhaust (int *ptr,u16 pkey) { -int err; +int err =0 ;int allocated_pkeys[NR_PKEYS ] = {0 };int nr_allocated_pkeys =0 ;int i;for (i =0 ; i <NR_PKEYS *2 ; i++) {int new_pkey; +dprintf1 ("%s() alloc loop: %d\n" , __func__, i); new_pkey =alloc_pkey (); dprintf4("%s()::%d, err: %d pkey_reg: 0x"PKEY_REG_FMT
@@ -1122,9 +1140,11 @@ void test_pkey_alloc_exhaust(int *ptr, u16 pkey) __func__, __LINE__, err,__read_pkey_reg (), shadow_pkey_reg);read_pkey_reg ();/* for shadow checking */ -dprintf2 ("%s() errno: %d ENOSPC: %d\n" , __func__, errno,ENOSPC ); +dprintf2 ("%s() errno: %d ENOSPC: %d\n" , + __func__, errno,ENOSPC );if ((new_pkey ==-1 ) && (errno ==ENOSPC )) { -dprintf2 ("%s() failed to allocate pkey after %d tries\n" , +dprintf2 ("%s() failed to allocate pkey " +"after %d tries\n" , __func__, nr_allocated_pkeys);break ; }
@@ -1416,7 +1436,8 @@ void run_tests_once(void)tracing_off ();close_test_fds (); -printf ("test %2d PASSED (iteration %d)\n" , test_nr, iteration_nr); +printf ("test %2d PASSED (iteration %d)\n" , + test_nr, iteration_nr);dprintf1 ("======================\n\n" ); } iteration_nr++;
@@ -1428,7 +1449,7 @@ int main(void)setup_handlers (); -printf ("has pku: %d\n" ,cpu_has_pku ()); +printf ("has pkey: %d\n" ,cpu_has_pku ());if (!cpu_has_pku ()) {int size =PAGE_SIZE ;
@@ -1436,7 +1457,8 @@ int main(void)printf ("running PKEY tests for unsupported CPU/OS\n" ); - ptr =mmap (NULL , size,PROT_NONE ,MAP_ANONYMOUS |MAP_PRIVATE ,-1 ,0 ); + ptr =mmap (NULL , size,PROT_NONE , +MAP_ANONYMOUS |MAP_PRIVATE ,-1 ,0 );assert (ptr != (void *)-1 );test_mprotect_pkey_on_unsupported_cpu (ptr,1 );exit (0 );
Signature · 1 line
1.7.1
Re: [PATCH v14 13/22] selftests/vm: generic cleanup
Quoted 14 lines · Ram Pai
cleanup the code to satisfy coding styles.
cc Dave Hansen <[email protected]>cc Florian Weimer <[email protected]>Signed-off-by Ram Pai <[email protected]>--- tools/testing/selftests/vm/protection_keys.c | 64 +++++++++++++++++-------- 1 files changed, 43 insertions(+), 21 deletions(-)tools/testing/selftests/vm/protection_keys.c +0
-0
tools/testing/selftests/vm/protection_keys.c line 7
Quoted 5 lines
tools/testing/selftests/vm/protection_keys.c +1
-1@@ -4,7 +4,7 @@* * There are examples in here of: * * how to set protection keys on memory -* * how to set/clear bits in pkey registers (the rights register) +* * how to set/clear bits in Protection Key registers (the rights register)
Huh? Which coding style says that we can't say "pkey"?
Quoted 6 lines
tools/testing/selftests/vm/protection_keys.c +0
-0* * how to handle SEGV_PKUERR signals and extract pkey-relevant * information from the siginfo * @@ -13,13 +13,18 @@* prefault pages in at malloc, or not * protect MPX bounds tables with protection keys?
tools/testing/selftests/vm/protection_keys.c line 21
Quoted 10 lines
tools/testing/selftests/vm/protection_keys.c +6
-3* make sure VMA splitting/merging is working correctly -* OOMs can destroy mm->mmap (see exit_mmap()), so make sure it is immune to pkeys -* look for pkey "leaks" where it is still set on a VMA but "freed" back to the kernel -* do a plain mprotect() to a mprotect_pkey() area and make sure the pkey sticks +* OOMs can destroy mm->mmap (see exit_mmap()), +* so make sure it is immune to pkeys +* look for pkey "leaks" where it is still set on a VMA +* but "freed" back to the kernel +* do a plain mprotect() to a mprotect_pkey() area and make +* sure the pkey sticks
This makes it work substantially worse. That's not acceptable, even if you did move it under 80 columns.
tools/testing/selftests/vm/protection_keys.c line 28
Quoted 8 lines
tools/testing/selftests/vm/protection_keys.c +4
-2* Compile like this: -* gcc -o protection_keys -O2 -g -std=gnu99 -pthread -Wall protection_keys.c -lrt -ldl -lm -* gcc -m32 -o protection_keys_32 -O2 -g -std=gnu99 -pthread -Wall protection_keys.c -lrt -ldl -lm +* gcc -o protection_keys -O2 -g -std=gnu99 +* -pthread -Wall protection_keys.c -lrt -ldl -lm +* gcc -m32 -o protection_keys_32 -O2 -g -std=gnu99 +* -pthread -Wall protection_keys.c -lrt -ldl -lm */
Why was this on one line? Because it was easier to copy and paste. Please leave it on one line, CodingStyle be damned.
tools/testing/selftests/vm/protection_keys.c void signal_handler(int signum, siginfo_t *si, void *vucontext) · line 272
Quoted 9 lines
tools/testing/selftests/vm/protection_keys.c +2
-1#define _GNU_SOURCE#include <errno.h> @@ -263,10 +268,12 @@ void signal_handler(int signum, siginfo_t *si, void *vucontext) __read_pkey_reg());dprintf1 ("pkey from siginfo: %jx\n" , siginfo_pkey); *(u64 *)pkey_reg_ptr =0x00000000 ; -dprintf1 ("WARNING: set PRKU=0 to allow faulting instruction to continue\n" ); +dprintf1 ("WARNING: set PKEY_REG=0 to allow faulting instruction " +"to continue\n" );
It's actually totally OK to let printk strings go over 80 columns.
tools/testing/selftests/vm/protection_keys.c void signal_handler(int signum, siginfo_t *si, void *vucontext) · line 277
Quoted 5 lines
tools/testing/selftests/vm/protection_keys.c +1
-0pkey_faults++;dprintf1 ("<<<<==================================================\n" ); dprint_in_signal =0 ; +return ; }
Now we're just being silly.
tools/testing/selftests/vm/protection_keys.c void pkey_disable_set(int pkey, int flags) · line 394
Quoted 8 lines
tools/testing/selftests/vm/protection_keys.c +1
-1int wait_all_children (void )@@ -384,7 +391,7 @@ void pkey_disable_set(int pkey, int flags) {unsigned long syscall_flags =0 ;int ret; -int pkey_rights; +u32 pkey_rights;
This is not CodingStyle. Shouldn't this be the pkey_reg_t that you introduced earlier in the series?
tools/testing/selftests/vm/protection_keys.c int sys_mprotect_pkey(void *ptr, size_t size, unsigned long orig_prot, · line 498
Quoted 3 lines
tools/testing/selftests/vm/protection_keys.c +1
-1-int sys_pkey_alloc (unsigned long flags,unsigned long init_val) +int sys_pkey_alloc (unsigned long flags,u64 init_val) {
Um, this is actually a 'unsigned long' in the ABI.
Can you go back through this and actually make sure that these are real coding style cleanups?
PATCH v14 14/22 selftests/vm: Introduce generic abstractions
In reply to Ram PaiIntroduce generic abstractions and provide architecture specific implementation for the abstractions.
--- tools/testing/selftests/vm/pkey-helpers.h | 16 ++++- tools/testing/selftests/vm/pkey-powerpc.h | 93 ++++++++++++++++++++++++++ tools/testing/selftests/vm/pkey-x86.h | 15 ++++ tools/testing/selftests/vm/protection_keys.c | 52 ++++++++------ 4 files changed, 153 insertions(+), 23 deletions(-) create mode 100644 tools/testing/selftests/vm/pkey-powerpc.h
tools/testing/selftests/vm/pkey-helpers.h +15 -1
@@ -74,8 +74,13 @@ static inline void sigsafe_printf(const char *format, ...) } \ }while (0 ) +__attribute__((noinline)) int read_ptr(int *ptr); +void expected_pkey_fault (int pkey); +#if defined(__i386__) || defined(__x86_64__)/* arch */ #include "pkey-x86.h" +#elif defined(__powerpc64__)/* arch */ +#include "pkey-powerpc.h" #else /* arch */ #error Architecture not supported#endif /* arch */
@@ -186,7 +191,16 @@ static inline int open_hugepage_file(int flag)static inline int get_start_key (void ) { -return 1 ; +return 0 ; +} + +static inline u32 *siginfo_get_pkey_ptr (siginfo_t *si) +{ +#ifdef si_pkey +return &si->si_pkey; +#else +return (u32 *)(((u8 *)si) + si_pkey_offset); +#endif }#endif /* _PKEYS_HELPER_H */
tools/testing/selftests/vm/pkey-powerpc.h new file +93 -0
93 lines not on this page · raw message
tools/testing/selftests/vm/pkey-x86.h +15 -0
@@ -42,6 +42,7 @@#endif #define NR_PKEYS 16 +#define NR_RESERVED_PKEYS 2/* pkey-0 and exec-only-pkey */ #define PKEY_BITS_PER_PKEY 2#define HPAGE_SIZE (1UL<<21)#define PAGE_SIZE 4096
@@ -161,4 +162,18 @@ int pkey_reg_xstate_offset(void) return xstate_offset; } +static inline int arch_reserved_keys (void ) +{ +return NR_RESERVED_PKEYS ; +} + +void expect_fault_on_read_execonly_key (void *p1,int pkey) +{ +int ptr_contents; + + ptr_contents =read_ptr (p1); +dprintf2 ("ptr (%p) contents@%d: %x\n" , p1, __LINE__, ptr_contents); +expected_pkey_fault (pkey); +} +#endif /* _PKEYS_X86_H */
tools/testing/selftests/vm/protection_keys.c +30 -22
119 lines not on this page · raw message
Signature · 1 line
1.7.1
Re: [PATCH v14 14/22] selftests/vm: Introduce generic abstractions
Quoted 2 lines · Ram Pai
Introduce generic abstractions and provide architecture specific implementation for the abstractions.
I really wanted to see these two things separated:
1. introduce abstractions
2. introduce ppc implementation
But, I guess most of it is done except for the siginfo stuff.
Quoted 8 lines
#if
defined(__i386__)||defined(__x86_64__)/* arch */ #include "pkey-x86.h"tools/testing/selftests/vm/pkey-helpers.h +2
-0+#elif defined(__powerpc64__)/* arch */ +#include "pkey-powerpc.h" #else /* arch */ #error Architecture not supported#endif /* arch */
tools/testing/selftests/vm/pkey-helpers.h static inline int open_hugepage_file(int flag) · line 195
Quoted 6 lines
tools/testing/selftests/vm/pkey-helpers.h +2
-1@@ -186,7 +191,16 @@ static inline int open_hugepage_file(int flag)static inline int get_start_key (void ) { -return 1 ; +return 0 ; +}
How does this not now break x86?
tools/testing/selftests/vm/protection_keys.c void dump_mem(void *dumpme, int len_bytes) · line 200
Quoted 10 lines
tools/testing/selftests/vm/pkey-x86.h +0
-0#endif /* _PKEYS_X86_H */ tools/testing/selftests/vm/protection_keys.c +1
-0@@ -197,17 +197,18 @@ void dump_mem(void *dumpme, int len_bytes)int pkey_faults;int last_si_pkey =-1 ; +void pkey_access_allow (int pkey);
Please just move the function.
Quoted 36 lines
tools/testing/selftests/vm/protection_keys.c +7
-5void signal_handler (int signum,siginfo_t *si,void *vucontext) {ucontext_t *uctxt = vucontext;int trapno;unsigned long ip;char *fpregs; +#if defined(__i386__) || defined(__x86_64__)/* arch */ pkey_reg_t *pkey_reg_ptr; -u64 siginfo_pkey; +#endif /* defined(__i386__) || defined(__x86_64__) */ +u32 siginfo_pkey;u32 *si_pkey_ptr; -int pkey_reg_offset; -fpregset_t fpregset; dprint_in_signal =1 ;dprintf1 (">>>>===============SIGSEGV============================\n" );@@ -217,12 +218,14 @@ void signal_handler(int signum, siginfo_t *si, void *vucontext) trapno = uctxt->uc_mcontext.gregs[REG_TRAPNO ]; ip = uctxt->uc_mcontext.gregs[REG_IP_IDX ]; - fpregset = uctxt->uc_mcontext.fpregs; - fpregs = (void *)fpregset; + fpregs = (char *) uctxt->uc_mcontext.fpregs;dprintf2 ("%s() trapno: %d ip: 0x%016lx info->si_code: %s/%d\n" , __func__, trapno, ip,si_code_str (si->si_code), si->si_code); + +#if defined(__i386__) || defined(__x86_64__)/* arch */ +#ifdef __i386__/* * 32-bit has some extra padding so that userspace can tell whether @@ -230,20 +233,21 @@ void signal_handler(int signum, siginfo_t *si, void *vucontext)* state. We just assume that it is here.
tools/testing/selftests/vm/protection_keys.c void signal_handler(int signum, siginfo_t *si, void *vucontext) · line 238
Quoted 10 lines
tools/testing/selftests/vm/protection_keys.c +2
-5*/ fpregs +=0x70 ; -#endif - pkey_reg_offset =pkey_reg_xstate_offset (); - pkey_reg_ptr = (void *)(&fpregs[pkey_reg_offset]); +#endif /* __i386__ */ -dprintf1 ("siginfo: %p\n" , si); -dprintf1 (" fpregs: %p\n" , fpregs); + pkey_reg_ptr = (void *)(&fpregs[pkey_reg_xstate_offset ()]);
There are unnecessary parenthesis here.
Also, why are you bothering to mess with this? This is inside the x86 #ifdef, right?
Quoted 17 lines
tools/testing/selftests/vm/protection_keys.c +5
-1/* -* If we got a PKEY fault, we *HAVE* to have at least one bit set in +* If we got a key fault, we *HAVE* to have at least one bit set in * here. */ dprintf1 ("pkey_reg_xstate_offset: %d\n" ,pkey_reg_xstate_offset ());if (DEBUG_LEVEL >4 )dump_mem (pkey_reg_ptr -128 ,256 );pkey_assert (*pkey_reg_ptr); +#endif /* defined(__i386__) || defined(__x86_64__) */ + +dprintf1 ("siginfo: %p\n" , si); +dprintf1 (" fpregs: %p\n" , fpregs); if ((si->si_code == SEGV_MAPERR) || (si->si_code == SEGV_ACCERR) ||
tools/testing/selftests/vm/protection_keys.c void signal_handler(int signum, siginfo_t *si, void *vucontext) · line 261
Quoted 8 lines
tools/testing/selftests/vm/protection_keys.c +2
-2@@ -252,22 +256,28 @@ void signal_handler(int signum, siginfo_t *si, void *vucontext)exit (4 ); } - si_pkey_ptr = (u32 *)(((u8 *)si) + si_pkey_offset); + si_pkey_ptr =siginfo_get_pkey_ptr (si);dprintf1 ("si_pkey_ptr: %p\n" , si_pkey_ptr); -dump_mem ((u8 *)si_pkey_ptr -8 ,24 ); +dump_mem (si_pkey_ptr -8 ,24 );
You removed the cast here, why? That changes the pointer math.
Can we merge this as-is. No, I do not think we can. If it were _just_ the #ifdefs, we could let it pass, but there are a bunch of rough spots, not just the #ifdefs.
PATCH v14 15/22 selftests/vm: powerpc implementation to check support for pkey
In reply to Ram Paipkey subsystem is supported if the hardware and kernel has support. We determine that by checking if allocation of a key succeeds or not.
--- tools/testing/selftests/vm/pkey-helpers.h | 2 ++ tools/testing/selftests/vm/pkey-powerpc.h | 14 ++++++++++++-- tools/testing/selftests/vm/pkey-x86.h | 8 ++++---- tools/testing/selftests/vm/protection_keys.c | 9 +++++---- 4 files changed, 23 insertions(+), 10 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +2 -0
@@ -76,6 +76,8 @@ static inline void sigsafe_printf(const char *format, ...) __attribute__((noinline))int read_ptr (int *ptr);void expected_pkey_fault (int pkey); +int sys_pkey_alloc (unsigned long flags,u64 init_val); +int sys_pkey_free (unsigned long pkey);#if defined(__i386__) || defined(__x86_64__)/* arch */ #include "pkey-x86.h"
tools/testing/selftests/vm/pkey-powerpc.h +12 -2
@@ -64,9 +64,19 @@ static inline void __write_pkey_reg(pkey_reg_t pkey_reg) pkey_reg); } -static inline int cpu_has_pku (void ) +static inline bool is_pkey_supported (void ) { -return 1 ; +/* +* No simple way to determine this. +* Lets try allocating a key and see if it succeeds. +*/ +int ret =sys_pkey_alloc (0 ,0 ); + +if (ret >0 ) { +sys_pkey_free (ret); +return true; + } +return false; }static inline int arch_reserved_keys (void )
tools/testing/selftests/vm/pkey-x86.h +4 -4
@@ -105,7 +105,7 @@ static inline void __cpuid(unsigned int *eax, unsigned int *ebx,#define X86_FEATURE_PKU (1<<3)/* Protection Keys for Userspace */ #define X86_FEATURE_OSPKE (1<<4)/* OS Protection Keys Enable */ -static inline int cpu_has_pku (void ) +static inline bool is_pkey_supported (void ) {unsigned int eax;unsigned int ebx;
@@ -118,13 +118,13 @@ static inline int cpu_has_pku(void)if (!(ecx &X86_FEATURE_PKU )) {dprintf2 ("cpu does not have PKU\n" ); -return 0 ; +return false; }if (!(ecx &X86_FEATURE_OSPKE )) {dprintf2 ("cpu does not have OSPKE\n" ); -return 0 ; +return false; } -return 1 ; +return true; }#define XSTATE_PKEY_BIT (9)
tools/testing/selftests/vm/protection_keys.c +5 -4
@@ -1389,8 +1389,8 @@ void test_mprotect_pkey_on_unsupported_cpu(int *ptr, u16 pkey)int size =PAGE_SIZE ;int sret; -if (cpu_has_pku ()) { -dprintf1 ("SKIP: %s: no CPU support\n" , __func__); +if (is_pkey_supported ()) { +dprintf1 ("SKIP: %s: no CPU/kernel support\n" , __func__);return ; }
@@ -1454,12 +1454,13 @@ void run_tests_once(void)int main (void ) {int nr_iterations =22 ; +int pkey_supported =is_pkey_supported ();setup_handlers (); -printf ("has pkey: %d\n" ,cpu_has_pku ()); +printf ("has pkey: %s\n" , pkey_supported ?"Yes" :"No" ); -if (!cpu_has_pku ()) { +if (!pkey_supported) {int size =PAGE_SIZE ;int *ptr;
Signature · 1 line
1.7.1
Re: [PATCH v14 15/22] selftests/vm: powerpc implementation to check support for pkey
Quoted 6 lines · Ram Pai
tools/testing/selftests/vm/pkey-powerpc.h +3
-2-static inline int cpu_has_pku (void ) +static inline bool is_pkey_supported (void ) { -return 1 ; +/* +* No simple way to determine this.
tools/testing/selftests/vm/pkey-powerpc.h static inline void __write_pkey_reg(pkey_reg_t pkey_reg) · line 80
Quoted 10 lines
tools/testing/selftests/vm/pkey-powerpc.h +9
-0+* Lets try allocating a key and see if it succeeds. +*/ +int ret =sys_pkey_alloc (0 ,0 ); + +if (ret >0 ) { +sys_pkey_free (ret); +return true; + } +return false; }
This actually works on x86 too.
Quoted 38 lines
static inline int
arch_reserved_keys(void)tools/testing/selftests/vm/pkey-x86.h +4
-4@@ -105,7 +105,7 @@ static inline void __cpuid(unsigned int *eax, unsigned int *ebx,#define X86_FEATURE_PKU (1<<3)/* Protection Keys for Userspace */ #define X86_FEATURE_OSPKE (1<<4)/* OS Protection Keys Enable */ -static inline int cpu_has_pku (void ) +static inline bool is_pkey_supported (void ) {unsigned int eax;unsigned int ebx;@@ -118,13 +118,13 @@ static inline int cpu_has_pku(void)if (!(ecx &X86_FEATURE_PKU )) {dprintf2 ("cpu does not have PKU\n" ); -return 0 ; +return false; }if (!(ecx &X86_FEATURE_OSPKE )) {dprintf2 ("cpu does not have OSPKE\n" ); -return 0 ; +return false; } -return 1 ; +return true; }#define
XSTATE_PKEY_BIT(9)tools/testing/selftests/vm/protection_keys.c +0
-0
tools/testing/selftests/vm/protection_keys.c void test_mprotect_pkey_on_unsupported_cpu(int *ptr, u16 pkey) · line 1395
Quoted 9 lines
tools/testing/selftests/vm/protection_keys.c +2
-2@@ -1389,8 +1389,8 @@ void test_mprotect_pkey_on_unsupported_cpu(int *ptr, u16 pkey)int size =PAGE_SIZE ;int sret; -if (cpu_has_pku ()) { -dprintf1 ("SKIP: %s: no CPU support\n" , __func__); +if (is_pkey_supported ()) { +dprintf1 ("SKIP: %s: no CPU/kernel support\n" , __func__);return ; }
I actually wanted a kernel-independent check, based entirely on CPUID. That's specifically why I said "no CPU support".
If you want to do this, please do:
/* powerpc has no enumeration, just assume it has support: */ static inline bool cpu_has_cpu(void) { return true; };
if (cpu_has_pku()) {
dprintf1("SKIP: %s: no CPU support\n", __func__);
return
}
if (kernel_pkey_supported()) {
dprintf1("SKIP: %s: no kernel support\n", __func__);
return;}
PATCH v14 16/22 selftests/vm: fix an assertion in test_pkey_alloc_exhaust()
In reply to Ram PaiThe maximum number of keys that can be allocated has to take into consideration, that some keys are reserved by
the architecture for specific purpose. Hence cannot
be allocated.
Fix the assertion in test_pkey_alloc_exhaust()
--- tools/testing/selftests/vm/protection_keys.c | 12 ++++-------- 1 files changed, 4 insertions(+), 8 deletions(-)
tools/testing/selftests/vm/protection_keys.c +4 -8
@@ -1171,15 +1171,11 @@ void test_pkey_alloc_exhaust(int *ptr, u16 pkey)pkey_assert (i <NR_PKEYS *2 );/* -* There are 16 pkeys supported in hardware. Three are -* allocated by the time we get here: -* 1. The default key (0) -* 2. One possibly consumed by an execute-only mapping. -* 3. One allocated by the test code and passed in via -* 'pkey' to this function. -* Ensure that we can allocate at least another 13 (16-3). +* There are NR_PKEYS pkeys supported in hardware. arch_reserved_keys() +* are reserved. And one key is allocated by the test code and passed +* in via 'pkey' to this function. */ -pkey_assert (i >=NR_PKEYS -3 ); +pkey_assert (i >= (NR_PKEYS -arch_reserved_keys ()-1 ));for (i =0 ; i < nr_allocated_pkeys; i++) { err =sys_pkey_free (allocated_pkeys[i]);
Signature · 1 line
1.7.1
Re: [PATCH v14 16/22] selftests/vm: fix an assertion in test_pkey_alloc_exhaust()
Quoted 4 lines · Ram Pai
The maximum number of keys that can be allocated has to take into consideration, that some keys are reserved by
the architecture for specific purpose. Hence cannot
be allocated.
Back to incomplete sentences, I see. :)
How about:
Some pkeys which are valid to the hardware are not available for application use. Those can not be allocated.
test_pkey_alloc_exhaust() tries to account for these but ___FILL_IN_WHAT_IT_DID_WRONG____. We fix this by ___FILL_IN_WAY_IT_WAS_FIXED____.
Quoted 14 lines
tools/testing/selftests/vm/protection_keys.c +0
-6@@ -1171,15 +1171,11 @@ void test_pkey_alloc_exhaust(int *ptr, u16 pkey)pkey_assert (i <NR_PKEYS *2 );/* -* There are 16 pkeys supported in hardware. Three are -* allocated by the time we get here: -* 1. The default key (0) -* 2. One possibly consumed by an execute-only mapping. -* 3. One allocated by the test code and passed in via -* 'pkey' to this function.
tools/testing/selftests/vm/protection_keys.c void test_pkey_alloc_exhaust(int *ptr, u16 pkey) · line 1181
Quoted 10 lines
tools/testing/selftests/vm/protection_keys.c +4
-2-* Ensure that we can allocate at least another 13 (16-3). +* There are NR_PKEYS pkeys supported in hardware. arch_reserved_keys() +* are reserved. And one key is allocated by the test code and passed +* in via 'pkey' to this function. */ -pkey_assert (i >=NR_PKEYS -3 ); +pkey_assert (i >= (NR_PKEYS -arch_reserved_keys ()-1 ));for (i =0 ; i < nr_allocated_pkeys; i++) { err =sys_pkey_free (allocated_pkeys[i]);
You also killed my nice, shiny, new comment. You made an attempt to make up for it two patches ago, but it pales in comparison to mine. The fact that I wrote it only a few short week ago makes me very attached to it, kinda like a new puppy. I don't want to throw it to the wolves quite yet. So, please preserve as much of it as possible, even if it has to live in the x86 header.
For bonus points, axe this comment in the same patch that you create the x86 header comment for easier review.
PATCH v14 17/22 selftests/vm: associate key on a mapped page and detect access violation
In reply to Ram Paidetect access-violation on a page to which access-disabled key is associated much after the page is mapped.
--- tools/testing/selftests/vm/protection_keys.c | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-)
tools/testing/selftests/vm/protection_keys.c +19 -0
@@ -1020,6 +1020,24 @@ void test_read_of_access_disabled_region(int *ptr, u16 pkey)dprintf1 ("*ptr: %d\n" , ptr_contents);expected_pkey_fault (pkey); } + +void test_read_of_access_disabled_region_with_page_already_mapped (int *ptr, +u16 pkey) +{ +int ptr_contents; + +dprintf1 ("disabling access to PKEY[%02d], doing read @ %p\n" , + pkey, ptr); + ptr_contents =read_ptr (ptr); +dprintf1 ("reading ptr before disabling the read : %d\n" , + ptr_contents); +read_pkey_reg (); +pkey_access_deny (pkey); + ptr_contents =read_ptr (ptr); +dprintf1 ("*ptr: %d\n" , ptr_contents); +expected_pkey_fault (pkey); +} +void test_write_of_write_disabled_region (int *ptr,u16 pkey) {dprintf1 ("disabling write access to PKEY[%02d], doing write\n" , pkey);
@@ -1397,6 +1415,7 @@ void test_mprotect_pkey_on_unsupported_cpu(int *ptr, u16 pkey)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, test_write_of_write_disabled_region, test_write_of_access_disabled_region, test_kernel_write_of_access_disabled_region,
Signature · 1 line
1.7.1
PATCH v14 18/22 selftests/vm: associate key on a mapped page and detect write violation
In reply to Ram Paidetect write-violation on a page to which write-disabled key is associated much after the page is mapped.
--- tools/testing/selftests/vm/protection_keys.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)
tools/testing/selftests/vm/protection_keys.c +12 -0
@@ -1038,6 +1038,17 @@ 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, +u16 pkey) +{ + *ptr = __LINE__; +dprintf1 ("disabling write access; after accessing the page, " +"to PKEY[%02d], doing write\n" , pkey); +pkey_write_deny (pkey); + *ptr = __LINE__; +expected_pkey_fault (pkey); +} +void test_write_of_write_disabled_region (int *ptr,u16 pkey) {dprintf1 ("disabling write access to PKEY[%02d], doing write\n" , pkey);
@@ -1417,6 +1428,7 @@ void test_mprotect_pkey_on_unsupported_cpu(int *ptr, u16 pkey) test_read_of_access_disabled_region, test_read_of_access_disabled_region_with_page_already_mapped, test_write_of_write_disabled_region, + test_write_of_write_disabled_region_with_page_already_mapped, test_write_of_access_disabled_region, test_kernel_write_of_access_disabled_region, test_kernel_write_of_write_disabled_region,
Signature · 1 line
1.7.1
PATCH v14 19/22 selftests/vm: detect write violation on a mapped access-denied-key page
In reply to Ram Paidetect write-violation on a page to which access-disabled key is associated much after the page is mapped.
--- tools/testing/selftests/vm/protection_keys.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
tools/testing/selftests/vm/protection_keys.c +13 -0
@@ -1063,6 +1063,18 @@ void test_write_of_access_disabled_region(int *ptr, u16 pkey) *ptr = __LINE__;expected_pkey_fault (pkey); } + +void test_write_of_access_disabled_region_with_page_already_mapped (int *ptr, +u16 pkey) +{ + *ptr = __LINE__; +dprintf1 ("disabling access; after accessing the page, " +" to PKEY[%02d], doing write\n" , pkey); +pkey_access_deny (pkey); + *ptr = __LINE__; +expected_pkey_fault (pkey); +} +void test_kernel_write_of_access_disabled_region (int *ptr,u16 pkey) {int ret;
@@ -1430,6 +1442,7 @@ void test_mprotect_pkey_on_unsupported_cpu(int *ptr, u16 pkey) test_write_of_write_disabled_region, test_write_of_write_disabled_region_with_page_already_mapped, test_write_of_access_disabled_region, + test_write_of_access_disabled_region_with_page_already_mapped, test_kernel_write_of_access_disabled_region, test_kernel_write_of_write_disabled_region, test_kernel_gup_of_access_disabled_region,
Signature · 1 line
1.7.1
PATCH v14 20/22 selftests/vm: testcases must restore pkey-permissions
In reply to Ram PaiGenerally the signal handler restores the state of the pkey register before returning. However there are times when the read/write operation can legitamely fail without invoking the signal handler. Eg: A sys_read() operaton to a write-protected page should be disallowed. In such a case the state of the pkey register is not restored to its original state. Test cases may not remember to restoring the key register state. During cleanup generically restore the key permissions.
--- tools/testing/selftests/vm/protection_keys.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
tools/testing/selftests/vm/protection_keys.c +5 -0
@@ -1476,8 +1476,13 @@ void run_tests_once(void) pkey_tests[test_nr](ptr, pkey);dprintf1 ("freeing test memory: %p\n" , ptr);free_pkey_malloc (ptr); + +/* restore the permission on the key after use */ +pkey_access_allow (pkey); +pkey_write_allow (pkey);sys_pkey_free (pkey); +dprintf1 ("pkey_faults: %d\n" , pkey_faults);dprintf1 ("orig_pkey_faults: %d\n" , orig_pkey_faults);
Signature · 1 line
1.7.1
Re: [PATCH v14 20/22] selftests/vm: testcases must restore pkey-permissions
Quoted 7 lines · Ram Pai
Generally the signal handler restores the state of the pkey register before returning. However there are times when the read/write operation can legitamely fail without invoking the signal handler. Eg: A
sys_read()operaton to a write-protected page should be disallowed. In such a case the state of the pkey register is not restored to its original state. Test cases may not remember to restoring the key register state. During cleanup generically restore the key permissions.
This would, indeed be a good thing to do for a well-behaved application.
But, for selftests, why does it matter what state we leave the key in? Doesn't the test itself need to establish permissions? Don't we *do* that at pkey_alloc() anyway?
What problem does this solve?
Quoted 17 lines
tools/testing/selftests/vm/protection_keys.c +5
-0@@ -1476,8 +1476,13 @@ void run_tests_once(void) pkey_tests[test_nr](ptr, pkey);dprintf1 ("freeing test memory: %p\n" , ptr);free_pkey_malloc (ptr); + +/* restore the permission on the key after use */ +pkey_access_allow (pkey); +pkey_write_allow (pkey);sys_pkey_free (pkey); +dprintf1 ("pkey_faults: %d\n" , pkey_faults);dprintf1 ("orig_pkey_faults: %d\n" , orig_pkey_faults);
PATCH v14 21/22 selftests/vm: sub-page allocator
In reply to Ram Paiintroduce a new allocator that allocates 4k hardware-pages to back 64k linux-page. This allocator is only applicable on powerpc.
--- tools/testing/selftests/vm/pkey-helpers.h | 6 ++++++ tools/testing/selftests/vm/pkey-powerpc.h | 25 +++++++++++++++++++++++++ tools/testing/selftests/vm/pkey-x86.h | 5 +++++ tools/testing/selftests/vm/protection_keys.c | 1 + 4 files changed, 37 insertions(+), 0 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +6 -0
@@ -28,6 +28,9 @@extern int dprint_in_signal;extern char dprint_in_signal_buffer[DPRINT_IN_SIGNAL_BUF_SIZE ]; +extern int test_nr; +extern int iteration_nr; +#ifdef __GNUC__ __attribute__((format(printf, 1, 2)))#endif
@@ -78,6 +81,9 @@ static inline void sigsafe_printf(const char *format, ...)void expected_pkey_fault (int pkey);int sys_pkey_alloc (unsigned long flags,u64 init_val);int sys_pkey_free (unsigned long 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);#if defined(__i386__) || defined(__x86_64__)/* arch */ #include "pkey-x86.h"
tools/testing/selftests/vm/pkey-powerpc.h +25 -0
@@ -100,4 +100,29 @@ void expect_fault_on_read_execonly_key(void *p1, int pkey)/* 8-bytes of instruction * 16384bytes = 1 page */ #define __page_o_noops () asm(".rept 16384 ; nop; .endr") +void *malloc_pkey_with_mprotect_subpage (long size,int prot,u16 pkey) +{ +void *ptr; +int ret; + +dprintf1 ("doing %s(size=%ld, prot=0x%x, pkey=%d)\n" , __func__, + size, prot, pkey); +pkey_assert (pkey <NR_PKEYS ); + ptr =mmap (NULL , size, prot,MAP_ANONYMOUS |MAP_PRIVATE ,-1 ,0 ); +pkey_assert (ptr != (void *)-1 ); + + ret =syscall (__NR_subpage_prot, ptr, size,NULL ); +if (ret) { +perror ("subpage_perm" ); +return PTR_ERR_ENOTSUP ; + } + + ret =mprotect_pkey ((void *)ptr,PAGE_SIZE , prot, pkey); +pkey_assert (!ret); +record_pkey_malloc (ptr, size, prot); + +dprintf1 ("%s() for pkey %d @ %p\n" , __func__, pkey, ptr); +return ptr; +} +#endif /* _PKEYS_POWERPC_H */
tools/testing/selftests/vm/pkey-x86.h +5 -0
@@ -176,4 +176,9 @@ 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) +{ +return PTR_ERR_ENOTSUP ; +} +#endif /* _PKEYS_X86_H */
tools/testing/selftests/vm/protection_keys.c +1 -0
@@ -883,6 +883,7 @@ void setup_hugetlbfs(void)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:
Signature · 1 line
1.7.1
PATCH v14 22/22 selftests/vm: test correct behavior of pkey-0
In reply to Ram PaiEnsure pkey-0 is allocated on start. Ensure pkey-0 can be attached dynamically in various modes, without failures. Ensure pkey-0 can be freed and allocated.
--- tools/testing/selftests/vm/protection_keys.c | 66 +++++++++++++++++++++++++- 1 files changed, 64 insertions(+), 2 deletions(-)
tools/testing/selftests/vm/protection_keys.c +64 -2
@@ -999,6 +999,67 @@ void close_test_fds(void)return *ptr; } +void test_pkey_alloc_free_attach_pkey0 (int *ptr,u16 pkey) +{ +int i, err; +int max_nr_pkey_allocs; +int alloced_pkeys[NR_PKEYS ]; +int nr_alloced =0 ; +int newpkey; +long size; + +assert (pkey_last_malloc_record); + size = pkey_last_malloc_record->size; +/* +* This is a bit of a hack. But mprotect() requires +* huge-page-aligned sizes when operating on hugetlbfs. +* So, make sure that we use something that's a multiple +* of a huge page when we can. +*/ +if (size >=HPAGE_SIZE ) + size =HPAGE_SIZE ; + + +/* allocate every possible key and make sure key-0 never got allocated */ + max_nr_pkey_allocs =NR_PKEYS ; +for (i =0 ; i < max_nr_pkey_allocs; i++) { +int new_pkey =alloc_pkey (); +assert (new_pkey !=0 ); + +if (new_pkey <0 ) +break ; + alloced_pkeys[nr_alloced++] = new_pkey; + } +/* free all the allocated keys */ +for (i =0 ; i < nr_alloced; i++) { +int free_ret; + +if (!alloced_pkeys[i]) +continue ; + free_ret =sys_pkey_free (alloced_pkeys[i]); +pkey_assert (!free_ret); + } + +/* attach key-0 in various modes */ + err =sys_mprotect_pkey (ptr, size,PROT_READ ,0 ); +pkey_assert (!err); + err =sys_mprotect_pkey (ptr, size,PROT_WRITE ,0 ); +pkey_assert (!err); + err =sys_mprotect_pkey (ptr, size,PROT_EXEC ,0 ); +pkey_assert (!err); + err =sys_mprotect_pkey (ptr, size,PROT_READ |PROT_WRITE ,0 ); +pkey_assert (!err); + err =sys_mprotect_pkey (ptr, size,PROT_READ |PROT_WRITE |PROT_EXEC ,0 ); +pkey_assert (!err); + +/* free key-0 */ + err =sys_pkey_free (0 ); +pkey_assert (!err); + + newpkey =sys_pkey_alloc (0 ,0x0 ); +assert (newpkey ==0 ); +} +void test_read_of_write_disabled_region (int *ptr,u16 pkey) {int ptr_contents;
@@ -1144,10 +1205,10 @@ void test_kernel_gup_write_to_write_disabled_region(int *ptr, u16 pkey)void test_pkey_syscalls_on_non_allocated_pkey (int *ptr,u16 pkey) {int err; -int i =get_start_key (); +int i;/* Note: 0 is the default pkey, so don't mess with it */ -for (; i <NR_PKEYS ; i++) { +for (i=1 ; i <NR_PKEYS ; i++) {if (pkey == i)continue ;
@@ -1455,6 +1516,7 @@ void test_mprotect_pkey_on_unsupported_cpu(int *ptr, u16 pkey) test_pkey_syscalls_on_non_allocated_pkey, test_pkey_syscalls_bad_args, test_pkey_alloc_exhaust, + test_pkey_alloc_free_attach_pkey0, };void run_tests_once (void )
Signature · 1 line
1.7.1
Re: [PATCH v14 22/22] selftests/vm: test correct behavior of pkey-0
Quoted 33 lines · Ram Pai
Ensure pkey-0 is allocated on start. Ensure pkey-0 can be attached dynamically in various modes, without failures. Ensure pkey-0 can be freed and allocated.
Signed-off-by Ram Pai <[email protected]>--- tools/testing/selftests/vm/protection_keys.c | 66 +++++++++++++++++++++++++- 1 files changed, 64 insertions(+), 2 deletions(-)tools/testing/selftests/vm/protection_keys.c +16
-0@@ -999,6 +999,67 @@ void close_test_fds(void)return *ptr; } +void test_pkey_alloc_free_attach_pkey0 (int *ptr,u16 pkey) +{ +int i, err; +int max_nr_pkey_allocs; +int alloced_pkeys[NR_PKEYS ]; +int nr_alloced =0 ; +int newpkey; +long size; + +assert (pkey_last_malloc_record); + size = pkey_last_malloc_record->size; +/* +* This is a bit of a hack. But mprotect() requires +* huge-page-aligned sizes when operating on hugetlbfs. +* So, make sure that we use something that's a multiple +* of a huge page when we can.
tools/testing/selftests/vm/protection_keys.c void close_test_fds(void) · line 1027
Quoted 10 lines
tools/testing/selftests/vm/protection_keys.c +10
-0+*/ +if (size >=HPAGE_SIZE ) + size =HPAGE_SIZE ; + + +/* allocate every possible key and make sure key-0 never got allocated */ + max_nr_pkey_allocs =NR_PKEYS ; +for (i =0 ; i < max_nr_pkey_allocs; i++) { +int new_pkey =alloc_pkey (); +assert (new_pkey !=0 );
Missed these earlier. This needs to be pkey_assert(). We don't want these tests to ever _actually_ crash.
tools/testing/selftests/vm/protection_keys.c void close_test_fds(void) · line 1053
Quoted 11 lines
tools/testing/selftests/vm/protection_keys.c +11
-0+/* attach key-0 in various modes */ + err =sys_mprotect_pkey (ptr, size,PROT_READ ,0 ); +pkey_assert (!err); + err =sys_mprotect_pkey (ptr, size,PROT_WRITE ,0 ); +pkey_assert (!err); + err =sys_mprotect_pkey (ptr, size,PROT_EXEC ,0 ); +pkey_assert (!err); + err =sys_mprotect_pkey (ptr, size,PROT_READ |PROT_WRITE ,0 ); +pkey_assert (!err); + err =sys_mprotect_pkey (ptr, size,PROT_READ |PROT_WRITE |PROT_EXEC ,0 ); +pkey_assert (!err);
This is all fine.
tools/testing/selftests/vm/protection_keys.c void close_test_fds(void) · line 1057
Quoted 3 lines
tools/testing/selftests/vm/protection_keys.c +3
-0+/* free key-0 */ + err =sys_pkey_free (0 ); +pkey_assert (!err);
This part is called out as undefined behavior in the manpage:
tools/testing/selftests/vm/protection_keys.c void close_test_fds(void)
Quoted 4 lines
tools/testing/selftests/vm/protection_keys.c +0
-0An application should not call pkey_free() on any protection key which has been assigned to an address range by pkey_mprotect(2) and which is still in use. The behavior in this case is undefined and may result in an error.
I don't think we should be testing for undefined behavior.
Quoted 9 lines
tools/testing/selftests/vm/protection_keys.c +4
-0+ newpkey =sys_pkey_alloc (0 ,0x0 ); +assert (newpkey ==0 ); +} +void test_read_of_write_disabled_region (int *ptr,u16 pkey) {int ptr_contents;@@ -1144,10 +1205,10 @@ void test_kernel_gup_write_to_write_disabled_region(int *ptr, u16 pkey)void test_pkey_syscalls_on_non_allocated_pkey (int *ptr,u16 pkey)
tools/testing/selftests/vm/protection_keys.c void test_kernel_gup_write_to_write_disabled_region(int *ptr, u16 pkey) · line 1213
Quoted 10 lines
tools/testing/selftests/vm/protection_keys.c +2
-2{int err; -int i =get_start_key (); +int i;/* Note: 0 is the default pkey, so don't mess with it */ -for (; i <NR_PKEYS ; i++) { +for (i=1 ; i <NR_PKEYS ; i++) {if (pkey == i)continue ;
This seems to be randomly reverting earlier changes.