Skip to contentSkip to conversation

selftests, powerpc, x86 : Memory Protection Keys

PATCH Superseded Ram Pai 3 participants 49 messages Jun 14, 2018 – Jul 17, 2018

Subsystems (4)

v13 of 14 Superseded Latest is v14 →

lore
Share & tools
lore

Ram Pai

author
#0 lore raw

v13 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. Need help verifying on x86. Compiles on x86.

History:
-------

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 (22):
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: clear the bits in shadow reg when a pkey is freed. 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: powerpc implementation for generic abstraction selftests/vm: clear the bits in shadow reg when a pkey is freed. 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     |  126 ++
 tools/testing/selftests/vm/pkey-x86.h         |  184 +++
 tools/testing/selftests/vm/protection_keys.c  | 1598 +++++++++++++++++++++++++
 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, 2124 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 v13 1/24 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
#2 lore raw

PATCH v13 2/24 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
#3 lore raw

PATCH v13 3/24 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
#4 lore raw

PATCH v13 4/24 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
author
#5 lore raw

PATCH v13 5/24 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
#6 lore raw

PATCH v13 6/24 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
author
#7 lore raw

PATCH v13 7/24 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 90b3a41..57340b3 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
author
#8 lore raw

PATCH v13 8/24 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 57340b3..5fcccdb 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
#9 lore raw

Re: [PATCH v13 08/24] 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 6 lines
tools/testing/selftests/vm/protection_keys.c +1 -1
 	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);
 }

This is the kind of thing where I'd love to hear the motivation and background. This "disable a key that was already disabled" operation obviously doesn't happen today. What motivated you to change it now?

author
#10 lore raw

Re: [PATCH v13 08/24] selftests/vm: fix the wrong assert in pkey_disable_set()

Quoted 5 lines · Dave Hansen
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

...

Quoted 10 lines

tools/testing/selftests/vm/protection_keys.c void pkey_disable_set(int pkey, int flags) · line 421

Quoted 6 lines
tools/testing/selftests/vm/protection_keys.c +1 -1
 	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);
 }

This is the kind of thing where I'd love to hear the motivation and background. This "disable a key that was already disabled" operation obviously doesn't happen today. What motivated you to change it now?

On powerpc, hardware supports READ_DISABLE and WRITE_DISABLE. ACCESS_DISABLE is basically READ_DISABLE|WRITE_DISABLE on powerpc.

If access disable is called on a key followed by a write disable, the second operation becomes a nop. In such cases,

       read_pkey_reg() == orig_pkey_reg

Hence the code above is modified to

	pkey_assert(read_pkey_reg() >= orig_pkey_reg);
Signature · 1 line
Ram Pai
#11 lore raw

Re: [PATCH v13 08/24] selftests/vm: fix the wrong assert in pkey_disable_set()

Quoted 18 lines · Ram Pai
Quoted 5 lines · Dave Hansen
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

...

Quoted 9 lines

tools/testing/selftests/vm/protection_keys.c void pkey_disable_set(int pkey, int flags) · line 421

Quoted 6 lines
tools/testing/selftests/vm/protection_keys.c +1 -1
 	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);
 }

This is the kind of thing where I'd love to hear the motivation and background. This "disable a key that was already disabled" operation obviously doesn't happen today. What motivated you to change it now?

On powerpc, hardware supports READ_DISABLE and WRITE_DISABLE. ACCESS_DISABLE is basically READ_DISABLE|WRITE_DISABLE on powerpc.

Quoted 6 lines

If access disable is called on a key followed by a write disable, the second operation becomes a nop. In such cases,

       read_pkey_reg() == orig_pkey_reg

Hence the code above is modified to

	pkey_assert(read_pkey_reg() >= orig_pkey_reg);

Makes sense. Do we have a comment for that now?

author
#12 lore raw

PATCH v13 9/24 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 5fcccdb..da4f5d5 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
author
#13 lore raw

PATCH v13 10/24 selftests/vm: clear the bits in shadow reg when a pkey is freed.

In reply to Ram Pai

When a key is freed, the key is no more effective. Clear the bits corresponding to the pkey in the shadow register. Otherwise it will carry some spurious bits which can trigger false-positive asserts.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/protection_keys.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
tools/testing/selftests/vm/protection_keys.c +3 -0
index da4f5d5..42a91c7 100644
@@ -556,6 +556,9 @@ int alloc_pkey(void)
 int sys_pkey_free(unsigned long pkey)
 {
 	int ret = syscall(SYS_pkey_free, pkey);
+
+	if (!ret)
+		shadow_pkey_reg &= clear_pkey_flags(pkey, PKEY_DISABLE_ACCESS);
 	dprintf1("%s(pkey=%ld) syscall ret: %d\n", __func__, pkey, ret);
 	return ret;
 }
Signature · 1 line
1.7.1
#14 lore raw

Re: [PATCH v13 10/24] selftests/vm: clear the bits in shadow reg when a pkey is freed.

Quoted 4 lines · Ram Pai

When a key is freed, the key is no more effective. Clear the bits corresponding to the pkey in the shadow register. Otherwise it will carry some spurious bits which can trigger false-positive asserts.

...--- a/tools/testing/selftests/vm/protection_keys.c

tools/testing/selftests/vm/protection_keys.c int alloc_pkey(void) · line 564

Quoted 11 lines

+++ b/tools/testing/selftests/vm/protection_keys.c

tools/testing/selftests/vm/protection_keys.c +3 -0
@@ -556,6 +556,9 @@ int alloc_pkey(void)
 int sys_pkey_free(unsigned long pkey)
 {
 	int ret = syscall(SYS_pkey_free, pkey);
+
+	if (!ret)
+		shadow_pkey_reg &= clear_pkey_flags(pkey, PKEY_DISABLE_ACCESS);
 	dprintf1("%s(pkey=%ld) syscall ret: %d\n", __func__, pkey, ret);
 	return ret;
 }

This would be great code for an actual application. But, I'm not immediately convinced we want sane, kind behavior in our selftest. x86 doesn't clear the hardware register at pkey_free, so wouldn't this cause the shadow and the hardware register to diverge?

author
#15 lore raw

Re: [PATCH v13 10/24] selftests/vm: clear the bits in shadow reg when a pkey is freed.

Quoted 6 lines · Dave Hansen
Quoted 4 lines · Ram Pai

When a key is freed, the key is no more effective. Clear the bits corresponding to the pkey in the shadow register. Otherwise it will carry some spurious bits which can trigger false-positive asserts.

...--- a/tools/testing/selftests/vm/protection_keys.c

Quoted 16 lines

tools/testing/selftests/vm/protection_keys.c int alloc_pkey(void) · line 564

Quoted 11 lines

+++ b/tools/testing/selftests/vm/protection_keys.c

tools/testing/selftests/vm/protection_keys.c +3 -0
@@ -556,6 +556,9 @@ int alloc_pkey(void)
 int sys_pkey_free(unsigned long pkey)
 {
 	int ret = syscall(SYS_pkey_free, pkey);
+
+	if (!ret)
+		shadow_pkey_reg &= clear_pkey_flags(pkey, PKEY_DISABLE_ACCESS);
 	dprintf1("%s(pkey=%ld) syscall ret: %d\n", __func__, pkey, ret);
 	return ret;
 }

This would be great code for an actual application. But, I'm not immediately convinced we want sane, kind behavior in our selftest. x86 doesn't clear the hardware register at pkey_free, so wouldn't this cause the shadow and the hardware register to diverge?

Have deleted the code in the newer version.

RP

author
#16 lore raw

PATCH v13 11/24 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 42a91c7..c5f9776 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>
@@ -576,13 +577,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;
@@ -598,6 +601,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
author
#17 lore raw

PATCH v13 12/24 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 c5f9776..9afe894 100644
@@ -791,7 +791,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;
@@ -1078,10 +1078,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
author
#18 lore raw

PATCH v13 13/24 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 9afe894..adcae4a 100644
@@ -916,10 +916,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
#19 lore raw

Re: [PATCH v13 13/24] selftests/vm: pkey register should match shadow pkey

Quoted 4 lines · Ram Pai

+++ b/tools/testing/selftests/vm/protection_keys.c

tools/testing/selftests/vm/protection_keys.c +0 -0
@@ -916,10 +916,10 @@ void expected_pkey_fault(int pkey)
 		pkey_assert(last_si_pkey == pkey);
 

tools/testing/selftests/vm/protection_keys.c void expected_pkey_fault(int pkey) · line 925

Quoted 10 lines
tools/testing/selftests/vm/protection_keys.c +2 -2
 	/*
-	 * 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);

I think this is wrong on x86.

When we leave the signal handler, we zero out PKRU so that the faulting instruction can continue, that's why we have the check against zero. I'm actually kinda surprised this works.

Logically, this patch does:

	if (hardware != shadow)
		error();
	hardware = shadow;

That does not look right to me. What we want is:

	if (hardware != signal_return_pkey_reg)
		error();
	hardware = shadow;
author
#20 lore raw

Re: [PATCH v13 13/24] selftests/vm: pkey register should match shadow pkey

Quoted 14 lines · Dave Hansen
Quoted 13 lines · Ram Pai

+++ b/tools/testing/selftests/vm/protection_keys.c

tools/testing/selftests/vm/protection_keys.c +2 -2
@@ -916,10 +916,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);
 
Quoted 7 lines

tools/testing/selftests/vm/protection_keys.c void expected_pkey_fault(int pkey) · line 925

Quoted 1 line
tools/testing/selftests/vm/protection_keys.c +0 -0
 	__write_pkey_reg(shadow_pkey_reg);

I think this is wrong on x86.

When we leave the signal handler, we zero out PKRU so that the faulting instruction can continue, that's why we have the check against zero. I'm actually kinda surprised this works.

The code is modified to zero out only the violated key in the signal handler. Hence it works. Have verified it to work on x86 aswell.

RP

author
#21 lore raw

PATCH v13 14/24 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 adcae4a..f43a319 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);
@@ -672,7 +680,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);
@@ -698,9 +708,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) ||
@@ -781,11 +793,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
@@ -806,7 +820,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;
 	}
@@ -948,6 +963,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;
 }
@@ -989,7 +1005,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);
@@ -1111,13 +1128,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
@@ -1125,9 +1143,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;
 		}
@@ -1419,7 +1439,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++;
@@ -1431,7 +1452,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;
@@ -1439,7 +1460,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
#22 lore raw

Re: [PATCH v13 14/24] selftests/vm: generic cleanup

Quoted 1 line · Ram Pai

cleanup the code to satisfy coding styles.

A lot of this makes the code look worse and more unreadable than before.
I think someone just ran it through lindent or something.

I also took a few CodingStyle liberties in here because it's _not_ main kernel code. I think the occasional 85-column line is probably OK in here.

author
#23 lore raw

PATCH v13 15/24 selftests/vm: powerpc implementation for generic abstraction

In reply to Ram Pai

Introduce powerpc implementation for the various
abstractions.

---
 tools/testing/selftests/vm/pkey-helpers.h    |   16 ++++-
 tools/testing/selftests/vm/pkey-powerpc.h    |   91 ++++++++++++++++++++++++++
 tools/testing/selftests/vm/pkey-x86.h        |   15 ++++
 tools/testing/selftests/vm/protection_keys.c |   62 ++++++++++--------
 4 files changed, 156 insertions(+), 28 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 +91 -0

91 lines not on this page · raw message

tools/testing/selftests/vm/pkey-x86.h +15 -0
index d5fa299..95ee952 100644
@@ -42,6 +42,7 @@
 #endif
 
 #define NR_PKEYS		16
+#define NR_RESERVED_PKEYS	1
 #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, u16 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 +35 -27

124 lines not on this page · raw message

Signature · 1 line
1.7.1
#24 lore raw

Re: [PATCH v13 15/24] selftests/vm: powerpc implementation for generic abstraction

tools/testing/selftests/vm/pkey-helpers.h static inline int open_hugepage_file(int flag) · line 204

Quoted 8 lines
tools/testing/selftests/vm/pkey-helpers.h +7 -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
 }

FWIW, this isn't ppc-specific.

Quoted 55 lines
tools/testing/selftests/vm/protection_keys.c +10 -10
index f43a319..88dfa40 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);
 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,28 @@ void signal_handler(int signum, siginfo_t *si, void *vucontext)
 	 * state.  We just assume that it is here.
 	 */
 	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()]);

tools/testing/selftests/vm/protection_keys.c void signal_handler(int signum, siginfo_t *si, void *vucontext) · line 247

Quoted 10 lines
tools/testing/selftests/vm/protection_keys.c +2 -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__) */

The series up to this point has been looking pretty nice and broken out and easy to read. It goes off the rails a bit here. Adding #ifdefs and..

Quoted 13 lines
tools/testing/selftests/vm/protection_keys.c +9 -0
+	dprintf1("siginfo: %p\n", si);
+	dprintf1(" fpregs: %p\n", fpregs);
+
+	si_pkey_ptr = siginfo_get_pkey_ptr(si);
+	dprintf1("si_pkey_ptr: %p\n", si_pkey_ptr);
+	dump_mem(si_pkey_ptr - 8, 24);
+	siginfo_pkey = *si_pkey_ptr;
+	pkey_assert(siginfo_pkey < NR_PKEYS);
+	last_si_pkey = siginfo_pkey;
 
 	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 260

Quoted 9 lines
tools/testing/selftests/vm/protection_keys.c +0 -6
@@ -252,22 +263,21 @@ void signal_handler(int signum, siginfo_t *si, void *vucontext)
 		exit(4);
 	}
 
-	si_pkey_ptr = (u32 *)(((u8 *)si) + si_pkey_offset);
-	dprintf1("si_pkey_ptr: %p\n", si_pkey_ptr);
-	dump_mem((u8 *)si_pkey_ptr - 8, 24);
-	siginfo_pkey = *si_pkey_ptr;
-	pkey_assert(siginfo_pkey < NR_PKEYS);
-	last_si_pkey = siginfo_pkey;

Moving random code around with no explanation.

Quoted 22 lines
tools/testing/selftests/vm/protection_keys.c +9 -3
-	dprintf1("signal pkey_reg from xsave: "PKEY_REG_FMT"\n", *pkey_reg_ptr);
 	/*
 	 * need __read_pkey_reg() version so we do not do shadow_pkey_reg
 	 * checking
 	 */
 	dprintf1("signal pkey_reg from  pkey_reg: "PKEY_REG_FMT"\n",
 			__read_pkey_reg());
-	dprintf1("pkey from siginfo: %jx\n", siginfo_pkey);
-	*(u64 *)pkey_reg_ptr = 0x00000000;
+#if defined(__i386__) || defined(__x86_64__) /* arch */
+	dprintf1("signal pkey_reg from xsave: "PKEY_REG_FMT"\n", *pkey_reg_ptr);
+	*(u64 *)pkey_reg_ptr &= clear_pkey_flags(siginfo_pkey,
+			PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE);
+#elif __powerpc64__
+	pkey_access_allow(siginfo_pkey);
+#endif
+	shadow_pkey_reg &= clear_pkey_flags(siginfo_pkey,
+			PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE);
 	dprintf1("WARNING: set PKEY_REG=0 to allow faulting instruction "
 			"to continue\n");
 	pkey_faults++;

tools/testing/selftests/vm/protection_keys.c void test_executing_on_unreadable_memory(int *ptr, u16 pkey) · line 1346

Quoted 9 lines
tools/testing/selftests/vm/protection_keys.c +2 -3
@@ -1331,9 +1341,8 @@ void test_executing_on_unreadable_memory(int *ptr, u16 pkey)
 	madvise(p1, PAGE_SIZE, MADV_DONTNEED);
 	lots_o_noops_around_write(&scratch);
 	do_not_expect_pkey_fault("executing on PROT_EXEC memory");
-	ptr_contents = read_ptr(p1);
-	dprintf2("ptr (%p) contents@%d: %x\n", p1, __LINE__, ptr_contents);
-	expected_pkey_fault(pkey);
+
+	expect_fault_on_read_execonly_key(p1, pkey);
 }

While none of this is a deal-breaker (as I said, I feel like the selftests/ rules are a bit more lax) this does kinda break the illusion of a nice, broken out series.

Could you address this a bit in the changelog at least, please?

author
#25 lore raw

PATCH v13 16/24 selftests/vm: clear the bits in shadow reg when a pkey is freed.

In reply to Ram Pai

When a key is freed, the key is no more effective. Clear the bits corresponding to the pkey in the shadow register. Otherwise it will carry some spurious bits which can trigger false-positive asserts.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/protection_keys.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
tools/testing/selftests/vm/protection_keys.c +2 -1
index 88dfa40..ba184ca 100644
@@ -577,7 +577,8 @@ int sys_pkey_free(unsigned long pkey)
 	int ret = syscall(SYS_pkey_free, pkey);
 
 	if (!ret)
-		shadow_pkey_reg &= clear_pkey_flags(pkey, PKEY_DISABLE_ACCESS);
+		shadow_pkey_reg &= clear_pkey_flags(pkey,
+				PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE);
 	dprintf1("%s(pkey=%ld) syscall ret: %d\n", __func__, pkey, ret);
 	return ret;
 }
Signature · 1 line
1.7.1
#26 lore raw

Re: [PATCH v13 16/24] selftests/vm: clear the bits in shadow reg when a pkey is freed.

tools/testing/selftests/vm/protection_keys.c int sys_pkey_free(unsigned long pkey) · line 584

Quoted 12 lines · Ram Pai
tools/testing/selftests/vm/protection_keys.c +2 -1
@@ -577,7 +577,8 @@ int sys_pkey_free(unsigned long pkey)
 	int ret = syscall(SYS_pkey_free, pkey);
 
 	if (!ret)
-		shadow_pkey_reg &= clear_pkey_flags(pkey, PKEY_DISABLE_ACCESS);
+		shadow_pkey_reg &= clear_pkey_flags(pkey,
+				PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE);
 	dprintf1("%s(pkey=%ld) syscall ret: %d\n", __func__, pkey, ret);
 	return ret;
 }

Why did you introduce this code earlier and then modify it now?

BTW, my original aversion to this code still stands.

author
#27 lore raw

Re: [PATCH v13 16/24] selftests/vm: clear the bits in shadow reg when a pkey is freed.

Quoted 16 lines · Dave Hansen

tools/testing/selftests/vm/protection_keys.c int sys_pkey_free(unsigned long pkey) · line 584

Quoted 12 lines · Ram Pai
tools/testing/selftests/vm/protection_keys.c +2 -1
@@ -577,7 +577,8 @@ int sys_pkey_free(unsigned long pkey)
 	int ret = syscall(SYS_pkey_free, pkey);
 
 	if (!ret)
-		shadow_pkey_reg &= clear_pkey_flags(pkey, PKEY_DISABLE_ACCESS);
+		shadow_pkey_reg &= clear_pkey_flags(pkey,
+				PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE);
 	dprintf1("%s(pkey=%ld) syscall ret: %d\n", __func__, pkey, ret);
 	return ret;
 }

Why did you introduce this code earlier and then modify it now?

Quoted 1 line

BTW, my original aversion to this code still stands.

Have entirely got rid of this code in the new version.

Signature · 1 line
Ram Pai
author
#28 lore raw

PATCH v13 17/24 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 ec6f5d7..957f6f6 100644
@@ -62,9 +62,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 95ee952..6820c10 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 ba184ca..cb81a47 100644
@@ -1393,8 +1393,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;
 	}
 
@@ -1458,12 +1458,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
#29 lore raw

Re: [PATCH v13 17/24] selftests/vm: powerpc implementation to check support for pkey

tools/testing/selftests/vm/protection_keys.c void test_mprotect_pkey_on_unsupported_cpu(int *ptr, u16 pkey) · line 1399

Quoted 6 lines
tools/testing/selftests/vm/protection_keys.c +2 -2
-	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 kinda wanted a specific message for when the *CPU* doesn't support the feature.

author
#30 lore raw

Re: [PATCH v13 17/24] selftests/vm: powerpc implementation to check support for pkey

Quoted 9 lines · Dave Hansen

tools/testing/selftests/vm/protection_keys.c void test_mprotect_pkey_on_unsupported_cpu(int *ptr, u16 pkey) · line 1399

Quoted 6 lines
tools/testing/selftests/vm/protection_keys.c +2 -2
-	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 kinda wanted a specific message for when the *CPU* doesn't support the feature.

is_pkey_supported() x86 implementation has specific messages. it will print if the CPU doesn't support the feature.

RP

Signature · 1 line
Ram Pai
author
#31 lore raw

PATCH v13 18/24 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 |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)
tools/testing/selftests/vm/protection_keys.c +5 -8
index cb81a47..e8ad970 100644
@@ -1175,15 +1175,12 @@ 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. One of which is the default key(0). One can be taken
+	 * up by an execute-only mapping.
+	 * Ensure that we can allocate at least the remaining.
 	 */
-	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
#32 lore raw

Re: [PATCH v13 18/24] selftests/vm: fix an assertion in test_pkey_alloc_exhaust()

Quoted 5 lines · Ram Pai
tools/testing/selftests/vm/protection_keys.c +0 -4
 	/*
-	 * 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.

tools/testing/selftests/vm/protection_keys.c void test_pkey_alloc_exhaust(int *ptr, u16 pkey) · line 1183

Quoted 10 lines
tools/testing/selftests/vm/protection_keys.c +5 -4
-	 *   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. One of which is the default key(0). One can be taken
+	 * up by an execute-only mapping.
+	 * Ensure that we can allocate at least the remaining.
 	 */
-	pkey_assert(i >= NR_PKEYS-3);
+	pkey_assert(i >= (NR_PKEYS-arch_reserved_keys()-1));

We recently had a bug here. I fixed it and left myself a really nice comment so I and others wouldn't screw it up in the future.

Does this kill my nice, new comment?

author
#33 lore raw

Re: [PATCH v13 18/24] selftests/vm: fix an assertion in test_pkey_alloc_exhaust()

Quoted 20 lines · Dave Hansen
Quoted 5 lines · Ram Pai
tools/testing/selftests/vm/protection_keys.c +0 -4
 	/*
-	 * 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.

tools/testing/selftests/vm/protection_keys.c void test_pkey_alloc_exhaust(int *ptr, u16 pkey) · line 1183

Quoted 10 lines
tools/testing/selftests/vm/protection_keys.c +5 -4
-	 *   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. One of which is the default key(0). One can be taken
+	 * up by an execute-only mapping.
+	 * Ensure that we can allocate at least the remaining.
 	 */
-	pkey_assert(i >= NR_PKEYS-3);
+	pkey_assert(i >= (NR_PKEYS-arch_reserved_keys()-1));

We recently had a bug here. I fixed it and left myself a really nice comment so I and others wouldn't screw it up in the future.

Quoted 1 line

Does this kill my nice, new comment?

part of your nice comment has been moved into the header file. The arch specific header file explains where and how the reserved keys are used.

Signature · 1 line
Ram Pai
author
#34 lore raw

PATCH v13 19/24 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 e8ad970..04d0249 100644
@@ -1024,6 +1024,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);
@@ -1402,6 +1420,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
#35 lore raw

Re: [PATCH v13 19/24] selftests/vm: associate key on a mapped page and detect access violation

Quoted 6 lines · Ram Pai
tools/testing/selftests/vm/protection_keys.c +6 -0
+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",

tools/testing/selftests/vm/protection_keys.c void test_read_of_access_disabled_region(int *ptr, u16 pkey) · line 1043

Quoted 10 lines
tools/testing/selftests/vm/protection_keys.c +10 -0
+				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);
+}

Looks fine to me. I'm a bit surprised we didn't do this already, which is a good thing for this patch.

FWIW, if you took patches like this and put them first, you could probably get it merged now. Yes, I know it would mean redoing some of the later code move and rename ones.

author
#36 lore raw

Re: [PATCH v13 19/24] selftests/vm: associate key on a mapped page and detect access violation

Quoted 21 lines · Dave Hansen
Quoted 6 lines · Ram Pai
tools/testing/selftests/vm/protection_keys.c +6 -0
+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",

tools/testing/selftests/vm/protection_keys.c void test_read_of_access_disabled_region(int *ptr, u16 pkey) · line 1043

Quoted 10 lines
tools/testing/selftests/vm/protection_keys.c +10 -0
+				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);
+}

Looks fine to me. I'm a bit surprised we didn't do this already, which is a good thing for this patch.

Quoted 3 lines

FWIW, if you took patches like this and put them first, you could probably get it merged now. Yes, I know it would mean redoing some of the later code move and rename ones.

I have incorporated almost all of your comments. But there are some comments that take some effort to implement. Shall we get the patches merged in the current form? This code has been sitting out for a while.

In the current form its tested and works on powerpc and on x86, and incorporates about 95% of your suggestions. The rest I will take care as we go.

Signature · 1 line
Ram Pai
#37 lore raw

Re: [PATCH v13 19/24] selftests/vm: associate key on a mapped page and detect access violation

Quoted 7 lines · Ram Pai

I have incorporated almost all of your comments. But there are some comments that take some effort to implement. Shall we get the patches merged in the current form? This code has been sitting out for a while.

In the current form its tested and works on powerpc and on x86, and incorporates about 95% of your suggestions. The rest I will take care as we go.

What constitutes the remaining 5%?

author
#38 lore raw

Re: [PATCH v13 19/24] selftests/vm: associate key on a mapped page and detect access violation

Quoted 10 lines · Dave Hansen
Quoted 7 lines · Ram Pai

I have incorporated almost all of your comments. But there are some comments that take some effort to implement. Shall we get the patches merged in the current form? This code has been sitting out for a while.

In the current form its tested and works on powerpc and on x86, and incorporates about 95% of your suggestions. The rest I will take care as we go.

What constitutes the remaining 5%?

Mostly your comments on code-organization in the signal handler. There are still some #if defined(__i386__) ..... Can be cleaned up and abstracted further.

Also your questions on some of the code changes, the rationale for which is not obvious. Will help to spinkle in some descriptive comments there.

Have fixed up a lot of codying style issues. But there could till be a few that may spew warning by checkpatch.pl.

There are no functional issues AFAICT.

RP

author
#39 lore raw

PATCH v13 20/24 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 04d0249..f4acd72 100644
@@ -1042,6 +1042,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);
@@ -1422,6 +1433,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
#40 lore raw

PATCH v13 21/24 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 f4acd72..caf634e 100644
@@ -1067,6 +1067,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;
@@ -1435,6 +1447,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
#41 lore raw

PATCH v13 22/24 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. The test case is responsible for restoring the key register state to its original value.

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 caf634e..b5a9e6c 100644
@@ -1011,6 +1011,7 @@ void test_read_of_write_disabled_region(int *ptr, u16 pkey)
 	ptr_contents = read_ptr(ptr);
 	dprintf1("*ptr: %d\n", ptr_contents);
 	dprintf1("\n");
+	pkey_write_allow(pkey);
 }
 void test_read_of_access_disabled_region(int *ptr, u16 pkey)
 {
@@ -1090,6 +1091,7 @@ void test_kernel_write_of_access_disabled_region(int *ptr, u16 pkey)
 	ret = read(test_fd, ptr, 1);
 	dprintf1("read ret: %d\n", ret);
 	pkey_assert(ret);
+	pkey_access_allow(pkey);
 }
 void test_kernel_write_of_write_disabled_region(int *ptr, u16 pkey)
 {
@@ -1102,6 +1104,7 @@ void test_kernel_write_of_write_disabled_region(int *ptr, u16 pkey)
 	if (ret < 0 && (DEBUG_LEVEL > 0))
 		perror("verbose read result (OK for this to be bad)");
 	pkey_assert(ret);
+	pkey_write_allow(pkey);
 }
 
 void test_kernel_gup_of_access_disabled_region(int *ptr, u16 pkey)
@@ -1121,6 +1124,7 @@ void test_kernel_gup_of_access_disabled_region(int *ptr, u16 pkey)
 	vmsplice_ret = vmsplice(pipe_fds[1], &iov, 1, SPLICE_F_GIFT);
 	dprintf1("vmsplice() ret: %d\n", vmsplice_ret);
 	pkey_assert(vmsplice_ret == -1);
+	pkey_access_allow(pkey);
 
 	close(pipe_fds[0]);
 	close(pipe_fds[1]);
@@ -1141,6 +1145,7 @@ void test_kernel_gup_write_to_write_disabled_region(int *ptr, u16 pkey)
 	if (DEBUG_LEVEL > 0)
 		perror("futex");
 	dprintf1("futex() ret: %d\n", futex_ret);
+	pkey_write_allow(pkey);
 }
 
 /* Assumes that all pkeys other than 'pkey' are unallocated */
Signature · 1 line
1.7.1
#42 lore raw

Re: [PATCH v13 22/24] 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. The test case is responsible for restoring the key register state to its original value.

Seems fragile. Can't we just do this in common code? We could just loop through and restore the default permissions. That seems much more resistant to a bad test case.

author
#43 lore raw

Re: [PATCH v13 22/24] selftests/vm: testcases must restore pkey-permissions

Quoted 9 lines · Dave Hansen
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. The test case is responsible for restoring the key register state to its original value.

Quoted 3 lines

Seems fragile. Can't we just do this in common code? We could just loop through and restore the default permissions. That seems much more resistant to a bad test case.

Yes. done. fixed it the way you suggested in the new version.

Signature · 1 line
Ram Pai
author
#44 lore raw

PATCH v13 23/24 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 957f6f6..af44eed 100644
@@ -98,4 +98,29 @@ void expect_fault_on_read_execonly_key(void *p1, u16 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 6820c10..322da49 100644
@@ -176,4 +176,9 @@ void expect_fault_on_read_execonly_key(void *p1, u16 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 b5a9e6c..cbd87f8 100644
@@ -887,6 +887,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
#45 lore raw

PATCH v13 24/24 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 cbd87f8..f37b031 100644
@@ -1003,6 +1003,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;
@@ -1153,10 +1214,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;
 
@@ -1465,6 +1526,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
Acked-by on v13 24/24
#46 lore raw

Re: [PATCH v13 24/24] selftests/vm: test correct behavior of pkey-0

Quoted 3 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.

I like this. Looks very useful.

#47 lore raw
In reply to Ram Pai
Quoted 4 lines · Ram Pai

Test
----
Verified for correctness on powerpc. Need help verifying on x86. Compiles on x86.

It breaks make in tools/testing/selftests/x86:

make: *** No rule to make target protection_keys.c', needed by /home/linux/tools/testing/selftests/x86/protection_keys_64'. Stop.

The generic implementation no longer builds 32-bit binaries. Is this the intent?

It's possible to build 32-bit binaries with “make CC='gcc -m32'”, so perhaps this is good enough?

But with that, I get a warning:

protection_keys.c: In function ‘dump_mem’:
protection_keys.c:172:3: warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘uint64_t’ [-Wformat=]
dprintf1("dump[%03d][@%p]: %016lx\n", i, ptr, *ptr);
^

I suppose you could use %016llx and add a cast to unsigned long long to fix this.

Anyway, both the 32-bit and 64-bit tests fail here:

assert() at protection_keys.c::943 test_nr: 12 iteration: 1 running abort_hooks()...

I've yet checked what causes this. It's with the kernel headers from 4.17, but with other userspace headers based on glibc 2.17. I hope to look into this some more before the weekend, but I eventually have to return the test machine to the pool.

Thanks,
Florian

author
#48 lore raw
Quoted 10 lines · Florian Weimer
Quoted 4 lines · Ram Pai

Test
----
Verified for correctness on powerpc. Need help verifying on x86. Compiles on x86.

It breaks make in tools/testing/selftests/x86:

make: *** No rule to make target protection_keys.c', needed by /home/linux/tools/testing/selftests/x86/protection_keys_64'. Stop.

Ah.. it has to be taken out from the Makefile of
/home/linux/tools/testing/selftests/x86/

The sources have been moved to /home/linux/tools/testing/selftests/mm/

Quoted 3 lines

The generic implementation no longer builds 32-bit binaries. Is this the intent?

No. But building it 32-bit after moving it to a the new directory needs some special code in the Makefile.

Quoted 3 lines

It's possible to build 32-bit binaries with “make CC='gcc -m32'”, so perhaps this is good enough?

Dave Hansen did mention it, but he did not complain too much. So I kept quite.

Quoted 7 lines

But with that, I get a warning:

protection_keys.c: In function ‘dump_mem’:
protection_keys.c:172:3: warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘uint64_t’ [-Wformat=]

Quoted 5 lines
   dprintf1("dump[%03d][@%p]: %016lx\n", i, ptr, *ptr);
   ^

I suppose you could use %016llx and add a cast to unsigned long long to fix this.

yes.

Quoted 10 lines

Anyway, both the 32-bit and 64-bit tests fail here:

assert() at protection_keys.c::943 test_nr: 12 iteration: 1 running abort_hooks()...

I've yet checked what causes this. It's with the kernel headers from 4.17, but with other userspace headers based on glibc 2.17. I hope to look into this some more before the weekend, but I eventually have to return the test machine to the pool.

I wish I could get a x86 machine which could do memory keys. Had a AWS instance, but struggled to boot my kernel. Can't get to the console... gave up. If someone can give me a ready-made machine with support for memkeys, I can quickly fix all the outstanding x86 issues. But if someone can just fix it for me, .... ;)

RP