[v2,1/6] powerpc/pkeys: Enable all user-allocatable pkeys at init.

Message ID 1528936144-6696-2-git-send-email-linuxram@us.ibm.com
State Superseded
Headers show
Series
  • powerpc/pkeys: fixes to pkeys
Related show

Commit Message

Ram Pai June 14, 2018, 12:28 a.m.
In a multithreaded application, a key allocated by one thread must
be activate and usable on all threads.

Currently this is not the case, because the UAMOR bits for all keys are
disabled by default. When a new key is allocated in one thread, though
the corresponding UAMOR bits for that thread get enabled, the UAMOR bits
for all other existing threads continue to have their bits disabled.
Other threads have no way to set permissions on the key, effectively
making the key useless.

Enable the UAMOR bits for all keys, at process creation. Since the
contents of UAMOR are inherited at fork, all threads are capable of
modifying the permissions on any key.

BTW: changing the permission on unallocated keys has no effect, till
those keys are not associated with any PTEs. The kernel will anyway
disallow to association of unallocated keys with PTEs.

CC: Andy Lutomirski <luto@kernel.org>
CC: Florian Weimer <fweimer@redhat.com>
CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
CC: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/powerpc/mm/pkeys.c |   40 ++++++++++++++++++++++------------------
 1 files changed, 22 insertions(+), 18 deletions(-)

Comments

Michael Ellerman June 19, 2018, 12:39 p.m. | #1
Ram Pai <linuxram@us.ibm.com> writes:

> In a multithreaded application, a key allocated by one thread must
> be activate and usable on all threads.
>
> Currently this is not the case, because the UAMOR bits for all keys are
> disabled by default. When a new key is allocated in one thread, though
> the corresponding UAMOR bits for that thread get enabled, the UAMOR bits
> for all other existing threads continue to have their bits disabled.
> Other threads have no way to set permissions on the key, effectively
> making the key useless.

This all seems a bit strongly worded to me. It's arguable whether a key
should be usable by the thread that allocated it or all threads.

You could conceivably have a design where threads are blocked from using
a key until they're given permission to do so by the thread that
allocated the key.

But we're changing the behaviour to match x86 and because we don't have
an API to grant another thread access to a key. Right?

> Enable the UAMOR bits for all keys, at process creation. Since the
> contents of UAMOR are inherited at fork, all threads are capable of
> modifying the permissions on any key.
>
> BTW: changing the permission on unallocated keys has no effect, till
> those keys are not associated with any PTEs. The kernel will anyway
> disallow to association of unallocated keys with PTEs.

This is an ABI change, which is bad, but I guess we call it a bug fix
because things didn't really work previously?

I'll tag it:

  Fixes: cf43d3b26452 ("powerpc: Enable pkey subsystem")
  Cc: stable@vger.kernel.org # v4.16+


cheers
Ram Pai June 19, 2018, 2:25 p.m. | #2
On Tue, Jun 19, 2018 at 10:39:52PM +1000, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > In a multithreaded application, a key allocated by one thread must
> > be activate and usable on all threads.
> >
> > Currently this is not the case, because the UAMOR bits for all keys are
> > disabled by default. When a new key is allocated in one thread, though
> > the corresponding UAMOR bits for that thread get enabled, the UAMOR bits
> > for all other existing threads continue to have their bits disabled.
> > Other threads have no way to set permissions on the key, effectively
> > making the key useless.
> 
> This all seems a bit strongly worded to me. It's arguable whether a key
> should be usable by the thread that allocated it or all threads.
> 
> You could conceivably have a design where threads are blocked from using
> a key until they're given permission to do so by the thread that
> allocated the key.
> 
> But we're changing the behaviour to match x86 and because we don't have
> an API to grant another thread access to a key. Right?
> 

correct. The other threads have no way to access or change the
permissions on the key.


> > Enable the UAMOR bits for all keys, at process creation. Since the
> > contents of UAMOR are inherited at fork, all threads are capable of
> > modifying the permissions on any key.
> >
> > BTW: changing the permission on unallocated keys has no effect, till
> > those keys are not associated with any PTEs. The kernel will anyway
> > disallow to association of unallocated keys with PTEs.
> 
> This is an ABI change, which is bad, but I guess we call it a bug fix
> because things didn't really work previously?

Yes its a behaviorial change for the better. There is no downside
to the change because no applications should break. Single threaded
apps will continue to just work fine. Multithreaded applications,
which were unable to consume the API/ABI, will now be able to do so.

> 
> I'll tag it:
> 
>   Fixes: cf43d3b26452 ("powerpc: Enable pkey subsystem")
>   Cc: stable@vger.kernel.org # v4.16+

thanks,
RP
Michael Ellerman June 21, 2018, 4:14 a.m. | #3
Ram Pai <linuxram@us.ibm.com> writes:
> On Tue, Jun 19, 2018 at 10:39:52PM +1000, Michael Ellerman wrote:
>> Ram Pai <linuxram@us.ibm.com> writes:
>> 
>> > In a multithreaded application, a key allocated by one thread must
>> > be activate and usable on all threads.
>> >
>> > Currently this is not the case, because the UAMOR bits for all keys are
>> > disabled by default. When a new key is allocated in one thread, though
>> > the corresponding UAMOR bits for that thread get enabled, the UAMOR bits
>> > for all other existing threads continue to have their bits disabled.
>> > Other threads have no way to set permissions on the key, effectively
>> > making the key useless.
>> 
>> This all seems a bit strongly worded to me. It's arguable whether a key
>> should be usable by the thread that allocated it or all threads.
>> 
>> You could conceivably have a design where threads are blocked from using
>> a key until they're given permission to do so by the thread that
>> allocated the key.
>> 
>> But we're changing the behaviour to match x86 and because we don't have
>> an API to grant another thread access to a key. Right?
>
> correct. The other threads have no way to access or change the
> permissions on the key.

OK.

Though prior to patch 6 all threads have read/write permissions for all
keys, so they don't necessarily need to change permissions on a key
allocated by another thread.

>> > Enable the UAMOR bits for all keys, at process creation. Since the
>> > contents of UAMOR are inherited at fork, all threads are capable of
>> > modifying the permissions on any key.
>> >
>> > BTW: changing the permission on unallocated keys has no effect, till
>> > those keys are not associated with any PTEs. The kernel will anyway
>> > disallow to association of unallocated keys with PTEs.
>> 
>> This is an ABI change, which is bad, but I guess we call it a bug fix
>> because things didn't really work previously?
>
> Yes its a behaviorial change for the better. There is no downside
> to the change because no applications should break. Single threaded
> apps will continue to just work fine. Multithreaded applications,
> which were unable to consume the API/ABI, will now be able to do so.

Multi-threaded applications were able to use the API, as long as they
were satisfied with the semantics it provided, ie. that restrictions on
a key were only possible on the thread that allocated the key.

I'm not trying to argue for the sake of it, it's important that we
understand the subtleties of what we're changing and how it affects
existing software - even if we think there is essentially no existing
software.

I'll try and massage the change log to capture it.

I ended up with what's below.

cheers

  powerpc/pkeys: Give all threads control of their key permissions
  
  Currently in a multithreaded application, a key allocated by one
  thread is not usable by other threads. By "not usable" we mean that
  other threads are unable to change the access permissions for that
  key for themselves.
  
  When a new key is allocated in one thread, the corresponding UAMOR
  bits for that thread get enabled, however the UAMOR bits for that key
  for all other threads remain disabled.
  
  Other threads have no way to set permissions on the key, and the
  current default permissions are that read/write is enabled for all
  keys, which means the key has no effect for other threads. Although
  that may be the desired behaviour in some circumstances, having all
  threads able to control their permissions for the key is more
  flexible.
  
  The current behaviour also differs from the x86 behaviour, which is
  problematic for users.
  
  To fix this, enable the UAMOR bits for all keys, at process
  creation (in start_thread(), ie exec time). Since the contents of
  UAMOR are inherited at fork, all threads are capable of modifying the
  permissions on any key.
  
  This is technically an ABI break on powerpc, but pkey support is
  fairly new on powerpc and not widely used, and this brings us into
  line with x86.
  
  Fixes: cf43d3b26452 ("powerpc: Enable pkey subsystem")
  Cc: stable@vger.kernel.org # v4.16+
  Tested-by: Florian Weimer <fweimer@redhat.com>
  Signed-off-by: Ram Pai <linuxram@us.ibm.com>
  [mpe: Reword some of the changelog]
  Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Ram Pai June 21, 2018, 5:24 p.m. | #4
On Thu, Jun 21, 2018 at 02:14:53PM +1000, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> > On Tue, Jun 19, 2018 at 10:39:52PM +1000, Michael Ellerman wrote:
> >> Ram Pai <linuxram@us.ibm.com> writes:
> >> 
> >> > In a multithreaded application, a key allocated by one thread must
> >> > be activate and usable on all threads.
> >> >
> >> > Currently this is not the case, because the UAMOR bits for all keys are
> >> > disabled by default. When a new key is allocated in one thread, though
> >> > the corresponding UAMOR bits for that thread get enabled, the UAMOR bits
> >> > for all other existing threads continue to have their bits disabled.
> >> > Other threads have no way to set permissions on the key, effectively
> >> > making the key useless.
> >> 
> >> This all seems a bit strongly worded to me. It's arguable whether a key
> >> should be usable by the thread that allocated it or all threads.
> >> 
> >> You could conceivably have a design where threads are blocked from using
> >> a key until they're given permission to do so by the thread that
> >> allocated the key.
> >> 
> >> But we're changing the behaviour to match x86 and because we don't have
> >> an API to grant another thread access to a key. Right?
> >
> > correct. The other threads have no way to access or change the
> > permissions on the key.
> 
> OK.
> 
> Though prior to patch 6 all threads have read/write permissions for all
> keys, so they don't necessarily need to change permissions on a key
> allocated by another thread.
> 
> >> > Enable the UAMOR bits for all keys, at process creation. Since the
> >> > contents of UAMOR are inherited at fork, all threads are capable of
> >> > modifying the permissions on any key.
> >> >
> >> > BTW: changing the permission on unallocated keys has no effect, till
> >> > those keys are not associated with any PTEs. The kernel will anyway
> >> > disallow to association of unallocated keys with PTEs.
> >> 
> >> This is an ABI change, which is bad, but I guess we call it a bug fix
> >> because things didn't really work previously?
> >
> > Yes its a behaviorial change for the better. There is no downside
> > to the change because no applications should break. Single threaded
> > apps will continue to just work fine. Multithreaded applications,
> > which were unable to consume the API/ABI, will now be able to do so.
> 
> Multi-threaded applications were able to use the API, as long as they
> were satisfied with the semantics it provided, ie. that restrictions on
> a key were only possible on the thread that allocated the key.
> 
> I'm not trying to argue for the sake of it, it's important that we
> understand the subtleties of what we're changing and how it affects
> existing software - even if we think there is essentially no existing
> software.
> 
> I'll try and massage the change log to capture it.
> 
> I ended up with what's below.
> 
> cheers
> 
>   powerpc/pkeys: Give all threads control of their key permissions
>   
>   Currently in a multithreaded application, a key allocated by one
>   thread is not usable by other threads. By "not usable" we mean that
>   other threads are unable to change the access permissions for that
>   key for themselves.
>   
>   When a new key is allocated in one thread, the corresponding UAMOR
>   bits for that thread get enabled, however the UAMOR bits for that key
>   for all other threads remain disabled.
>   
>   Other threads have no way to set permissions on the key, and the
>   current default permissions are that read/write is enabled for all
>   keys, which means the key has no effect for other threads. Although
>   that may be the desired behaviour in some circumstances, having all
>   threads able to control their permissions for the key is more
>   flexible.
>   
>   The current behaviour also differs from the x86 behaviour, which is
>   problematic for users.
>   
>   To fix this, enable the UAMOR bits for all keys, at process
>   creation (in start_thread(), ie exec time). Since the contents of
>   UAMOR are inherited at fork, all threads are capable of modifying the
>   permissions on any key.
>   
>   This is technically an ABI break on powerpc, but pkey support is
>   fairly new on powerpc and not widely used, and this brings us into
>   line with x86.

Wow! yes it crisply captures the subtle API change and the reasoning
behind it.

RP

Patch

diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index e6f500f..6529f4e 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -15,8 +15,9 @@ 
 int  pkeys_total;		/* Total pkeys as per device tree */
 bool pkeys_devtree_defined;	/* pkey property exported by device tree */
 u32  initial_allocation_mask;	/* Bits set for reserved keys */
-u64  pkey_amr_uamor_mask;	/* Bits in AMR/UMOR not to be touched */
+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 */
 
 #define AMR_BITS_PER_PKEY 2
 #define AMR_RD_BIT 0x1UL
@@ -119,20 +120,22 @@  int pkey_initialize(void)
 #else
 	os_reserved = 0;
 #endif
-	initial_allocation_mask = ~0x0;
-	pkey_amr_uamor_mask = ~0x0ul;
+	initial_allocation_mask  = (0x1 << 0) | (0x1 << 1);
+
+	/* register mask is in BE format */
+	pkey_amr_mask = ~0x0ul;
 	pkey_iamr_mask = ~0x0ul;
-	/*
-	 * key 0, 1 are reserved.
-	 * key 0 is the default key, which allows read/write/execute.
-	 * key 1 is recommended not to be used. PowerISA(3.0) page 1015,
-	 * programming note.
-	 */
-	for (i = 2; i < (pkeys_total - os_reserved); i++) {
-		initial_allocation_mask &= ~(0x1 << i);
-		pkey_amr_uamor_mask &= ~(0x3ul << pkeyshift(i));
+
+	for (i = 0; i < (pkeys_total - os_reserved); i++) {
+		pkey_amr_mask &= ~(0x3ul << pkeyshift(i));
 		pkey_iamr_mask &= ~(0x1ul << pkeyshift(i));
 	}
+
+	pkey_uamor_mask = ~0x0ul;
+	pkey_uamor_mask &= ~(0x3ul << pkeyshift(0));
+	for (i = (pkeys_total - os_reserved); i < pkeys_total; i++)
+		pkey_uamor_mask &= ~(0x3ul << pkeyshift(i));
+
 	return 0;
 }
 
@@ -289,9 +292,6 @@  void thread_pkey_regs_restore(struct thread_struct *new_thread,
 	if (static_branch_likely(&pkey_disabled))
 		return;
 
-	/*
-	 * TODO: Just set UAMOR to zero if @new_thread hasn't used any keys yet.
-	 */
 	if (old_thread->amr != new_thread->amr)
 		write_amr(new_thread->amr);
 	if (old_thread->iamr != new_thread->iamr)
@@ -305,9 +305,13 @@  void thread_pkey_regs_init(struct thread_struct *thread)
 	if (static_branch_likely(&pkey_disabled))
 		return;
 
-	thread->amr = read_amr() & pkey_amr_uamor_mask;
-	thread->iamr = read_iamr() & pkey_iamr_mask;
-	thread->uamor = read_uamor() & pkey_amr_uamor_mask;
+	thread->amr = pkey_amr_mask;
+	thread->iamr = pkey_iamr_mask;
+ 	thread->uamor = pkey_uamor_mask;
+
+	write_uamor(pkey_uamor_mask);
+	write_amr(pkey_amr_mask);
+	write_iamr(pkey_iamr_mask);
 }
 
 static inline bool pkey_allows_readwrite(int pkey)