| Message ID | 1528936144-6696-2-git-send-email-linuxram@us.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/pkeys: fixes to pkeys | expand |
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
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
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>
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
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)
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(-)