Skip to contentSkip to conversation

selftests, powerpc, x86 : Memory Protection Keys

PATCH In review Ram Pai 2 participants 39 messages Jul 17, 2018 – Jul 18, 2018

Subsystems (4)
lore
Share & tools
lore

Ram Pai

author
#0 lore raw

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
author
#1 lore raw

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
index 342c7bc..0214fbf 100644
@@ -12,3 +12,4 @@ mlock-random-test
 virtual_address_range
 gup_benchmark
 va_128TBswitch
+protection_keys
tools/testing/selftests/vm/Makefile +1 -0
index fdefa22..9788a58 100644
@@ -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
index 7757f73..eb30ffd 100644
@@ -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.

author
#3 lore raw

PATCH v14 2/22 selftests/vm: rename all references to pkru to a generic name

In reply to Ram Pai

some 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
index 254e543..d5779be 100644
@@ -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;
 
 	asm volatile(".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);
 	asm volatile(".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
author
#4 lore raw

PATCH v14 3/22 selftests/vm: move generic definitions to header file

In reply to Ram Pai

Moved 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
index d5779be..6ad1bd5 100644
@@ -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
index 9f373cc..cad52dc 100644
@@ -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
author
#6 lore raw

PATCH v14 4/22 selftests/vm: move arch-specific definitions to arch-specific header

In reply to Ram Pai

From: 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
index 6ad1bd5..3ed2f02 100644
@@ -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;
-
-	asm volatile(".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);
-	asm volatile(".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. */
-	asm volatile(
-		"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
Acked-by on v14 4/22
#7 lore raw

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.

author
#8 lore raw

PATCH v14 5/22 selftests/vm: Make gcc check arguments of sigsafe_printf()

In reply to Ram Pai

From: 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
index 3ed2f02..7f18a82 100644
@@ -27,6 +27,10 @@
 #define DPRINT_IN_SIGNAL_BUF_SIZE 4096
 extern 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
author
#9 lore raw

PATCH v14 6/22 selftests/vm: typecast the pkey register

In reply to Ram Pai

This 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
index 7f18a82..2a1a024 100644
@@ -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
index 2f04ade..5f40901 100644
@@ -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;
 
 	asm volatile(".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);
 	asm volatile(".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
Acked-by on v14 6/22
#10 lore raw

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.

author
#11 lore raw

PATCH v14 7/22 selftests/vm: generic function to handle shadow key register

In reply to Ram Pai

helper 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
index 2a1a024..ada0146 100644
@@ -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
index 5f40901..2b3780d 100644
@@ -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
index d95fe20..fb7dd32 100644
@@ -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
#12 lore raw

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.

author
#13 lore raw

PATCH v14 8/22 selftests/vm: fix the wrong assert in pkey_disable_set()

In reply to 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.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/protection_keys.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
tools/testing/selftests/vm/protection_keys.c +1 -1
index fb7dd32..2dd94c3 100644
@@ -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
#14 lore raw

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 ..."

author
#15 lore raw

PATCH v14 9/22 selftests/vm: fixed bugs in pkey_disable_clear()

In reply to Ram Pai

instead 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.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/protection_keys.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
tools/testing/selftests/vm/protection_keys.c +2 -2
index 2dd94c3..8fa4f74 100644
@@ -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
Acked-by on v14 9/22
#16 lore raw

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]>
---
 tools/testing/selftests/vm/protection_keys.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
tools/testing/selftests/vm/protection_keys.c +1 -1
index 2dd94c3..8fa4f74 100644
@@ -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 -1
 	if (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.

author
#17 lore raw

PATCH v14 10/22 selftests/vm: fix alloc_random_pkey() to make it really random

In reply to Ram Pai

alloc_random_pkey() was allocating the same pkey every time. Not all pkeys were geting tested. fixed it.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/protection_keys.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
tools/testing/selftests/vm/protection_keys.c +5 -1
index 8fa4f74..2565b4c 100644
@@ -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
Acked-by on v14 10/22
#18 lore raw

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:

author
#19 lore raw

PATCH v14 11/22 selftests/vm: introduce two arch independent abstraction

In reply to Ram Pai

open_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
index ada0146..52a1152 100644
@@ -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
index 2b3780d..d5fa299 100644
@@ -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
index 2565b4c..2e448e0 100644
@@ -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
#20 lore raw

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
---
 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
index ada0146..52a1152 100644

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
index 2b3780d..d5fa299 100644
@@ -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
index 2565b4c..2e448e0 100644

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.

author
#21 lore raw

PATCH v14 12/22 selftests/vm: pkey register should match shadow pkey

In reply to 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

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/protection_keys.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
tools/testing/selftests/vm/protection_keys.c +2 -2
index 2e448e0..f50cce8 100644
@@ -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
#22 lore raw

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]>
---
 tools/testing/selftests/vm/protection_keys.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
tools/testing/selftests/vm/protection_keys.c +0 -0
index 2e448e0..f50cce8 100644

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?

author
#23 lore raw

PATCH v14 13/22 selftests/vm: generic cleanup

In reply to Ram Pai

cleanup the code to satisfy coding styles.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/protection_keys.c |   64 +++++++++++++++++--------
 1 files changed, 43 insertions(+), 21 deletions(-)
tools/testing/selftests/vm/protection_keys.c +43 -21
index f50cce8..304f74f 100644
@@ -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
#24 lore raw

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]>
---
 tools/testing/selftests/vm/protection_keys.c |   64 +++++++++++++++++--------
 1 files changed, 43 insertions(+), 21 deletions(-)
tools/testing/selftests/vm/protection_keys.c +0 -0
index f50cce8..304f74f 100644

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 -0
 	pkey_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 -1
 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;

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?

author
#25 lore raw

PATCH v14 14/22 selftests/vm: Introduce generic abstractions

In reply to Ram Pai

Introduce 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
index 52a1152..321bbbd 100644
@@ -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
index d5fa299..f5d0ff2 100644
@@ -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
#26 lore raw

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
index 304f74f..18e1bb7 100644
@@ -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 -5
 void 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.

author
#27 lore raw

PATCH v14 15/22 selftests/vm: powerpc implementation to check support for pkey

In reply to Ram Pai

pkey subsystem is supported if the hardware and kernel has support. We determine that by checking if allocation of a key succeeds or not.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 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
index 321bbbd..288ccff 100644
@@ -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
index b0d1abe..b649e85 100644
@@ -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
index f5d0ff2..887acf2 100644
@@ -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
index 18e1bb7..d27fa5e 100644
@@ -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
#28 lore raw

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
index f5d0ff2..887acf2 100644
@@ -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
index 18e1bb7..d27fa5e 100644

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;

}

author
#29 lore raw

PATCH v14 16/22 selftests/vm: fix an assertion in test_pkey_alloc_exhaust()

In reply to 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.

Fix the assertion in test_pkey_alloc_exhaust()

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/protection_keys.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)
tools/testing/selftests/vm/protection_keys.c +4 -8
index d27fa5e..67d841e 100644
@@ -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
#30 lore raw

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
index d27fa5e..67d841e 100644
@@ -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.

author
#31 lore raw

PATCH v14 17/22 selftests/vm: associate key on a mapped page and detect access violation

In reply to Ram Pai

detect 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
index 67d841e..d41b2dc 100644
@@ -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
author
#32 lore raw

PATCH v14 18/22 selftests/vm: associate key on a mapped page and detect write violation

In reply to Ram Pai

detect 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
index d41b2dc..59f6f33 100644
@@ -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
author
#33 lore raw

PATCH v14 19/22 selftests/vm: detect write violation on a mapped access-denied-key page

In reply to Ram Pai

detect 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
index 59f6f33..8a6afdd 100644
@@ -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
author
#34 lore raw

PATCH v14 20/22 selftests/vm: testcases must restore pkey-permissions

In reply to 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.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/protection_keys.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
tools/testing/selftests/vm/protection_keys.c +5 -0
index 8a6afdd..ea3cf04 100644
@@ -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
#35 lore raw

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
index 8a6afdd..ea3cf04 100644
@@ -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);
author
#36 lore raw

PATCH v14 21/22 selftests/vm: sub-page allocator

In reply to Ram Pai

introduce 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
index 288ccff..a00eee6 100644
@@ -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
index b649e85..ab60f74 100644
@@ -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
index 887acf2..a7e4648 100644
@@ -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
index ea3cf04..569faf1 100644
@@ -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
author
#37 lore raw

PATCH v14 22/22 selftests/vm: test correct behavior of pkey-0

In reply to 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.

---
 tools/testing/selftests/vm/protection_keys.c |   66 +++++++++++++++++++++++++-
 1 files changed, 64 insertions(+), 2 deletions(-)
tools/testing/selftests/vm/protection_keys.c +64 -2
index 569faf1..156b449 100644
@@ -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
#38 lore raw

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.

---
 tools/testing/selftests/vm/protection_keys.c |   66 +++++++++++++++++++++++++-
 1 files changed, 64 insertions(+), 2 deletions(-)
tools/testing/selftests/vm/protection_keys.c +16 -0
index 569faf1..156b449 100644
@@ -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 -0
       An 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.