Skip to contentSkip to conversation

selftests, powerpc, x86 : Memory Protection Keys

PATCH Superseded Ram Pai 5 participants 49 messages Feb 22, 2018 – Mar 28, 2018

Subsystems (5)

v12 of 14 Superseded Latest is v14 →

lore
Share & tools
lore

Ram Pai

author
#0 lore raw

v12 cover selftests, powerpc, x86 : Memory Protection Keys

Memory protection keys enables an application to protect its address space from inadvertent access by its own code.

This feature is now enabled on powerpc architecture and integrated in 4.16-rc1. The patches move the selftests to arch neutral directory and enhance their test coverage.

Test
----
Verified for correctness on powerpc
and on x86 architectures(using EC2 ubuntu VM instance).

History:
-------

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 (21):
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

Thiago Jung Bauermann (1):
selftests/vm: Fix deadlock in protection_keys.c

 tools/testing/selftests/vm/Makefile           |    1 +
 tools/testing/selftests/vm/pkey-helpers.h     |  434 ++++++++
 tools/testing/selftests/vm/protection_keys.c  | 1471 +++++++++++++++++++++++++
 tools/testing/selftests/x86/Makefile          |    2 +-
 tools/testing/selftests/x86/pkey-helpers.h    |  223 ----
 tools/testing/selftests/x86/protection_keys.c | 1407 -----------------------
 6 files changed, 1907 insertions(+), 1631 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
author
#1 lore raw

PATCH v12 1/22 selftests/x86: Move protecton key selftest to arch neutral directory

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/Makefile           |    1 +
 tools/testing/selftests/vm/pkey-helpers.h     |  223 ++++
 tools/testing/selftests/vm/protection_keys.c  | 1407 +++++++++++++++++++++++++
 tools/testing/selftests/x86/Makefile          |    2 +-
 tools/testing/selftests/x86/pkey-helpers.h    |  223 ----
 tools/testing/selftests/x86/protection_keys.c | 1407 -------------------------
 6 files changed, 1632 insertions(+), 1631 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/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 +223 -0

223 lines not on this page · raw message

tools/testing/selftests/vm/protection_keys.c new file +1407 -0

1,407 lines not on this page · raw message

tools/testing/selftests/x86/Makefile +1 -1
index 10ca46d..fb234e6 100644
@@ -7,7 +7,7 @@ include ../lib.mk
 
 TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt ptrace_syscall test_mremap_vdso \
 			check_initial_reg_state sigreturn ldt_gdt iopl mpx-mini-test ioperm \
-			protection_keys test_vdso test_vsyscall
+			test_vdso test_vsyscall
 TARGETS_C_32BIT_ONLY := entry_from_vm86 syscall_arg_fault test_syscall_vdso unwind_vdso \
 			test_FCMOV test_FCOMI test_FISTTP \
 			vdso_restorer
tools/testing/selftests/x86/pkey-helpers.h deleted +0 -223

223 lines not on this page · raw message

tools/testing/selftests/x86/protection_keys.c deleted +0 -1407

1,132 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.

Acked-by on v12 1/22
#2 lore raw

Re: [PATCH v12 01/22] selftests/x86: Move protecton key selftest to arch neutral directory

Quoted 15 lines · Ram Pai
cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/Makefile           |    1 +
 tools/testing/selftests/vm/pkey-helpers.h     |  223 ++++
 tools/testing/selftests/vm/protection_keys.c  | 1407 +++++++++++++++++++++++++
 tools/testing/selftests/x86/Makefile          |    2 +-
 tools/testing/selftests/x86/pkey-helpers.h    |  223 ----
 tools/testing/selftests/x86/protection_keys.c | 1407 -------------------------
 6 files changed, 1632 insertions(+), 1631 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

Thanks,

Ingo

author
#3 lore raw

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

In reply to Ram Pai

some pkru references are named to pkey_reg and some prku references are renamed to pkey

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/pkey-helpers.h    |   85 +++++-----
 tools/testing/selftests/vm/protection_keys.c |  227 ++++++++++++++------------
 2 files changed, 164 insertions(+), 148 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +44 -41
index b3cb767..a568166 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
@@ -57,85 +57,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 __rdpkey_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 _rdpkey_reg(int line)
 {
-	unsigned int pkru = __rdpkru();
+	unsigned int pkey_reg = __rdpkey_reg();
 
-	dprintf4("rdpkru(line=%d) pkru: %x shadow: %x\n",
-			line, pkru, shadow_pkru);
-	assert(pkru == shadow_pkru);
+	dprintf4("rdpkey_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 rdpkey_reg() _rdpkey_reg(__LINE__)
 
-static inline void __wrpkru(unsigned int pkru)
+static inline void __wrpkey_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__,
+			__rdpkey_reg(), pkey_reg);
 	asm volatile(".byte 0x0f,0x01,0xef\n\t"
 		     : : "a" (eax), "c" (ecx), "d" (edx));
-	assert(pkru == __rdpkru());
+	assert(pkey_reg == __rdpkey_reg());
 }
 
-static inline void wrpkru(unsigned int pkru)
+static inline void wrpkey_reg(unsigned int pkey_reg)
 {
-	dprintf4("%s() changing %08x to %08x\n", __func__, __rdpkru(), pkru);
+	dprintf4("%s() changing %08x to %08x\n", __func__,
+			__rdpkey_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());
+	rdpkey_reg();
+	__wrpkey_reg(pkey_reg);
+	shadow_pkey_reg = pkey_reg;
+	dprintf4("%s(%08x) pkey_reg: %08x\n", __func__,
+			pkey_reg, __rdpkey_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 = rdpkey_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", rdpkey_reg());
+	wrpkey_reg(pkey_reg);
 }
 
 static inline void __pkey_write_allow(int pkey, int do_allow_write)
 {
-	long pkru = rdpkru();
+	long pkey_reg = rdpkey_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());
+	wrpkey_reg(pkey_reg);
+	dprintf4("pkey_reg now: %08x\n", rdpkey_reg());
 }
 
 #define PROT_PKEY0     0x10            /* protection key value (bit 0) */
@@ -185,10 +188,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;
@@ -199,21 +202,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 +120 -107

493 lines not on this page · raw message

Signature · 1 line
1.7.1
Reviewed-by on v12 2/22
#4 lore raw

Re: [PATCH v12 02/22] selftests/vm: rename all references to pkru to a generic name

tools/testing/selftests/vm/protection_keys.c void dumpit(char *f) · line 456

Quoted 7 lines · Ram Pai

int pkey_set(int pkey, unsigned long rights, unsigned long flags) {

tools/testing/selftests/vm/protection_keys.c +2 -2
 	u32 mask = (PKEY_DISABLE_ACCESS|PKEY_DISABLE_WRITE);
-	u32 old_pkru = __rdpkru();
-	u32 new_pkru;
+	u32 old_pkey_reg = __rdpkey_reg();
+	u32 new_pkey_reg;

If we're not using the _actual_ instruction names ("rdpkru"), I think I'd rather this be something more readable, like: __read_pkey_reg().

But, it's OK-ish the way it is.

author
#5 lore raw

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

In reply to Ram Pai

Moved all the generic definition and helper functions to the header file

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/pkey-helpers.h    |   62 ++++++++++++++++++++++--
 tools/testing/selftests/vm/protection_keys.c |   66 --------------------------
 2 files changed, 57 insertions(+), 71 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +57 -5
index a568166..c1bc761 100644
@@ -13,8 +13,31 @@
 #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
+
+#ifdef __i386__
+#define SYS_mprotect_key 380
+#define SYS_pkey_alloc	 381
+#define SYS_pkey_free	 382
+#define REG_IP_IDX REG_EIP
+#define si_pkey_offset 0x14
+#else
+#define SYS_mprotect_key 329
+#define SYS_pkey_alloc	 330
+#define SYS_pkey_free	 331
+#define REG_IP_IDX REG_RIP
+#define si_pkey_offset 0x20
+#endif
+
 #define NR_PKEYS 16
 #define PKEY_BITS_PER_PKEY 2
+#define PKEY_DISABLE_ACCESS    0x1
+#define PKEY_DISABLE_WRITE     0x2
+#define HPAGE_SIZE	(1UL<<21)
 
 #ifndef DEBUG_LEVEL
 #define DEBUG_LEVEL 0
@@ -141,11 +164,6 @@ static inline void __pkey_write_allow(int pkey, int do_allow_write)
 	dprintf4("pkey_reg now: %08x\n", rdpkey_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)
 
@@ -223,4 +241,38 @@ int pkey_reg_xstate_offset(void)
 	return xstate_offset;
 }
 
+static inline void __page_o_noops(void)
+{
+	/* 8-bytes of instruction * 512 bytes = 1 page */
+	asm(".rept 512 ; nopl 0x7eeeeeee(%eax) ; .endr");
+}
+
 #endif /* _PKEYS_HELPER_H */
+
+#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();			\
+		assert(condition);		\
+	}					\
+} while (0)
+#define raw_assert(cond) assert(cond)
tools/testing/selftests/vm/protection_keys.c +0 -66

93 lines not on this page · raw message

Signature · 1 line
1.7.1
author
#6 lore raw

PATCH v12 4/22 selftests/vm: typecast the pkey register

In reply to Ram Pai

This is in preparation to accomadate a differing size register across architectures.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/pkey-helpers.h    |   27 +++++-----
 tools/testing/selftests/vm/protection_keys.c |   69 ++++++++++++++------------
 2 files changed, 51 insertions(+), 45 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +14 -13
index c1bc761..b6c2133 100644
@@ -18,6 +18,7 @@
 #define u16 uint16_t
 #define u32 uint32_t
 #define u64 uint64_t
+#define pkey_reg_t u32
 
 #ifdef __i386__
 #define SYS_mprotect_key 380
@@ -80,12 +81,12 @@ 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_pkey_reg;
-static inline unsigned int __rdpkey_reg(void)
+extern pkey_reg_t shadow_pkey_reg;
+static inline pkey_reg_t __rdpkey_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)
@@ -94,11 +95,11 @@ static inline unsigned int __rdpkey_reg(void)
 	return pkey_reg;
 }
 
-static inline unsigned int _rdpkey_reg(int line)
+static inline pkey_reg_t _rdpkey_reg(int line)
 {
-	unsigned int pkey_reg = __rdpkey_reg();
+	pkey_reg_t pkey_reg = __rdpkey_reg();
 
-	dprintf4("rdpkey_reg(line=%d) pkey_reg: %x shadow: %x\n",
+	dprintf4("rdpkey_reg(line=%d) pkey_reg: %016lx shadow: %016lx\n",
 			line, pkey_reg, shadow_pkey_reg);
 	assert(pkey_reg == shadow_pkey_reg);
 
@@ -107,11 +108,11 @@ static inline unsigned int _rdpkey_reg(int line)
 
 #define rdpkey_reg() _rdpkey_reg(__LINE__)
 
-static inline void __wrpkey_reg(unsigned int pkey_reg)
+static inline void __wrpkey_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__,
 			__rdpkey_reg(), pkey_reg);
@@ -120,7 +121,7 @@ static inline void __wrpkey_reg(unsigned int pkey_reg)
 	assert(pkey_reg == __rdpkey_reg());
 }
 
-static inline void wrpkey_reg(unsigned int pkey_reg)
+static inline void wrpkey_reg(pkey_reg_t pkey_reg)
 {
 	dprintf4("%s() changing %08x to %08x\n", __func__,
 			__rdpkey_reg(), pkey_reg);
@@ -138,7 +139,7 @@ static inline void wrpkey_reg(unsigned int pkey_reg)
  */
 static inline void __pkey_access_allow(int pkey, int do_allow)
 {
-	unsigned int pkey_reg = rdpkey_reg();
+	pkey_reg_t pkey_reg = rdpkey_reg();
 	int bit = pkey * 2;
 
 	if (do_allow)
@@ -152,7 +153,7 @@ static inline void __pkey_access_allow(int pkey, int do_allow)
 
 static inline void __pkey_write_allow(int pkey, int do_allow_write)
 {
-	long pkey_reg = rdpkey_reg();
+	pkey_reg_t pkey_reg = rdpkey_reg();
 	int bit = pkey * 2 + 1;
 
 	if (do_allow_write)
tools/testing/selftests/vm/protection_keys.c +37 -32

203 lines not on this page · raw message

Signature · 1 line
1.7.1
#7 lore raw

Re: [PATCH v12 04/22] selftests/vm: typecast the pkey register

tools/testing/selftests/vm/pkey-helpers.h static inline unsigned int __rdpkey_reg(void) · line 104

Quoted 10 lines · Ram Pai
tools/testing/selftests/vm/pkey-helpers.h +3 -3
-static inline unsigned int _rdpkey_reg(int line)
+static inline pkey_reg_t _rdpkey_reg(int line)
 {
-	unsigned int pkey_reg = __rdpkey_reg();
+	pkey_reg_t pkey_reg = __rdpkey_reg();
 
-	dprintf4("rdpkey_reg(line=%d) pkey_reg: %x shadow: %x\n",
+	dprintf4("rdpkey_reg(line=%d) pkey_reg: %016lx shadow: %016lx\n",
 			line, pkey_reg, shadow_pkey_reg);
 	assert(pkey_reg == shadow_pkey_reg);

Hmm. So we're using %lx for an int? Doesn't the compiler complain about this?

#8 lore raw

Re: [PATCH v12 04/22] selftests/vm: typecast the pkey register

Quoted 7 lines · Dave Hansen
Quoted 6 lines · Ram Pai
(quoted diff) +2 -2
-static inline unsigned int _rdpkey_reg(int line)
+static inline pkey_reg_t _rdpkey_reg(int line)
 {
-	unsigned int pkey_reg = __rdpkey_reg();
+	pkey_reg_t pkey_reg = __rdpkey_reg();
Quoted 7 lines

Review

Quoted 4 lines
(quoted diff) +1 -1
-	dprintf4("rdpkey_reg(line=%d) pkey_reg: %x shadow: %x\n",
+	dprintf4("rdpkey_reg(line=%d) pkey_reg: %016lx shadow: %016lx\n",
 			line, pkey_reg, shadow_pkey_reg);
 	assert(pkey_reg == shadow_pkey_reg);

Hmm. So we're using %lx for an int? Doesn't the compiler complain about this?

It doesn't because dprintf4() doesn't have the annotation that tells the compiler that it takes printf-like arguments. Once I add it:

tools/testing/selftests/vm/pkey-helpers.h +4 -0
@@ -54,6 +54,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;

Then it does complain about it. I'm working on a fix where each arch will define a format string to use for its pkey_reg_t and use it like this:

tools/testing/selftests/vm/pkey-helpers.h +3 -1
@@ -19,6 +19,7 @@
 #define u32 uint32_t
 #define u64 uint64_t
 #define pkey_reg_t u32
+#define PKEY_REG_FMT "%016x"
 
 #ifdef __i386__
 #ifndef SYS_mprotect_key
@@ -112,7 +113,8 @@ static inline pkey_reg_t _read_pkey_reg(int line)
 {
 	pkey_reg_t pkey_reg = __read_pkey_reg();
 
-	dprintf4("read_pkey_reg(line=%d) pkey_reg: %016lx shadow: %016lx\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);
 

--
Thiago Jung Bauermann
IBM Linux Technology Center

author
#9 lore raw

PATCH v12 5/22 selftests/vm: generic function to handle shadow key register

In reply to Ram Pai

helper functions to handler shadow pkey register

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/pkey-helpers.h    |   27 ++++++++++++++++++++
 tools/testing/selftests/vm/protection_keys.c |   34 ++++++++++++++++---------
 2 files changed, 49 insertions(+), 12 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +27 -0
index b6c2133..7c979ad 100644
@@ -44,6 +44,33 @@
 #define DEBUG_LEVEL 0
 #endif
 #define DPRINT_IN_SIGNAL_BUF_SIZE 4096
+
+static inline u32 pkey_to_shift(int pkey)
+{
+	return pkey * PKEY_BITS_PER_PKEY;
+}
+
+static inline pkey_reg_t reset_bits(int pkey, pkey_reg_t bits)
+{
+	u32 shift = pkey_to_shift(pkey);
+
+	return ~(bits << shift);
+}
+
+static inline pkey_reg_t left_shift_bits(int pkey, pkey_reg_t bits)
+{
+	u32 shift = pkey_to_shift(pkey);
+
+	return (bits << shift);
+}
+
+static inline pkey_reg_t right_shift_bits(int pkey, pkey_reg_t bits)
+{
+	u32 shift = pkey_to_shift(pkey);
+
+	return (bits >> shift);
+}
+
 extern int dprint_in_signal;
 extern char dprint_in_signal_buffer[DPRINT_IN_SIGNAL_BUF_SIZE];
 static inline void sigsafe_printf(const char *format, ...)
tools/testing/selftests/vm/protection_keys.c +22 -12
index 3ef2569..83216c5 100644
@@ -374,7 +374,7 @@ u32 pkey_get(int pkey, unsigned long flags)
 			__func__, pkey, flags, 0, 0);
 	dprintf2("%s() raw pkey_reg: %x\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: %x\n", __func__, shifted_pkey_reg);
 	masked_pkey_reg = shifted_pkey_reg & mask;
 	dprintf2("%s() masked  pkey_reg: %x\n", __func__, masked_pkey_reg);
@@ -397,9 +397,9 @@ int 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 &= reset_bits(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);
 
 	__wrpkey_reg(new_pkey_reg);
 
@@ -430,7 +430,7 @@ void pkey_disable_set(int pkey, int flags)
 	ret = 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%016lx\n",
 		__func__, pkey, shadow_pkey_reg);
 
@@ -465,7 +465,7 @@ void pkey_disable_clear(int pkey, int flags)
 
 	ret = pkey_set(pkey, pkey_rights, 0);
 	/* pkey_reg and flags have the same format */
-	shadow_pkey_reg &= ~(flags << (pkey * 2));
+	shadow_pkey_reg &= reset_bits(pkey, flags);
 	pkey_assert(ret >= 0);
 
 	pkey_rights = pkey_get(pkey, syscall_flags);
@@ -523,6 +523,21 @@ int sys_pkey_alloc(unsigned long flags, unsigned long init_val)
 	return ret;
 }
 
+void pkey_setup_shadow(void)
+{
+	shadow_pkey_reg = __rdpkey_reg();
+}
+
+void pkey_reset_shadow(u32 key)
+{
+	shadow_pkey_reg &= reset_bits(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;
@@ -540,7 +555,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%016lx "
 				"shadow: 0x%016lx\n",
 				__func__,
@@ -550,7 +565,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%016lx shadow: 0x%016lx\n",
 			__func__, __LINE__, ret, __rdpkey_reg(),
@@ -1322,11 +1337,6 @@ void run_tests_once(void)
 	iteration_nr++;
 }
 
-void pkey_setup_shadow(void)
-{
-	shadow_pkey_reg = __rdpkey_reg();
-}
-
 int main(void)
 {
 	int nr_iterations = 22;
Signature · 1 line
1.7.1
#10 lore raw

Re: [PATCH v12 05/22] selftests/vm: generic function to handle shadow key register

tools/testing/selftests/vm/pkey-helpers.h line 51

Quoted 4 lines · Ram Pai
tools/testing/selftests/vm/pkey-helpers.h +4 -0
+static inline u32 pkey_to_shift(int pkey)
+{
+	return pkey * PKEY_BITS_PER_PKEY;
+}

pkey_bit_position(), perhaps?

tools/testing/selftests/vm/pkey-helpers.h line 58

Quoted 6 lines
tools/testing/selftests/vm/pkey-helpers.h +6 -0
+static inline pkey_reg_t reset_bits(int pkey, pkey_reg_t bits)
+{
+	u32 shift = pkey_to_shift(pkey);
+
+	return ~(bits << shift);
+}

I'd prefer clear_pkey_flags() or maybe clear_pkey_bits(). "reset" can mean "reset to 0" or "reset to 1".

Also, why the u32 here? Isn't an int more appropriate?

tools/testing/selftests/vm/pkey-helpers.h line 72

Quoted 13 lines
tools/testing/selftests/vm/pkey-helpers.h +13 -0
+static inline pkey_reg_t left_shift_bits(int pkey, pkey_reg_t bits)
+{
+	u32 shift = pkey_to_shift(pkey);
+
+	return (bits << shift);
+}
+
+static inline pkey_reg_t right_shift_bits(int pkey, pkey_reg_t bits)
+{
+	u32 shift = pkey_to_shift(pkey);
+
+	return (bits >> shift);
+}

Some comments on these would be handy. Basically that this takes a per-key flags value and puts it at the right position so it can be shoved in the register.

author
#11 lore raw

PATCH v12 6/22 selftests/vm: fix the wrong assert in pkey_disable_set()

In reply to Ram Pai

If the flag is 0, no bits will be set. Hence we cant expect the resulting bitmap to have a higher value than what it was earlier.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/protection_keys.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
tools/testing/selftests/vm/protection_keys.c +1 -1
index 83216c5..0109388 100644
@@ -443,7 +443,7 @@ void pkey_disable_set(int pkey, int flags)
 	dprintf1("%s(%d) pkey_reg: 0x%lx\n",
 		__func__, pkey, rdpkey_reg());
 	if (flags)
-		pkey_assert(rdpkey_reg() > orig_pkey_reg);
+		pkey_assert(rdpkey_reg() >= orig_pkey_reg);
 	dprintf1("END<---%s(%d, 0x%x)\n", __func__,
 		pkey, flags);
 }
Signature · 1 line
1.7.1
#12 lore raw

Re: [PATCH v12 06/22] selftests/vm: fix the wrong assert in pkey_disable_set()

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

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 +0 -0
index 83216c5..0109388 100644

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

Quoted 8 lines
tools/testing/selftests/vm/protection_keys.c +1 -1
@@ -443,7 +443,7 @@ void pkey_disable_set(int pkey, int flags)
 	dprintf1("%s(%d) pkey_reg: 0x%lx\n",
 		__func__, pkey, rdpkey_reg());
 	if (flags)
-		pkey_assert(rdpkey_reg() > orig_pkey_reg);
+		pkey_assert(rdpkey_reg() >= orig_pkey_reg);
 	dprintf1("END<---%s(%d, 0x%x)\n", __func__,
 		pkey, flags);
 }

I'm not sure about this one. Did this cause a problem for you?

Why would you call this and ask no bits to be set?

author
#13 lore raw

PATCH v12 7/22 selftests/vm: fixed bugs in pkey_disable_clear()

In reply to Ram Pai

instead of clearing the bits, pkey_disable_clear() was setting the bits. Fixed it.

Also fixed a wrong assertion in that function. When bits are cleared, the resulting bit value will be less than the original.

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 0109388..ca54a95 100644
@@ -461,7 +461,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 = pkey_set(pkey, pkey_rights, 0);
 	/* pkey_reg and flags have the same format */
@@ -475,7 +475,7 @@ void pkey_disable_clear(int pkey, int flags)
 	dprintf1("%s(%d) pkey_reg: 0x%016lx\n", __func__,
 			pkey, rdpkey_reg());
 	if (flags)
-		assert(rdpkey_reg() > orig_pkey_reg);
+		assert(rdpkey_reg() < orig_pkey_reg);
 }
 
 void pkey_write_allow(int pkey)
Signature · 1 line
1.7.1
#14 lore raw

Re: [PATCH v12 07/22] selftests/vm: fixed bugs in pkey_disable_clear()

Quoted 12 lines · Ram Pai
tools/testing/selftests/vm/protection_keys.c +1 -1
@@ -461,7 +461,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 = pkey_set(pkey, pkey_rights, 0);
 	/* pkey_reg and flags have the same format */

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

Quoted 8 lines
tools/testing/selftests/vm/protection_keys.c +1 -1
@@ -475,7 +475,7 @@ void pkey_disable_clear(int pkey, int flags)
 	dprintf1("%s(%d) pkey_reg: 0x%016lx\n", __func__,
 			pkey, rdpkey_reg());
 	if (flags)
-		assert(rdpkey_reg() > orig_pkey_reg);
+		assert(rdpkey_reg() < orig_pkey_reg);
 }
 
 void pkey_write_allow(int pkey)

This seems so horribly wrong that I wonder how it worked in the first place. Any idea?

#15 lore raw

Re: [PATCH v12 07/22] selftests/vm: fixed bugs in pkey_disable_clear()

Quoted 20 lines · Dave Hansen
Quoted 19 lines · Ram Pai
tools/testing/selftests/vm/protection_keys.c +2 -2
@@ -461,7 +461,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 = pkey_set(pkey, pkey_rights, 0);
 	/* pkey_reg and flags have the same format */
@@ -475,7 +475,7 @@ void pkey_disable_clear(int pkey, int flags)
 	dprintf1("%s(%d) pkey_reg: 0x%016lx\n", __func__,
 			pkey, rdpkey_reg());
 	if (flags)
-		assert(rdpkey_reg() > orig_pkey_reg);
+		assert(rdpkey_reg() < orig_pkey_reg);
 }
 
Quoted 4 lines

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

Quoted 1 line
tools/testing/selftests/vm/protection_keys.c +0 -0
 void pkey_write_allow(int pkey)

This seems so horribly wrong that I wonder how it worked in the first place. Any idea?

The code simply wasn't used. pkey_disable_clear() is called by pkey_write_allow() and pkey_access_allow(), but before this patch series nothing called either of these functions.

--
Thiago Jung Bauermann
IBM Linux Technology Center

#16 lore raw

Re: [PATCH v12 07/22] selftests/vm: fixed bugs in pkey_disable_clear()

Quoted 5 lines · Thiago Jung Bauermann
Quoted 5 lines
Quoted 5 lines
tools/testing/selftests/vm/protection_keys.c +1 -1
 	if (flags)
-		assert(rdpkey_reg() > orig_pkey_reg);
+		assert(rdpkey_reg() < orig_pkey_reg);
 }
Quoted 7 lines
Quoted 3 lines
Quoted 1 line

void pkey_write_allow(int pkey)

This seems so horribly wrong that I wonder how it worked in the first place. Any idea?

The code simply wasn't used. pkey_disable_clear() is called by pkey_write_allow() and pkey_access_allow(), but before this patch series nothing called either of these functions.

Ahh, that explains it. Can that get stuck in the changelog, please?

#17 lore raw

Re: [PATCH v12 07/22] selftests/vm: fixed bugs in pkey_disable_clear()

Quoted 9 lines · Dave Hansen
Quoted 8 lines · Thiago Jung Bauermann
Quoted 8 lines

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

Quoted 6 lines
tools/testing/selftests/vm/protection_keys.c +1 -1
 	if (flags)
-		assert(rdpkey_reg() > orig_pkey_reg);
+		assert(rdpkey_reg() < orig_pkey_reg);
 }
 
 void pkey_write_allow(int pkey)

This seems so horribly wrong that I wonder how it worked in the first place. Any idea?

Quoted 6 lines
Quoted 4 lines

The code simply wasn't used. pkey_disable_clear() is called by pkey_write_allow() and pkey_access_allow(), but before this patch series nothing called either of these functions.

Ahh, that explains it. Can that get stuck in the changelog, please?

Yes, will be in the next version.

Signature · 2 lines
Thiago Jung Bauermann
IBM Linux Technology Center
author
#18 lore raw

PATCH v12 8/22 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 ca54a95..aaf9f09 100644
@@ -582,6 +582,9 @@ int alloc_pkey(void)
 int sys_pkey_free(unsigned long pkey)
 {
 	int ret = syscall(SYS_pkey_free, pkey);
+
+	if (!ret)
+		shadow_pkey_reg &= reset_bits(pkey, PKEY_DISABLE_ACCESS);
 	dprintf1("%s(pkey=%ld) syscall ret: %d\n", __func__, pkey, ret);
 	return ret;
 }
Signature · 1 line
1.7.1
#19 lore raw

Re: [PATCH v12 08/22] 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.

...

Quoted 5 lines
tools/testing/selftests/vm/protection_keys.c +0 -0
index ca54a95..aaf9f09 100644

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

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

Did this cause problems for you in practice?

On x86, sys_pkey_free() does not affect PKRU, so this isn't quite right.
I'd much rather have the actual tests explicitly clear the PKRU bits
and also in the process clear the shadow bits.

author
#20 lore raw

PATCH v12 9/22 selftests/vm: fix alloc_random_pkey() to make it really random

In reply to Ram Pai

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

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/protection_keys.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)
tools/testing/selftests/vm/protection_keys.c +7 -3
index aaf9f09..2e4b636 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>
@@ -602,13 +603,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;
@@ -624,13 +627,14 @@ 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]);
 		pkey_assert(!free_ret);
 	}
-	dprintf1("%s()::%d, ret: %d pkey_reg: 0x%x shadow: 0x%x\n", __func__,
-			__LINE__, ret, __rdpkey_reg(), shadow_pkey_reg);
+	dprintf1("%s()::%d, ret: %d pkey_reg: 0x%x shadow: 0x%016lx\n",
+		__func__, __LINE__, ret, __rdpkey_reg(), shadow_pkey_reg);
 	return ret;
 }
 
Signature · 1 line
1.7.1
#21 lore raw

Re: [PATCH v12 09/22] selftests/vm: fix alloc_random_pkey() to make it really random

Quoted 2 lines · Ram Pai

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

...

tools/testing/selftests/vm/protection_keys.c int alloc_random_pkey(void) · line 613

Quoted 13 lines
tools/testing/selftests/vm/protection_keys.c +2 -1
@@ -602,13 +603,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();

The srand() is probably useful, but won't this always just do a single alloc_pkey() now? That seems like it will mean we always use the first one the kernel gives us, which isn't random.

tools/testing/selftests/vm/protection_keys.c int alloc_random_pkey(void) · line 639

Quoted 6 lines
tools/testing/selftests/vm/protection_keys.c +2 -2
-	dprintf1("%s()::%d, ret: %d pkey_reg: 0x%x shadow: 0x%x\n", __func__,
-			__LINE__, ret, __rdpkey_reg(), shadow_pkey_reg);
+	dprintf1("%s()::%d, ret: %d pkey_reg: 0x%x shadow: 0x%016lx\n",
+		__func__, __LINE__, ret, __rdpkey_reg(), shadow_pkey_reg);
 	return ret;
 }

This belonged in the pkey_reg_t patch, I think.

author
#22 lore raw

PATCH v12 10/22 selftests/vm: introduce two arch independent abstraction

In reply to Ram Pai

open_hugepage_file() <- opens the huge page file get_start_key() <-- provides the first non-reserved key.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/pkey-helpers.h    |   11 +++++++++++
 tools/testing/selftests/vm/protection_keys.c |    6 +++---
 2 files changed, 14 insertions(+), 3 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +11 -0
index 7c979ad..c8f5739 100644
@@ -304,3 +304,14 @@ static inline void __page_o_noops(void)
 	}					\
 } while (0)
 #define raw_assert(cond) assert(cond)
+
+static inline int open_hugepage_file(int flag)
+{
+	return open("/sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages",
+		 O_RDONLY);
+}
+
+static inline int get_start_key(void)
+{
+	return 1;
+}
tools/testing/selftests/vm/protection_keys.c +3 -3
index 2e4b636..254b66d 100644
@@ -809,7 +809,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;
@@ -1087,10 +1087,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
Reviewed-by on v12 10/22
#23 lore raw

Re: [PATCH v12 10/22] selftests/vm: introduce two arch independent abstraction

Quoted 3 lines · Ram Pai

open_hugepage_file() <- opens the huge page file get_start_key() <-- provides the first non-reserved key.

Looks reasonable.

author
#24 lore raw

PATCH v12 11/22 selftests/vm: pkey register should match shadow pkey

In reply to Ram Pai

expected_pkey_fault() is comparing the contents of pkey register with 0. This may not be true all the time. There could be bits set by default by the architecture which can never be changed. Hence compare the value against shadow pkey register, which is supposed to track the bits accurately all throughout

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/protection_keys.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
tools/testing/selftests/vm/protection_keys.c +2 -2
index 254b66d..6054093 100644
@@ -926,10 +926,10 @@ void expected_pkey_fault(int pkey)
 	pkey_assert(last_pkey_faults + 1 == pkey_faults);
 	pkey_assert(last_si_pkey == pkey);
 	/*
-	 * The signal handler shold have cleared out PKEY register to let the
+	 * The signal handler shold have cleared out pkey-register to let the
 	 * test program continue.  We now have to restore it.
 	 */
-	if (__rdpkey_reg() != 0)
+	if (__rdpkey_reg() != shadow_pkey_reg)
 		pkey_assert(0);
 
 	__wrpkey_reg(shadow_pkey_reg);
Signature · 1 line
1.7.1
#25 lore raw

Re: [PATCH v12 11/22] selftests/vm: pkey register should match shadow pkey

Quoted 19 lines · Ram Pai

expected_pkey_fault() is comparing the contents of pkey register with 0. This may not be true all the time. There could be bits set by default by the architecture which can never be changed. Hence compare the value against shadow pkey register, which is supposed to track the bits accurately all throughout

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/protection_keys.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
tools/testing/selftests/vm/protection_keys.c +0 -0
index 254b66d..6054093 100644

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

Quoted 5 lines
tools/testing/selftests/vm/protection_keys.c +1 -1
@@ -926,10 +926,10 @@ void expected_pkey_fault(int pkey)
 	pkey_assert(last_pkey_faults + 1 == pkey_faults);
 	pkey_assert(last_si_pkey == pkey);
 	/*
-	 * The signal handler shold have cleared out PKEY register to let the
+	 * The signal handler shold have cleared out pkey-register to let the

Heh, you randomly changed the formatting and didn't bother with my awful typo. :)

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

Quoted 8 lines
tools/testing/selftests/vm/protection_keys.c +1 -1
 	 * test program continue.  We now have to restore it.
 	 */
-	if (__rdpkey_reg() != 0)
+	if (__rdpkey_reg() != shadow_pkey_reg)
 		pkey_assert(0);
 
 	__wrpkey_reg(shadow_pkey_reg);

I don't think this should be "shadow_pkey_reg". This was just trying to double-check that the signal handler messed around with PKRU the way we expected.

We could also just check that the disable bits for 'pkey' are clear at this point. That would be almost as good.

author
#26 lore raw

PATCH v12 12/22 selftests/vm: generic cleanup

In reply to Ram Pai

cleanup the code to satisfy coding styles.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/protection_keys.c |   81 ++++++++++++++------------
 1 files changed, 43 insertions(+), 38 deletions(-)
tools/testing/selftests/vm/protection_keys.c +43 -38
index 6054093..6fdd8f5 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>
@@ -251,26 +256,11 @@ void signal_handler(int signum, siginfo_t *si, void *vucontext)
 	dprintf1("signal pkey_reg from  pkey_reg: %016lx\n", __rdpkey_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");
 	return;
-	if (trapno == 14) {
-		fprintf(stderr,
-			"ERROR: In signal handler, page fault, trapno = %d, ip = %016lx\n",
-			trapno, ip);
-		fprintf(stderr, "si_addr %p\n", si->si_addr);
-		fprintf(stderr, "REG_ERR: %lx\n",
-				(unsigned long)uctxt->uc_mcontext.gregs[REG_ERR]);
-		exit(1);
-	} else {
-		fprintf(stderr, "unexpected trap %d! at 0x%lx\n", trapno, ip);
-		fprintf(stderr, "si_addr %p\n", si->si_addr);
-		fprintf(stderr, "REG_ERR: %lx\n",
-				(unsigned long)uctxt->uc_mcontext.gregs[REG_ERR]);
-		exit(2);
-	}
-	dprint_in_signal = 0;
 }
 
 int wait_all_children(void)
@@ -415,7 +405,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 = rdpkey_reg();
 
 	dprintf1("START->%s(%d, 0x%x)\n", __func__,
@@ -453,7 +443,7 @@ void pkey_disable_clear(int pkey, int flags)
 {
 	unsigned long syscall_flags = 0;
 	int ret;
-	int pkey_rights = pkey_get(pkey, syscall_flags);
+	u32 pkey_rights = pkey_get(pkey, syscall_flags);
 	pkey_reg_t orig_pkey_reg = rdpkey_reg();
 
 	pkey_assert(flags & (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));
@@ -516,9 +506,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;
@@ -542,7 +533,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: 0x%016lx shadow: %016lx\n", __func__,
 			__LINE__, __rdpkey_reg(), shadow_pkey_reg);
@@ -692,7 +683,9 @@ void record_pkey_malloc(void *ptr, long size)
 		/* 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);
@@ -716,9 +709,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) ||
@@ -799,11 +794,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
@@ -824,7 +821,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;
 	}
@@ -957,6 +955,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;
 }
@@ -998,7 +997,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);
 	rdpkey_reg();
 	pkey_access_deny(pkey);
 	ptr_contents = read_ptr(ptr);
@@ -1120,13 +1120,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%016lx "
@@ -1134,9 +1135,11 @@ void test_pkey_alloc_exhaust(int *ptr, u16 pkey)
 			__func__, __LINE__, err, __rdpkey_reg(),
 			shadow_pkey_reg);
 		rdpkey_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;
 		}
@@ -1338,7 +1341,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++;
@@ -1350,7 +1354,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;
@@ -1358,7 +1362,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
#27 lore raw

Re: [PATCH v12 12/22] selftests/vm: generic cleanup

Quoted 14 lines · Ram Pai

cleanup the code to satisfy coding styles.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/protection_keys.c |   81 ++++++++++++++------------
 1 files changed, 43 insertions(+), 38 deletions(-)
tools/testing/selftests/vm/protection_keys.c +0 -0
index 6054093..6fdd8f5 100644

tools/testing/selftests/vm/protection_keys.c line 7

Quoted 5 lines
tools/testing/selftests/vm/protection_keys.c +1 -1
@@ -4,7 +4,7 @@
  *
  * There are examples in here of:
  *  * how to set protection keys on memory
- *  * how to set/clear bits in pkey registers (the rights register)
+ *  * how to set/clear bits in Protection Key registers (the rights register)

I don't think CodingStyle says to do this. :)

Quoted 6 lines
tools/testing/selftests/vm/protection_keys.c +0 -0
  *  * how to handle SEGV_PKUERR signals and extract pkey-relevant
  *    information from the siginfo
  *
@@ -13,13 +13,18 @@
  *	prefault pages in at malloc, or not
  *	protect MPX bounds tables with protection keys?

tools/testing/selftests/vm/protection_keys.c line 21

Quoted 10 lines
tools/testing/selftests/vm/protection_keys.c +6 -3
  *	make sure VMA splitting/merging is working correctly
- *	OOMs can destroy mm->mmap (see exit_mmap()), so make sure it is immune to pkeys
- *	look for pkey "leaks" where it is still set on a VMA but "freed" back to the kernel
- *	do a plain mprotect() to a mprotect_pkey() area and make sure the pkey sticks
+ *	OOMs can destroy mm->mmap (see exit_mmap()),
+ *			so make sure it is immune to pkeys
+ *	look for pkey "leaks" where it is still set on a VMA
+ *			 but "freed" back to the kernel
+ *	do a plain mprotect() to a mprotect_pkey() area and make
+ *			 sure the pkey sticks

Ram, I'm not sure where this came from, but this looks horrid. Please don't do this to the file

tools/testing/selftests/vm/protection_keys.c line 28

Quoted 8 lines
tools/testing/selftests/vm/protection_keys.c +4 -2
  * Compile like this:
- *	gcc      -o protection_keys    -O2 -g -std=gnu99 -pthread -Wall protection_keys.c -lrt -ldl -lm
- *	gcc -m32 -o protection_keys_32 -O2 -g -std=gnu99 -pthread -Wall protection_keys.c -lrt -ldl -lm
+ *	gcc      -o protection_keys    -O2 -g -std=gnu99
+ *			 -pthread -Wall protection_keys.c -lrt -ldl -lm
+ *	gcc -m32 -o protection_keys_32 -O2 -g -std=gnu99
+ *			 -pthread -Wall protection_keys.c -lrt -ldl -lm
  */

Please just leave this, or remove it from the file. It was a long line so it could be copied and pasted, this ruins that.

Quoted 19 lines
tools/testing/selftests/vm/protection_keys.c +2 -8
 #define _GNU_SOURCE
 #include <errno.h>
@@ -251,26 +256,11 @@ void signal_handler(int signum, siginfo_t *si, void *vucontext)
 	dprintf1("signal pkey_reg from  pkey_reg: %016lx\n", __rdpkey_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");
 	return;
-	if (trapno == 14) {
-		fprintf(stderr,
-			"ERROR: In signal handler, page fault, trapno = %d, ip = %016lx\n",
-			trapno, ip);
-		fprintf(stderr, "si_addr %p\n", si->si_addr);
-		fprintf(stderr, "REG_ERR: %lx\n",
-				(unsigned long)uctxt->uc_mcontext.gregs[REG_ERR]);

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

Quoted 10 lines
tools/testing/selftests/vm/protection_keys.c +0 -9
-		exit(1);
-	} else {
-		fprintf(stderr, "unexpected trap %d! at 0x%lx\n", trapno, ip);
-		fprintf(stderr, "si_addr %p\n", si->si_addr);
-		fprintf(stderr, "REG_ERR: %lx\n",
-				(unsigned long)uctxt->uc_mcontext.gregs[REG_ERR]);
-		exit(2);
-	}
-	dprint_in_signal = 0;
 }

I think this is just randomly removing code now.

I think you should probably just drop this patch. It's not really brining anything useful.

author
#28 lore raw

PATCH v12 13/22 selftests/vm: powerpc implementation for generic abstraction

In reply to Ram Pai

Introduce powerpc implementation for the various
abstractions.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/pkey-helpers.h    |  128 ++++++++++++++++++++++----
 tools/testing/selftests/vm/protection_keys.c |   42 +++++---
 2 files changed, 136 insertions(+), 34 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +110 -18
index c8f5739..c47aead 100644
@@ -18,27 +18,51 @@
 #define u16 uint16_t
 #define u32 uint32_t
 #define u64 uint64_t
-#define pkey_reg_t u32
 
-#ifdef __i386__
+#if defined(__i386__) || defined(__x86_64__) /* arch */
+
+#ifdef __i386__ /* arch */
 #define SYS_mprotect_key 380
-#define SYS_pkey_alloc	 381
-#define SYS_pkey_free	 382
+#define SYS_pkey_alloc   381
+#define SYS_pkey_free    382
 #define REG_IP_IDX REG_EIP
-#define si_pkey_offset 0x14
-#else
+#elif __x86_64__
 #define SYS_mprotect_key 329
-#define SYS_pkey_alloc	 330
-#define SYS_pkey_free	 331
+#define SYS_pkey_alloc   330
+#define SYS_pkey_free    331
 #define REG_IP_IDX REG_RIP
-#define si_pkey_offset 0x20
-#endif
+#endif /* __x86_64__ */
+
+#define NR_PKEYS		16
+#define NR_RESERVED_PKEYS	1
+#define PKEY_BITS_PER_PKEY	2
+#define PKEY_DISABLE_ACCESS	0x1
+#define PKEY_DISABLE_WRITE	0x2
+#define HPAGE_SIZE		(1UL<<21)
+#define pkey_reg_t u32
 
-#define NR_PKEYS 16
-#define PKEY_BITS_PER_PKEY 2
-#define PKEY_DISABLE_ACCESS    0x1
-#define PKEY_DISABLE_WRITE     0x2
-#define HPAGE_SIZE	(1UL<<21)
+#elif __powerpc64__ /* arch */
+
+#define SYS_mprotect_key 386
+#define SYS_pkey_alloc	 384
+#define SYS_pkey_free	 385
+#define REG_IP_IDX PT_NIP
+#define REG_TRAPNO PT_TRAP
+#define gregs gp_regs
+#define fpregs fp_regs
+
+#define NR_PKEYS		32
+#define NR_RESERVED_PKEYS_4K	26
+#define NR_RESERVED_PKEYS_64K	3
+#define PKEY_BITS_PER_PKEY	2
+#define PKEY_DISABLE_ACCESS	0x3  /* disable read and write */
+#define PKEY_DISABLE_WRITE	0x2
+#define HPAGE_SIZE		(1UL<<24)
+#define pkey_reg_t u64
+
+#else /* arch */
+	NOT SUPPORTED
+#endif /* arch */
 
 #ifndef DEBUG_LEVEL
 #define DEBUG_LEVEL 0
@@ -47,7 +71,11 @@
 
 static inline u32 pkey_to_shift(int pkey)
 {
+#if defined(__i386__) || defined(__x86_64__) /* arch */
 	return pkey * PKEY_BITS_PER_PKEY;
+#elif __powerpc64__ /* arch */
+	return (NR_PKEYS - pkey - 1) * PKEY_BITS_PER_PKEY;
+#endif /* arch */
 }
 
 static inline pkey_reg_t reset_bits(int pkey, pkey_reg_t bits)
@@ -111,6 +139,7 @@ static inline void sigsafe_printf(const char *format, ...)
 extern pkey_reg_t shadow_pkey_reg;
 static inline pkey_reg_t __rdpkey_reg(void)
 {
+#if defined(__i386__) || defined(__x86_64__) /* arch */
 	unsigned int eax, edx;
 	unsigned int ecx = 0;
 	pkey_reg_t pkey_reg;
@@ -118,7 +147,13 @@ static inline pkey_reg_t __rdpkey_reg(void)
 	asm volatile(".byte 0x0f,0x01,0xee\n\t"
 		     : "=a" (eax), "=d" (edx)
 		     : "c" (ecx));
-	pkey_reg = eax;
+#elif __powerpc64__ /* arch */
+	pkey_reg_t eax;
+	pkey_reg_t pkey_reg;
+
+	asm volatile("mfspr %0, 0xd" : "=r" ((pkey_reg_t)(eax)));
+#endif /* arch */
+	pkey_reg = (pkey_reg_t)eax;
 	return pkey_reg;
 }
 
@@ -138,6 +173,7 @@ static inline pkey_reg_t _rdpkey_reg(int line)
 static inline void __wrpkey_reg(pkey_reg_t pkey_reg)
 {
 	pkey_reg_t eax = pkey_reg;
+#if defined(__i386__) || defined(__x86_64__) /* arch */
 	pkey_reg_t ecx = 0;
 	pkey_reg_t edx = 0;
 
@@ -146,6 +182,14 @@ static inline void __wrpkey_reg(pkey_reg_t pkey_reg)
 	asm volatile(".byte 0x0f,0x01,0xef\n\t"
 		     : : "a" (eax), "c" (ecx), "d" (edx));
 	assert(pkey_reg == __rdpkey_reg());
+
+#elif __powerpc64__ /* arch */
+	dprintf4("%s() changing %llx to %llx\n",
+			 __func__, __rdpkey_reg(), pkey_reg);
+	asm volatile("mtspr 0xd, %0" : : "r" ((unsigned long)(eax)) : "memory");
+#endif /* arch */
+	dprintf4("%s() pkey register after changing %016lx to %016lx\n",
+			 __func__, __rdpkey_reg(), pkey_reg);
 }
 
 static inline void wrpkey_reg(pkey_reg_t pkey_reg)
@@ -192,6 +236,8 @@ static inline void __pkey_write_allow(int pkey, int do_allow_write)
 	dprintf4("pkey_reg now: %08x\n", rdpkey_reg());
 }
 
+#if defined(__i386__) || defined(__x86_64__) /* arch */
+
 #define PAGE_SIZE 4096
 #define MB	(1<<20)
 
@@ -274,8 +320,18 @@ static inline void __page_o_noops(void)
 	/* 8-bytes of instruction * 512 bytes = 1 page */
 	asm(".rept 512 ; nopl 0x7eeeeeee(%eax) ; .endr");
 }
+#elif __powerpc64__ /* arch */
 
-#endif /* _PKEYS_HELPER_H */
+#define PAGE_SIZE (0x1UL << 16)
+static inline int cpu_has_pku(void)
+{
+	return 1;
+}
+
+/* 8-bytes of instruction * 16384bytes = 1 page */
+#define __page_o_noops() asm(".rept 16384 ; nop; .endr")
+
+#endif /* arch */
 
 #define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
 #define ALIGN_UP(x, align_to)	(((x) + ((align_to)-1)) & ~((align_to)-1))
@@ -307,11 +363,47 @@ static inline void __page_o_noops(void)
 
 static inline int open_hugepage_file(int flag)
 {
-	return open("/sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages",
+	int fd;
+
+#if defined(__i386__) || defined(__x86_64__) /* arch */
+	fd = open("/sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages",
 		 O_RDONLY);
+#elif __powerpc64__ /* arch */
+	fd = open("/sys/kernel/mm/hugepages/hugepages-16384kB/nr_hugepages",
+		O_RDONLY);
+#else /* arch */
+	NOT SUPPORTED
+#endif /* arch */
+	return fd;
 }
 
 static inline int get_start_key(void)
 {
+#if defined(__i386__) || defined(__x86_64__) /* arch */
 	return 1;
+#elif __powerpc64__ /* arch */
+	return 0;
+#else /* arch */
+	NOT SUPPORTED
+#endif /* arch */
+}
+
+static inline u32 *siginfo_get_pkey_ptr(siginfo_t *si)
+{
+#ifdef si_pkey
+	return &si->si_pkey;
+#else
+
+#ifdef __i386__ /* arch */
+#define si_pkey_offset 0x14
+#elif __x86_64__ /*arch */
+#define si_pkey_offset 0x1C
+#elif __powerpc64__ /*arch */
+#define si_pkey_offset 0x1C
+#endif /*arch */
+
+	return (u32 *)(((u8 *)si) + si_pkey_offset);
+#endif
 }
+
+#endif /* _PKEYS_HELPER_H */
tools/testing/selftests/vm/protection_keys.c +26 -16

96 lines not on this page · raw message

Signature · 1 line
1.7.1
#29 lore raw

Re: [PATCH v12 13/22] selftests/vm: powerpc implementation for generic abstraction

tools/testing/selftests/vm/pkey-helpers.h line 79

Quoted 8 lines · Ram Pai

static inline u32 pkey_to_shift(int pkey)
{

tools/testing/selftests/vm/pkey-helpers.h +4 -0
+#if defined(__i386__) || defined(__x86_64__) /* arch */
 	return pkey * PKEY_BITS_PER_PKEY;
+#elif __powerpc64__ /* arch */
+	return (NR_PKEYS - pkey - 1) * PKEY_BITS_PER_PKEY;
+#endif /* arch */
 }

I really detest the #if #else style. Can't we just have a pkey_ppc.h and a pkey_x86.h or something?

author
#30 lore raw

PATCH v12 14/22 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 c4c73e6..e82bd88 100644
@@ -586,7 +586,8 @@ int sys_pkey_free(unsigned long pkey)
 	int ret = syscall(SYS_pkey_free, pkey);
 
 	if (!ret)
-		shadow_pkey_reg &= reset_bits(pkey, PKEY_DISABLE_ACCESS);
+		shadow_pkey_reg &= reset_bits(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
#31 lore raw

Re: [PATCH v12 14/22] selftests/vm: clear the bits in shadow reg when a pkey is freed.

Quoted 5 lines · Ram Pai
tools/testing/selftests/vm/protection_keys.c +0 -0
index c4c73e6..e82bd88 100644

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

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

What about your EXEC bit?

author
#32 lore raw

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

In reply to Ram Pai

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

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/pkey-helpers.h    |   22 ++++++++++++++++------
 tools/testing/selftests/vm/protection_keys.c |    9 +++++----
 2 files changed, 21 insertions(+), 10 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +16 -6
index c47aead..88ef58f 100644
@@ -258,7 +258,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;
@@ -271,13 +271,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)
@@ -323,9 +323,19 @@ static inline void __page_o_noops(void)
 #elif __powerpc64__ /* arch */
 
 #define PAGE_SIZE (0x1UL << 16)
-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;
 }
 
 /* 8-bytes of instruction * 16384bytes = 1 page */
tools/testing/selftests/vm/protection_keys.c +5 -4
index e82bd88..58da5a0 100644
@@ -1299,8 +1299,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;
 	}
 
@@ -1362,12 +1362,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
#33 lore raw

Re: [PATCH v12 15/22] selftests/vm: powerpc implementation to check support for pkey

Quoted 7 lines · Ram Pai

#define PAGE_SIZE (0x1UL << 16)

tools/testing/selftests/vm/pkey-helpers.h +3 -2
-static inline int cpu_has_pku(void)
+static inline bool is_pkey_supported(void)
 {
-	return 1;
+	/*
+	 * No simple way to determine this.

tools/testing/selftests/vm/pkey-helpers.h static inline void __page_o_noops(void) · line 339

Quoted 10 lines
tools/testing/selftests/vm/pkey-helpers.h +9 -0
+	 * lets try allocating a key and see if it succeeds.
+	 */
+	int ret = sys_pkey_alloc(0, 0);
+
+	if (ret > 0) {
+		sys_pkey_free(ret);
+		return true;
+	}
+	return false;
 }

The point of doing this was to have a test for the CPU that way separate from the syscalls.

Can you leave cpu_has_pkeys() in place?

author
#34 lore raw

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

In reply to Ram Pai

The maximum number of keys that can be allocated has to take into consideration, that some keys are reserved by
the architecture for specific purpose. Hence cannot
be allocated.

Fix the assertion in test_pkey_alloc_exhaust()

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/pkey-helpers.h    |   14 ++++++++++++++
 tools/testing/selftests/vm/protection_keys.c |    9 ++++-----
 2 files changed, 18 insertions(+), 5 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +14 -0
index 88ef58f..67f9b0f 100644
@@ -416,4 +416,18 @@ static inline int get_start_key(void)
 #endif
 }
 
+static inline int arch_reserved_keys(void)
+{
+#if defined(__i386__) || defined(__x86_64__) /* arch */
+	return NR_RESERVED_PKEYS;
+#elif __powerpc64__ /* arch */
+	if (sysconf(_SC_PAGESIZE) == 4096)
+		return NR_RESERVED_PKEYS_4K;
+	else
+		return NR_RESERVED_PKEYS_64K;
+#else /* arch */
+	NOT SUPPORTED
+#endif /* arch */
+}
+
 #endif /* _PKEYS_HELPER_H */
tools/testing/selftests/vm/protection_keys.c +4 -5
index 58da5a0..aae6771 100644
@@ -1167,12 +1167,11 @@ void test_pkey_alloc_exhaust(int *ptr, u16 pkey)
 	pkey_assert(i < NR_PKEYS*2);
 
 	/*
-	 * There are 16 pkeys supported in hardware.  One is taken
-	 * up for the default (0) and another can be taken up by
-	 * an execute-only mapping.  Ensure that we can allocate
-	 * at least 14 (16-2).
+	 * There are NR_PKEYS pkeys supported in hardware. arch_reserved_keys()
+	 * are reserved. One   can   be   taken   up by an execute-only mapping.
+	 * Ensure that we can allocate at least the remaining.
 	 */
-	pkey_assert(i >= NR_PKEYS-2);
+	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
#35 lore raw

Re: [PATCH v12 16/22] selftests/vm: fix an assertion in test_pkey_alloc_exhaust()

tools/testing/selftests/vm/pkey-helpers.h static inline int get_start_key(void) · line 431

Quoted 13 lines · Ram Pai
tools/testing/selftests/vm/pkey-helpers.h +13 -0
+static inline int arch_reserved_keys(void)
+{
+#if defined(__i386__) || defined(__x86_64__) /* arch */
+	return NR_RESERVED_PKEYS;
+#elif __powerpc64__ /* arch */
+	if (sysconf(_SC_PAGESIZE) == 4096)
+		return NR_RESERVED_PKEYS_4K;
+	else
+		return NR_RESERVED_PKEYS_64K;
+#else /* arch */
+	NOT SUPPORTED
+#endif /* arch */
+}

Yeah, this is hideous.

Please either do it in one header:

#ifdef x86..
static inline int arch_reserved_keys(void)
{
}
...
#elif ppc
static inline int arch_reserved_keys(void)
{
}
...
#else
#error
#endif

Or in multiple:

#ifdef x86..
#include <pkey_x86.h>
#elif ppc
#include <pkey_ppc.h>
#else
#error
#endif

author
#36 lore raw

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

In reply to Ram Pai

detect access-violation on a page to which access-disabled key is associated much after the page is mapped.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 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 aae6771..dba9162 100644
@@ -1016,6 +1016,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);
+	rdpkey_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);
@@ -1310,6 +1328,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
Acked-by on v12 17/22
#37 lore raw

Re: [PATCH v12 17/22] selftests/vm: associate key on a mapped page and detect access violation

Quoted 2 lines · Ram Pai

detect access-violation on a page to which access-disabled key is associated much after the page is mapped.

Looks fine to me. Did this actually find a bug for you?

author
#38 lore raw

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

In reply to Ram Pai

detect write-violation on a page to which write-disabled key is associated much after the page is mapped.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 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 dba9162..b685e26 100644
@@ -1034,6 +1034,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);
@@ -1330,6 +1341,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
Acked-by on v12 18/22
#39 lore raw

Re: [PATCH v12 18/22] selftests/vm: associate key on a mapped page and detect write violation

Quoted 2 lines · Ram Pai

detect write-violation on a page to which write-disabled key is associated much after the page is mapped.

The more tests the merrier.

author
#40 lore raw

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

In reply to Ram Pai

detect write-violation on a page to which access-disabled key is associated much after the page is mapped.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 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 b685e26..437ee74 100644
@@ -1059,6 +1059,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;
@@ -1343,6 +1355,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
Acked-by on v12 19/22
#41 lore raw

Re: [PATCH v12 19/22] selftests/vm: detect write violation on a mapped access-denied-key page

Quoted 2 lines · Ram Pai

detect write-violation on a page to which access-disabled key is associated much after the page is mapped.

author
#42 lore raw

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

In reply to Ram Pai

Generally the signal handler restores the state of the pkey register before returning. However there are times when the read/write operation can legitamely fail without invoking the signal handler. Eg: A sys_read() operaton to a write-protected page should be disallowed. In such a case the state of the pkey register is not restored to its original state. 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 437ee74..42c068a 100644
@@ -1003,6 +1003,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)
 {
@@ -1082,6 +1083,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)
 {
@@ -1094,6 +1096,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)
@@ -1113,6 +1116,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]);
@@ -1133,6 +1137,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
#43 lore raw

Re: [PATCH v12 20/22] selftests/vm: testcases must restore pkey-permissions

Quoted 7 lines · Ram Pai

Generally the signal handler restores the state of the pkey register before returning. However there are times when the read/write operation can legitamely fail without invoking the signal handler. Eg: A sys_read() operaton to a write-protected page should be disallowed. In such a case the state of the pkey register is not restored to its original state. The test case is responsible for restoring the key register state to its original value.

Oh, that's a good point.

Could we just do this in a common place, though? Like reset the register after each test? Seems more foolproof.

author
#44 lore raw

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

In reply to Ram Pai

introduce a new allocator that allocates 4k hardware-pages to back 64k linux-page. This allocator is only applicable on powerpc.

cc Dave Hansen <[email protected]>
cc Florian Weimer <[email protected]>
---
 tools/testing/selftests/vm/protection_keys.c |   30 ++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)
tools/testing/selftests/vm/protection_keys.c +30 -0
index 42c068a..1b06e59 100644
@@ -766,6 +766,35 @@ void free_pkey_malloc(void *ptr)
 	return ptr;
 }
 
+void *malloc_pkey_with_mprotect_subpage(long size, int prot, u16 pkey)
+{
+#ifdef __powerpc64__
+	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);
+
+	dprintf1("%s() for pkey %d @ %p\n", __func__, pkey, ptr);
+	return ptr;
+#else /*  __powerpc64__ */
+	return PTR_ERR_ENOTSUP;
+#endif /*  __powerpc64__ */
+}
+
 void *malloc_pkey_anon_huge(long size, int prot, u16 pkey)
 {
 	int ret;
@@ -888,6 +917,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
#45 lore raw

Re: [PATCH v12 21/22] selftests/vm: sub-page allocator

On 02/21/2018 05:55 PM, Ram Pai wrote:
...

tools/testing/selftests/vm/protection_keys.c void setup_hugetlbfs(void) · line 923

Quoted 8 lines
tools/testing/selftests/vm/protection_keys.c +1 -0
@@ -888,6 +917,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:

I think I'd rather have an #ifdef on the array entries than have the malloc entry do nothing on x86. Maybe have a ppc-specific section at the end?

author
#46 lore raw

PATCH v12 22/22 selftests/vm: Fix deadlock in protection_keys.c

In reply to Ram Pai

From: Thiago Jung Bauermann <[email protected]>

The sig_chld() handler calls dprintf2() taking care of setting dprint_in_signal so that sigsafe_printf() won't call printf(). Unfortunately, this precaution is is negated by dprintf_level(), which has a call to fflush().

This function acquires a lock, which means that if the signal interrupts an ongoing fflush() the process will deadlock. At least on powerpc this is easy to trigger, resulting in the following backtrace when attaching to the frozen process:

(gdb) bt

  #0  0x00007fff9f96c7d8 in __lll_lock_wait_private () from /lib64/power8/libc.so.6
  #1  0x00007fff9f8cba4c in _IO_flush_all_lockp () from /lib64/power8/libc.so.6
  #2  0x00007fff9f8cbd1c in __GI__IO_flush_all () from /lib64/power8/libc.so.6
  #3  0x00007fff9f8b7424 in fflush () from /lib64/power8/libc.so.6
  #4  0x00000000100504f8 in sig_chld (x=17) at protection_keys.c:283
  #5  <signal handler called>
  #6  0x00007fff9f8cb8ac in _IO_flush_all_lockp () from /lib64/power8/libc.so.6
  #7  0x00007fff9f8cbd1c in __GI__IO_flush_all () from /lib64/power8/libc.so.6
  #8  0x00007fff9f8b7424 in fflush () from /lib64/power8/libc.so.6
  #9  0x0000000010050b50 in pkey_get (pkey=7, flags=0) at protection_keys.c:379

#10 0x0000000010050dc0 in pkey_disable_set (pkey=7, flags=2) at protection_keys.c:423 #11 0x0000000010051414 in pkey_write_deny (pkey=7) at protection_keys.c:486
#12 0x00000000100556bc in test_ptrace_of_child (ptr=0x7fff9f7f0000, pkey=7) at protection_keys.c:1288 #13 0x0000000010055f60 in run_tests_once () at protection_keys.c:1414
#14 0x00000000100561a4 in main () at protection_keys.c:1459

The fix is to refrain from calling fflush() when inside a signal handler. The output may not be as pretty but at least the testcase will be able to move on.

 tools/testing/selftests/vm/pkey-helpers.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
---
 tools/testing/selftests/vm/pkey-helpers.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
tools/testing/selftests/vm/pkey-helpers.h +2 -1
index 67f9b0f..7240598 100644
@@ -128,7 +128,8 @@ static inline void sigsafe_printf(const char *format, ...)
 #define dprintf_level(level, args...) do {	\
 	if (level <= DEBUG_LEVEL)		\
 		sigsafe_printf(args);		\
-	fflush(NULL);				\
+	if (!dprint_in_signal)			\
+		fflush(NULL);			\
 } while (0)
 #define dprintf0(args...) dprintf_level(0, args)
 #define dprintf1(args...) dprintf_level(1, args)
Signature · 1 line
1.7.1
Acked-by on v12 22/22
#47 lore raw

Re: [PATCH v12 22/22] selftests/vm: Fix deadlock in protection_keys.c

Quoted 7 lines · On Wed, 21 Feb 2018 17:55:41 -0800 Ram Pai <[email protected]> wrote:

From: Thiago Jung Bauermann <[email protected]>

The sig_chld() handler calls dprintf2() taking care of setting dprint_in_signal so that sigsafe_printf() won't call printf(). Unfortunately, this precaution is is negated by dprintf_level(), which has a call to fflush().

fflush() is not the signal-safe function list, so this makes sense. I wonder if fflush() is needed in sigsafe_printf()?

How about?

tools/testing/selftests/x86/pkey-helpers.h +1 -1
index b3cb7670e026..2c3b39851f10 100644
@@ -29,6 +29,7 @@ static inline void sigsafe_printf(const char *format, ...)
 	va_start(ap, format);
 	if (!dprint_in_signal) {
 		vprintf(format, ap);
+		fflush(NULL);				\
 	} else {
 		int ret;
 		int len = vsnprintf(dprint_in_signal_buffer,
@@ -49,7 +50,6 @@ static inline void sigsafe_printf(const char *format, ...)
 #define dprintf_level(level, args...) do {	\
 	if (level <= DEBUG_LEVEL)		\
 		sigsafe_printf(args);		\
-	fflush(NULL);				\
 } while (0)
 #define dprintf0(args...) dprintf_level(0, args)
 #define dprintf1(args...) dprintf_level(1, args)

But both are equivalent I guess, so

Acked-by on v12 22/22
#48 lore raw

Re: [PATCH v12 22/22] selftests/vm: Fix deadlock in protection_keys.c

In reply to Ram Pai
Quoted 11 lines · Ram Pai

From: Thiago Jung Bauermann <[email protected]>

The sig_chld() handler calls dprintf2() taking care of setting dprint_in_signal so that sigsafe_printf() won't call printf(). Unfortunately, this precaution is is negated by dprintf_level(), which has a call to fflush().

This function acquires a lock, which means that if the signal interrupts an ongoing fflush() the process will deadlock. At least on powerpc this is easy to trigger, resulting in the following backtrace when attaching to the frozen process:

Ugh, yeah, I've run into this too.