From: Florian Weimer <fweimer@redhat.com> To: Ram Pai <linuxram@us.ibm.com> Cc: linux-mm@kvack.org, linux-api@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, Dave Hansen <dave.hansen@intel.com> Subject: Re: pkeys: Reserve PKEY_DISABLE_READ Date: Mon, 12 Nov 2018 13:00:19 +0100 [thread overview] Message-ID: <87efbqqze4.fsf@oldenburg.str.redhat.com> (raw) In-Reply-To: <20181109180947.GF5481@ram.oc3035372033.ibm.com> (Ram Pai's message of "Fri, 9 Nov 2018 10:09:47 -0800") * Ram Pai: > On Thu, Nov 08, 2018 at 09:23:35PM +0100, Florian Weimer wrote: >> * Ram Pai: >> >> > Florian, >> > >> > I can. But I am struggling to understand the requirement. Why is >> > this needed? Are we proposing a enhancement to the sys_pkey_alloc(), >> > to be able to allocate keys that are initialied to disable-read >> > only? >> >> Yes, I think that would be a natural consequence. >> >> However, my immediate need comes from the fact that the AMR register can >> contain a flag combination that is not possible to represent with the >> existing PKEY_DISABLE_WRITE and PKEY_DISABLE_ACCESS flags. User code >> could write to AMR directly, so I cannot rule out that certain flag >> combinations exist there. >> >> So I came up with this: >> >> int >> pkey_get (int key) >> { >> if (key < 0 || key > PKEY_MAX) >> { >> __set_errno (EINVAL); >> return -1; >> } >> unsigned int index = pkey_index (key); >> unsigned long int amr = pkey_read (); >> unsigned int bits = (amr >> index) & 3; >> >> /* Translate from AMR values. PKEY_AMR_READ standing alone is not >> currently representable. */ >> if (bits & PKEY_AMR_READ) > > this should be > if (bits & (PKEY_AMR_READ|PKEY_AMR_WRITE)) This would return zero for PKEY_AMR_READ alone. >> return PKEY_DISABLE_ACCESS; > > >> else if (bits == PKEY_AMR_WRITE) >> return PKEY_DISABLE_WRITE; >> return 0; >> } It's hard to tell whether PKEY_DISABLE_ACCESS is better in this case. Which is why I want PKEY_DISABLE_READ. >> And this is not ideal. I would prefer something like this instead: >> >> switch (bits) >> { >> case PKEY_AMR_READ | PKEY_AMR_WRITE: >> return PKEY_DISABLE_ACCESS; >> case PKEY_AMR_READ: >> return PKEY_DISABLE_READ; >> case PKEY_AMR_WRITE: >> return PKEY_DISABLE_WRITE; >> case 0: >> return 0; >> } > > yes. > and on x86 it will be something like: > switch (bits) > { > case PKEY_PKRU_ACCESS : > return PKEY_DISABLE_ACCESS; > case PKEY_AMR_WRITE: > return PKEY_DISABLE_WRITE; > case 0: > return 0; > } x86 returns the PKRU bits directly, including the nonsensical case (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE). > But for this to work, why do you need to enhance the sys_pkey_alloc() > interface? Not that I am against it. Trying to understand if the > enhancement is really needed. sys_pkey_alloc performs an implicit pkey_set for the newly allocated key (that is, it updates the PKRU/AMR register). It makes sense to match the behavior of the userspace implementation. Thanks, Florian
WARNING: multiple messages have this Message-ID (diff)
From: Florian Weimer <fweimer@redhat.com> To: Ram Pai <linuxram@us.ibm.com> Cc: linux-mm@kvack.org, Dave Hansen <dave.hansen@intel.com>, linuxppc-dev@lists.ozlabs.org, linux-api@vger.kernel.org Subject: Re: pkeys: Reserve PKEY_DISABLE_READ Date: Mon, 12 Nov 2018 13:00:19 +0100 [thread overview] Message-ID: <87efbqqze4.fsf@oldenburg.str.redhat.com> (raw) In-Reply-To: <20181109180947.GF5481@ram.oc3035372033.ibm.com> (Ram Pai's message of "Fri, 9 Nov 2018 10:09:47 -0800") * Ram Pai: > On Thu, Nov 08, 2018 at 09:23:35PM +0100, Florian Weimer wrote: >> * Ram Pai: >> >> > Florian, >> > >> > I can. But I am struggling to understand the requirement. Why is >> > this needed? Are we proposing a enhancement to the sys_pkey_alloc(), >> > to be able to allocate keys that are initialied to disable-read >> > only? >> >> Yes, I think that would be a natural consequence. >> >> However, my immediate need comes from the fact that the AMR register can >> contain a flag combination that is not possible to represent with the >> existing PKEY_DISABLE_WRITE and PKEY_DISABLE_ACCESS flags. User code >> could write to AMR directly, so I cannot rule out that certain flag >> combinations exist there. >> >> So I came up with this: >> >> int >> pkey_get (int key) >> { >> if (key < 0 || key > PKEY_MAX) >> { >> __set_errno (EINVAL); >> return -1; >> } >> unsigned int index = pkey_index (key); >> unsigned long int amr = pkey_read (); >> unsigned int bits = (amr >> index) & 3; >> >> /* Translate from AMR values. PKEY_AMR_READ standing alone is not >> currently representable. */ >> if (bits & PKEY_AMR_READ) > > this should be > if (bits & (PKEY_AMR_READ|PKEY_AMR_WRITE)) This would return zero for PKEY_AMR_READ alone. >> return PKEY_DISABLE_ACCESS; > > >> else if (bits == PKEY_AMR_WRITE) >> return PKEY_DISABLE_WRITE; >> return 0; >> } It's hard to tell whether PKEY_DISABLE_ACCESS is better in this case. Which is why I want PKEY_DISABLE_READ. >> And this is not ideal. I would prefer something like this instead: >> >> switch (bits) >> { >> case PKEY_AMR_READ | PKEY_AMR_WRITE: >> return PKEY_DISABLE_ACCESS; >> case PKEY_AMR_READ: >> return PKEY_DISABLE_READ; >> case PKEY_AMR_WRITE: >> return PKEY_DISABLE_WRITE; >> case 0: >> return 0; >> } > > yes. > and on x86 it will be something like: > switch (bits) > { > case PKEY_PKRU_ACCESS : > return PKEY_DISABLE_ACCESS; > case PKEY_AMR_WRITE: > return PKEY_DISABLE_WRITE; > case 0: > return 0; > } x86 returns the PKRU bits directly, including the nonsensical case (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE). > But for this to work, why do you need to enhance the sys_pkey_alloc() > interface? Not that I am against it. Trying to understand if the > enhancement is really needed. sys_pkey_alloc performs an implicit pkey_set for the newly allocated key (that is, it updates the PKRU/AMR register). It makes sense to match the behavior of the userspace implementation. Thanks, Florian
next prev parent reply other threads:[~2018-11-12 12:00 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-11-08 12:05 pkeys: Reserve PKEY_DISABLE_READ Florian Weimer 2018-11-08 14:57 ` Dave Hansen 2018-11-08 15:01 ` Florian Weimer 2018-11-08 17:14 ` Dave Hansen 2018-11-08 17:37 ` Florian Weimer 2018-11-08 20:12 ` Ram Pai 2018-11-08 20:12 ` Ram Pai 2018-11-08 20:23 ` Florian Weimer 2018-11-08 20:23 ` Florian Weimer 2018-11-09 18:09 ` Ram Pai 2018-11-09 18:09 ` Ram Pai 2018-11-12 12:00 ` Florian Weimer [this message] 2018-11-12 12:00 ` Florian Weimer 2018-11-27 10:23 ` Ram Pai 2018-11-27 10:23 ` Ram Pai 2018-11-27 11:57 ` Florian Weimer 2018-11-27 11:57 ` Florian Weimer 2018-11-27 15:31 ` Dave Hansen 2018-11-27 15:31 ` Dave Hansen 2018-11-29 11:37 ` Florian Weimer 2018-11-29 11:37 ` Florian Weimer 2018-12-03 4:02 ` Ram Pai 2018-12-03 4:02 ` Ram Pai 2018-12-03 15:52 ` Florian Weimer 2018-12-03 15:52 ` Florian Weimer 2018-12-04 6:23 ` Ram Pai 2018-12-04 6:23 ` Ram Pai 2018-12-05 13:00 ` Florian Weimer 2018-12-05 13:00 ` Florian Weimer 2018-12-05 20:23 ` Ram Pai 2018-12-05 20:23 ` Ram Pai 2018-12-05 16:21 ` Andy Lutomirski 2018-12-05 16:21 ` Andy Lutomirski 2018-12-05 20:36 ` Ram Pai 2018-12-05 20:36 ` Ram Pai 2018-11-08 20:08 ` Ram Pai 2018-11-08 20:11 ` Dave Hansen 2018-11-08 20:14 ` Florian Weimer 2018-11-08 19:22 ` Ram Pai 2018-11-08 19:22 ` Ram Pai 2018-11-12 10:29 ` Florian Weimer 2018-11-12 10:29 ` Florian Weimer
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87efbqqze4.fsf@oldenburg.str.redhat.com \ --to=fweimer@redhat.com \ --cc=dave.hansen@intel.com \ --cc=linux-api@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=linuxram@us.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.