[RFC PATCH 0/6] User pkey minor bug fixes

[RFC PATCH 0/6] User pkey minor bug fixes

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>


While evaluating the possibility of defining a new type for pkeys within the
kernel I found a couple of minor bugs.

Because these patches clean up the return codes from system calls I'm sending
this out RFC hoping that users will speak up if anything breaks.

I'm not too concerned about pkey_free() because it is unlikely that anyone is
checking the return code.  Interestingly enough, glibc recommends not calling
pkey_free() because it does not change the access rights to the key and may be
subsequently allocated again.[1][2]

The pkey_alloc() is more concerning.  However, I checked the Chrome source and
it does not differentiate among the return codes and maps all errors into
kNoMemoryProtectionKey.

glibc says it returns ENOSYS if the system does not support pkeys but I don't
see where ENOSYS is returned?  AFAICS it just returns what the kernel returns.
So it is probably up to user of glibc.

In addition I've enhanced the pkey tests to verify and test the changes.

Thanks to Rick Edgecombe and Sohil Mehta for internal review.


[1] Quote from manual/memory.texi:

Calling this function does not change the access rights of the freed
protection key.  The calling thread and other threads may retain access
to it, even if it is subsequently allocated again.  For this reason, it
is not recommended to call the @code{pkey_free} function.

[2] PKS had a similar issue and went to statically allocated keys instead.


Ira Weiny (6):
  testing/pkeys: Add command line options
  testing/pkeys: Don't use uninitialized variable
  testing/pkeys: Add additional test for pkey_alloc()
  pkeys: Lift pkey hardware check for pkey_alloc()
  pkeys: Up level pkey_free() checks
  pkeys: Change mm_pkey_free() to void

 arch/powerpc/include/asm/pkeys.h             | 18 ++---
 arch/x86/include/asm/pkeys.h                 |  7 +-
 include/linux/pkeys.h                        |  5 +-
 mm/mprotect.c                                | 13 +++-
 tools/testing/selftests/vm/pkey-helpers.h    |  7 +-
 tools/testing/selftests/vm/protection_keys.c | 75 +++++++++++++++++---
 6 files changed, 86 insertions(+), 39 deletions(-)


base-commit: 874c8ca1e60b2c564a48f7e7acc40d328d5c8733
-- 
2.35.1

[RFC PATCH 0/6] User pkey minor bug fixes

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>


While evaluating the possibility of defining a new type for pkeys within the
kernel I found a couple of minor bugs.

Because these patches clean up the return codes from system calls I'm sending
this out RFC hoping that users will speak up if anything breaks.

I'm not too concerned about pkey_free() because it is unlikely that anyone is
checking the return code.  Interestingly enough, glibc recommends not calling
pkey_free() because it does not change the access rights to the key and may be
subsequently allocated again.[1][2]

The pkey_alloc() is more concerning.  However, I checked the Chrome source and
it does not differentiate among the return codes and maps all errors into
kNoMemoryProtectionKey.

glibc says it returns ENOSYS if the system does not support pkeys but I don't
see where ENOSYS is returned?  AFAICS it just returns what the kernel returns.
So it is probably up to user of glibc.

In addition I've enhanced the pkey tests to verify and test the changes.

Thanks to Rick Edgecombe and Sohil Mehta for internal review.


[1] Quote from manual/memory.texi:

Calling this function does not change the access rights of the freed
protection key.  The calling thread and other threads may retain access
to it, even if it is subsequently allocated again.  For this reason, it
is not recommended to call the @code{pkey_free} function.

[2] PKS had a similar issue and went to statically allocated keys instead.


Ira Weiny (6):
  testing/pkeys: Add command line options
  testing/pkeys: Don't use uninitialized variable
  testing/pkeys: Add additional test for pkey_alloc()
  pkeys: Lift pkey hardware check for pkey_alloc()
  pkeys: Up level pkey_free() checks
  pkeys: Change mm_pkey_free() to void

 arch/powerpc/include/asm/pkeys.h             | 18 ++---
 arch/x86/include/asm/pkeys.h                 |  7 +-
 include/linux/pkeys.h                        |  5 +-
 mm/mprotect.c                                | 13 +++-
 tools/testing/selftests/vm/pkey-helpers.h    |  7 +-
 tools/testing/selftests/vm/protection_keys.c | 75 +++++++++++++++++---
 6 files changed, 86 insertions(+), 39 deletions(-)


base-commit: 874c8ca1e60b2c564a48f7e7acc40d328d5c8733
-- 
2.35.1

[RFC PATCH 1/6] testing/pkeys: Add command line options

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

It is more convenient to use command line options for debug and
iterations vs changing the code and recompiling.

Add command line options for debug level and number of iterations.

$ ./protection_keys_64 -h
Usage: ./protection_keys_64 [-h,-d,-i <iter>]
        --help,-h   This help
	--debug,-d  Increase debug level for each -d
	--iterations,-i <iter>  repeate test <iter> times
		default: 22

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 tools/testing/selftests/vm/pkey-helpers.h    |  7 +--
 tools/testing/selftests/vm/protection_keys.c | 59 +++++++++++++++++---
 2 files changed, 55 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/vm/pkey-helpers.h b/tools/testing/selftests/vm/pkey-helpers.h
index 92f3be3dd8e5..7aaac1c8ebca 100644
--- a/tools/testing/selftests/vm/pkey-helpers.h
+++ b/tools/testing/selftests/vm/pkey-helpers.h
@@ -23,9 +23,8 @@
 
 #define PTR_ERR_ENOTSUP ((void *)-ENOTSUP)
 
-#ifndef DEBUG_LEVEL
-#define DEBUG_LEVEL 0
-#endif
+extern int debug_level;
+
 #define DPRINT_IN_SIGNAL_BUF_SIZE 4096
 extern int dprint_in_signal;
 extern char dprint_in_signal_buffer[DPRINT_IN_SIGNAL_BUF_SIZE];
@@ -58,7 +57,7 @@ static inline void sigsafe_printf(const char *format, ...)
 	}
 }
 #define dprintf_level(level, args...) do {	\
-	if (level <= DEBUG_LEVEL)		\
+	if (level <= debug_level)		\
 		sigsafe_printf(args);		\
 } while (0)
 #define dprintf0(args...) dprintf_level(0, args)
diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c
index 291bc1e07842..d0183c381859 100644
--- a/tools/testing/selftests/vm/protection_keys.c
+++ b/tools/testing/selftests/vm/protection_keys.c
@@ -44,9 +44,13 @@
 #include <unistd.h>
 #include <sys/ptrace.h>
 #include <setjmp.h>
+#include <getopt.h>
 
 #include "pkey-helpers.h"
 
+#define DEFAULT_ITERATIONS 22
+
+int debug_level;
 int iteration_nr = 1;
 int test_nr;
 
@@ -361,7 +365,7 @@ void signal_handler(int signum, siginfo_t *si, void *vucontext)
 	 * here.
 	 */
 	dprintf1("pkey_reg_xstate_offset: %d\n", pkey_reg_xstate_offset());
-	if (DEBUG_LEVEL > 4)
+	if (debug_level > 4)
 		dump_mem(pkey_reg_ptr - 128, 256);
 	pkey_assert(*pkey_reg_ptr);
 #endif /* arch */
@@ -480,7 +484,7 @@ int sys_mprotect_pkey(void *ptr, size_t size, unsigned long orig_prot,
 		dprintf2("SYS_mprotect_key sret: %d\n", sret);
 		dprintf2("SYS_mprotect_key prot: 0x%lx\n", orig_prot);
 		dprintf2("SYS_mprotect_key failed, errno: %d\n", errno);
-		if (DEBUG_LEVEL >= 2)
+		if (debug_level >= 2)
 			perror("SYS_mprotect_pkey");
 	}
 	return sret;
@@ -1116,7 +1120,7 @@ void test_kernel_write_of_write_disabled_region(int *ptr, u16 pkey)
 	pkey_write_deny(pkey);
 	ret = read(test_fd, ptr, 100);
 	dprintf1("read ret: %d\n", ret);
-	if (ret < 0 && (DEBUG_LEVEL > 0))
+	if (ret < 0 && (debug_level > 0))
 		perror("verbose read result (OK for this to be bad)");
 	pkey_assert(ret);
 }
@@ -1155,7 +1159,7 @@ void test_kernel_gup_write_to_write_disabled_region(int *ptr, u16 pkey)
 	pkey_write_deny(pkey);
 	futex_ret = syscall(SYS_futex, ptr, FUTEX_WAIT, some_int-1, NULL,
 			&ignored, ignored);
-	if (DEBUG_LEVEL > 0)
+	if (debug_level > 0)
 		perror("futex");
 	dprintf1("futex() ret: %d\n", futex_ret);
 }
@@ -1626,11 +1630,52 @@ void pkey_setup_shadow(void)
 	shadow_pkey_reg = __read_pkey_reg();
 }
 
-int main(void)
+static void print_help_and_exit(char *argv0)
+{
+	printf("Usage: %s [-h,-d,-i <iter>]\n", argv0);
+	printf("	--help,-h   This help\n");
+	printf("	--debug,-d  Increase debug level for each -d\n");
+	printf("	--iterations,-i <iter>  repeate test <iter> times\n");
+	printf("		default: %d\n", DEFAULT_ITERATIONS);
+	printf("\n");
+}
+
+int main(int argc, char *argv[])
 {
-	int nr_iterations = 22;
-	int pkeys_supported = is_pkeys_supported();
+	int nr_iterations = DEFAULT_ITERATIONS;
+	int pkeys_supported;
+
+	while (1) {
+		static struct option long_options[] = {
+			{"help",	no_argument,		0,	'h' },
+			{"debug",	no_argument,		0,	'd' },
+			{"iterations",	required_argument,	0,	'i' },
+			{0,		0,			0,	0 }
+		};
+		int option_index = 0;
+		int c;
+
+		c = getopt_long(argc, argv, "hdi:", long_options, &option_index);
+		if (c == -1)
+			break;
+
+		switch (c) {
+		case 'h':
+			print_help_and_exit(argv[0]);
+			return 0;
+		case 'd':
+			debug_level++;
+			break;
+		case 'i':
+			nr_iterations = strtoul(optarg, NULL, 0);
+			break;
+		default:
+			print_help_and_exit(argv[0]);
+			exit(-1);
+		}
+	}
 
+	pkeys_supported = is_pkeys_supported();
 	srand((unsigned int)time(NULL));
 
 	setup_handlers();
-- 
2.35.1

[RFC PATCH 1/6] testing/pkeys: Add command line options

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

It is more convenient to use command line options for debug and
iterations vs changing the code and recompiling.

Add command line options for debug level and number of iterations.

$ ./protection_keys_64 -h
Usage: ./protection_keys_64 [-h,-d,-i <iter>]
        --help,-h   This help
	--debug,-d  Increase debug level for each -d
	--iterations,-i <iter>  repeate test <iter> times
		default: 22

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 tools/testing/selftests/vm/pkey-helpers.h    |  7 +--
 tools/testing/selftests/vm/protection_keys.c | 59 +++++++++++++++++---
 2 files changed, 55 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/vm/pkey-helpers.h b/tools/testing/selftests/vm/pkey-helpers.h
index 92f3be3dd8e5..7aaac1c8ebca 100644
--- a/tools/testing/selftests/vm/pkey-helpers.h
+++ b/tools/testing/selftests/vm/pkey-helpers.h
@@ -23,9 +23,8 @@
 
 #define PTR_ERR_ENOTSUP ((void *)-ENOTSUP)
 
-#ifndef DEBUG_LEVEL
-#define DEBUG_LEVEL 0
-#endif
+extern int debug_level;
+
 #define DPRINT_IN_SIGNAL_BUF_SIZE 4096
 extern int dprint_in_signal;
 extern char dprint_in_signal_buffer[DPRINT_IN_SIGNAL_BUF_SIZE];
@@ -58,7 +57,7 @@ static inline void sigsafe_printf(const char *format, ...)
 	}
 }
 #define dprintf_level(level, args...) do {	\
-	if (level <= DEBUG_LEVEL)		\
+	if (level <= debug_level)		\
 		sigsafe_printf(args);		\
 } while (0)
 #define dprintf0(args...) dprintf_level(0, args)
diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c
index 291bc1e07842..d0183c381859 100644
--- a/tools/testing/selftests/vm/protection_keys.c
+++ b/tools/testing/selftests/vm/protection_keys.c
@@ -44,9 +44,13 @@
 #include <unistd.h>
 #include <sys/ptrace.h>
 #include <setjmp.h>
+#include <getopt.h>
 
 #include "pkey-helpers.h"
 
+#define DEFAULT_ITERATIONS 22
+
+int debug_level;
 int iteration_nr = 1;
 int test_nr;
 
@@ -361,7 +365,7 @@ void signal_handler(int signum, siginfo_t *si, void *vucontext)
 	 * here.
 	 */
 	dprintf1("pkey_reg_xstate_offset: %d\n", pkey_reg_xstate_offset());
-	if (DEBUG_LEVEL > 4)
+	if (debug_level > 4)
 		dump_mem(pkey_reg_ptr - 128, 256);
 	pkey_assert(*pkey_reg_ptr);
 #endif /* arch */
@@ -480,7 +484,7 @@ int sys_mprotect_pkey(void *ptr, size_t size, unsigned long orig_prot,
 		dprintf2("SYS_mprotect_key sret: %d\n", sret);
 		dprintf2("SYS_mprotect_key prot: 0x%lx\n", orig_prot);
 		dprintf2("SYS_mprotect_key failed, errno: %d\n", errno);
-		if (DEBUG_LEVEL >= 2)
+		if (debug_level >= 2)
 			perror("SYS_mprotect_pkey");
 	}
 	return sret;
@@ -1116,7 +1120,7 @@ void test_kernel_write_of_write_disabled_region(int *ptr, u16 pkey)
 	pkey_write_deny(pkey);
 	ret = read(test_fd, ptr, 100);
 	dprintf1("read ret: %d\n", ret);
-	if (ret < 0 && (DEBUG_LEVEL > 0))
+	if (ret < 0 && (debug_level > 0))
 		perror("verbose read result (OK for this to be bad)");
 	pkey_assert(ret);
 }
@@ -1155,7 +1159,7 @@ void test_kernel_gup_write_to_write_disabled_region(int *ptr, u16 pkey)
 	pkey_write_deny(pkey);
 	futex_ret = syscall(SYS_futex, ptr, FUTEX_WAIT, some_int-1, NULL,
 			&ignored, ignored);
-	if (DEBUG_LEVEL > 0)
+	if (debug_level > 0)
 		perror("futex");
 	dprintf1("futex() ret: %d\n", futex_ret);
 }
@@ -1626,11 +1630,52 @@ void pkey_setup_shadow(void)
 	shadow_pkey_reg = __read_pkey_reg();
 }
 
-int main(void)
+static void print_help_and_exit(char *argv0)
+{
+	printf("Usage: %s [-h,-d,-i <iter>]\n", argv0);
+	printf("	--help,-h   This help\n");
+	printf("	--debug,-d  Increase debug level for each -d\n");
+	printf("	--iterations,-i <iter>  repeate test <iter> times\n");
+	printf("		default: %d\n", DEFAULT_ITERATIONS);
+	printf("\n");
+}
+
+int main(int argc, char *argv[])
 {
-	int nr_iterations = 22;
-	int pkeys_supported = is_pkeys_supported();
+	int nr_iterations = DEFAULT_ITERATIONS;
+	int pkeys_supported;
+
+	while (1) {
+		static struct option long_options[] = {
+			{"help",	no_argument,		0,	'h' },
+			{"debug",	no_argument,		0,	'd' },
+			{"iterations",	required_argument,	0,	'i' },
+			{0,		0,			0,	0 }
+		};
+		int option_index = 0;
+		int c;
+
+		c = getopt_long(argc, argv, "hdi:", long_options, &option_index);
+		if (c == -1)
+			break;
+
+		switch (c) {
+		case 'h':
+			print_help_and_exit(argv[0]);
+			return 0;
+		case 'd':
+			debug_level++;
+			break;
+		case 'i':
+			nr_iterations = strtoul(optarg, NULL, 0);
+			break;
+		default:
+			print_help_and_exit(argv[0]);
+			exit(-1);
+		}
+	}
 
+	pkeys_supported = is_pkeys_supported();
 	srand((unsigned int)time(NULL));
 
 	setup_handlers();
-- 
2.35.1

[RFC PATCH 2/6] testing/pkeys: Don't use uninitialized variable

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

err was being used in test_pkey_alloc_exhaust() prior to being assigned.
errno is useful to know after a failed alloc_pkey() call.

Change err to errno in the debug print.

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 tools/testing/selftests/vm/protection_keys.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c
index d0183c381859..43e47de19c0d 100644
--- a/tools/testing/selftests/vm/protection_keys.c
+++ b/tools/testing/selftests/vm/protection_keys.c
@@ -1225,9 +1225,9 @@ void test_pkey_alloc_exhaust(int *ptr, u16 pkey)
 		int new_pkey;
 		dprintf1("%s() alloc loop: %d\n", __func__, i);
 		new_pkey = alloc_pkey();
-		dprintf4("%s()::%d, err: %d pkey_reg: 0x%016llx"
+		dprintf4("%s()::%d, errno: %d pkey_reg: 0x%016llx"
 				" shadow: 0x%016llx\n",
-				__func__, __LINE__, err, __read_pkey_reg(),
+				__func__, __LINE__, errno, __read_pkey_reg(),
 				shadow_pkey_reg);
 		read_pkey_reg(); /* for shadow checking */
 		dprintf2("%s() errno: %d ENOSPC: %d\n", __func__, errno, ENOSPC);
-- 
2.35.1

[RFC PATCH 2/6] testing/pkeys: Don't use uninitialized variable

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

err was being used in test_pkey_alloc_exhaust() prior to being assigned.
errno is useful to know after a failed alloc_pkey() call.

Change err to errno in the debug print.

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 tools/testing/selftests/vm/protection_keys.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c
index d0183c381859..43e47de19c0d 100644
--- a/tools/testing/selftests/vm/protection_keys.c
+++ b/tools/testing/selftests/vm/protection_keys.c
@@ -1225,9 +1225,9 @@ void test_pkey_alloc_exhaust(int *ptr, u16 pkey)
 		int new_pkey;
 		dprintf1("%s() alloc loop: %d\n", __func__, i);
 		new_pkey = alloc_pkey();
-		dprintf4("%s()::%d, err: %d pkey_reg: 0x%016llx"
+		dprintf4("%s()::%d, errno: %d pkey_reg: 0x%016llx"
 				" shadow: 0x%016llx\n",
-				__func__, __LINE__, err, __read_pkey_reg(),
+				__func__, __LINE__, errno, __read_pkey_reg(),
 				shadow_pkey_reg);
 		read_pkey_reg(); /* for shadow checking */
 		dprintf2("%s() errno: %d ENOSPC: %d\n", __func__, errno, ENOSPC);
-- 
2.35.1

[RFC PATCH 3/6] testing/pkeys: Add additional test for pkey_alloc()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

When pkeys are not available on the hardware pkey_alloc() has specific
behavior which was previously untested.

Add test for this.

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 tools/testing/selftests/vm/protection_keys.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c
index 43e47de19c0d..4b733a75606f 100644
--- a/tools/testing/selftests/vm/protection_keys.c
+++ b/tools/testing/selftests/vm/protection_keys.c
@@ -1554,6 +1554,16 @@ void test_implicit_mprotect_exec_only_memory(int *ptr, u16 pkey)
 	do_not_expect_pkey_fault("plain read on recently PROT_EXEC area");
 }
 
+void test_pkey_alloc_on_unsupported_cpu(void)
+{
+	int test_pkey = sys_pkey_alloc(0, 0);
+
+	dprintf1("pkey_alloc: %d (%d %s)\n", test_pkey, errno,
+		 strerror(errno));
+	pkey_assert(test_pkey < 0);
+	pkey_assert(errno == ENOSPC);
+}
+
 void test_mprotect_pkey_on_unsupported_cpu(int *ptr, u16 pkey)
 {
 	int size = PAGE_SIZE;
@@ -1688,6 +1698,8 @@ int main(int argc, char *argv[])
 
 		printf("running PKEY tests for unsupported CPU/OS\n");
 
+		test_pkey_alloc_on_unsupported_cpu();
+
 		ptr  = mmap(NULL, size, PROT_NONE, MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
 		assert(ptr != (void *)-1);
 		test_mprotect_pkey_on_unsupported_cpu(ptr, 1);
-- 
2.35.1

[RFC PATCH 3/6] testing/pkeys: Add additional test for pkey_alloc()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

When pkeys are not available on the hardware pkey_alloc() has specific
behavior which was previously untested.

Add test for this.

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 tools/testing/selftests/vm/protection_keys.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c
index 43e47de19c0d..4b733a75606f 100644
--- a/tools/testing/selftests/vm/protection_keys.c
+++ b/tools/testing/selftests/vm/protection_keys.c
@@ -1554,6 +1554,16 @@ void test_implicit_mprotect_exec_only_memory(int *ptr, u16 pkey)
 	do_not_expect_pkey_fault("plain read on recently PROT_EXEC area");
 }
 
+void test_pkey_alloc_on_unsupported_cpu(void)
+{
+	int test_pkey = sys_pkey_alloc(0, 0);
+
+	dprintf1("pkey_alloc: %d (%d %s)\n", test_pkey, errno,
+		 strerror(errno));
+	pkey_assert(test_pkey < 0);
+	pkey_assert(errno == ENOSPC);
+}
+
 void test_mprotect_pkey_on_unsupported_cpu(int *ptr, u16 pkey)
 {
 	int size = PAGE_SIZE;
@@ -1688,6 +1698,8 @@ int main(int argc, char *argv[])
 
 		printf("running PKEY tests for unsupported CPU/OS\n");
 
+		test_pkey_alloc_on_unsupported_cpu();
+
 		ptr  = mmap(NULL, size, PROT_NONE, MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
 		assert(ptr != (void *)-1);
 		test_mprotect_pkey_on_unsupported_cpu(ptr, 1);
-- 
2.35.1

[RFC PATCH 4/6] pkeys: Lift pkey hardware check for pkey_alloc()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

pkey_alloc() is documented to return ENOSPC when the hardware does not
support pkeys.  On x86, pkey_alloc() incorrectly returns EINVAL.

This is because mm_pkey_alloc() does not check for pkey support before
returning a key.  Therefore, if the keys are not exhausted pkey_alloc()
continues on to call arch_set_user_pkey_access().  Unfortunately, when
arch_set_user_pkey_access() detects the failed support it overwrites the
ENOSPC return value with EINVAL.

Ensure consistent behavior across architectures by lifting this check to
the core mm code.

Remove a couple of 'we' references in code comments as well.

Cc: ahaas@chromium.org
Cc: clemensb@chromium.org
Cc: gdeepti@chromium.org
Cc: jkummerow@chromium.org
Cc: manoskouk@chromium.org
Cc: thibaudm@chromium.org
Cc: Florian Weimer <fweimer@redhat.com>
Cc: Sohil Mehta <sohil.mehta@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Cc: linux-api@vger.kernel.org
Fixes: e8c24d3a23a4 ("x86/pkeys: Allocation/free syscalls")
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Thanks to Sohil for pointing out that the commit message could be more
clear WRT how EINVAL is returned incorrectly.
---
 arch/powerpc/include/asm/pkeys.h | 8 +++-----
 mm/mprotect.c                    | 3 +++
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 59a2c7dbc78f..2c8351248793 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -85,18 +85,16 @@ static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
 static inline int mm_pkey_alloc(struct mm_struct *mm)
 {
 	/*
-	 * Note: this is the one and only place we make sure that the pkey is
+	 * Note: this is the one and only place to make sure that the pkey is
 	 * valid as far as the hardware is concerned. The rest of the kernel
 	 * trusts that only good, valid pkeys come out of here.
 	 */
 	u32 all_pkeys_mask = (u32)(~(0x0));
 	int ret;
 
-	if (!mmu_has_feature(MMU_FTR_PKEY))
-		return -1;
 	/*
-	 * Are we out of pkeys? We must handle this specially because ffz()
-	 * behavior is undefined if there are no zeros.
+	 * Out of pkeys?  Handle this specially because ffz() behavior is
+	 * undefined if there are no zeros.
 	 */
 	if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
 		return -1;
diff --git a/mm/mprotect.c b/mm/mprotect.c
index ba5592655ee3..56d35de33725 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -773,6 +773,9 @@ SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
 	int pkey;
 	int ret;
 
+	if (!arch_pkeys_enabled())
+		return -ENOSPC;
+
 	/* No flags supported yet. */
 	if (flags)
 		return -EINVAL;
-- 
2.35.1

[RFC PATCH 4/6] pkeys: Lift pkey hardware check for pkey_alloc()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

pkey_alloc() is documented to return ENOSPC when the hardware does not
support pkeys.  On x86, pkey_alloc() incorrectly returns EINVAL.

This is because mm_pkey_alloc() does not check for pkey support before
returning a key.  Therefore, if the keys are not exhausted pkey_alloc()
continues on to call arch_set_user_pkey_access().  Unfortunately, when
arch_set_user_pkey_access() detects the failed support it overwrites the
ENOSPC return value with EINVAL.

Ensure consistent behavior across architectures by lifting this check to
the core mm code.

Remove a couple of 'we' references in code comments as well.

Cc: ahaas@chromium.org
Cc: clemensb@chromium.org
Cc: gdeepti@chromium.org
Cc: jkummerow@chromium.org
Cc: manoskouk@chromium.org
Cc: thibaudm@chromium.org
Cc: Florian Weimer <fweimer@redhat.com>
Cc: Sohil Mehta <sohil.mehta@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Cc: linux-api@vger.kernel.org
Fixes: e8c24d3a23a4 ("x86/pkeys: Allocation/free syscalls")
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Thanks to Sohil for pointing out that the commit message could be more
clear WRT how EINVAL is returned incorrectly.
---
 arch/powerpc/include/asm/pkeys.h | 8 +++-----
 mm/mprotect.c                    | 3 +++
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 59a2c7dbc78f..2c8351248793 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -85,18 +85,16 @@ static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
 static inline int mm_pkey_alloc(struct mm_struct *mm)
 {
 	/*
-	 * Note: this is the one and only place we make sure that the pkey is
+	 * Note: this is the one and only place to make sure that the pkey is
 	 * valid as far as the hardware is concerned. The rest of the kernel
 	 * trusts that only good, valid pkeys come out of here.
 	 */
 	u32 all_pkeys_mask = (u32)(~(0x0));
 	int ret;
 
-	if (!mmu_has_feature(MMU_FTR_PKEY))
-		return -1;
 	/*
-	 * Are we out of pkeys? We must handle this specially because ffz()
-	 * behavior is undefined if there are no zeros.
+	 * Out of pkeys?  Handle this specially because ffz() behavior is
+	 * undefined if there are no zeros.
 	 */
 	if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
 		return -1;
diff --git a/mm/mprotect.c b/mm/mprotect.c
index ba5592655ee3..56d35de33725 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -773,6 +773,9 @@ SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
 	int pkey;
 	int ret;
 
+	if (!arch_pkeys_enabled())
+		return -ENOSPC;
+
 	/* No flags supported yet. */
 	if (flags)
 		return -EINVAL;
-- 
2.35.1

[RFC PATCH 5/6] pkeys: Up level pkey_free() checks

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

x86 is missing a hardware check for pkey support in pkey_free().  While
the net result is the same (-EINVAL returned), pkey_free() has well
defined behavior which will be easier to maintain in one place.

For powerpc the return code is -1 rather than -EINVAL.  This changes
that behavior slightly but this is very unlikely to break any user
space.

Lift the checks for pkey_free() to the core mm code and ensure
consistency with returning -EINVAL.

Cc: ahaas@chromium.org
Cc: clemensb@chromium.org
Cc: gdeepti@chromium.org
Cc: jkummerow@chromium.org
Cc: manoskouk@chromium.org
Cc: thibaudm@chromium.org
Cc: Florian Weimer <fweimer@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-api@vger.kernel.org
Cc: Sohil Mehta <sohil.mehta@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Thanks to Sohil for suggesting I mention the powerpc return value in the
commit message.

Also Sohil suggested changing mm_pkey_free() from int to void.  This is
added as a separate patch with his suggested by.
---
 arch/powerpc/include/asm/pkeys.h | 6 ------
 arch/x86/include/asm/pkeys.h     | 3 ---
 mm/mprotect.c                    | 8 ++++++--
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 2c8351248793..e96aa91f817b 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -107,12 +107,6 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
 
 static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
 {
-	if (!mmu_has_feature(MMU_FTR_PKEY))
-		return -1;
-
-	if (!mm_pkey_is_allocated(mm, pkey))
-		return -EINVAL;
-
 	__mm_pkey_free(mm, pkey);
 
 	return 0;
diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index 2e6c04d8a45b..da02737cc4d1 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h
@@ -107,9 +107,6 @@ int mm_pkey_alloc(struct mm_struct *mm)
 static inline
 int mm_pkey_free(struct mm_struct *mm, int pkey)
 {
-	if (!mm_pkey_is_allocated(mm, pkey))
-		return -EINVAL;
-
 	mm_set_pkey_free(mm, pkey);
 
 	return 0;
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 56d35de33725..41458e729c27 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -803,10 +803,14 @@ SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
 
 SYSCALL_DEFINE1(pkey_free, int, pkey)
 {
-	int ret;
+	int ret = -EINVAL;
+
+	if (!arch_pkeys_enabled())
+		return ret;
 
 	mmap_write_lock(current->mm);
-	ret = mm_pkey_free(current->mm, pkey);
+	if (mm_pkey_is_allocated(current->mm, pkey))
+		ret = mm_pkey_free(current->mm, pkey);
 	mmap_write_unlock(current->mm);
 
 	/*
-- 
2.35.1

[RFC PATCH 5/6] pkeys: Up level pkey_free() checks

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

x86 is missing a hardware check for pkey support in pkey_free().  While
the net result is the same (-EINVAL returned), pkey_free() has well
defined behavior which will be easier to maintain in one place.

For powerpc the return code is -1 rather than -EINVAL.  This changes
that behavior slightly but this is very unlikely to break any user
space.

Lift the checks for pkey_free() to the core mm code and ensure
consistency with returning -EINVAL.

Cc: ahaas@chromium.org
Cc: clemensb@chromium.org
Cc: gdeepti@chromium.org
Cc: jkummerow@chromium.org
Cc: manoskouk@chromium.org
Cc: thibaudm@chromium.org
Cc: Florian Weimer <fweimer@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-api@vger.kernel.org
Cc: Sohil Mehta <sohil.mehta@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Thanks to Sohil for suggesting I mention the powerpc return value in the
commit message.

Also Sohil suggested changing mm_pkey_free() from int to void.  This is
added as a separate patch with his suggested by.
---
 arch/powerpc/include/asm/pkeys.h | 6 ------
 arch/x86/include/asm/pkeys.h     | 3 ---
 mm/mprotect.c                    | 8 ++++++--
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 2c8351248793..e96aa91f817b 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -107,12 +107,6 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
 
 static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
 {
-	if (!mmu_has_feature(MMU_FTR_PKEY))
-		return -1;
-
-	if (!mm_pkey_is_allocated(mm, pkey))
-		return -EINVAL;
-
 	__mm_pkey_free(mm, pkey);
 
 	return 0;
diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index 2e6c04d8a45b..da02737cc4d1 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h
@@ -107,9 +107,6 @@ int mm_pkey_alloc(struct mm_struct *mm)
 static inline
 int mm_pkey_free(struct mm_struct *mm, int pkey)
 {
-	if (!mm_pkey_is_allocated(mm, pkey))
-		return -EINVAL;
-
 	mm_set_pkey_free(mm, pkey);
 
 	return 0;
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 56d35de33725..41458e729c27 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -803,10 +803,14 @@ SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
 
 SYSCALL_DEFINE1(pkey_free, int, pkey)
 {
-	int ret;
+	int ret = -EINVAL;
+
+	if (!arch_pkeys_enabled())
+		return ret;
 
 	mmap_write_lock(current->mm);
-	ret = mm_pkey_free(current->mm, pkey);
+	if (mm_pkey_is_allocated(current->mm, pkey))
+		ret = mm_pkey_free(current->mm, pkey);
 	mmap_write_unlock(current->mm);
 
 	/*
-- 
2.35.1

[RFC PATCH 6/6] pkeys: Change mm_pkey_free() to void

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Now that the pkey arch support is no longer checked in mm_pkey_free()
there is no reason to have it return int.

Change the return value to void.

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Suggested-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 arch/powerpc/include/asm/pkeys.h | 4 +---
 arch/x86/include/asm/pkeys.h     | 4 +---
 include/linux/pkeys.h            | 5 +----
 mm/mprotect.c                    | 6 ++++--
 4 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index e96aa91f817b..4d01a48ab941 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -105,11 +105,9 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
 	return ret;
 }
 
-static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
+static inline void mm_pkey_free(struct mm_struct *mm, int pkey)
 {
 	__mm_pkey_free(mm, pkey);
-
-	return 0;
 }
 
 /*
diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index da02737cc4d1..1f408f46fa9a 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h
@@ -105,11 +105,9 @@ int mm_pkey_alloc(struct mm_struct *mm)
 }
 
 static inline
-int mm_pkey_free(struct mm_struct *mm, int pkey)
+void mm_pkey_free(struct mm_struct *mm, int pkey)
 {
 	mm_set_pkey_free(mm, pkey);
-
-	return 0;
 }
 
 static inline int vma_pkey(struct vm_area_struct *vma)
diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
index 86be8bf27b41..bf98c50a3437 100644
--- a/include/linux/pkeys.h
+++ b/include/linux/pkeys.h
@@ -30,10 +30,7 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
 	return -1;
 }
 
-static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
-{
-	return -EINVAL;
-}
+static inline void mm_pkey_free(struct mm_struct *mm, int pkey) { }
 
 static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 			unsigned long init_val)
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 41458e729c27..e872bdd2e228 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -809,8 +809,10 @@ SYSCALL_DEFINE1(pkey_free, int, pkey)
 		return ret;
 
 	mmap_write_lock(current->mm);
-	if (mm_pkey_is_allocated(current->mm, pkey))
-		ret = mm_pkey_free(current->mm, pkey);
+	if (mm_pkey_is_allocated(current->mm, pkey)) {
+		mm_pkey_free(current->mm, pkey);
+		ret = 0;
+	}
 	mmap_write_unlock(current->mm);
 
 	/*
-- 
2.35.1

[RFC PATCH 6/6] pkeys: Change mm_pkey_free() to void

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Now that the pkey arch support is no longer checked in mm_pkey_free()
there is no reason to have it return int.

Change the return value to void.

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Suggested-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 arch/powerpc/include/asm/pkeys.h | 4 +---
 arch/x86/include/asm/pkeys.h     | 4 +---
 include/linux/pkeys.h            | 5 +----
 mm/mprotect.c                    | 6 ++++--
 4 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index e96aa91f817b..4d01a48ab941 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -105,11 +105,9 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
 	return ret;
 }
 
-static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
+static inline void mm_pkey_free(struct mm_struct *mm, int pkey)
 {
 	__mm_pkey_free(mm, pkey);
-
-	return 0;
 }
 
 /*
diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index da02737cc4d1..1f408f46fa9a 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h
@@ -105,11 +105,9 @@ int mm_pkey_alloc(struct mm_struct *mm)
 }
 
 static inline
-int mm_pkey_free(struct mm_struct *mm, int pkey)
+void mm_pkey_free(struct mm_struct *mm, int pkey)
 {
 	mm_set_pkey_free(mm, pkey);
-
-	return 0;
 }
 
 static inline int vma_pkey(struct vm_area_struct *vma)
diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
index 86be8bf27b41..bf98c50a3437 100644
--- a/include/linux/pkeys.h
+++ b/include/linux/pkeys.h
@@ -30,10 +30,7 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
 	return -1;
 }
 
-static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
-{
-	return -EINVAL;
-}
+static inline void mm_pkey_free(struct mm_struct *mm, int pkey) { }
 
 static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 			unsigned long init_val)
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 41458e729c27..e872bdd2e228 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -809,8 +809,10 @@ SYSCALL_DEFINE1(pkey_free, int, pkey)
 		return ret;
 
 	mmap_write_lock(current->mm);
-	if (mm_pkey_is_allocated(current->mm, pkey))
-		ret = mm_pkey_free(current->mm, pkey);
+	if (mm_pkey_is_allocated(current->mm, pkey)) {
+		mm_pkey_free(current->mm, pkey);
+		ret = 0;
+	}
 	mmap_write_unlock(current->mm);
 
 	/*
-- 
2.35.1

Re: [RFC PATCH 5/6] pkeys: Up level pkey_free() checks

[Christophe Leroy]

Le 11/06/2022 à 01:35, ira.weiny@intel.com a écrit :
> From: Ira Weiny <ira.weiny@intel.com>
> 
> x86 is missing a hardware check for pkey support in pkey_free().  While
> the net result is the same (-EINVAL returned), pkey_free() has well
> defined behavior which will be easier to maintain in one place.
> 
> For powerpc the return code is -1 rather than -EINVAL.  This changes
> that behavior slightly but this is very unlikely to break any user
> space.
> 
> Lift the checks for pkey_free() to the core mm code and ensure
> consistency with returning -EINVAL.
> 
> Cc: ahaas@chromium.org
> Cc: clemensb@chromium.org
> Cc: gdeepti@chromium.org
> Cc: jkummerow@chromium.org
> Cc: manoskouk@chromium.org
> Cc: thibaudm@chromium.org
> Cc: Florian Weimer <fweimer@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-api@vger.kernel.org
> Cc: Sohil Mehta <sohil.mehta@intel.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> Thanks to Sohil for suggesting I mention the powerpc return value in the
> commit message.
> 
> Also Sohil suggested changing mm_pkey_free() from int to void.  This is
> added as a separate patch with his suggested by.
> ---
>   arch/powerpc/include/asm/pkeys.h | 6 ------
>   arch/x86/include/asm/pkeys.h     | 3 ---
>   mm/mprotect.c                    | 8 ++++++--
>   3 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index 2c8351248793..e96aa91f817b 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -107,12 +107,6 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
>   
>   static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
>   {
> -	if (!mmu_has_feature(MMU_FTR_PKEY))
> -		return -1;
> -
> -	if (!mm_pkey_is_allocated(mm, pkey))
> -		return -EINVAL;
> -
>   	__mm_pkey_free(mm, pkey);
>   
>   	return 0;

If it returns always 0, the return value is pointless and the function 
mm_pkey_free() should be changed to return void.

> diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
> index 2e6c04d8a45b..da02737cc4d1 100644
> --- a/arch/x86/include/asm/pkeys.h
> +++ b/arch/x86/include/asm/pkeys.h
> @@ -107,9 +107,6 @@ int mm_pkey_alloc(struct mm_struct *mm)
>   static inline
>   int mm_pkey_free(struct mm_struct *mm, int pkey)
>   {
> -	if (!mm_pkey_is_allocated(mm, pkey))
> -		return -EINVAL;
> -
>   	mm_set_pkey_free(mm, pkey);
>   
>   	return 0;

Same.

> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 56d35de33725..41458e729c27 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -803,10 +803,14 @@ SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
>   
>   SYSCALL_DEFINE1(pkey_free, int, pkey)
>   {
> -	int ret;
> +	int ret = -EINVAL;

Don't initialise 'ret'

> +
> +	if (!arch_pkeys_enabled())
> +		return ret;

Make it explicit, do 'return -EINVAL'

Once that is done, is there any point in having a fallback version of 
mm_pkey_free() which returns -EINVAL ?

>   
>   	mmap_write_lock(current->mm);
> -	ret = mm_pkey_free(current->mm, pkey);
> +	if (mm_pkey_is_allocated(current->mm, pkey))
> +		ret = mm_pkey_free(current->mm, pkey);

Add:

	else
		ret = -EINVAL;

>   	mmap_write_unlock(current->mm);
>   
>   	/*

Re: [RFC PATCH 5/6] pkeys: Up level pkey_free() checks

[Christophe Leroy]

Le 11/06/2022 à 01:35, ira.weiny@intel.com a écrit :
> From: Ira Weiny <ira.weiny@intel.com>
> 
> x86 is missing a hardware check for pkey support in pkey_free().  While
> the net result is the same (-EINVAL returned), pkey_free() has well
> defined behavior which will be easier to maintain in one place.
> 
> For powerpc the return code is -1 rather than -EINVAL.  This changes
> that behavior slightly but this is very unlikely to break any user
> space.
> 
> Lift the checks for pkey_free() to the core mm code and ensure
> consistency with returning -EINVAL.
> 
> Cc: ahaas@chromium.org
> Cc: clemensb@chromium.org
> Cc: gdeepti@chromium.org
> Cc: jkummerow@chromium.org
> Cc: manoskouk@chromium.org
> Cc: thibaudm@chromium.org
> Cc: Florian Weimer <fweimer@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-api@vger.kernel.org
> Cc: Sohil Mehta <sohil.mehta@intel.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> Thanks to Sohil for suggesting I mention the powerpc return value in the
> commit message.
> 
> Also Sohil suggested changing mm_pkey_free() from int to void.  This is
> added as a separate patch with his suggested by.
> ---
>   arch/powerpc/include/asm/pkeys.h | 6 ------
>   arch/x86/include/asm/pkeys.h     | 3 ---
>   mm/mprotect.c                    | 8 ++++++--
>   3 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index 2c8351248793..e96aa91f817b 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -107,12 +107,6 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
>   
>   static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
>   {
> -	if (!mmu_has_feature(MMU_FTR_PKEY))
> -		return -1;
> -
> -	if (!mm_pkey_is_allocated(mm, pkey))
> -		return -EINVAL;
> -
>   	__mm_pkey_free(mm, pkey);
>   
>   	return 0;

If it returns always 0, the return value is pointless and the function 
mm_pkey_free() should be changed to return void.

> diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
> index 2e6c04d8a45b..da02737cc4d1 100644
> --- a/arch/x86/include/asm/pkeys.h
> +++ b/arch/x86/include/asm/pkeys.h
> @@ -107,9 +107,6 @@ int mm_pkey_alloc(struct mm_struct *mm)
>   static inline
>   int mm_pkey_free(struct mm_struct *mm, int pkey)
>   {
> -	if (!mm_pkey_is_allocated(mm, pkey))
> -		return -EINVAL;
> -
>   	mm_set_pkey_free(mm, pkey);
>   
>   	return 0;

Same.

> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 56d35de33725..41458e729c27 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -803,10 +803,14 @@ SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
>   
>   SYSCALL_DEFINE1(pkey_free, int, pkey)
>   {
> -	int ret;
> +	int ret = -EINVAL;

Don't initialise 'ret'

> +
> +	if (!arch_pkeys_enabled())
> +		return ret;

Make it explicit, do 'return -EINVAL'

Once that is done, is there any point in having a fallback version of 
mm_pkey_free() which returns -EINVAL ?

>   
>   	mmap_write_lock(current->mm);
> -	ret = mm_pkey_free(current->mm, pkey);
> +	if (mm_pkey_is_allocated(current->mm, pkey))
> +		ret = mm_pkey_free(current->mm, pkey);

Add:

	else
		ret = -EINVAL;

>   	mmap_write_unlock(current->mm);
>   
>   	/*

Re: [RFC PATCH 6/6] pkeys: Change mm_pkey_free() to void

[Christophe Leroy]

Le 11/06/2022 à 01:35, ira.weiny@intel.com a écrit :
> From: Ira Weiny <ira.weiny@intel.com>
> 
> Now that the pkey arch support is no longer checked in mm_pkey_free()
> there is no reason to have it return int.

Right, I see this is doing what I commented in previous patch.


> 
> Change the return value to void.
> 
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Suggested-by: Sohil Mehta <sohil.mehta@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>   arch/powerpc/include/asm/pkeys.h | 4 +---
>   arch/x86/include/asm/pkeys.h     | 4 +---
>   include/linux/pkeys.h            | 5 +----
>   mm/mprotect.c                    | 6 ++++--
>   4 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index e96aa91f817b..4d01a48ab941 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -105,11 +105,9 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
>   	return ret;
>   }
>   
> -static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
> +static inline void mm_pkey_free(struct mm_struct *mm, int pkey)
>   {
>   	__mm_pkey_free(mm, pkey);
> -
> -	return 0;
>   }
>   
>   /*
> diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
> index da02737cc4d1..1f408f46fa9a 100644
> --- a/arch/x86/include/asm/pkeys.h
> +++ b/arch/x86/include/asm/pkeys.h
> @@ -105,11 +105,9 @@ int mm_pkey_alloc(struct mm_struct *mm)
>   }
>   
>   static inline
> -int mm_pkey_free(struct mm_struct *mm, int pkey)
> +void mm_pkey_free(struct mm_struct *mm, int pkey)
>   {
>   	mm_set_pkey_free(mm, pkey);
> -
> -	return 0;
>   }
>   
>   static inline int vma_pkey(struct vm_area_struct *vma)
> diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
> index 86be8bf27b41..bf98c50a3437 100644
> --- a/include/linux/pkeys.h
> +++ b/include/linux/pkeys.h
> @@ -30,10 +30,7 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
>   	return -1;
>   }
>   
> -static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
> -{
> -	return -EINVAL;
> -}
> +static inline void mm_pkey_free(struct mm_struct *mm, int pkey) { }
>   
>   static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
>   			unsigned long init_val)
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 41458e729c27..e872bdd2e228 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -809,8 +809,10 @@ SYSCALL_DEFINE1(pkey_free, int, pkey)
>   		return ret;
>   
>   	mmap_write_lock(current->mm);
> -	if (mm_pkey_is_allocated(current->mm, pkey))
> -		ret = mm_pkey_free(current->mm, pkey);
> +	if (mm_pkey_is_allocated(current->mm, pkey)) {
> +		mm_pkey_free(current->mm, pkey);
> +		ret = 0;
> +	}

Or you could have ret = 0 by default and do

	if (mm_pkey_is_allocated(current->mm, pkey))
		mm_pkey_free(current->mm, pkey);
	else
		ret = -EINVAL;

>   	mmap_write_unlock(current->mm);
>   
>   	/*

Re: [RFC PATCH 6/6] pkeys: Change mm_pkey_free() to void

[Christophe Leroy]

Le 11/06/2022 à 01:35, ira.weiny@intel.com a écrit :
> From: Ira Weiny <ira.weiny@intel.com>
> 
> Now that the pkey arch support is no longer checked in mm_pkey_free()
> there is no reason to have it return int.

Right, I see this is doing what I commented in previous patch.


> 
> Change the return value to void.
> 
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Suggested-by: Sohil Mehta <sohil.mehta@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>   arch/powerpc/include/asm/pkeys.h | 4 +---
>   arch/x86/include/asm/pkeys.h     | 4 +---
>   include/linux/pkeys.h            | 5 +----
>   mm/mprotect.c                    | 6 ++++--
>   4 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index e96aa91f817b..4d01a48ab941 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -105,11 +105,9 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
>   	return ret;
>   }
>   
> -static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
> +static inline void mm_pkey_free(struct mm_struct *mm, int pkey)
>   {
>   	__mm_pkey_free(mm, pkey);
> -
> -	return 0;
>   }
>   
>   /*
> diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
> index da02737cc4d1..1f408f46fa9a 100644
> --- a/arch/x86/include/asm/pkeys.h
> +++ b/arch/x86/include/asm/pkeys.h
> @@ -105,11 +105,9 @@ int mm_pkey_alloc(struct mm_struct *mm)
>   }
>   
>   static inline
> -int mm_pkey_free(struct mm_struct *mm, int pkey)
> +void mm_pkey_free(struct mm_struct *mm, int pkey)
>   {
>   	mm_set_pkey_free(mm, pkey);
> -
> -	return 0;
>   }
>   
>   static inline int vma_pkey(struct vm_area_struct *vma)
> diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
> index 86be8bf27b41..bf98c50a3437 100644
> --- a/include/linux/pkeys.h
> +++ b/include/linux/pkeys.h
> @@ -30,10 +30,7 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
>   	return -1;
>   }
>   
> -static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
> -{
> -	return -EINVAL;
> -}
> +static inline void mm_pkey_free(struct mm_struct *mm, int pkey) { }
>   
>   static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
>   			unsigned long init_val)
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 41458e729c27..e872bdd2e228 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -809,8 +809,10 @@ SYSCALL_DEFINE1(pkey_free, int, pkey)
>   		return ret;
>   
>   	mmap_write_lock(current->mm);
> -	if (mm_pkey_is_allocated(current->mm, pkey))
> -		ret = mm_pkey_free(current->mm, pkey);
> +	if (mm_pkey_is_allocated(current->mm, pkey)) {
> +		mm_pkey_free(current->mm, pkey);
> +		ret = 0;
> +	}

Or you could have ret = 0 by default and do

	if (mm_pkey_is_allocated(current->mm, pkey))
		mm_pkey_free(current->mm, pkey);
	else
		ret = -EINVAL;

>   	mmap_write_unlock(current->mm);
>   
>   	/*

Re: [RFC PATCH 6/6] pkeys: Change mm_pkey_free() to void

[Ira Weiny]

On Mon, Jun 13, 2022 at 09:17:06AM +0000, Christophe Leroy wrote:
> 
> 
> Le 11/06/2022 à 01:35, ira.weiny@intel.com a écrit :
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > Now that the pkey arch support is no longer checked in mm_pkey_free()
> > there is no reason to have it return int.
> 
> Right, I see this is doing what I commented in previous patch.

Yes because it was suggested by Sohil I decided to make it a separate patch to
make the credit easier.

> > diff --git a/mm/mprotect.c b/mm/mprotect.c
> > index 41458e729c27..e872bdd2e228 100644
> > --- a/mm/mprotect.c
> > +++ b/mm/mprotect.c
> > @@ -809,8 +809,10 @@ SYSCALL_DEFINE1(pkey_free, int, pkey)
> >   		return ret;
> >   
> >   	mmap_write_lock(current->mm);
> > -	if (mm_pkey_is_allocated(current->mm, pkey))
> > -		ret = mm_pkey_free(current->mm, pkey);
> > +	if (mm_pkey_is_allocated(current->mm, pkey)) {
> > +		mm_pkey_free(current->mm, pkey);
> > +		ret = 0;
> > +	}
> 
> Or you could have ret = 0 by default and do
> 
> 	if (mm_pkey_is_allocated(current->mm, pkey))
> 		mm_pkey_free(current->mm, pkey);
> 	else
> 		ret = -EINVAL;

Yes that fits the kernel style better.

Thanks for the review!
Ira

> 
> >   	mmap_write_unlock(current->mm);
> >   
> >   	/*

Re: [RFC PATCH 6/6] pkeys: Change mm_pkey_free() to void

[Ira Weiny]

On Mon, Jun 13, 2022 at 09:17:06AM +0000, Christophe Leroy wrote:
> 
> 
> Le 11/06/2022 à 01:35, ira.weiny@intel.com a écrit :
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > Now that the pkey arch support is no longer checked in mm_pkey_free()
> > there is no reason to have it return int.
> 
> Right, I see this is doing what I commented in previous patch.

Yes because it was suggested by Sohil I decided to make it a separate patch to
make the credit easier.

> > diff --git a/mm/mprotect.c b/mm/mprotect.c
> > index 41458e729c27..e872bdd2e228 100644
> > --- a/mm/mprotect.c
> > +++ b/mm/mprotect.c
> > @@ -809,8 +809,10 @@ SYSCALL_DEFINE1(pkey_free, int, pkey)
> >   		return ret;
> >   
> >   	mmap_write_lock(current->mm);
> > -	if (mm_pkey_is_allocated(current->mm, pkey))
> > -		ret = mm_pkey_free(current->mm, pkey);
> > +	if (mm_pkey_is_allocated(current->mm, pkey)) {
> > +		mm_pkey_free(current->mm, pkey);
> > +		ret = 0;
> > +	}
> 
> Or you could have ret = 0 by default and do
> 
> 	if (mm_pkey_is_allocated(current->mm, pkey))
> 		mm_pkey_free(current->mm, pkey);
> 	else
> 		ret = -EINVAL;

Yes that fits the kernel style better.

Thanks for the review!
Ira

> 
> >   	mmap_write_unlock(current->mm);
> >   
> >   	/*

Re: [RFC PATCH 0/6] User pkey minor bug fixes

[Sohil Mehta]

On 6/10/2022 4:35 PM, ira.weiny@intel.com wrote:

> 
> glibc says it returns ENOSYS if the system does not support pkeys but I don't
> see where ENOSYS is returned?  AFAICS it just returns what the kernel returns.
> So it is probably up to user of glibc.
> 

Implementation of the pkeys system calls is arch specific and 
conditional. See kernel/sys_ni.c

glibc is probably talking about ENOSYS being returned when the 
architecture doesn't have support or the CONFIG option is disabled on 
supported architectures.

Thanks,
Sohil

Re: [RFC PATCH 0/6] User pkey minor bug fixes

[Sohil Mehta]

On 6/10/2022 4:35 PM, ira.weiny@intel.com wrote:

> 
> glibc says it returns ENOSYS if the system does not support pkeys but I don't
> see where ENOSYS is returned?  AFAICS it just returns what the kernel returns.
> So it is probably up to user of glibc.
> 

Implementation of the pkeys system calls is arch specific and 
conditional. See kernel/sys_ni.c

glibc is probably talking about ENOSYS being returned when the 
architecture doesn't have support or the CONFIG option is disabled on 
supported architectures.

Thanks,
Sohil

Re: [RFC PATCH 1/6] testing/pkeys: Add command line options

[Sohil Mehta]

On 6/10/2022 4:35 PM, ira.weiny@intel.com wrote:

> Add command line options for debug level and number of iterations.
> 
> $ ./protection_keys_64 -h
> Usage: ./protection_keys_64 [-h,-d,-i <iter>]
>          --help,-h   This help
> 	--debug,-d  Increase debug level for each -d

Is this mechanism (of counting d's) commonplace in other selftests as 
well? Looking at the test code for pkeys the debug levels run from 1-5. 
That feels like quite a few d's to input :)

Would it be easier to input the number in the command line directly?

Either way it would be useful to know the debug range in the help.
Maybe something like:
	--debug,-d  Increase debug level for each -d (1-5)

The patch seems fine to me otherwise.

> 	--iterations,-i <iter>  repeate test <iter> times
> 		default: 22
> 

Thanks,
Sohil

Re: [RFC PATCH 1/6] testing/pkeys: Add command line options

[Sohil Mehta]

On 6/10/2022 4:35 PM, ira.weiny@intel.com wrote:

> Add command line options for debug level and number of iterations.
> 
> $ ./protection_keys_64 -h
> Usage: ./protection_keys_64 [-h,-d,-i <iter>]
>          --help,-h   This help
> 	--debug,-d  Increase debug level for each -d

Is this mechanism (of counting d's) commonplace in other selftests as 
well? Looking at the test code for pkeys the debug levels run from 1-5. 
That feels like quite a few d's to input :)

Would it be easier to input the number in the command line directly?

Either way it would be useful to know the debug range in the help.
Maybe something like:
	--debug,-d  Increase debug level for each -d (1-5)

The patch seems fine to me otherwise.

> 	--iterations,-i <iter>  repeate test <iter> times
> 		default: 22
> 

Thanks,
Sohil

Re: [RFC PATCH 2/6] testing/pkeys: Don't use uninitialized variable

[Sohil Mehta]

On 6/10/2022 4:35 PM, ira.weiny@intel.com wrote:
> diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c
> index d0183c381859..43e47de19c0d 100644
> --- a/tools/testing/selftests/vm/protection_keys.c
> +++ b/tools/testing/selftests/vm/protection_keys.c
> @@ -1225,9 +1225,9 @@ void test_pkey_alloc_exhaust(int *ptr, u16 pkey)
>   		int new_pkey;
>   		dprintf1("%s() alloc loop: %d\n", __func__, i);
>   		new_pkey = alloc_pkey();
> -		dprintf4("%s()::%d, err: %d pkey_reg: 0x%016llx"
> +		dprintf4("%s()::%d, errno: %d pkey_reg: 0x%016llx"

What is errno referring to over here? There are a few things happening 
in alloc_pkey(). I guess it would show the latest error that happened. 
Does errno need to be set to 0 before the call?

Also, would it be useful to print the return value (new_pkey) from 
alloc_pkey() here?

>   				" shadow: 0x%016llx\n",
> -				__func__, __LINE__, err, __read_pkey_reg(),
> +				__func__, __LINE__, errno, __read_pkey_reg(),
>   				shadow_pkey_reg);
>   		read_pkey_reg(); /* for shadow checking */
>   		dprintf2("%s() errno: %d ENOSPC: %d\n", __func__, errno, ENOSPC);

Sohil

Re: [RFC PATCH 2/6] testing/pkeys: Don't use uninitialized variable

[Sohil Mehta]

On 6/10/2022 4:35 PM, ira.weiny@intel.com wrote:
> diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c
> index d0183c381859..43e47de19c0d 100644
> --- a/tools/testing/selftests/vm/protection_keys.c
> +++ b/tools/testing/selftests/vm/protection_keys.c
> @@ -1225,9 +1225,9 @@ void test_pkey_alloc_exhaust(int *ptr, u16 pkey)
>   		int new_pkey;
>   		dprintf1("%s() alloc loop: %d\n", __func__, i);
>   		new_pkey = alloc_pkey();
> -		dprintf4("%s()::%d, err: %d pkey_reg: 0x%016llx"
> +		dprintf4("%s()::%d, errno: %d pkey_reg: 0x%016llx"

What is errno referring to over here? There are a few things happening 
in alloc_pkey(). I guess it would show the latest error that happened. 
Does errno need to be set to 0 before the call?

Also, would it be useful to print the return value (new_pkey) from 
alloc_pkey() here?

>   				" shadow: 0x%016llx\n",
> -				__func__, __LINE__, err, __read_pkey_reg(),
> +				__func__, __LINE__, errno, __read_pkey_reg(),
>   				shadow_pkey_reg);
>   		read_pkey_reg(); /* for shadow checking */
>   		dprintf2("%s() errno: %d ENOSPC: %d\n", __func__, errno, ENOSPC);

Sohil

Re: [RFC PATCH 1/6] testing/pkeys: Add command line options

[Ira Weiny]

On Mon, Jun 13, 2022 at 03:31:02PM -0700, Mehta, Sohil wrote:
> On 6/10/2022 4:35 PM, ira.weiny@intel.com wrote:
> 
> > Add command line options for debug level and number of iterations.
> > 
> > $ ./protection_keys_64 -h
> > Usage: ./protection_keys_64 [-h,-d,-i <iter>]
> >          --help,-h   This help
> > 	--debug,-d  Increase debug level for each -d
> 
> Is this mechanism (of counting d's) commonplace in other selftests as well?
> Looking at the test code for pkeys the debug levels run from 1-5. That feels
> like quite a few d's to input :)

I've seen (and used) it before yes.  See ibnetdiscover.

...
# Debugging flags
-d raise the IB debugging level. May be used several times (-ddd or -d -d -d).
...
-v increase the application verbosity level. May be used several times (-vv or -v -v -v)
...
	- https://linux.die.net/man/8/ibnetdiscover

But a much more mainstream example I can think of is verbosity level with
lspci.

16:29:12 > lspci -h
...
Display options:
-v              Be verbose (-vv or -vvv for higher verbosity)
...

> 
> Would it be easier to input the number in the command line directly?
> 
> Either way it would be useful to know the debug range in the help.
> Maybe something like:
> 	--debug,-d  Increase debug level for each -d (1-5)

I'm inclined not to do this because it would encode the max debug level.  On
the other hand I'm not sure why there are 5 levels now.  ;-)

Having the multiple options specified was an easy way to maintain the large
number of levels.

Ira

> 
> The patch seems fine to me otherwise.
> 
> > 	--iterations,-i <iter>  repeate test <iter> times
> > 		default: 22
> > 
> 
> Thanks,
> Sohil

Re: [RFC PATCH 1/6] testing/pkeys: Add command line options

[Ira Weiny]

On Mon, Jun 13, 2022 at 03:31:02PM -0700, Mehta, Sohil wrote:
> On 6/10/2022 4:35 PM, ira.weiny@intel.com wrote:
> 
> > Add command line options for debug level and number of iterations.
> > 
> > $ ./protection_keys_64 -h
> > Usage: ./protection_keys_64 [-h,-d,-i <iter>]
> >          --help,-h   This help
> > 	--debug,-d  Increase debug level for each -d
> 
> Is this mechanism (of counting d's) commonplace in other selftests as well?
> Looking at the test code for pkeys the debug levels run from 1-5. That feels
> like quite a few d's to input :)

I've seen (and used) it before yes.  See ibnetdiscover.

...
# Debugging flags
-d raise the IB debugging level. May be used several times (-ddd or -d -d -d).
...
-v increase the application verbosity level. May be used several times (-vv or -v -v -v)
...
	- https://linux.die.net/man/8/ibnetdiscover

But a much more mainstream example I can think of is verbosity level with
lspci.

16:29:12 > lspci -h
...
Display options:
-v              Be verbose (-vv or -vvv for higher verbosity)
...

> 
> Would it be easier to input the number in the command line directly?
> 
> Either way it would be useful to know the debug range in the help.
> Maybe something like:
> 	--debug,-d  Increase debug level for each -d (1-5)

I'm inclined not to do this because it would encode the max debug level.  On
the other hand I'm not sure why there are 5 levels now.  ;-)

Having the multiple options specified was an easy way to maintain the large
number of levels.

Ira

> 
> The patch seems fine to me otherwise.
> 
> > 	--iterations,-i <iter>  repeate test <iter> times
> > 		default: 22
> > 
> 
> Thanks,
> Sohil

Re: [RFC PATCH 2/6] testing/pkeys: Don't use uninitialized variable

[Ira Weiny]

On Mon, Jun 13, 2022 at 03:48:56PM -0700, Mehta, Sohil wrote:
> On 6/10/2022 4:35 PM, ira.weiny@intel.com wrote:
> > diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c
> > index d0183c381859..43e47de19c0d 100644
> > --- a/tools/testing/selftests/vm/protection_keys.c
> > +++ b/tools/testing/selftests/vm/protection_keys.c
> > @@ -1225,9 +1225,9 @@ void test_pkey_alloc_exhaust(int *ptr, u16 pkey)
> >   		int new_pkey;
> >   		dprintf1("%s() alloc loop: %d\n", __func__, i);
> >   		new_pkey = alloc_pkey();
> > -		dprintf4("%s()::%d, err: %d pkey_reg: 0x%016llx"
> > +		dprintf4("%s()::%d, errno: %d pkey_reg: 0x%016llx"
> 
> What is errno referring to over here?  There are a few things happening in
> alloc_pkey().

Good point, but the only system call in alloc_pkey() is pkey_alloc() so it will
be the errno from there.

In test_pkey_alloc_exhaust() we are expecting the errno to be from pkey_alloc()

...
                if ((new_pkey == -1) && (errno == ENOSPC)) {
...


> I guess it would show the latest error that happened. Does
> errno need to be set to 0 before the call?

Maybe.  Now that I look again errno is printed just below at level 2.

                dprintf2("%s() errno: %d ENOSPC: %d\n", __func__, errno, ENOSPC);

I missed that.

> 
> Also, would it be useful to print the return value (new_pkey) from
> alloc_pkey() here?

Yea that might be useful.  Perhaps change err to new_pkey instead since errno
is already printed.

Ira

> 
> >   				" shadow: 0x%016llx\n",
> > -				__func__, __LINE__, err, __read_pkey_reg(),
> > +				__func__, __LINE__, errno, __read_pkey_reg(),
> >   				shadow_pkey_reg);
> >   		read_pkey_reg(); /* for shadow checking */
> >   		dprintf2("%s() errno: %d ENOSPC: %d\n", __func__, errno, ENOSPC);
> 
> Sohil

Re: [RFC PATCH 2/6] testing/pkeys: Don't use uninitialized variable

[Ira Weiny]

On Mon, Jun 13, 2022 at 03:48:56PM -0700, Mehta, Sohil wrote:
> On 6/10/2022 4:35 PM, ira.weiny@intel.com wrote:
> > diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c
> > index d0183c381859..43e47de19c0d 100644
> > --- a/tools/testing/selftests/vm/protection_keys.c
> > +++ b/tools/testing/selftests/vm/protection_keys.c
> > @@ -1225,9 +1225,9 @@ void test_pkey_alloc_exhaust(int *ptr, u16 pkey)
> >   		int new_pkey;
> >   		dprintf1("%s() alloc loop: %d\n", __func__, i);
> >   		new_pkey = alloc_pkey();
> > -		dprintf4("%s()::%d, err: %d pkey_reg: 0x%016llx"
> > +		dprintf4("%s()::%d, errno: %d pkey_reg: 0x%016llx"
> 
> What is errno referring to over here?  There are a few things happening in
> alloc_pkey().

Good point, but the only system call in alloc_pkey() is pkey_alloc() so it will
be the errno from there.

In test_pkey_alloc_exhaust() we are expecting the errno to be from pkey_alloc()

...
                if ((new_pkey == -1) && (errno == ENOSPC)) {
...


> I guess it would show the latest error that happened. Does
> errno need to be set to 0 before the call?

Maybe.  Now that I look again errno is printed just below at level 2.

                dprintf2("%s() errno: %d ENOSPC: %d\n", __func__, errno, ENOSPC);

I missed that.

> 
> Also, would it be useful to print the return value (new_pkey) from
> alloc_pkey() here?

Yea that might be useful.  Perhaps change err to new_pkey instead since errno
is already printed.

Ira

> 
> >   				" shadow: 0x%016llx\n",
> > -				__func__, __LINE__, err, __read_pkey_reg(),
> > +				__func__, __LINE__, errno, __read_pkey_reg(),
> >   				shadow_pkey_reg);
> >   		read_pkey_reg(); /* for shadow checking */
> >   		dprintf2("%s() errno: %d ENOSPC: %d\n", __func__, errno, ENOSPC);
> 
> Sohil

Re: [RFC PATCH 3/6] testing/pkeys: Add additional test for pkey_alloc()

[Sohil Mehta]

On 6/10/2022 4:35 PM, ira.weiny@intel.com wrote:
>   
> +void test_pkey_alloc_on_unsupported_cpu(void)
> +{
> +	int test_pkey = sys_pkey_alloc(0, 0);
> +
> +	dprintf1("pkey_alloc: %d (%d %s)\n", test_pkey, errno,
> +		 strerror(errno));
> +	pkey_assert(test_pkey < 0);
> +	pkey_assert(errno == ENOSPC);

This assert fails on a kernel with 
CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS disabled.

Since pkey_alloc() is an architecture dependent syscall, ENOSYS is 
returned instead of ENOSPC when support is disabled at compile time.
See kernel/sys_ni.c

This brings us to an interesting question.

Should we have different return error codes when compile support is 
disabled vs when runtime support is missing?

Here is the current behavior for pkey_alloc():

No compile time support -> return ENOSYS
No runtime support (but compile time support present) -> return ENOSPC

I would think applications would prefer the same error code. But, I am 
not sure if we can achieve this now due to ABI reasons.


> +}
> +

Re: [RFC PATCH 3/6] testing/pkeys: Add additional test for pkey_alloc()

[Sohil Mehta]

On 6/10/2022 4:35 PM, ira.weiny@intel.com wrote:
>   
> +void test_pkey_alloc_on_unsupported_cpu(void)
> +{
> +	int test_pkey = sys_pkey_alloc(0, 0);
> +
> +	dprintf1("pkey_alloc: %d (%d %s)\n", test_pkey, errno,
> +		 strerror(errno));
> +	pkey_assert(test_pkey < 0);
> +	pkey_assert(errno == ENOSPC);

This assert fails on a kernel with 
CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS disabled.

Since pkey_alloc() is an architecture dependent syscall, ENOSYS is 
returned instead of ENOSPC when support is disabled at compile time.
See kernel/sys_ni.c

This brings us to an interesting question.

Should we have different return error codes when compile support is 
disabled vs when runtime support is missing?

Here is the current behavior for pkey_alloc():

No compile time support -> return ENOSYS
No runtime support (but compile time support present) -> return ENOSPC

I would think applications would prefer the same error code. But, I am 
not sure if we can achieve this now due to ABI reasons.


> +}
> +

Re: [RFC PATCH 4/6] pkeys: Lift pkey hardware check for pkey_alloc()

[Sohil Mehta]

> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index ba5592655ee3..56d35de33725 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -773,6 +773,9 @@ SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
>   	int pkey;
>   	int ret;
>   
> +	if (!arch_pkeys_enabled())
> +		return -ENOSPC;
> +

See comments in patch 3/6. Since we are modifying (fixing) old behavior, 
should we just return ENOSYS to make this consistent?

Sohil

>   	/* No flags supported yet. */
>   	if (flags)
>   		return -EINVAL;

Re: [RFC PATCH 4/6] pkeys: Lift pkey hardware check for pkey_alloc()

[Sohil Mehta]

> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index ba5592655ee3..56d35de33725 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -773,6 +773,9 @@ SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
>   	int pkey;
>   	int ret;
>   
> +	if (!arch_pkeys_enabled())
> +		return -ENOSPC;
> +

See comments in patch 3/6. Since we are modifying (fixing) old behavior, 
should we just return ENOSYS to make this consistent?

Sohil

>   	/* No flags supported yet. */
>   	if (flags)
>   		return -EINVAL;

Re: [RFC PATCH 3/6] testing/pkeys: Add additional test for pkey_alloc()

[Dave Hansen]

On 6/16/22 12:25, Sohil Mehta wrote:
> Should we have different return error codes when compile support is 
> disabled vs when runtime support is missing?

It doesn't *really* matter.  Programs have to be able to run on old
kernels which will return ENOSYS.  So, _when_ new kernels return ENOSYS
or ENOSPC is pretty immaterial.

Re: [RFC PATCH 3/6] testing/pkeys: Add additional test for pkey_alloc()

[Dave Hansen]

On 6/16/22 12:25, Sohil Mehta wrote:
> Should we have different return error codes when compile support is 
> disabled vs when runtime support is missing?

It doesn't *really* matter.  Programs have to be able to run on old
kernels which will return ENOSYS.  So, _when_ new kernels return ENOSYS
or ENOSPC is pretty immaterial.