| Message ID | 20180604190229.GB10088@ram.oc3035372033.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/04/2018 09:02 PM, Ram Pai wrote: > On Mon, Jun 04, 2018 at 07:57:46PM +0200, Florian Weimer wrote: >> On 06/04/2018 04:01 PM, Ram Pai wrote: >>> On Mon, Jun 04, 2018 at 12:12:07PM +0200, Florian Weimer wrote: >>>> On 06/03/2018 10:18 PM, Ram Pai wrote: >>>>> On Mon, May 21, 2018 at 01:29:11PM +0200, Florian Weimer wrote: >>>>>> On 05/20/2018 09:11 PM, Ram Pai wrote: >>>>>>> Florian, >>>>>>> >>>>>>> Does the following patch fix the problem for you? Just like x86 >>>>>>> I am enabling all keys in the UAMOR register during >>>>>>> initialization itself. Hence any key created by any thread at >>>>>>> any time, will get activated on all threads. So any thread >>>>>>> can change the permission on that key. Smoke tested it >>>>>>> with your test program. >>>>>> >>>>>> I think this goes in the right direction, but the AMR value after >>>>>> fork is still strange: >>>>>> >>>>>> AMR (PID 34912): 0x0000000000000000 >>>>>> AMR after fork (PID 34913): 0x0000000000000000 >>>>>> AMR (PID 34913): 0x0000000000000000 >>>>>> Allocated key in subprocess (PID 34913): 2 >>>>>> Allocated key (PID 34912): 2 >>>>>> Setting AMR: 0xffffffffffffffff >>>>>> New AMR value (PID 34912): 0x0fffffffffffffff >>>>>> About to call execl (PID 34912) ... >>>>>> AMR (PID 34912): 0x0fffffffffffffff >>>>>> AMR after fork (PID 34914): 0x0000000000000003 >>>>>> AMR (PID 34914): 0x0000000000000003 >>>>>> Allocated key in subprocess (PID 34914): 2 >>>>>> Allocated key (PID 34912): 2 >>>>>> Setting AMR: 0xffffffffffffffff >>>>>> New AMR value (PID 34912): 0x0fffffffffffffff >>>>>> >>>>>> I mean this line: >>>>>> >>>>>> AMR after fork (PID 34914): 0x0000000000000003 >>>>>> >>>>>> Shouldn't it be the same as in the parent process? >>>>> >>>>> Fixed it. Please try this patch. If it all works to your satisfaction, I >>>>> will clean it up further and send to Michael Ellermen(ppc maintainer). >>>>> >>>>> >>>>> commit 51f4208ed5baeab1edb9b0f8b68d7144449b3527 >>>>> Author: Ram Pai <linuxram@us.ibm.com> >>>>> Date: Sun Jun 3 14:44:32 2018 -0500 >>>>> >>>>> Fix for the fork bug. >>>>> Signed-off-by: Ram Pai <linuxram@us.ibm.com> >>>> >>>> Is this on top of the previous patch, or a separate fix? >>> >>> top of previous patch. >> >> Thanks. With this patch, I get this on an LPAR: >> >> AMR (PID 1876): 0x0000000000000003 >> AMR after fork (PID 1877): 0x0000000000000003 >> AMR (PID 1877): 0x0000000000000003 >> Allocated key in subprocess (PID 1877): 2 >> Allocated key (PID 1876): 2 >> Setting AMR: 0xffffffffffffffff >> New AMR value (PID 1876): 0x0fffffffffffffff >> About to call execl (PID 1876) ... >> AMR (PID 1876): 0x0000000000000003 >> AMR after fork (PID 1878): 0x0000000000000003 >> AMR (PID 1878): 0x0000000000000003 >> Allocated key in subprocess (PID 1878): 2 >> Allocated key (PID 1876): 2 >> Setting AMR: 0xffffffffffffffff >> New AMR value (PID 1876): 0x0fffffffffffffff >> >> Test program is still this one: >> >> <https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-May/173198.html> >> >> So the process starts out with a different AMR value for some >> reason. That could be a pre-existing bug that was just hidden by the >> reset-to-zero on fork, or it could be intentional. But the kernel > > yes it is a bug, a patch for which is lined up for submission.. > > The fix is > > > commit eaf5b2ac002ad2f5bca118d7ce075ce28311aa8e > Author: Ram Pai <linuxram@us.ibm.com> > Date: Mon Jun 4 10:58:44 2018 -0500 > > powerpc/pkeys: fix total pkeys calculation > > Total number of pkeys calculation is off by 1. Fix it. > > Signed-off-by: Ram Pai <linuxram@us.ibm.com> Looks good to me now. Initial AMR value is zero, as is currently intended. So the remaining question at this point is whether the Intel behavior (default-deny instead of default-allow) is preferable. But if you can get the existing fixes into 4.18 and perhaps the relevant stable kernels, that would already be a great help for my glibc work. Thanks, Florian
> > So the remaining question at this point is whether the Intel > behavior (default-deny instead of default-allow) is preferable. Florian, remind me what behavior needs to fixed?
On 06/08/2018 04:34 AM, Ram Pai wrote: >> >> So the remaining question at this point is whether the Intel >> behavior (default-deny instead of default-allow) is preferable. > > Florian, remind me what behavior needs to fixed? See the other thread. The Intel register equivalent to the AMR by default disallows access to yet-unallocated keys, so that threads which are created before key allocation do not magically gain access to a key allocated by another thread. Thanks, Florian
On Fri, 8 Jun 2018 07:53:51 +0200 Florian Weimer <fweimer@redhat.com> wrote: > On 06/08/2018 04:34 AM, Ram Pai wrote: > >> > >> So the remaining question at this point is whether the Intel > >> behavior (default-deny instead of default-allow) is preferable. > > > > Florian, remind me what behavior needs to fixed? > > See the other thread. The Intel register equivalent to the AMR by > default disallows access to yet-unallocated keys, so that threads > which are created before key allocation do not magically gain access > to a key allocated by another thread. > That does not make any sense. The threads share the address space so they should also share the keys. Or in other words the keys are supposed to be acceleration of mprotect() so if mprotect() magically gives access to threads that did not call it so should pkey functions. If they cannot do that then they fail the primary purpose. And in any case what semantic that makes sense will you ever get by threads not magically getting new keys? Suppose you will spawn some threads, then allocate a new key, associate it with a piece of protected data, etc. Now the old threads do not have access to the protected data at all. So if they want to access it they have to call into a management thread that created the access key to give them the data. Which means they need to call into the kernel to switch to the management thread. Which completely defeats the purpose of the acceleration of mprotect() which was to avoid calling into the kernel to access the data. In other words you can as well spawn a management process and use shared memory. What's worse, if you wanted to opt out of this feature and give the old threads the new key that's going to be quite a bit of fiddling. That said there might be an enhancement that kind of makes sense. For example if you allocate a key to associate with an area that you want to read most of the time and update occasionally it might make sense to tell the kernel: make the read bit on this key process-global, make the write bit on this key thread-local, and do not allow setting the execute bit at all. Then a thread calling a function to update the data will get write access but other threads will continue to have read access unless they also call a function to update the data. Thanks Michal
On 06/08/2018 12:15 PM, Michal Suchánek wrote: > On Fri, 8 Jun 2018 07:53:51 +0200 > Florian Weimer <fweimer@redhat.com> wrote: > >> On 06/08/2018 04:34 AM, Ram Pai wrote: >>>> >>>> So the remaining question at this point is whether the Intel >>>> behavior (default-deny instead of default-allow) is preferable. >>> >>> Florian, remind me what behavior needs to fixed? >> >> See the other thread. The Intel register equivalent to the AMR by >> default disallows access to yet-unallocated keys, so that threads >> which are created before key allocation do not magically gain access >> to a key allocated by another thread. >> > > That does not make any sense. The threads share the address space so > they should also share the keys. > > Or in other words the keys are supposed to be acceleration of > mprotect() so if mprotect() magically gives access to threads that did > not call it so should pkey functions. If they cannot do that then they > fail the primary purpose. That's not how protection keys work. The access rights are thread-specific, so that you can change them locally, without synchronization and expensive inter-node communication. Thanks, Florian
On Fri, 8 Jun 2018 12:44:53 +0200 Florian Weimer <fweimer@redhat.com> wrote: > On 06/08/2018 12:15 PM, Michal Suchánek wrote: > > On Fri, 8 Jun 2018 07:53:51 +0200 > > Florian Weimer <fweimer@redhat.com> wrote: > > > >> On 06/08/2018 04:34 AM, Ram Pai wrote: > >>>> > >>>> So the remaining question at this point is whether the Intel > >>>> behavior (default-deny instead of default-allow) is preferable. > >>> > >>> Florian, remind me what behavior needs to fixed? > >> > >> See the other thread. The Intel register equivalent to the AMR by > >> default disallows access to yet-unallocated keys, so that threads > >> which are created before key allocation do not magically gain > >> access to a key allocated by another thread. > >> > > > > That does not make any sense. The threads share the address space so > > they should also share the keys. > > > > Or in other words the keys are supposed to be acceleration of > > mprotect() so if mprotect() magically gives access to threads that > > did not call it so should pkey functions. If they cannot do that > > then they fail the primary purpose. > > That's not how protection keys work. The access rights are > thread-specific, so that you can change them locally, without > synchronization and expensive inter-node communication. > And the association of a key with part of the address space is thread-local as well? Thanks Michal
On 06/08/2018 02:54 PM, Michal Suchánek wrote: > On Fri, 8 Jun 2018 12:44:53 +0200 > Florian Weimer <fweimer@redhat.com> wrote: > >> On 06/08/2018 12:15 PM, Michal Suchánek wrote: >>> On Fri, 8 Jun 2018 07:53:51 +0200 >>> Florian Weimer <fweimer@redhat.com> wrote: >>> >>>> On 06/08/2018 04:34 AM, Ram Pai wrote: >>>>>> >>>>>> So the remaining question at this point is whether the Intel >>>>>> behavior (default-deny instead of default-allow) is preferable. >>>>> >>>>> Florian, remind me what behavior needs to fixed? >>>> >>>> See the other thread. The Intel register equivalent to the AMR by >>>> default disallows access to yet-unallocated keys, so that threads >>>> which are created before key allocation do not magically gain >>>> access to a key allocated by another thread. >>>> >>> >>> That does not make any sense. The threads share the address space so >>> they should also share the keys. >>> >>> Or in other words the keys are supposed to be acceleration of >>> mprotect() so if mprotect() magically gives access to threads that >>> did not call it so should pkey functions. If they cannot do that >>> then they fail the primary purpose. >> >> That's not how protection keys work. The access rights are >> thread-specific, so that you can change them locally, without >> synchronization and expensive inter-node communication. >> > > And the association of a key with part of the address space is > thread-local as well? No, that part is still per-process. Florian
On Fri, 8 Jun 2018 14:57:06 +0200 Florian Weimer <fweimer@redhat.com> wrote: > On 06/08/2018 02:54 PM, Michal Suchánek wrote: > > On Fri, 8 Jun 2018 12:44:53 +0200 > > Florian Weimer <fweimer@redhat.com> wrote: > > > >> On 06/08/2018 12:15 PM, Michal Suchánek wrote: > >>> On Fri, 8 Jun 2018 07:53:51 +0200 > >>> Florian Weimer <fweimer@redhat.com> wrote: > >>> > >>>> On 06/08/2018 04:34 AM, Ram Pai wrote: > >>>>>> > >>>>>> So the remaining question at this point is whether the Intel > >>>>>> behavior (default-deny instead of default-allow) is > >>>>>> preferable. > >>>>> > >>>>> Florian, remind me what behavior needs to fixed? > >>>> > >>>> See the other thread. The Intel register equivalent to the AMR > >>>> by default disallows access to yet-unallocated keys, so that > >>>> threads which are created before key allocation do not magically > >>>> gain access to a key allocated by another thread. > >>>> > >>> > >>> That does not make any sense. The threads share the address space > >>> so they should also share the keys. > >>> > >>> Or in other words the keys are supposed to be acceleration of > >>> mprotect() so if mprotect() magically gives access to threads that > >>> did not call it so should pkey functions. If they cannot do that > >>> then they fail the primary purpose. > >> > >> That's not how protection keys work. The access rights are > >> thread-specific, so that you can change them locally, without > >> synchronization and expensive inter-node communication. > >> > > > > And the association of a key with part of the address space is > > thread-local as well? > > No, that part is still per-process. So as said above it does not make sense to make keys per-thread. Thanks Michal
On 06/08/2018 03:49 PM, Michal Suchánek wrote: > On Fri, 8 Jun 2018 14:57:06 +0200 > Florian Weimer <fweimer@redhat.com> wrote: > >> On 06/08/2018 02:54 PM, Michal Suchánek wrote: >>> On Fri, 8 Jun 2018 12:44:53 +0200 >>> Florian Weimer <fweimer@redhat.com> wrote: >>> >>>> On 06/08/2018 12:15 PM, Michal Suchánek wrote: >>>>> On Fri, 8 Jun 2018 07:53:51 +0200 >>>>> Florian Weimer <fweimer@redhat.com> wrote: >>>>> >>>>>> On 06/08/2018 04:34 AM, Ram Pai wrote: >>>>>>>> >>>>>>>> So the remaining question at this point is whether the Intel >>>>>>>> behavior (default-deny instead of default-allow) is >>>>>>>> preferable. >>>>>>> >>>>>>> Florian, remind me what behavior needs to fixed? >>>>>> >>>>>> See the other thread. The Intel register equivalent to the AMR >>>>>> by default disallows access to yet-unallocated keys, so that >>>>>> threads which are created before key allocation do not magically >>>>>> gain access to a key allocated by another thread. >>>>>> >>>>> >>>>> That does not make any sense. The threads share the address space >>>>> so they should also share the keys. >>>>> >>>>> Or in other words the keys are supposed to be acceleration of >>>>> mprotect() so if mprotect() magically gives access to threads that >>>>> did not call it so should pkey functions. If they cannot do that >>>>> then they fail the primary purpose. >>>> >>>> That's not how protection keys work. The access rights are >>>> thread-specific, so that you can change them locally, without >>>> synchronization and expensive inter-node communication. >>>> >>> >>> And the association of a key with part of the address space is >>> thread-local as well? >> >> No, that part is still per-process. > > So as said above it does not make sense to make keys per-thread. The keys are still global, but the access rights are per-thread and have to be for reliability reasons. Thanks, Florian
On Fri, 8 Jun 2018 15:51:03 +0200 Florian Weimer <fweimer@redhat.com> wrote: > On 06/08/2018 03:49 PM, Michal Suchánek wrote: > > On Fri, 8 Jun 2018 14:57:06 +0200 > > Florian Weimer <fweimer@redhat.com> wrote: > > > >> On 06/08/2018 02:54 PM, Michal Suchánek wrote: > >>> On Fri, 8 Jun 2018 12:44:53 +0200 > >>> Florian Weimer <fweimer@redhat.com> wrote: > >>> > >>>> On 06/08/2018 12:15 PM, Michal Suchánek wrote: > >>>>> On Fri, 8 Jun 2018 07:53:51 +0200 > >>>>> Florian Weimer <fweimer@redhat.com> wrote: > >>>>> > >>>>>> On 06/08/2018 04:34 AM, Ram Pai wrote: > >>>>>>>> > >>>>>>>> So the remaining question at this point is whether the Intel > >>>>>>>> behavior (default-deny instead of default-allow) is > >>>>>>>> preferable. > >>>>>>> > >>>>>>> Florian, remind me what behavior needs to fixed? > >>>>>> > >>>>>> See the other thread. The Intel register equivalent to the AMR > >>>>>> by default disallows access to yet-unallocated keys, so that > >>>>>> threads which are created before key allocation do not > >>>>>> magically gain access to a key allocated by another thread. > >>>>>> > >>>>> > >>>>> That does not make any sense. The threads share the address > >>>>> space so they should also share the keys. > >>>>> > >>>>> Or in other words the keys are supposed to be acceleration of > >>>>> mprotect() so if mprotect() magically gives access to threads > >>>>> that did not call it so should pkey functions. If they cannot > >>>>> do that then they fail the primary purpose. > >>>> > >>>> That's not how protection keys work. The access rights are > >>>> thread-specific, so that you can change them locally, without > >>>> synchronization and expensive inter-node communication. > >>>> > >>> > >>> And the association of a key with part of the address space is > >>> thread-local as well? > >> > >> No, that part is still per-process. > > > > So as said above it does not make sense to make keys per-thread. > > The keys are still global, but the access rights are per-thread and > have to be for reliability reasons. > Oh, right. The association of keys to memory is independent of key allocation. However, to change the key permissions or the memory association to a key you need to allocate it. And key allocation is propagated lazily between threads so you do not have to stop the world to allocate a key. So if default key permissions of an unallocated key allow access then allocating a key and associating it with memory makes that memory accessible to threads that are not yet aware of the fact the key has been allocated which is not desirable. Sounds sensible. Thanks Michal
On Fri, Jun 08, 2018 at 07:53:51AM +0200, Florian Weimer wrote: > On 06/08/2018 04:34 AM, Ram Pai wrote: > >> > >>So the remaining question at this point is whether the Intel > >>behavior (default-deny instead of default-allow) is preferable. > > > >Florian, remind me what behavior needs to fixed? > > See the other thread. The Intel register equivalent to the AMR by > default disallows access to yet-unallocated keys, so that threads > which are created before key allocation do not magically gain access > to a key allocated by another thread. Are you referring to the thread '[PATCH] pkeys: Introduce PKEY_ALLOC_SIGNALINHERIT and change signal semantics' If yes, I will wait for your next version of the patch. Otherwise please point me to the URL of that thread. Sorry and thankx. :) RP
On 06/11/2018 07:23 PM, Ram Pai wrote: > On Fri, Jun 08, 2018 at 07:53:51AM +0200, Florian Weimer wrote: >> On 06/08/2018 04:34 AM, Ram Pai wrote: >>>> >>>> So the remaining question at this point is whether the Intel >>>> behavior (default-deny instead of default-allow) is preferable. >>> >>> Florian, remind me what behavior needs to fixed? >> >> See the other thread. The Intel register equivalent to the AMR by >> default disallows access to yet-unallocated keys, so that threads >> which are created before key allocation do not magically gain access >> to a key allocated by another thread. > > Are you referring to the thread > '[PATCH] pkeys: Introduce PKEY_ALLOC_SIGNALINHERIT and change signal semantics' > Otherwise please point me to the URL of that thread. Sorry and thankx. :) No, it's this issue: <https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-May/173157.html> The UAMOR part has been fixed (thanks), but I think processes still start out with default-allow AMR. Thanks, Florian
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c index 4530cdf..3384c4e 100644 --- a/arch/powerpc/mm/pkeys.c +++ b/arch/powerpc/mm/pkeys.c @@ -93,7 +93,7 @@ int pkey_initialize(void) * arch-neutral code. */ pkeys_total = min_t(int, pkeys_total, - (ARCH_VM_PKEY_FLAGS >> VM_PKEY_SHIFT)); + ((ARCH_VM_PKEY_FLAGS >> VM_PKEY_SHIFT)+1)); if (!pkey_mmu_enabled() || radix_enabled() || !pkeys_total) static_branch_enable(&pkey_disabled);