Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1438570 > unrolled thread

[PATCH 6/9] x86, pkeys: add pkey set/get syscalls

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Dave Hansen <dave@sr71.net> - 2016-07-07 14:50 +0200
    Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Mel Gorman <mgorman@techsingularity.net> - 2016-07-07 16:50 +0200
      Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Dave Hansen <dave@sr71.net> - 2016-07-07 19:40 +0200
        Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Ingo Molnar <mingo@kernel.org> - 2016-07-08 09:20 +0200
          Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Dave Hansen <dave@sr71.net> - 2016-07-08 18:40 +0200
            Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Ingo Molnar <mingo@kernel.org> - 2016-07-09 10:40 +0200
          Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Dave Hansen <dave@sr71.net> - 2016-07-08 21:30 +0200
        Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Mel Gorman <mgorman@techsingularity.net> - 2016-07-08 12:30 +0200

#1438570 — [PATCH 6/9] x86, pkeys: add pkey set/get syscalls

From: Dave Hansen <dave.hansen@linux.intel.com>

This establishes two more system calls for protection key management:

	unsigned long pkey_get(int pkey);
	int pkey_set(int pkey, unsigned long access_rights);

The return value from pkey_get() and the 'access_rights' passed
to pkey_set() are the same format: a bitmask containing
PKEY_DENY_WRITE and/or PKEY_DENY_ACCESS, or nothing set at all.

These can replace userspace's direct use of the new rdpkru/wrpkru
instructions.

With current hardware, the kernel can not enforce that it has
control over a given key.  But, this at least allows the kernel
to indicate to userspace that userspace does not control a given
protection key.  This makes it more likely that situations like
using a pkey after sys_pkey_free() can be detected.

The kernel does _not_ enforce that this interface must be used for
changes to PKRU, whether or not a key has been "allocated".

This syscall interface could also theoretically be replaced with a
pair of vsyscalls.  The vsyscalls would just call WRPKRU/RDPKRU
directly in situations where they are drop-in equivalents for
what the kernel would be doing.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: linux-api@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: x86@kernel.org
Cc: torvalds@linux-foundation.org
Cc: akpm@linux-foundation.org
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: mgorman@techsingularity.net
Cc: hughd@google.com
Cc: viro@zeniv.linux.org.uk
---

 b/arch/x86/entry/syscalls/syscall_32.tbl |    2 +
 b/arch/x86/entry/syscalls/syscall_64.tbl |    2 +
 b/arch/x86/include/asm/pkeys.h           |    4 +-
 b/arch/x86/kernel/fpu/xstate.c           |   55 +++++++++++++++++++++++++++++--
 b/include/linux/pkeys.h                  |    8 ++++
 b/mm/mprotect.c                          |   41 +++++++++++++++++++++++
 6 files changed, 109 insertions(+), 3 deletions(-)

diff -puN arch/x86/entry/syscalls/syscall_32.tbl~pkeys-118-syscalls-set-get arch/x86/entry/syscalls/syscall_32.tbl
--- a/arch/x86/entry/syscalls/syscall_32.tbl~pkeys-118-syscalls-set-get	2016-07-07 05:47:02.197865624 -0700
+++ b/arch/x86/entry/syscalls/syscall_32.tbl	2016-07-07 05:47:02.209866169 -0700
@@ -389,3 +389,5 @@
 380	i386	pkey_mprotect		sys_pkey_mprotect
 381	i386	pkey_alloc		sys_pkey_alloc
 382	i386	pkey_free		sys_pkey_free
+383	i386	pkey_get		sys_pkey_get
+384	i386	pkey_set		sys_pkey_set
diff -puN arch/x86/entry/syscalls/syscall_64.tbl~pkeys-118-syscalls-set-get arch/x86/entry/syscalls/syscall_64.tbl
--- a/arch/x86/entry/syscalls/syscall_64.tbl~pkeys-118-syscalls-set-get	2016-07-07 05:47:02.199865715 -0700
+++ b/arch/x86/entry/syscalls/syscall_64.tbl	2016-07-07 05:47:02.211866259 -0700
@@ -338,6 +338,8 @@
 329	common	pkey_mprotect		sys_pkey_mprotect
 330	common	pkey_alloc		sys_pkey_alloc
 331	common	pkey_free		sys_pkey_free
+332	common	pkey_get		sys_pkey_get
+333	common	pkey_set		sys_pkey_set
 
 #
 # x32-specific system call numbers start at 512 to avoid cache impact
diff -puN arch/x86/include/asm/pkeys.h~pkeys-118-syscalls-set-get arch/x86/include/asm/pkeys.h
--- a/arch/x86/include/asm/pkeys.h~pkeys-118-syscalls-set-get	2016-07-07 05:47:02.201865805 -0700
+++ b/arch/x86/include/asm/pkeys.h	2016-07-07 05:47:02.211866259 -0700
@@ -56,7 +56,7 @@ static inline bool validate_pkey(int pke
 }
 
 static inline
-bool mm_pkey_is_allocated(struct mm_struct *mm, unsigned long pkey)
+bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
 {
 	if (!validate_pkey(pkey))
 		return true;
@@ -107,4 +107,6 @@ extern int arch_set_user_pkey_access(str
 extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 		unsigned long init_val);
 
+extern unsigned long arch_get_user_pkey_access(struct task_struct *tsk,
+		int pkey);
 #endif /*_ASM_X86_PKEYS_H */
diff -puN arch/x86/kernel/fpu/xstate.c~pkeys-118-syscalls-set-get arch/x86/kernel/fpu/xstate.c
--- a/arch/x86/kernel/fpu/xstate.c~pkeys-118-syscalls-set-get	2016-07-07 05:47:02.203865896 -0700
+++ b/arch/x86/kernel/fpu/xstate.c	2016-07-07 05:47:02.211866259 -0700
@@ -708,7 +708,7 @@ void fpu__resume_cpu(void)
  *
  * Note: does not work for compacted buffers.
  */
-void *__raw_xsave_addr(struct xregs_state *xsave, int xstate_feature_mask)
+static void *__raw_xsave_addr(struct xregs_state *xsave, int xstate_feature_mask)
 {
 	int feature_nr = fls64(xstate_feature_mask) - 1;
 
@@ -882,6 +882,7 @@ out:
 
 #define NR_VALID_PKRU_BITS (CONFIG_NR_PROTECTION_KEYS * 2)
 #define PKRU_VALID_MASK (NR_VALID_PKRU_BITS - 1)
+#define PKRU_INIT_STATE	0
 
 /*
  * This will go out and modify the XSAVE buffer so that PKRU is
@@ -900,6 +901,9 @@ int __arch_set_user_pkey_access(struct t
 	int pkey_shift = (pkey * PKRU_BITS_PER_PKEY);
 	u32 new_pkru_bits = 0;
 
+	/* Only support manipulating current task for now */
+	if (tsk != current)
+		return -EINVAL;
 	/*
 	 * This check implies XSAVE support.  OSPKE only gets
 	 * set if we enable XSAVE and we enable PKU in XCR0.
@@ -925,7 +929,7 @@ int __arch_set_user_pkey_access(struct t
 	 * state.
 	 */
 	if (!old_pkru_state)
-		new_pkru_state.pkru = 0;
+		new_pkru_state.pkru = PKRU_INIT_STATE;
 	else
 		new_pkru_state.pkru = old_pkru_state->pkru;
 
@@ -963,4 +967,51 @@ int arch_set_user_pkey_access(struct tas
 		return -EINVAL;
 	return __arch_set_user_pkey_access(tsk, pkey, init_val);
 }
+
+/*
+ * Figures out what the rights are currently for 'pkey'.
+ * Converts from PKRU's format to the user-visible PKEY_DISABLE_*
+ * format.
+ */
+unsigned long arch_get_user_pkey_access(struct task_struct *tsk, int pkey)
+{
+	struct fpu *fpu = &current->thread.fpu;
+	u32 pkru_reg;
+	int ret = 0;
+
+	/* Only support manipulating current task for now */
+	if (tsk != current)
+		return -1;
+	if (!cpu_feature_enabled(X86_FEATURE_OSPKE))
+		return -1;
+	/*
+	 * The contents of PKRU itself are invalid.  Consult the
+	 * task's XSAVE buffer for PKRU contents.  This is much
+	 * more expensive than reading PKRU directly, but should
+	 * be rare or impossible with eagerfpu mode.
+	 */
+	if (!fpu->fpregs_active) {
+		struct xregs_state *xsave = &fpu->state.xsave;
+		struct pkru_state *pkru_state =
+			get_xsave_addr(xsave, XFEATURE_MASK_PKRU);
+		/*
+		 * PKRU is in its init state and not present in
+		 * the buffer in a saved form.
+		 */
+		if (!pkru_state)
+			return PKRU_INIT_STATE;
+
+		return pkru_state->pkru;
+	}
+	/*
+	 * Consult the user register directly.
+	 */
+	pkru_reg = read_pkru();
+	if (!__pkru_allows_read(pkru_reg, pkey))
+		ret |= PKEY_DISABLE_ACCESS;
+	if (!__pkru_allows_write(pkru_reg, pkey))
+		ret |= PKEY_DISABLE_WRITE;
+
+	return ret;
+}
 #endif /* CONFIG_ARCH_HAS_PKEYS */
diff -puN include/linux/pkeys.h~pkeys-118-syscalls-set-get include/linux/pkeys.h
--- a/include/linux/pkeys.h~pkeys-118-syscalls-set-get	2016-07-07 05:47:02.204865942 -0700
+++ b/include/linux/pkeys.h	2016-07-07 05:47:02.212866305 -0700
@@ -44,6 +44,14 @@ static inline int mm_pkey_free(struct mm
 static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 			unsigned long init_val)
 {
+	return -EINVAL;
+}
+
+static inline
+unsigned long arch_get_user_pkey_access(struct task_struct *tsk, int pkey)
+{
+	if (pkey)
+		return -1;
 	return 0;
 }
 
diff -puN mm/mprotect.c~pkeys-118-syscalls-set-get mm/mprotect.c
--- a/mm/mprotect.c~pkeys-118-syscalls-set-get	2016-07-07 05:47:02.206866032 -0700
+++ b/mm/mprotect.c	2016-07-07 05:47:02.212866305 -0700
@@ -537,3 +537,44 @@ SYSCALL_DEFINE1(pkey_free, int, pkey)
 	 */
 	return ret;
 }
+
+SYSCALL_DEFINE2(pkey_get, int, pkey, unsigned long, flags)
+{
+	unsigned long ret = 0;
+
+	if (flags)
+		return -EINVAL;
+
+	down_write(&current->mm->mmap_sem);
+	if (!mm_pkey_is_allocated(current->mm, pkey))
+		ret = -EBADF;
+	up_write(&current->mm->mmap_sem);
+
+	if (ret)
+		return ret;
+
+	ret = arch_get_user_pkey_access(current, pkey);
+
+	return ret;
+}
+
+SYSCALL_DEFINE3(pkey_set, int, pkey, unsigned long, access_rights,
+		unsigned long, flags)
+{
+	unsigned long ret = 0;
+
+	if (flags)
+		return -EINVAL;
+
+	down_write(&current->mm->mmap_sem);
+	if (!mm_pkey_is_allocated(current->mm, pkey))
+		ret = -EBADF;
+	up_write(&current->mm->mmap_sem);
+
+	if (ret)
+		return ret;
+
+	ret = arch_set_user_pkey_access(current, pkey, access_rights);
+
+	return ret;
+}
_

[toc] | [next] | [standalone]

#1438659

On Thu, Jul 07, 2016 at 05:47:28AM -0700, Dave Hansen wrote:
> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> This establishes two more system calls for protection key management:
> 
> 	unsigned long pkey_get(int pkey);
> 	int pkey_set(int pkey, unsigned long access_rights);
> 
> The return value from pkey_get() and the 'access_rights' passed
> to pkey_set() are the same format: a bitmask containing
> PKEY_DENY_WRITE and/or PKEY_DENY_ACCESS, or nothing set at all.
> 
> These can replace userspace's direct use of the new rdpkru/wrpkru
> instructions.
> 
> With current hardware, the kernel can not enforce that it has
> control over a given key.  But, this at least allows the kernel
> to indicate to userspace that userspace does not control a given
> protection key.  This makes it more likely that situations like
> using a pkey after sys_pkey_free() can be detected.
> 
> The kernel does _not_ enforce that this interface must be used for
> changes to PKRU, whether or not a key has been "allocated".
> 
> This syscall interface could also theoretically be replaced with a
> pair of vsyscalls.  The vsyscalls would just call WRPKRU/RDPKRU
> directly in situations where they are drop-in equivalents for
> what the kernel would be doing.
> 

This one feels like something that can or should be implemented in
glibc.

There is no real enforcement of the values yet looking them up or
setting them takes mmap_sem for write. Applications that frequently get
called will get hammed into the ground with serialisation on mmap_sem
not to mention the cost of the syscall entry/exit.

RIght now, I'm seeing a lot of cost and not much benefit with this
specific patch.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1438772

On 07/07/2016 07:45 AM, Mel Gorman wrote:
> On Thu, Jul 07, 2016 at 05:47:28AM -0700, Dave Hansen wrote:
>> > 
>> > From: Dave Hansen <dave.hansen@linux.intel.com>
>> > 
>> > This establishes two more system calls for protection key management:
>> > 
>> > 	unsigned long pkey_get(int pkey);
>> > 	int pkey_set(int pkey, unsigned long access_rights);
>> > 
>> > The return value from pkey_get() and the 'access_rights' passed
>> > to pkey_set() are the same format: a bitmask containing
>> > PKEY_DENY_WRITE and/or PKEY_DENY_ACCESS, or nothing set at all.
>> > 
>> > These can replace userspace's direct use of the new rdpkru/wrpkru
>> > instructions.
...
> This one feels like something that can or should be implemented in
> glibc.

I generally agree, except that glibc doesn't have any visibility into
whether a pkey is currently valid or not.

> There is no real enforcement of the values yet looking them up or
> setting them takes mmap_sem for write.

There are checks for mm_pkey_is_allocated().  That's the main thing
these syscalls add on top of the raw instructions.

> Applications that frequently get
> called will get hammed into the ground with serialisation on mmap_sem
> not to mention the cost of the syscall entry/exit.

I think we can do both of them without mmap_sem, as long as we resign
ourselves to this just being fundamentally racy (which it is already, I
think).  But, is it worth performance-tuning things that we don't expect
performance-sensitive apps to be using in the first place?  They'll just
use the RDPKRU/WRPKRU instructions directly.

Ingo, do you still feel strongly that these syscalls (pkey_set/get())
should be included?  Of the 5, they're definitely the two with the
weakest justification.

[toc] | [prev] | [next] | [standalone]

#1439145

* Dave Hansen <dave@sr71.net> wrote:

> > Applications that frequently get called will get hammed into the ground with 
> > serialisation on mmap_sem not to mention the cost of the syscall entry/exit.
>
> I think we can do both of them without mmap_sem, as long as we resign ourselves 
> to this just being fundamentally racy (which it is already, I think).  But, is 
> it worth performance-tuning things that we don't expect performance-sensitive 
> apps to be using in the first place?  They'll just use the RDPKRU/WRPKRU 
> instructions directly.
>
> Ingo, do you still feel strongly that these syscalls (pkey_set/get()) should be 
> included?  Of the 5, they're definitely the two with the weakest justification.

Firstly, I'd like to thank Mel for the review, having this kind of high level 
interface discussion was exactly what I was hoping for before we merged any ABI 
patches.

So my hope was that we'd also grow some debugging features: such as a periodic 
watchdog timer clearing all non-allocated pkeys of a task and re-setting them to 
their (kernel-)known values and thus forcing user-space to coordinate key 
allocation/freeing.

While allocation/freeing of keys is very likely a slow path in any reasonable 
workload, _setting_ the values of pkeys could easily be a fast path. The whole 
point of pkeys is to allow both thread local and dynamic mapping and unmapping of 
memory ranges, without having to touch any page table attributes in the hot path.

Now another, more fundamental question is that pkeys are per-CPU (per thread) on 
the hardware side - so why do we even care about the mmap_sem in the syscalls in 
the first place? If we want any serialization wouldn't a pair of 
get_cpu()/put_cpu() preemption control calls be enough? Those would also be much 
cheaper.

The idea behind my suggestion to manage all pkey details via a kernel interface 
and 'shadow' the pkeys state in the kernel was that if we don't even offer a 
complete system call interface then user-space is _forced_ into using the raw 
instructions and into implementing a (potentially crappy) uncoordinated API or not 
implementing any API at all but randomly using fixed pkey indices.

My hope was to avoid that, especially since currently _all_ memory mapping details 
on x86 are controlled via system calls. If we don't offer that kind of facility 
then we force user-space into using the raw instructions and will likely end up 
with a poor user-space interface.

So the question is, what is user-space going to do? Do any glibc patches exist? 
How are the user-space library side APIs going to look like?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1439653

On 07/08/2016 12:18 AM, Ingo Molnar wrote:
> So the question is, what is user-space going to do? Do any glibc
> patches exist? How are the user-space library side APIs going to look
> like?

My goal at the moment is to get folks enabled to the point that they can
start modifying apps to use pkeys without having to patch their kernels.
 I don't have confidence that we can design good high-level userspace
interfaces without seeing some real apps try to use the low-level ones
and seeing how they struggle.

I had some glibc code to do the pkey alloc/free operations, but those
aren't necessary if we're doing it in the kernel.  Other than getting
the syscall wrappers in place, I don't have any immediate plans to do
anything in glibc.

Was there something you were expecting to see?

[toc] | [prev] | [next] | [standalone]

#1439915

* Dave Hansen <dave@sr71.net> wrote:

> On 07/08/2016 12:18 AM, Ingo Molnar wrote:
>
> > So the question is, what is user-space going to do? Do any glibc patches 
> > exist? How are the user-space library side APIs going to look like?
> 
> My goal at the moment is to get folks enabled to the point that they can start 
> modifying apps to use pkeys without having to patch their kernels.
>  I don't have confidence that we can design good high-level userspace interfaces 
> without seeing some real apps try to use the low-level ones and seeing how they 
> struggle.
> 
> I had some glibc code to do the pkey alloc/free operations, but those aren't 
> necessary if we're doing it in the kernel.  Other than getting the syscall 
> wrappers in place, I don't have any immediate plans to do anything in glibc.
> 
> Was there something you were expecting to see?

Yeah, so (as you probably guessed!) I'm starting to have second thoughts about the 
complexity of the alloc/free/set/get interface I suggested, and Mel's review 
certainly strengthened that feeling.

I have two worries:

1)

A technical worry I have is that the 'pkey allocation interface' does not seem to 
be taking the per thread property of pkeys into account - while that property 
would be useful for apps. That is a limitation that seems unjustified.

The reason for this is that we are storing the key allocation bitmap in struct_mm, 
in mm->context.pkey_allocation_map - while we should be storing it in task_struct 
or thread_info.

We could solve this by moving the allocation bitmap to the task struct, but:

2)

My main worry is that it appears at this stage that we are still pretty far away 
from completely shadowing the hardware pkey state in the kernel - and without that 
we cannot really force user-space to use the 'proper' APIs. They can just use the 
raw instructions, condition them on a CPUID and be done with it: everything can be 
organized in user-space.

Furthermore, implementing it in a high performance fashion would be pretty complex 
- at minimum we'd have to register a per thread read-write user-space data area 
where the kernel could store pkeys management data so that vsyscalls can access it 
... None of that facility exists today.

And without vsyscall optimizations user-space might legitimately use its own 
implementation for performance reasons and we'd end up with twice the complexity 
and a largely unused piece of kernel infrastructure ...

So how about the following minimalistic approach instead, to get the ball rolling 
without making ABI decisions we might regret:

 - There are 16 pkey indices on x86 currently. We already use index 15 for the 
   true PROT_EXEC implementation. Let's set aside another pkey index for the 
   kernel's potential future use (index 14), and clear it explicitly in the 
   FPU context on every context switch if CONFIG_X86_DEBUG_FPU is enabled to make 
   sure it remains unallocated.

 - Expose just the new mprotect_pkey() system call to install a pkey index into 
   the page tables - but we let user-space organize its key allocations.

 - Give user-space an idea about limits:

     "ALL THESE WORLDS ARE YOURS—EXCEPT EUROPA ATTEMPT NO LANDING THERE"

   Ooops, wrong one. Lets try this instead:

     Expose the current maximum user-space usable pkeys index in some
     programmatically accessible fashion. Maybe mprotect_pkey() could reject a 
     permanently allocated kernel pkey index via a distinctive error code?

   I.e. this pattern:

     ret = pkey_mprotect(NULL, PAGE_SIZE, real_prot, pkey);

   ... would validate the pkey and we'd return -EOPNOTSUPP for pkey that is not 
   available? This would allow maximum future flexibility as it would not define 
   kernel allocated pkeys as a 'range'.

 - ... and otherwise leave the remaining 14 pkey indices for user-space to manage.

If in the future user-space pkeys usage grows to such a level that kernel 
arbitration becomes desirable then we can still implement the get/set/alloc/free 
system calls as well: the first use of those system calls would switch on the 
kernel's pkey management facilities and from that point on user-space is supposed 
to use the published system calls only. Applications using pkey instructions 
directly would still work just fine: they'd never use the new system calls.

I.e. we can actually keep a bigger ABI flexibility by introducing the simplest 
possible ABI at this stage. Maybe user-space usage of this hardware feature will 
never grow beyond that simple ABI - in which case we've saved quite a bit of 
ongoing maintenance complexity...

And yes, I realize that we've come a full round since the very first version of 
this patch set, but I think the extra hoops were still worth it, because the 
true-PROT_EXEC feature came out of it which is very useful IMHO. But my more 
complex pkey management syscall ideas don't seem to be all that useful anymore.

So what do you think about this direction? This would simplify the patch set quite 
a bit and would touch very little MM code beyond the mprotect_pkey() bits.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1439758

On 07/08/2016 12:18 AM, Ingo Molnar wrote:
> So my hope was that we'd also grow some debugging features: such as a periodic 
> watchdog timer clearing all non-allocated pkeys of a task and re-setting them to 
> their (kernel-)known values and thus forcing user-space to coordinate key 
> allocation/freeing.

I'm glad you mentioned this.  I've explicitly called out this behavior
in the manpages now, or at least called out the fact that the kernel
will not preserve PKRU contents for unallocated keys.

[toc] | [prev] | [next] | [standalone]

#1439279

On Thu, Jul 07, 2016 at 10:33:00AM -0700, Dave Hansen wrote:
> On 07/07/2016 07:45 AM, Mel Gorman wrote:
> > On Thu, Jul 07, 2016 at 05:47:28AM -0700, Dave Hansen wrote:
> >> > 
> >> > From: Dave Hansen <dave.hansen@linux.intel.com>
> >> > 
> >> > This establishes two more system calls for protection key management:
> >> > 
> >> > 	unsigned long pkey_get(int pkey);
> >> > 	int pkey_set(int pkey, unsigned long access_rights);
> >> > 
> >> > The return value from pkey_get() and the 'access_rights' passed
> >> > to pkey_set() are the same format: a bitmask containing
> >> > PKEY_DENY_WRITE and/or PKEY_DENY_ACCESS, or nothing set at all.
> >> > 
> >> > These can replace userspace's direct use of the new rdpkru/wrpkru
> >> > instructions.
> ...
> > This one feels like something that can or should be implemented in
> > glibc.
> 
> I generally agree, except that glibc doesn't have any visibility into
> whether a pkey is currently valid or not.
> 

Well, it could if it tracked the pkey_alloc/pkey_free calls too. I accept
that's not perfect as nothing prevents the syscalls being used directly.

> > Applications that frequently get
> > called will get hammed into the ground with serialisation on mmap_sem
> > not to mention the cost of the syscall entry/exit.
> 
> I think we can do both of them without mmap_sem, as long as we resign
> ourselves to this just being fundamentally racy (which it is already, I
> think).  But, is it worth performance-tuning things that we don't expect
> performance-sensitive apps to be using in the first place?  They'll just
> use the RDPKRU/WRPKRU instructions directly.
> 

I accept the premature optimisation arguement but I think it'll eventually
bite us. Why this red-flagged for me was because so many people have
complained about just system call overhead when using particular types of
hardware -- DAX springs to mind with the MAP_PMEM_AWARE discussions. Using
mmap_sem means that pkey operations stop parallel faults, mmaps and so on. If
the applications that care are trying to minimise page table operations,
TLB flushes and so on, they might not be that happy if parallel faults
are stalled.

I think whether you serialise pkey_get/pkey_set operations or not, it's
going to be inherently racy with different sized windows. A sequence counter
would be sufficient to protect it to prevent partial reads. If userspace
cares about the race, then userspace is going to have to serialise its
threads access to the keys anyway.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web