Gabriel Paubert <paub...@iram.es> writes:
> On Thu, Jun 28, 2018 at 11:56:34PM -0300, Thiago Jung Bauermann wrote: >> >> Hello, >> >> Ram Pai <linux...@us.ibm.com> writes: >> >> > 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. >> >> > Signed-off-by: Ram Pai <linux...@us.ibm.com> >> > --- >> > 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. > 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? >> 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). > > 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