Re: [PATCH 1/2] powerpc/pkeys: preallocate execute_only key only if the key is available.

On Fri, Jun 29, 2018 at 09:58:37PM -0300, Thiago Jung Bauermann wrote:

Gabriel Paubert [off-list ref] writes:

quoted

On Thu, Jun 28, 2018 at 11:56:34PM -0300, Thiago Jung Bauermann wrote:

quoted

Hello,

Ram Pai [off-list ref] writes:

quoted

Key 2 is preallocated and reserved for execute-only key. In rare
cases if key-2 is unavailable, mprotect(PROT_EXEC) will behave
incorrectly. NOTE: mprotect(PROT_EXEC) uses execute-only key.

Ensure key 2 is available for preallocation before reserving it for
execute_only purpose.  Problem noticed by Michael Ellermen.

Since "powerpc/pkeys: Preallocate execute-only key" isn't upstream yet,
this patch could be squashed into it.

quoted

Signed-off-by: Ram Pai <redacted>
---
 arch/powerpc/mm/pkeys.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index cec990c..0b03914 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c

@@ -19,6 +19,7 @@
 u64  pkey_amr_mask;		/* Bits in AMR not to be touched */
 u64  pkey_iamr_mask;		/* Bits in AMR not to be touched */
 u64  pkey_uamor_mask;		/* Bits in UMOR not to be touched */
+int  execute_only_key = 2;

 #define AMR_BITS_PER_PKEY 2
 #define AMR_RD_BIT 0x1UL

@@ -26,7 +27,6 @@
 #define IAMR_EX_BIT 0x1UL
 #define PKEY_REG_BITS (sizeof(u64)*8)
 #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
-#define EXECUTE_ONLY_KEY 2

 static void scan_pkey_feature(void)
 {

@@ -122,8 +122,12 @@ int pkey_initialize(void)
 #else
 	os_reserved = 0;
 #endif
+
+	if ((pkeys_total - os_reserved) <= execute_only_key)
+		execute_only_key = -1;
+
 	/* Bits are in LE format. */
-	reserved_allocation_mask = (0x1 << 1) | (0x1 << EXECUTE_ONLY_KEY);
+	reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key);

My understanding is that left-shifting by a negative amount is undefined
behavior in C. A quick test tells me that at least on the couple of
machines I tested, 1 < -1 = 0. Does GCC guarantee that behavior?

Not in general. It probably always works on Power because of the definition
of the machine instruction for shifts with variable amount (consider the
shift amount unsigned and take it modulo twice the width of the operand),

Ok, thanks for confirming.

quoted

but for example it fails on x86 (1<<-1 gives 0x80000000).

Strange, this works on my laptop with an Intel(R) Core(TM) i5-7300U CPU:

$ cat blah.c
#include <stdio.h>

int main(int argc, char *argv[])
{
        printf("1 << -1 = %llx\n", (unsigned long long) 1 << -1);
        return 0;
}
$ make blah
cc     blah.c   -o blah
blah.c: In function 'main':
blah.c:5:52: warning: left shift count is negative [-Wshift-count-negative]
  printf("1 << -1 = %llx\n", (unsigned long long) 1 << -1);
                                                    ^~
$ ./blah
1 << -1 = 0

Even if I change the cast and printf format to int, the result is the
same. Or am I doing it wrong?

Try something more dynamic, 1 << -1 is evaluated at compile time by gcc,
and when you get a warning, the compile time expression evaluation does
not always give the same result as the run time machine instructions.

To test this I wrote (yes, the error checking is approximate, it would
be better to use strtol):

/***************************************************************************/
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char **argv)
{
	int val, amount, valid;
	if (argc != 3) {
		printf("Needs 2 arguments!\n");
		return EXIT_FAILURE;
	}
	valid = sscanf(argv[1], "%d", &val);
	if (valid == 1) {
		valid = sscanf(argv[2], "%d", &amount);
	}
	if (valid == 1) {
		printf("%d shifted by %d is %d\n", val, amount, val<<amount);
		return EXIT_SUCCESS;
	} else {
		printf("Both arguments must be integers!\n");
		return EXIT_FAILURE;
	}
}
/***************************************************************************/

Compile it, and then run it with 1 and -1 as parameters. The result is
not the same on PPC and x86.

quoted

quoted

If so, a comment pointing this out would make this less confusing.

Unless I miss something, this code is run once at boot, so its
performance is irrelevant.

In this case simply rewrite it as:

	reserved_allocation_mask = 0x1 << 1;
	if ( (pkeys_total - os_reserved) <= execute_only_key) {
		execute_only_key = -1;
	} else {
		reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key);
	}

I agree it's clearer and more robust code (except that the first
line should be inside the if block).

Indeed, sorry for this.

	Gabriel

quoted

Caveat, I have assumed that the code will either:
- only run once,
- pkeys_total and os_reserved are int, not unsigned

Both of the above are true.

--
Thiago Jung Bauermann
IBM Linux Technology Center