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

[Ram Pai]

On Fri, Jun 29, 2018 at 09:58:37PM -0300, Thiago Jung Bauermann wrote:
> 
> Gabriel Paubert <paubert at iram.es> writes:
> 
> > On Thu, Jun 28, 2018 at 11:56:34PM -0300, Thiago Jung Bauermann wrote:
> >>
> >> Hello,
> >>
> >> Ram Pai <linuxram at 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 <linuxram at 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[])
> {

>         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

My intel box does the same. It makes it zero. So does my
powerpc box.  Mathematically, (1 << -1) is nothing but 2^-1,
which is 1/2, which is 0.5, and when rounded it has to be 0.

However, yes, GCC defines it to be 'undefined'.  gcc compiler does
warn 'left shift count is negative'. Will have to fix it.

Thanks for catching this.

> 
> 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 tried hard not to sprikle if-then-else in the code. Makes it less
	elegant.  But then, correctness is  more important than
	elegance.

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

yes.

Thanks,
RP