pkeys: Support setting access rights for signal handlers

pkeys: Support setting access rights for signal handlers

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 2446 bytes --]

The attached patch addresses a problem with the current x86 pkey 
implementation, which makes default-readable pkeys unusable from signal 
handlers because the default init_pkru value blocks access.

With this patch, the following program:

#include <sys/syscall.h>
#include <unistd.h>
#include <stdio.h>
#include <err.h>
#include <signal.h>

#define PKEY_ALLOC_SETSIGNAL 1

#define PKEY_DISABLE_WRITE 2

static inline unsigned int
pkey_read (void)
{
   unsigned int result;
   __asm__ volatile (".byte 0x0f, 0x01, 0xee"
                     : "=a" (result) : "c" (0) : "rdx");
   return result;
}

static void
print_pkru (const char *where)
{
   printf ("PKRU (%s): %08x\n", where, pkey_read ());
}

static void
sigusr1 (int signo)
{
   print_pkru ("signal handler");
}

int
main (void)
{
   if (signal (SIGUSR1, sigusr1) == SIG_ERR)
     err (1, "signal");
   print_pkru ("main");
   raise (SIGUSR1);

   puts ("allocating key 1");
   int key1 = syscall (SYS_pkey_alloc, 0, 0);
   if (key1 < 0)
     err (1, "pkey_alloc");
   print_pkru ("main");
   raise (SIGUSR1);

   puts ("allocating key 2");
   int key2 = syscall (SYS_pkey_alloc, PKEY_ALLOC_SETSIGNAL, 0);
   if (key2 < 0)
     err (1, "pkey_alloc");
   print_pkru ("main");
   raise (SIGUSR1);

   puts ("allocating key 3");
   int key3 = syscall (SYS_pkey_alloc, PKEY_ALLOC_SETSIGNAL, 
PKEY_DISABLE_WRITE);
   if (key3 < 0)
     err (1, "pkey_alloc");
   print_pkru ("main");
   raise (SIGUSR1);

   puts ("freeing key 3");
   if (syscall (SYS_pkey_free, key3) < 0)
     err (1, "pkey_free");
   print_pkru ("main");
   raise (SIGUSR1);

   puts ("freeing key 2");
   if (syscall (SYS_pkey_free, key2) < 0)
     err (1, "pkey_free");
   print_pkru ("main");
   raise (SIGUSR1);

   return 0;
}

prints this:

PKRU (main): 55555554
PKRU (signal handler): 55555554
allocating key 1
PKRU (main): 55555550
PKRU (signal handler): 55555554
allocating key 2
PKRU (main): 55555540
PKRU (signal handler): 55555544
allocating key 3
PKRU (main): 55555580
PKRU (signal handler): 55555584
freeing key 3
PKRU (main): 55555580
PKRU (signal handler): 55555544
freeing key 2
PKRU (main): 55555580
PKRU (signal handler): 55555554

Something like this is required before we can use memory protection keys 
in glibc for mostly-read-only data structures which need to be 
accessible from signal handlers.

I'm not sure if I got the locking for mm->context right.  Please check 
carefully.

Thanks,
Florian

[-- Attachment #2: pkeys-setsignal.patch --]
[-- Type: text/x-patch, Size: 13066 bytes --]

commit 21ff3eaed8565e9b130380abf084e761f20e6fea
Author: Florian Weimer <fweimer@redhat.com>
Date:   Sat Dec 9 22:03:26 2017 +0100

    pkeys: Support setting access rights for signal handlers
    
    The new pkey_alloc flag PKEY_ALLOC_SETSIGNAL requests that the
    kernel uses the specified access rights as the default when the
    a signal handler is entered, instead of the access rights
    specified by init_pkru.
    
    This leads to a divergence in the FPU initialization for signal
    handlers and for exeve, so a for_signal flag is added to fpu__clear.
    
    Signed-off-by: Florian Weimer <fweimer@redhat.com>

diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
index 2dbdf59258d9..343251fed6fc 100644
--- a/arch/alpha/include/uapi/asm/mman.h
+++ b/arch/alpha/include/uapi/asm/mman.h
@@ -72,6 +72,8 @@
 /* compatibility flags */
 #define MAP_FILE	0
 
+#define PKEY_ALLOC_SETSIGNAL 0x1
+
 #define PKEY_DISABLE_ACCESS	0x1
 #define PKEY_DISABLE_WRITE	0x2
 #define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\
diff --git a/arch/mips/include/uapi/asm/mman.h b/arch/mips/include/uapi/asm/mman.h
index 606e02ca4b6c..be1677e31899 100644
--- a/arch/mips/include/uapi/asm/mman.h
+++ b/arch/mips/include/uapi/asm/mman.h
@@ -99,6 +99,8 @@
 /* compatibility flags */
 #define MAP_FILE	0
 
+#define PKEY_ALLOC_SETSIGNAL 0x1
+
 #define PKEY_DISABLE_ACCESS	0x1
 #define PKEY_DISABLE_WRITE	0x2
 #define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\
diff --git a/arch/parisc/include/uapi/asm/mman.h b/arch/parisc/include/uapi/asm/mman.h
index 80510ba44c08..2fc9d1bf98eb 100644
--- a/arch/parisc/include/uapi/asm/mman.h
+++ b/arch/parisc/include/uapi/asm/mman.h
@@ -69,6 +69,8 @@
 #define MAP_FILE	0
 #define MAP_VARIABLE	0
 
+#define PKEY_ALLOC_SETSIGNAL 0x1
+
 #define PKEY_DISABLE_ACCESS	0x1
 #define PKEY_DISABLE_WRITE	0x2
 #define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\
diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index a38bf5a1e37a..5a7a72dd589c 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -32,7 +32,7 @@ extern void fpu__restore(struct fpu *fpu);
 extern int  fpu__restore_sig(void __user *buf, int ia32_frame);
 extern void fpu__drop(struct fpu *fpu);
 extern int  fpu__copy(struct fpu *dst_fpu, struct fpu *src_fpu);
-extern void fpu__clear(struct fpu *fpu);
+extern void fpu__clear(struct fpu *fpu, bool for_signal);
 extern int  fpu__exception_code(struct fpu *fpu, int trap_nr);
 extern int  dump_fpu(struct pt_regs *ptregs, struct user_i387_struct *fpstate);
 
diff --git a/arch/x86/include/asm/mmu.h b/arch/x86/include/asm/mmu.h
index 9ea26f167497..0f041ed10c83 100644
--- a/arch/x86/include/asm/mmu.h
+++ b/arch/x86/include/asm/mmu.h
@@ -47,6 +47,13 @@ typedef struct {
 	 */
 	u16 pkey_allocation_map;
 	s16 execute_only_pkey;
+
+	/*
+	 * Used to derive the PKRU register value from init_pkru in a
+	 * signal handler.
+	 */
+	u32 pkey_signal_mask;
+	u32 pkey_signal_value;
 #endif
 #ifdef CONFIG_X86_INTEL_MPX
 	/* address of the bounds directory */
diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
index 6d16d15d09a0..d402496714ad 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -141,6 +141,7 @@ static inline int init_new_context(struct task_struct *tsk,
 		mm->context.pkey_allocation_map = 0x1;
 		/* -1 means unallocated or invalid */
 		mm->context.execute_only_pkey = -1;
+		mm->context.pkey_signal_mask = ~0U;
 	}
 	#endif
 	return init_new_context_ldt(tsk, mm);
diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index a0ba1ffda0df..a9e20e4f6bb2 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h
@@ -7,6 +7,10 @@
 extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 		unsigned long init_val);
 
+extern void arch_set_pkey_signal_default(struct mm_struct *mm,
+		int pkey, u32 rights);
+extern void arch_reset_pkey_signal_default(struct mm_struct *mm, int pkey);
+
 /*
  * Try to dedicate one of the protection keys to be used as an
  * execute-only protection key.
@@ -104,6 +108,6 @@ extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 		unsigned long init_val);
 extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 		unsigned long init_val);
-extern void copy_init_pkru_to_fpregs(void);
+extern void copy_init_pkru_to_fpregs(bool for_signal);
 
 #endif /*_ASM_X86_PKEYS_H */
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index f92a6593de1e..970e5b01ade4 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -362,7 +362,7 @@ void fpu__drop(struct fpu *fpu)
  * Clear FPU registers by setting them up from
  * the init fpstate:
  */
-static inline void copy_init_fpstate_to_fpregs(void)
+static inline void copy_init_fpstate_to_fpregs(bool for_signal)
 {
 	if (use_xsave())
 		copy_kernel_to_xregs(&init_fpstate.xsave, -1);
@@ -372,7 +372,7 @@ static inline void copy_init_fpstate_to_fpregs(void)
 		copy_kernel_to_fregs(&init_fpstate.fsave);
 
 	if (boot_cpu_has(X86_FEATURE_OSPKE))
-		copy_init_pkru_to_fpregs();
+		copy_init_pkru_to_fpregs(for_signal);
 }
 
 /*
@@ -381,7 +381,7 @@ static inline void copy_init_fpstate_to_fpregs(void)
  * Called by sys_execve(), by the signal handler code and by various
  * error paths.
  */
-void fpu__clear(struct fpu *fpu)
+void fpu__clear(struct fpu *fpu, bool for_signal)
 {
 	WARN_ON_FPU(fpu != &current->thread.fpu); /* Almost certainly an anomaly */
 
@@ -394,7 +394,7 @@ void fpu__clear(struct fpu *fpu)
 		preempt_disable();
 		fpu__initialize(fpu);
 		user_fpu_begin();
-		copy_init_fpstate_to_fpregs();
+		copy_init_fpstate_to_fpregs(for_signal);
 		preempt_enable();
 	}
 }
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 23f1691670b6..8e9709c1270b 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -277,7 +277,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 			 IS_ENABLED(CONFIG_IA32_EMULATION));
 
 	if (!buf) {
-		fpu__clear(fpu);
+		fpu__clear(fpu, false);
 		return 0;
 	}
 
@@ -358,7 +358,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		 */
 		user_fpu_begin();
 		if (copy_user_to_fpregs_zeroing(buf_fx, xfeatures, fx_only)) {
-			fpu__clear(fpu);
+			fpu__clear(fpu, false);
 			return -1;
 		}
 	}
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index bb988a24db92..36d59d8c684a 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -129,7 +129,7 @@ void flush_thread(void)
 	flush_ptrace_hw_breakpoint(tsk);
 	memset(tsk->thread.tls_array, 0, sizeof(tsk->thread.tls_array));
 
-	fpu__clear(&tsk->thread.fpu);
+	fpu__clear(&tsk->thread.fpu, false);
 }
 
 void disable_TSC(void)
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index b9e00e8f1c9b..86e7cf5d38e9 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -757,7 +757,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 		 * Ensure the signal handler starts with the new fpu state.
 		 */
 		if (fpu->initialized)
-			fpu__clear(fpu);
+			fpu__clear(fpu, true);
 	}
 	signal_setup_done(failed, ksig, stepping);
 }
diff --git a/arch/x86/mm/pkeys.c b/arch/x86/mm/pkeys.c
index d7bc0eea20a5..f7447512833c 100644
--- a/arch/x86/mm/pkeys.c
+++ b/arch/x86/mm/pkeys.c
@@ -19,6 +19,26 @@
 #include <asm/cpufeature.h>             /* boot_cpu_has, ...            */
 #include <asm/mmu_context.h>            /* vma_pkey()                   */
 
+void arch_set_pkey_signal_default(struct mm_struct *mm, int pkey, u32 rights)
+{
+	int shift = pkey * PKRU_BITS_PER_PKEY;
+	u32 mask = ~(3U << shift);
+	u32 value = rights << shift;
+
+	mm->context.pkey_signal_mask &= mask;
+	mm->context.pkey_signal_value =
+		(mm->context.pkey_signal_value & mask) | value;
+}
+
+void arch_reset_pkey_signal_default(struct mm_struct *mm, int pkey)
+{
+	int shift = pkey * PKRU_BITS_PER_PKEY;
+	u32 mask = 3U << shift;
+
+	mm->context.pkey_signal_mask |= mask;
+	mm->context.pkey_signal_value &= ~mask;
+}
+
 int __execute_only_pkey(struct mm_struct *mm)
 {
 	bool need_to_set_mm_pkey = false;
@@ -142,21 +162,30 @@ u32 init_pkru_value = PKRU_AD_KEY( 1) | PKRU_AD_KEY( 2) | PKRU_AD_KEY( 3) |
  * we know the FPU regstiers are safe for use and we can use PKRU
  * directly.
  */
-void copy_init_pkru_to_fpregs(void)
+void copy_init_pkru_to_fpregs(bool for_signal)
 {
 	u32 init_pkru_value_snapshot = READ_ONCE(init_pkru_value);
+	u32 desired_pkru;
+
+	if (for_signal)
+		desired_pkru = (init_pkru_value_snapshot
+				& current->mm->context.pkey_signal_mask)
+			| current->mm->context.pkey_signal_value;
+	else
+		desired_pkru = init_pkru_value_snapshot;
+
 	/*
 	 * Any write to PKRU takes it out of the XSAVE 'init
 	 * state' which increases context switch cost.  Avoid
 	 * writing 0 when PKRU was already 0.
 	 */
-	if (!init_pkru_value_snapshot && !read_pkru())
+	if (!desired_pkru && !read_pkru())
 		return;
 	/*
 	 * Override the PKRU state that came from 'init_fpstate'
 	 * with the baseline from the process.
 	 */
-	write_pkru(init_pkru_value_snapshot);
+	write_pkru(desired_pkru);
 }
 
 static ssize_t init_pkru_read_file(struct file *file, char __user *user_buf,
diff --git a/arch/xtensa/include/uapi/asm/mman.h b/arch/xtensa/include/uapi/asm/mman.h
index 3e9d01ada81f..5abb66526e3e 100644
--- a/arch/xtensa/include/uapi/asm/mman.h
+++ b/arch/xtensa/include/uapi/asm/mman.h
@@ -111,6 +111,8 @@
 /* compatibility flags */
 #define MAP_FILE	0
 
+#define PKEY_ALLOC_SETSIGNAL 0x1
+
 #define PKEY_DISABLE_ACCESS	0x1
 #define PKEY_DISABLE_WRITE	0x2
 #define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\
diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
index 0794ca78c379..0ab73297b752 100644
--- a/include/linux/pkeys.h
+++ b/include/linux/pkeys.h
@@ -29,13 +29,23 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
 	return -EINVAL;
 }
 
+static inline void arch_set_pkey_signal_default(struct mm_struct *mm,
+			int pkey, u32 rights)
+{
+}
+
+static inline void arch_reset_pkey_signal_default(struct mm_struct *mm,
+			int pkey)
+{
+}
+
 static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 			unsigned long init_val)
 {
 	return 0;
 }
 
-static inline void copy_init_pkru_to_fpregs(void)
+static inline void copy_init_pkru_to_fpregs(bool for_signal)
 {
 }
 
diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index f8b134f5608f..78772266e3cd 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -66,6 +66,8 @@
 /* compatibility flags */
 #define MAP_FILE	0
 
+#define PKEY_ALLOC_SETSIGNAL 0x1
+
 #define PKEY_DISABLE_ACCESS	0x1
 #define PKEY_DISABLE_WRITE	0x2
 #define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\
diff --git a/mm/mprotect.c b/mm/mprotect.c
index ec39f730a0bf..021f1d465649 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -523,14 +523,17 @@ SYSCALL_DEFINE4(pkey_mprotect, unsigned long, start, size_t, len,
 	return do_mprotect_pkey(start, len, prot, pkey);
 }
 
+#define PKEY_ALLOC_FLAGS ((unsigned long) (PKEY_ALLOC_SETSIGNAL))
+
 SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
 {
 	int pkey;
 	int ret;
 
-	/* No flags supported yet. */
-	if (flags)
+	/* check for unsupported flags */
+	if (flags & ~PKEY_ALLOC_FLAGS)
 		return -EINVAL;
+
 	/* check for unsupported init values */
 	if (init_val & ~PKEY_ACCESS_MASK)
 		return -EINVAL;
@@ -547,6 +550,10 @@ SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
 		mm_pkey_free(current->mm, pkey);
 		goto out;
 	}
+
+	if (flags & PKEY_ALLOC_SETSIGNAL)
+		arch_set_pkey_signal_default(current->mm, pkey, init_val);
+
 	ret = pkey;
 out:
 	up_write(&current->mm->mmap_sem);
@@ -559,6 +566,8 @@ SYSCALL_DEFINE1(pkey_free, int, pkey)
 
 	down_write(&current->mm->mmap_sem);
 	ret = mm_pkey_free(current->mm, pkey);
+	if (!ret)
+		arch_reset_pkey_signal_default(current->mm, pkey);
 	up_write(&current->mm->mmap_sem);
 
 	/*
diff --git a/tools/include/uapi/asm-generic/mman-common.h b/tools/include/uapi/asm-generic/mman-common.h
index f8b134f5608f..78772266e3cd 100644
--- a/tools/include/uapi/asm-generic/mman-common.h
+++ b/tools/include/uapi/asm-generic/mman-common.h
@@ -66,6 +66,8 @@
 /* compatibility flags */
 #define MAP_FILE	0
 
+#define PKEY_ALLOC_SETSIGNAL 0x1
+
 #define PKEY_DISABLE_ACCESS	0x1
 #define PKEY_DISABLE_WRITE	0x2
 #define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\
diff --git a/tools/testing/selftests/x86/protection_keys.c b/tools/testing/selftests/x86/protection_keys.c
index bc1b0735bb50..ea161916e378 100644
--- a/tools/testing/selftests/x86/protection_keys.c
+++ b/tools/testing/selftests/x86/protection_keys.c
@@ -421,6 +421,8 @@ void dumpit(char *f)
 	close(fd);
 }
 
+#define PKEY_ALLOC_SETSIGNAL 0x1
+
 #define PKEY_DISABLE_ACCESS    0x1
 #define PKEY_DISABLE_WRITE     0x2
 

Re: pkeys: Support setting access rights for signal handlers

[Dave Hansen]

On 12/09/2017 01:16 PM, Florian Weimer wrote:
> The attached patch addresses a problem with the current x86 pkey
> implementation, which makes default-readable pkeys unusable from signal
> handlers because the default init_pkru value blocks access.

Thanks for looking into this!

What do you mean by "default-readable pkeys"?

I think you mean that, for any data that needs to be accessed to enter a
signal handler, it must be set to pkey=0 with the current
implementation.  All other keys are inaccessible when entering a signal
handler because the "init" value disables access.

My only nit with this is whether it is the *right* interface.  The
signal vs. XSAVE state thing is pretty x86 specific and I doubt that
this will be the last feature that we encounter that needs special
signal behavior.

A question more for the x86 maintainers is whether they would rather see
a pkeys-specific interface for this, or an XSAVE-specific interface
where you could specify a non-init XSAVE state for a set of XSAVE
components.

Re: pkeys: Support setting access rights for signal handlers

[Dave Hansen]

On 12/09/2017 01:16 PM, Florian Weimer wrote:
> The attached patch addresses a problem with the current x86 pkey
> implementation, which makes default-readable pkeys unusable from signal
> handlers because the default init_pkru value blocks access.

Thanks for looking into this!

What do you mean by "default-readable pkeys"?

I think you mean that, for any data that needs to be accessed to enter a
signal handler, it must be set to pkey=0 with the current
implementation.  All other keys are inaccessible when entering a signal
handler because the "init" value disables access.

My only nit with this is whether it is the *right* interface.  The
signal vs. XSAVE state thing is pretty x86 specific and I doubt that
this will be the last feature that we encounter that needs special
signal behavior.

A question more for the x86 maintainers is whether they would rather see
a pkeys-specific interface for this, or an XSAVE-specific interface
where you could specify a non-init XSAVE state for a set of XSAVE
components.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: pkeys: Support setting access rights for signal handlers

[Dave Hansen]

On 12/09/2017 01:16 PM, Florian Weimer wrote:
> The attached patch addresses a problem with the current x86 pkey
> implementation, which makes default-readable pkeys unusable from signal
> handlers because the default init_pkru value blocks access.

Thanks for looking into this!

What do you mean by "default-readable pkeys"?

I think you mean that, for any data that needs to be accessed to enter a
signal handler, it must be set to pkey=0 with the current
implementation.  All other keys are inaccessible when entering a signal
handler because the "init" value disables access.

My only nit with this is whether it is the *right* interface.  The
signal vs. XSAVE state thing is pretty x86 specific and I doubt that
this will be the last feature that we encounter that needs special
signal behavior.

A question more for the x86 maintainers is whether they would rather see
a pkeys-specific interface for this, or an XSAVE-specific interface
where you could specify a non-init XSAVE state for a set of XSAVE
components.

Re: pkeys: Support setting access rights for signal handlers

[Florian Weimer]

On 12/10/2017 01:17 AM, Dave Hansen wrote:
> On 12/09/2017 01:16 PM, Florian Weimer wrote:
>> The attached patch addresses a problem with the current x86 pkey
>> implementation, which makes default-readable pkeys unusable from signal
>> handlers because the default init_pkru value blocks access.
> 
> Thanks for looking into this!
> 
> What do you mean by "default-readable pkeys"?
> 
> I think you mean that, for any data that needs to be accessed to enter a
> signal handler, it must be set to pkey=0 with the current
> implementation.  All other keys are inaccessible when entering a signal
> handler because the "init" value disables access.

Right, and for keys which are readable (but not writable) most of the 
time, so that date is readable, this breaks things.

> My only nit with this is whether it is the *right* interface.  The
> signal vs. XSAVE state thing is pretty x86 specific and I doubt that
> this will be the last feature that we encounter that needs special
> signal behavior.

The interface is not specific to XSAVE.  To generic code, only the two 
signal mask manipulation functions are exposed.  And I expect that we're 
going to need that for other (non-x86) implementations because they will 
have the same issue because the signal handler behavior will be identical.

Thanks,
Florian

Re: pkeys: Support setting access rights for signal handlers

[Florian Weimer]

On 12/10/2017 01:17 AM, Dave Hansen wrote:
> On 12/09/2017 01:16 PM, Florian Weimer wrote:
>> The attached patch addresses a problem with the current x86 pkey
>> implementation, which makes default-readable pkeys unusable from signal
>> handlers because the default init_pkru value blocks access.
> 
> Thanks for looking into this!
> 
> What do you mean by "default-readable pkeys"?
> 
> I think you mean that, for any data that needs to be accessed to enter a
> signal handler, it must be set to pkey=0 with the current
> implementation.  All other keys are inaccessible when entering a signal
> handler because the "init" value disables access.

Right, and for keys which are readable (but not writable) most of the 
time, so that date is readable, this breaks things.

> My only nit with this is whether it is the *right* interface.  The
> signal vs. XSAVE state thing is pretty x86 specific and I doubt that
> this will be the last feature that we encounter that needs special
> signal behavior.

The interface is not specific to XSAVE.  To generic code, only the two 
signal mask manipulation functions are exposed.  And I expect that we're 
going to need that for other (non-x86) implementations because they will 
have the same issue because the signal handler behavior will be identical.

Thanks,
Florian

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: pkeys: Support setting access rights for signal handlers

[Dave Hansen]

On 12/09/2017 10:42 PM, Florian Weimer wrote:
>> My only nit with this is whether it is the *right* interface.  The 
>> signal vs. XSAVE state thing is pretty x86 specific and I doubt
>> that this will be the last feature that we encounter that needs
>> special signal behavior.
> 
> The interface is not specific to XSAVE.  To generic code, only the
> two signal mask manipulation functions are exposed.  And I expect
> that we're going to need that for other (non-x86) implementations
> because they will have the same issue because the signal handler
> behavior will be identical.

Let's check with the other implementation...

Ram, this is a question about the signal handler behavior on POWER.  I
thought you ended up having different behavior in signal handlers than x86.

In any case, I think the question still stands: Do we want this to be
pkeys-only, or build it so that it can be used for MPX and any future
XSAVE features that need non-init values when entering a signal handler.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: pkeys: Support setting access rights for signal handlers

[Dave Hansen]

On 12/09/2017 10:42 PM, Florian Weimer wrote:
>> My only nit with this is whether it is the *right* interface.  The 
>> signal vs. XSAVE state thing is pretty x86 specific and I doubt
>> that this will be the last feature that we encounter that needs
>> special signal behavior.
> 
> The interface is not specific to XSAVE.  To generic code, only the
> two signal mask manipulation functions are exposed.  And I expect
> that we're going to need that for other (non-x86) implementations
> because they will have the same issue because the signal handler
> behavior will be identical.

Let's check with the other implementation...

Ram, this is a question about the signal handler behavior on POWER.  I
thought you ended up having different behavior in signal handlers than x86.

In any case, I think the question still stands: Do we want this to be
pkeys-only, or build it so that it can be used for MPX and any future
XSAVE features that need non-init values when entering a signal handler.

Re: pkeys: Support setting access rights for signal handlers

[Ram Pai]

On Mon, Dec 11, 2017 at 08:13:12AM -0800, Dave Hansen wrote:
> On 12/09/2017 10:42 PM, Florian Weimer wrote:
> >> My only nit with this is whether it is the *right* interface.  The 
> >> signal vs. XSAVE state thing is pretty x86 specific and I doubt
> >> that this will be the last feature that we encounter that needs
> >> special signal behavior.
> > 
> > The interface is not specific to XSAVE.  To generic code, only the
> > two signal mask manipulation functions are exposed.  And I expect
> > that we're going to need that for other (non-x86) implementations
> > because they will have the same issue because the signal handler
> > behavior will be identical.
> 
> Let's check with the other implementation...
> 
> Ram, this is a question about the signal handler behavior on POWER.  I
> thought you ended up having different behavior in signal handlers than x86.

On POWER, the value of the pkey_read() i.e contents the AMR
register(pkru equivalent), is always the same regardless of its
context; signal handler or not.

In other words, the permission of any allocated key will not
reset in a signal handler context.

I was not aware that x86 would reset the key permissions in signal
handler.  I think, the proposed behavior for PKEY_ALLOC_SETSIGNAL should
actually be the default behavior.


RP

Re: pkeys: Support setting access rights for signal handlers

[Ram Pai]

On Mon, Dec 11, 2017 at 08:13:12AM -0800, Dave Hansen wrote:
> On 12/09/2017 10:42 PM, Florian Weimer wrote:
> >> My only nit with this is whether it is the *right* interface.  The 
> >> signal vs. XSAVE state thing is pretty x86 specific and I doubt
> >> that this will be the last feature that we encounter that needs
> >> special signal behavior.
> > 
> > The interface is not specific to XSAVE.  To generic code, only the
> > two signal mask manipulation functions are exposed.  And I expect
> > that we're going to need that for other (non-x86) implementations
> > because they will have the same issue because the signal handler
> > behavior will be identical.
> 
> Let's check with the other implementation...
> 
> Ram, this is a question about the signal handler behavior on POWER.  I
> thought you ended up having different behavior in signal handlers than x86.

On POWER, the value of the pkey_read() i.e contents the AMR
register(pkru equivalent), is always the same regardless of its
context; signal handler or not.

In other words, the permission of any allocated key will not
reset in a signal handler context.

I was not aware that x86 would reset the key permissions in signal
handler.  I think, the proposed behavior for PKEY_ALLOC_SETSIGNAL should
actually be the default behavior.


RP

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: pkeys: Support setting access rights for signal handlers

[Florian Weimer]

On 12/13/2017 12:13 AM, Ram Pai wrote:

> On POWER, the value of the pkey_read() i.e contents the AMR
> register(pkru equivalent), is always the same regardless of its
> context; signal handler or not.
> 
> In other words, the permission of any allocated key will not
> reset in a signal handler context.

That's certainly the simpler semantics, but I don't like how they differ 
from x86.

Is the AMR register reset to the original value upon (regular) return 
from the signal handler?

> I was not aware that x86 would reset the key permissions in signal
> handler.  I think, the proposed behavior for PKEY_ALLOC_SETSIGNAL should
> actually be the default behavior.

Note that PKEY_ALLOC_SETSIGNAL does something different: It requests 
that the kernel sets the access rights for the key to the bits specified 
at pkey_alloc time when the signal handler is invoked.  So there is 
still a reset with PKEY_ALLOC_SETSIGNAL, but to a different value.  It 
did not occur to me that it might be desirable to avoid resetting the 
value on a per-key basis.

Thanks,
Florian

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: pkeys: Support setting access rights for signal handlers

[Florian Weimer]

On 12/13/2017 12:13 AM, Ram Pai wrote:

> On POWER, the value of the pkey_read() i.e contents the AMR
> register(pkru equivalent), is always the same regardless of its
> context; signal handler or not.
> 
> In other words, the permission of any allocated key will not
> reset in a signal handler context.

That's certainly the simpler semantics, but I don't like how they differ 
from x86.

Is the AMR register reset to the original value upon (regular) return 
from the signal handler?

> I was not aware that x86 would reset the key permissions in signal
> handler.  I think, the proposed behavior for PKEY_ALLOC_SETSIGNAL should
> actually be the default behavior.

Note that PKEY_ALLOC_SETSIGNAL does something different: It requests 
that the kernel sets the access rights for the key to the bits specified 
at pkey_alloc time when the signal handler is invoked.  So there is 
still a reset with PKEY_ALLOC_SETSIGNAL, but to a different value.  It 
did not occur to me that it might be desirable to avoid resetting the 
value on a per-key basis.

Thanks,
Florian

Re: pkeys: Support setting access rights for signal handlers

[Ram Pai]

On Wed, Dec 13, 2017 at 03:14:36AM +0100, Florian Weimer wrote:
> On 12/13/2017 12:13 AM, Ram Pai wrote:
> 
> >On POWER, the value of the pkey_read() i.e contents the AMR
> >register(pkru equivalent), is always the same regardless of its
> >context; signal handler or not.
> >
> >In other words, the permission of any allocated key will not
> >reset in a signal handler context.
> 
> That's certainly the simpler semantics, but I don't like how they
> differ from x86.
> 
> Is the AMR register reset to the original value upon (regular)
> return from the signal handler?

The AMR bits are not touched upon (regular) return from the signal
handler.

If the signal handler changes the bits in the AMR, they will continue
to be so, even after return from the signal handler.

To illustrate with an example, lets say AMR value is 'x' and signal
handler is invoked.  The value of AMR will be 'x' in the context of the
signal handler.  On return from the signal handler the value of AMR will
continue to be 'x'. However if signal handler changes the value of AMR
to 'y', the value of AMR will be 'y' on return from the signal handler.


> 
> >I was not aware that x86 would reset the key permissions in signal
> >handler.  I think, the proposed behavior for PKEY_ALLOC_SETSIGNAL should
> >actually be the default behavior.
> 
> Note that PKEY_ALLOC_SETSIGNAL does something different: It requests
> that the kernel sets the access rights for the key to the bits
> specified at pkey_alloc time when the signal handler is invoked.  So
> there is still a reset with PKEY_ALLOC_SETSIGNAL, but to a different
> value.  It did not occur to me that it might be desirable to avoid
> resetting the value on a per-key basis.

Ah. ok i see the subtle difference proposed by your semantics.

Will the following behavior work?

'No bits will be reset to its initial value unless the key has been
allocated with PKEY_ALLOC_*RE*SETSIGNAL flag'.

> 
> Thanks,
> Florian

-- 
Ram Pai

Re: pkeys: Support setting access rights for signal handlers

[Ram Pai]

On Wed, Dec 13, 2017 at 03:14:36AM +0100, Florian Weimer wrote:
> On 12/13/2017 12:13 AM, Ram Pai wrote:
> 
> >On POWER, the value of the pkey_read() i.e contents the AMR
> >register(pkru equivalent), is always the same regardless of its
> >context; signal handler or not.
> >
> >In other words, the permission of any allocated key will not
> >reset in a signal handler context.
> 
> That's certainly the simpler semantics, but I don't like how they
> differ from x86.
> 
> Is the AMR register reset to the original value upon (regular)
> return from the signal handler?

The AMR bits are not touched upon (regular) return from the signal
handler.

If the signal handler changes the bits in the AMR, they will continue
to be so, even after return from the signal handler.

To illustrate with an example, lets say AMR value is 'x' and signal
handler is invoked.  The value of AMR will be 'x' in the context of the
signal handler.  On return from the signal handler the value of AMR will
continue to be 'x'. However if signal handler changes the value of AMR
to 'y', the value of AMR will be 'y' on return from the signal handler.


> 
> >I was not aware that x86 would reset the key permissions in signal
> >handler.  I think, the proposed behavior for PKEY_ALLOC_SETSIGNAL should
> >actually be the default behavior.
> 
> Note that PKEY_ALLOC_SETSIGNAL does something different: It requests
> that the kernel sets the access rights for the key to the bits
> specified at pkey_alloc time when the signal handler is invoked.  So
> there is still a reset with PKEY_ALLOC_SETSIGNAL, but to a different
> value.  It did not occur to me that it might be desirable to avoid
> resetting the value on a per-key basis.

Ah. ok i see the subtle difference proposed by your semantics.

Will the following behavior work?

'No bits will be reset to its initial value unless the key has been
allocated with PKEY_ALLOC_*RE*SETSIGNAL flag'.

> 
> Thanks,
> Florian

-- 
Ram Pai

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: pkeys: Support setting access rights for signal handlers

[Florian Weimer]

On 12/13/2017 12:35 PM, Ram Pai wrote:
> On Wed, Dec 13, 2017 at 03:14:36AM +0100, Florian Weimer wrote:
>> On 12/13/2017 12:13 AM, Ram Pai wrote:
>>
>>> On POWER, the value of the pkey_read() i.e contents the AMR
>>> register(pkru equivalent), is always the same regardless of its
>>> context; signal handler or not.
>>>
>>> In other words, the permission of any allocated key will not
>>> reset in a signal handler context.
>>
>> That's certainly the simpler semantics, but I don't like how they
>> differ from x86.
>>
>> Is the AMR register reset to the original value upon (regular)
>> return from the signal handler?
> 
> The AMR bits are not touched upon (regular) return from the signal
> handler.
> 
> If the signal handler changes the bits in the AMR, they will continue
> to be so, even after return from the signal handler.
> 
> To illustrate with an example, lets say AMR value is 'x' and signal
> handler is invoked.  The value of AMR will be 'x' in the context of the
> signal handler.  On return from the signal handler the value of AMR will
> continue to be 'x'. However if signal handler changes the value of AMR
> to 'y', the value of AMR will be 'y' on return from the signal handler.

Okay, this model is really quite different from x86.  Is there a good 
reason for the difference?  Could we change the x86 implementation to 
behave in the same way?  Or alternatively, change the POWER 
implementation to match the existing x86 behavior?

>>> I was not aware that x86 would reset the key permissions in signal
>>> handler.  I think, the proposed behavior for PKEY_ALLOC_SETSIGNAL should
>>> actually be the default behavior.
>>
>> Note that PKEY_ALLOC_SETSIGNAL does something different: It requests
>> that the kernel sets the access rights for the key to the bits
>> specified at pkey_alloc time when the signal handler is invoked.  So
>> there is still a reset with PKEY_ALLOC_SETSIGNAL, but to a different
>> value.  It did not occur to me that it might be desirable to avoid
>> resetting the value on a per-key basis.
> 
> Ah. ok i see the subtle difference proposed by your semantics.
> 
> Will the following behavior work?
> 
> 'No bits will be reset to its initial value unless the key has been
> allocated with PKEY_ALLOC_*RE*SETSIGNAL flag'.

The existing x86 interface defaults to resetting the bits, 
unfortunately.  I'm not sure if we can or should change this now.

For my purposes, the POWER semantics would work fine as far as I can 
see.  The reset-to-default is really problematic.  I don't actually need 
the configurable behavior, but I implemented it this way to achieve a 
maximum of backwards compatibility.

Thanks,
Florian

Re: pkeys: Support setting access rights for signal handlers

[Florian Weimer]

On 12/13/2017 12:35 PM, Ram Pai wrote:
> On Wed, Dec 13, 2017 at 03:14:36AM +0100, Florian Weimer wrote:
>> On 12/13/2017 12:13 AM, Ram Pai wrote:
>>
>>> On POWER, the value of the pkey_read() i.e contents the AMR
>>> register(pkru equivalent), is always the same regardless of its
>>> context; signal handler or not.
>>>
>>> In other words, the permission of any allocated key will not
>>> reset in a signal handler context.
>>
>> That's certainly the simpler semantics, but I don't like how they
>> differ from x86.
>>
>> Is the AMR register reset to the original value upon (regular)
>> return from the signal handler?
> 
> The AMR bits are not touched upon (regular) return from the signal
> handler.
> 
> If the signal handler changes the bits in the AMR, they will continue
> to be so, even after return from the signal handler.
> 
> To illustrate with an example, lets say AMR value is 'x' and signal
> handler is invoked.  The value of AMR will be 'x' in the context of the
> signal handler.  On return from the signal handler the value of AMR will
> continue to be 'x'. However if signal handler changes the value of AMR
> to 'y', the value of AMR will be 'y' on return from the signal handler.

Okay, this model is really quite different from x86.  Is there a good 
reason for the difference?  Could we change the x86 implementation to 
behave in the same way?  Or alternatively, change the POWER 
implementation to match the existing x86 behavior?

>>> I was not aware that x86 would reset the key permissions in signal
>>> handler.  I think, the proposed behavior for PKEY_ALLOC_SETSIGNAL should
>>> actually be the default behavior.
>>
>> Note that PKEY_ALLOC_SETSIGNAL does something different: It requests
>> that the kernel sets the access rights for the key to the bits
>> specified at pkey_alloc time when the signal handler is invoked.  So
>> there is still a reset with PKEY_ALLOC_SETSIGNAL, but to a different
>> value.  It did not occur to me that it might be desirable to avoid
>> resetting the value on a per-key basis.
> 
> Ah. ok i see the subtle difference proposed by your semantics.
> 
> Will the following behavior work?
> 
> 'No bits will be reset to its initial value unless the key has been
> allocated with PKEY_ALLOC_*RE*SETSIGNAL flag'.

The existing x86 interface defaults to resetting the bits, 
unfortunately.  I'm not sure if we can or should change this now.

For my purposes, the POWER semantics would work fine as far as I can 
see.  The reset-to-default is really problematic.  I don't actually need 
the configurable behavior, but I implemented it this way to achieve a 
maximum of backwards compatibility.

Thanks,
Florian

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: pkeys: Support setting access rights for signal handlers

[Florian Weimer]

On 12/13/2017 12:35 PM, Ram Pai wrote:
> On Wed, Dec 13, 2017 at 03:14:36AM +0100, Florian Weimer wrote:
>> On 12/13/2017 12:13 AM, Ram Pai wrote:
>>
>>> On POWER, the value of the pkey_read() i.e contents the AMR
>>> register(pkru equivalent), is always the same regardless of its
>>> context; signal handler or not.
>>>
>>> In other words, the permission of any allocated key will not
>>> reset in a signal handler context.
>>
>> That's certainly the simpler semantics, but I don't like how they
>> differ from x86.
>>
>> Is the AMR register reset to the original value upon (regular)
>> return from the signal handler?
> 
> The AMR bits are not touched upon (regular) return from the signal
> handler.
> 
> If the signal handler changes the bits in the AMR, they will continue
> to be so, even after return from the signal handler.
> 
> To illustrate with an example, lets say AMR value is 'x' and signal
> handler is invoked.  The value of AMR will be 'x' in the context of the
> signal handler.  On return from the signal handler the value of AMR will
> continue to be 'x'. However if signal handler changes the value of AMR
> to 'y', the value of AMR will be 'y' on return from the signal handler.

Okay, this model is really quite different from x86.  Is there a good 
reason for the difference?  Could we change the x86 implementation to 
behave in the same way?  Or alternatively, change the POWER 
implementation to match the existing x86 behavior?

>>> I was not aware that x86 would reset the key permissions in signal
>>> handler.  I think, the proposed behavior for PKEY_ALLOC_SETSIGNAL should
>>> actually be the default behavior.
>>
>> Note that PKEY_ALLOC_SETSIGNAL does something different: It requests
>> that the kernel sets the access rights for the key to the bits
>> specified at pkey_alloc time when the signal handler is invoked.  So
>> there is still a reset with PKEY_ALLOC_SETSIGNAL, but to a different
>> value.  It did not occur to me that it might be desirable to avoid
>> resetting the value on a per-key basis.
> 
> Ah. ok i see the subtle difference proposed by your semantics.
> 
> Will the following behavior work?
> 
> 'No bits will be reset to its initial value unless the key has been
> allocated with PKEY_ALLOC_*RE*SETSIGNAL flag'.

The existing x86 interface defaults to resetting the bits, 
unfortunately.  I'm not sure if we can or should change this now.

For my purposes, the POWER semantics would work fine as far as I can 
see.  The reset-to-default is really problematic.  I don't actually need 
the configurable behavior, but I implemented it this way to achieve a 
maximum of backwards compatibility.

Thanks,
Florian

Re: pkeys: Support setting access rights for signal handlers

[Dave Hansen]

On 12/13/2017 07:08 AM, Florian Weimer wrote:
> Okay, this model is really quite different from x86.  Is there a
> good reason for the difference?

Yes, both implementations are simple and take the "natural" behavior.
x86 changes XSAVE-controlled register values on entering a signal, so we
let them be changed (including PKRU).  POWER hardware does not do this
to its PKRU-equivalent, so we do not force it to.

x86 didn't have to do this for *signals*.  But, we kinda went on this
trajectory when we decided to clear/restore FPU state on
entering/exiting signals before XSAVE even existed.

FWIW, I do *not* think we have to do this for future XSAVE states.  But,
if we do that, we probably need an interface for apps to tell us which
states to save/restore and which state to set upon entering a signal
handler.  That's what I was trying to get you to consider instead of
just a one-off hack to fix this for pkeys.

Re: pkeys: Support setting access rights for signal handlers

[Dave Hansen]

On 12/13/2017 07:08 AM, Florian Weimer wrote:
> Okay, this model is really quite different from x86.  Is there a
> good reason for the difference?

Yes, both implementations are simple and take the "natural" behavior.
x86 changes XSAVE-controlled register values on entering a signal, so we
let them be changed (including PKRU).  POWER hardware does not do this
to its PKRU-equivalent, so we do not force it to.

x86 didn't have to do this for *signals*.  But, we kinda went on this
trajectory when we decided to clear/restore FPU state on
entering/exiting signals before XSAVE even existed.

FWIW, I do *not* think we have to do this for future XSAVE states.  But,
if we do that, we probably need an interface for apps to tell us which
states to save/restore and which state to set upon entering a signal
handler.  That's what I was trying to get you to consider instead of
just a one-off hack to fix this for pkeys.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: pkeys: Support setting access rights for signal handlers

[Florian Weimer]

On 12/13/2017 04:22 PM, Dave Hansen wrote:
> On 12/13/2017 07:08 AM, Florian Weimer wrote:
>> Okay, this model is really quite different from x86.  Is there a
>> good reason for the difference?
> 
> Yes, both implementations are simple and take the "natural" behavior.
> x86 changes XSAVE-controlled register values on entering a signal, so we
> let them be changed (including PKRU).  POWER hardware does not do this
> to its PKRU-equivalent, so we do not force it to.

Why?  Is there a technical reason not have fully-aligned behavior?  Can 
POWER at least implement the original PKEY_ALLOC_SETSIGNAL semantics 
(reset the access rights for certain keys before switching to the signal 
handler) in a reasonably efficient manner?

At the very least, if we add a pkey_alloc flag, it should have identical 
behavior on both POWER and x86.  So it should either reset the access 
rights to a fixed value (as posted) or mask out the PKRU reset on x86 
(if that's even possible).  In the latter case, the POWER would not even 
have to change if we keep saying that the default key behavior (without 
the flag) is undefined regarding signal handlers.

> x86 didn't have to do this for *signals*.  But, we kinda went on this
> trajectory when we decided to clear/restore FPU state on
> entering/exiting signals before XSAVE even existed.

 From a userspace perspective, I find this variance rather 
disappointing.  It's particularly problematic for something like PKRU, 
which comes with an entire set of separately configurable keys.  I 
implemented a per-key knob, but who says that someone else doesn't need 
a per-thread or per-signal knob to switch between these incompatible 
behaviors?

What can a library assume regarding pkeys behavior if there are 
process-global flags that completely alter certain aspects of their 
behavior?

> FWIW, I do *not* think we have to do this for future XSAVE states.  But,
> if we do that, we probably need an interface for apps to tell us which
> states to save/restore and which state to set upon entering a signal
> handler.  That's what I was trying to get you to consider instead of
> just a one-off hack to fix this for pkeys.

I get that now.

But for pkeys and their access rights, having this configurable at the 
PKRU level (as opposed the individual key level) would completely rule 
out any use of pkeys in the glibc dynamic linker.

Thanks,
Florian

Re: pkeys: Support setting access rights for signal handlers

[Florian Weimer]

On 12/13/2017 04:22 PM, Dave Hansen wrote:
> On 12/13/2017 07:08 AM, Florian Weimer wrote:
>> Okay, this model is really quite different from x86.  Is there a
>> good reason for the difference?
> 
> Yes, both implementations are simple and take the "natural" behavior.
> x86 changes XSAVE-controlled register values on entering a signal, so we
> let them be changed (including PKRU).  POWER hardware does not do this
> to its PKRU-equivalent, so we do not force it to.

Why?  Is there a technical reason not have fully-aligned behavior?  Can 
POWER at least implement the original PKEY_ALLOC_SETSIGNAL semantics 
(reset the access rights for certain keys before switching to the signal 
handler) in a reasonably efficient manner?

At the very least, if we add a pkey_alloc flag, it should have identical 
behavior on both POWER and x86.  So it should either reset the access 
rights to a fixed value (as posted) or mask out the PKRU reset on x86 
(if that's even possible).  In the latter case, the POWER would not even 
have to change if we keep saying that the default key behavior (without 
the flag) is undefined regarding signal handlers.

> x86 didn't have to do this for *signals*.  But, we kinda went on this
> trajectory when we decided to clear/restore FPU state on
> entering/exiting signals before XSAVE even existed.

 From a userspace perspective, I find this variance rather 
disappointing.  It's particularly problematic for something like PKRU, 
which comes with an entire set of separately configurable keys.  I 
implemented a per-key knob, but who says that someone else doesn't need 
a per-thread or per-signal knob to switch between these incompatible 
behaviors?

What can a library assume regarding pkeys behavior if there are 
process-global flags that completely alter certain aspects of their 
behavior?

> FWIW, I do *not* think we have to do this for future XSAVE states.  But,
> if we do that, we probably need an interface for apps to tell us which
> states to save/restore and which state to set upon entering a signal
> handler.  That's what I was trying to get you to consider instead of
> just a one-off hack to fix this for pkeys.

I get that now.

But for pkeys and their access rights, having this configurable at the 
PKRU level (as opposed the individual key level) would completely rule 
out any use of pkeys in the glibc dynamic linker.

Thanks,
Florian

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: pkeys: Support setting access rights for signal handlers

[Ram Pai]

On Wed, Dec 13, 2017 at 04:40:11PM +0100, Florian Weimer wrote:
> On 12/13/2017 04:22 PM, Dave Hansen wrote:
> >On 12/13/2017 07:08 AM, Florian Weimer wrote:
> >>Okay, this model is really quite different from x86.  Is there a
> >>good reason for the difference?
> >
> >Yes, both implementations are simple and take the "natural" behavior.
> >x86 changes XSAVE-controlled register values on entering a signal, so we
> >let them be changed (including PKRU).  POWER hardware does not do this
> >to its PKRU-equivalent, so we do not force it to.
> 
> Whuy?  Is there a technical reason not have fully-aligned behavior?
> Can POWER at least implement the original PKEY_ALLOC_SETSIGNAL
> semantics (reset the access rights for certain keys before switching
> to the signal handler) in a reasonably efficient manner?

This can be done on POWER. I can also change the behavior on POWER
to exactly match x86; i.e reset the value to init value before
calling the signal handler.

But I think, we should clearly define the default behavior, the behavior
when no flag is specified. Applications tend to rely on default behavior
and expect the most intuitive behavior to be the default behavior.

I tend to think; keeping my biases aside, that the most intuitive
behavior is to preserve access/write permissions of any key, i.e not
reset to the init value.  If the application has set the permissions of
a key to some value, it would'nt expect anyone to change them,
irrespective of which context it is in.

RP

Re: pkeys: Support setting access rights for signal handlers

[Ram Pai]

On Wed, Dec 13, 2017 at 04:40:11PM +0100, Florian Weimer wrote:
> On 12/13/2017 04:22 PM, Dave Hansen wrote:
> >On 12/13/2017 07:08 AM, Florian Weimer wrote:
> >>Okay, this model is really quite different from x86.  Is there a
> >>good reason for the difference?
> >
> >Yes, both implementations are simple and take the "natural" behavior.
> >x86 changes XSAVE-controlled register values on entering a signal, so we
> >let them be changed (including PKRU).  POWER hardware does not do this
> >to its PKRU-equivalent, so we do not force it to.
> 
> Whuy?  Is there a technical reason not have fully-aligned behavior?
> Can POWER at least implement the original PKEY_ALLOC_SETSIGNAL
> semantics (reset the access rights for certain keys before switching
> to the signal handler) in a reasonably efficient manner?

This can be done on POWER. I can also change the behavior on POWER
to exactly match x86; i.e reset the value to init value before
calling the signal handler.

But I think, we should clearly define the default behavior, the behavior
when no flag is specified. Applications tend to rely on default behavior
and expect the most intuitive behavior to be the default behavior.

I tend to think; keeping my biases aside, that the most intuitive
behavior is to preserve access/write permissions of any key, i.e not
reset to the init value.  If the application has set the permissions of
a key to some value, it would'nt expect anyone to change them,
irrespective of which context it is in.

RP

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: pkeys: Support setting access rights for signal handlers

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 2765 bytes --]

On 12/14/2017 01:17 AM, Ram Pai wrote:
> On Wed, Dec 13, 2017 at 04:40:11PM +0100, Florian Weimer wrote:
>> On 12/13/2017 04:22 PM, Dave Hansen wrote:
>>> On 12/13/2017 07:08 AM, Florian Weimer wrote:
>>>> Okay, this model is really quite different from x86.  Is there a
>>>> good reason for the difference?
>>>
>>> Yes, both implementations are simple and take the "natural" behavior.
>>> x86 changes XSAVE-controlled register values on entering a signal, so we
>>> let them be changed (including PKRU).  POWER hardware does not do this
>>> to its PKRU-equivalent, so we do not force it to.
>>
>> Whuy?  Is there a technical reason not have fully-aligned behavior?
>> Can POWER at least implement the original PKEY_ALLOC_SETSIGNAL
>> semantics (reset the access rights for certain keys before switching
>> to the signal handler) in a reasonably efficient manner?
> 
> This can be done on POWER. I can also change the behavior on POWER
> to exactly match x86; i.e reset the value to init value before
> calling the signal handler.

Maybe we can implement a compromise?

Assuming I got the attached patch right, it implements PKRU inheritance 
in signal handlers, similar to what you intend to implement for POWER. 
It still restores the PKRU register value upon regular exit from the 
signal handler, which I think is something we should keep.

I think we still should add a flag, so that applications can easily 
determine if a kernel has this patch.  Setting up a signal handler, 
sending the signal, and thus checking for inheritance is a bit involved, 
and we'd have to do this in the dynamic linker before we can use pkeys 
to harden lazy binding.  The flag could just be a no-op, apart from the 
lack of an EINVAL failure if it is specified.

> But I think, we should clearly define the default behavior, the behavior
> when no flag is specified. Applications tend to rely on default behavior
> and expect the most intuitive behavior to be the default behavior.

Because this feature already shipped on x86, we already have the 
unspecified signal handler behavior in the wild, and if applications 
need the new, clearly defined semantics, there has to be a way to detect 
that the kernel makes this guarantee.

> I tend to think; keeping my biases aside, that the most intuitive
> behavior is to preserve access/write permissions of any key, i.e not
> reset to the init value.  If the application has set the permissions of
> a key to some value, it would'nt expect anyone to change them,
> irrespective of which context it is in.

Sure, it also fixes the siglongjmp issue:

   https://sourceware.org/bugzilla/show_bug.cgi?id=22396

If we do not reset the PKRU register on x86 anymore, a non-pkeys-aware 
signal handler will not clobber it.

Thanks,
Florian

[-- Attachment #2: pkeys-signal-inherit.patch --]
[-- Type: text/x-patch, Size: 3695 bytes --]

commit 148de78dc299b87ea3dcf13d73c92431f94643a0
Author: Florian Weimer <fweimer@redhat.com>
Date:   Thu Dec 14 11:12:02 2017 -0500

    x86 pkeys: Use PKRU register of interrupted thread in signal handlers
    
    pkeys support for IBM POWER intends to inherited the access rights of
    the current thread in signal handlers.  The advantage is that this
    preserves access to memory regions associated with non-default keys,
    enabling additional usage scenarios for memory protection keys which
    currently do not work due to the unconditional reset to the
    (configurable) default key in signal handlers.
    
    This change does not affect the init_pkru optimization because if the
    thread's PKRU register is zero due to the init_pkru setting, it will
    remain zero in the signal handler through inheritance.
    
    Signed-off-by: Florian Weimer <fweimer@redhat.com>

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index a38bf5a..a87e99f7 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -33,6 +33,7 @@
 extern void fpu__drop(struct fpu *fpu);
 extern int  fpu__copy(struct fpu *dst_fpu, struct fpu *src_fpu);
 extern void fpu__clear(struct fpu *fpu);
+extern void fpu__clear_signal(struct fpu *fpu);
 extern int  fpu__exception_code(struct fpu *fpu, int trap_nr);
 extern int  dump_fpu(struct pt_regs *ptregs, struct user_i387_struct *fpstate);
 
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index f92a659..a3b3048 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -370,21 +370,16 @@ static inline void copy_init_fpstate_to_fpregs(void)
 		copy_kernel_to_fxregs(&init_fpstate.fxsave);
 	else
 		copy_kernel_to_fregs(&init_fpstate.fsave);
-
-	if (boot_cpu_has(X86_FEATURE_OSPKE))
-		copy_init_pkru_to_fpregs();
 }
 
-/*
- * Clear the FPU state back to init state.
- *
- * Called by sys_execve(), by the signal handler code and by various
- * error paths.
- */
-void fpu__clear(struct fpu *fpu)
+static void __fpu_clear(struct fpu *fpu, bool for_signal)
 {
+	u32 pkru;
+
 	WARN_ON_FPU(fpu != &current->thread.fpu); /* Almost certainly an anomaly */
 
+	if (for_signal)
+		pkru = read_pkru();
 	fpu__drop(fpu);
 
 	/*
@@ -395,11 +390,43 @@ void fpu__clear(struct fpu *fpu)
 		fpu__initialize(fpu);
 		user_fpu_begin();
 		copy_init_fpstate_to_fpregs();
+		if (boot_cpu_has(X86_FEATURE_OSPKE)) {
+			/* A signal handler inherits the original PKRU
+			 * value of the interrupted thread.
+			 */
+			if (for_signal)
+				__write_pkru(pkru);
+			else
+				copy_init_pkru_to_fpregs();
+		}
 		preempt_enable();
 	}
 }
 
 /*
+ * Clear the FPU state back to init state.
+ *
+ * Called by sys_execve(), the signal handler return code, and by
+ * various error paths.
+ */
+void fpu__clear(struct fpu *fpu)
+{
+	return __fpu_clear(fpu, false);
+}
+
+/*
+ * Prepare the FPU for invoking a signal handler.
+ *
+ * This is like fpu__clear(), but some CPU registers are inherited
+ * from the current thread and not restored to their initial values,
+ * to match behavior on other architectures.
+ */
+void fpu__clear_signal(struct fpu *fpu)
+{
+	return __fpu_clear(fpu, true);
+}
+
+/*
  * x87 math exception handling:
  */
 
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index b9e00e8..4263f18 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -757,7 +757,7 @@ static inline int is_x32_frame(struct ksignal *ksig)
 		 * Ensure the signal handler starts with the new fpu state.
 		 */
 		if (fpu->initialized)
-			fpu__clear(fpu);
+			fpu__clear_signal(fpu);
 	}
 	signal_setup_done(failed, ksig, stepping);
 }

Re: pkeys: Support setting access rights for signal handlers

[Ram Pai]

On Thu, Dec 14, 2017 at 12:21:44PM +0100, Florian Weimer wrote:
> On 12/14/2017 01:17 AM, Ram Pai wrote:
> >On Wed, Dec 13, 2017 at 04:40:11PM +0100, Florian Weimer wrote:
> >>On 12/13/2017 04:22 PM, Dave Hansen wrote:
> >>>On 12/13/2017 07:08 AM, Florian Weimer wrote:
> >>>>Okay, this model is really quite different from x86.  Is there a
> >>>>good reason for the difference?
> >>>
> >>>Yes, both implementations are simple and take the "natural" behavior.
> >>>x86 changes XSAVE-controlled register values on entering a signal, so we
> >>>let them be changed (including PKRU).  POWER hardware does not do this
> >>>to its PKRU-equivalent, so we do not force it to.
> >>
> >>Whuy?  Is there a technical reason not have fully-aligned behavior?
> >>Can POWER at least implement the original PKEY_ALLOC_SETSIGNAL
> >>semantics (reset the access rights for certain keys before switching
> >>to the signal handler) in a reasonably efficient manner?
> >
> >This can be done on POWER. I can also change the behavior on POWER
> >to exactly match x86; i.e reset the value to init value before
> >calling the signal handler.
> 
> Maybe we can implement a compromise?
> 
> Assuming I got the attached patch right, it implements PKRU
> inheritance in signal handlers, similar to what you intend to
> implement for POWER.

Ok.

> It still restores the PKRU register value upon
> regular exit from the signal handler, which I think is something we
> should keep.

On x86, the pkru value is restored, on return from the signal handler,
to the value before the signal handler was called. right?

In other words, if 'x' was the value when signal handler was called, it
will be 'x' when return from the signal handler.

If correct, than it is consistent with the behavior on POWER.

> 
> I think we still should add a flag, so that applications can easily
> determine if a kernel has this patch.  Setting up a signal handler,
> sending the signal, and thus checking for inheritance is a bit
> involved, and we'd have to do this in the dynamic linker before we
> can use pkeys to harden lazy binding.  The flag could just be a
> no-op, apart from the lack of an EINVAL failure if it is specified.

Sorry. I am little confused.  What should I implement on POWER? 
PKEY_ALLOC_SETSIGNAL semantics?

Let me know. Thanks for driving this to some consistency.
RP

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: pkeys: Support setting access rights for signal handlers

[Ram Pai]

On Thu, Dec 14, 2017 at 12:21:44PM +0100, Florian Weimer wrote:
> On 12/14/2017 01:17 AM, Ram Pai wrote:
> >On Wed, Dec 13, 2017 at 04:40:11PM +0100, Florian Weimer wrote:
> >>On 12/13/2017 04:22 PM, Dave Hansen wrote:
> >>>On 12/13/2017 07:08 AM, Florian Weimer wrote:
> >>>>Okay, this model is really quite different from x86.  Is there a
> >>>>good reason for the difference?
> >>>
> >>>Yes, both implementations are simple and take the "natural" behavior.
> >>>x86 changes XSAVE-controlled register values on entering a signal, so we
> >>>let them be changed (including PKRU).  POWER hardware does not do this
> >>>to its PKRU-equivalent, so we do not force it to.
> >>
> >>Whuy?  Is there a technical reason not have fully-aligned behavior?
> >>Can POWER at least implement the original PKEY_ALLOC_SETSIGNAL
> >>semantics (reset the access rights for certain keys before switching
> >>to the signal handler) in a reasonably efficient manner?
> >
> >This can be done on POWER. I can also change the behavior on POWER
> >to exactly match x86; i.e reset the value to init value before
> >calling the signal handler.
> 
> Maybe we can implement a compromise?
> 
> Assuming I got the attached patch right, it implements PKRU
> inheritance in signal handlers, similar to what you intend to
> implement for POWER.

Ok.

> It still restores the PKRU register value upon
> regular exit from the signal handler, which I think is something we
> should keep.

On x86, the pkru value is restored, on return from the signal handler,
to the value before the signal handler was called. right?

In other words, if 'x' was the value when signal handler was called, it
will be 'x' when return from the signal handler.

If correct, than it is consistent with the behavior on POWER.

> 
> I think we still should add a flag, so that applications can easily
> determine if a kernel has this patch.  Setting up a signal handler,
> sending the signal, and thus checking for inheritance is a bit
> involved, and we'd have to do this in the dynamic linker before we
> can use pkeys to harden lazy binding.  The flag could just be a
> no-op, apart from the lack of an EINVAL failure if it is specified.

Sorry. I am little confused.  What should I implement on POWER? 
PKEY_ALLOC_SETSIGNAL semantics?

Let me know. Thanks for driving this to some consistency.
RP

Re: pkeys: Support setting access rights for signal handlers

[Florian Weimer]

On 12/16/2017 04:09 PM, Ram Pai wrote:

>> It still restores the PKRU register value upon
>> regular exit from the signal handler, which I think is something we
>> should keep.
> 
> On x86, the pkru value is restored, on return from the signal handler,
> to the value before the signal handler was called. right?
> 
> In other words, if 'x' was the value when signal handler was called, it
> will be 'x' when return from the signal handler.
> 
> If correct, than it is consistent with the behavior on POWER.

That's good to know.  I tended to implement the same semantics on x86.

>> I think we still should add a flag, so that applications can easily
>> determine if a kernel has this patch.  Setting up a signal handler,
>> sending the signal, and thus checking for inheritance is a bit
>> involved, and we'd have to do this in the dynamic linker before we
>> can use pkeys to harden lazy binding.  The flag could just be a
>> no-op, apart from the lack of an EINVAL failure if it is specified.
> 
> Sorry. I am little confused.  What should I implement on POWER?
> PKEY_ALLOC_SETSIGNAL semantics?

No, we would add a flag, with a different name, and this patch only:

diff --git a/mm/mprotect.c b/mm/mprotect.c
index ec39f73..021f1d4 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -523,14 +523,17 @@ static int do_mprotect_pkey(unsigned long start, 
size_t l
         return do_mprotect_pkey(start, len, prot, pkey);
  }

+#define PKEY_ALLOC_FLAGS ((unsigned long) (PKEY_ALLOC_SETSIGNAL))
+
  SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
  {
         int pkey;
         int ret;

-       /* No flags supported yet. */
-       if (flags)
+       /* check for unsupported flags */
+       if (flags & ~PKEY_ALLOC_FLAGS)
                 return -EINVAL;
+
         /* check for unsupported init values */
         if (init_val & ~PKEY_ACCESS_MASK)
                 return -EINVAL;


This way, an application can specify the flag during key allocation, and 
knows that if the allocation succeeds, the kernel implements access 
rights inheritance in signal handlers.  I think we need this so that 
applications which are incompatible with the earlier x86 implementation 
of memory protection keys do not use them.

With my second patch (not the first one implementing 
PKEY_ALLOC_SETSIGNAL), no further changes to architecture=specific code 
are needed, except for the definition of the flag in the header files.

I'm open to a different way towards conveying this information to 
userspace.  I don't want to probe for the behavior by sending a signal 
because that is quite involved and would also be visible in debuggers, 
confusing programmers.

Thanks,
Florian

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: pkeys: Support setting access rights for signal handlers

[Florian Weimer]

On 12/16/2017 04:09 PM, Ram Pai wrote:

>> It still restores the PKRU register value upon
>> regular exit from the signal handler, which I think is something we
>> should keep.
> 
> On x86, the pkru value is restored, on return from the signal handler,
> to the value before the signal handler was called. right?
> 
> In other words, if 'x' was the value when signal handler was called, it
> will be 'x' when return from the signal handler.
> 
> If correct, than it is consistent with the behavior on POWER.

That's good to know.  I tended to implement the same semantics on x86.

>> I think we still should add a flag, so that applications can easily
>> determine if a kernel has this patch.  Setting up a signal handler,
>> sending the signal, and thus checking for inheritance is a bit
>> involved, and we'd have to do this in the dynamic linker before we
>> can use pkeys to harden lazy binding.  The flag could just be a
>> no-op, apart from the lack of an EINVAL failure if it is specified.
> 
> Sorry. I am little confused.  What should I implement on POWER?
> PKEY_ALLOC_SETSIGNAL semantics?

No, we would add a flag, with a different name, and this patch only:

diff --git a/mm/mprotect.c b/mm/mprotect.c
index ec39f73..021f1d4 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -523,14 +523,17 @@ static int do_mprotect_pkey(unsigned long start, 
size_t l
         return do_mprotect_pkey(start, len, prot, pkey);
  }

+#define PKEY_ALLOC_FLAGS ((unsigned long) (PKEY_ALLOC_SETSIGNAL))
+
  SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
  {
         int pkey;
         int ret;

-       /* No flags supported yet. */
-       if (flags)
+       /* check for unsupported flags */
+       if (flags & ~PKEY_ALLOC_FLAGS)
                 return -EINVAL;
+
         /* check for unsupported init values */
         if (init_val & ~PKEY_ACCESS_MASK)
                 return -EINVAL;


This way, an application can specify the flag during key allocation, and 
knows that if the allocation succeeds, the kernel implements access 
rights inheritance in signal handlers.  I think we need this so that 
applications which are incompatible with the earlier x86 implementation 
of memory protection keys do not use them.

With my second patch (not the first one implementing 
PKEY_ALLOC_SETSIGNAL), no further changes to architecture=specific code 
are needed, except for the definition of the flag in the header files.

I'm open to a different way towards conveying this information to 
userspace.  I don't want to probe for the behavior by sending a signal 
because that is quite involved and would also be visible in debuggers, 
confusing programmers.

Thanks,
Florian

Re: pkeys: Support setting access rights for signal handlers

[Ram Pai]

On Sat, Dec 16, 2017 at 04:25:14PM +0100, Florian Weimer wrote:
> On 12/16/2017 04:09 PM, Ram Pai wrote:
> 
> >>It still restores the PKRU register value upon
> >>regular exit from the signal handler, which I think is something we
> >>should keep.
> >
> >On x86, the pkru value is restored, on return from the signal handler,
> >to the value before the signal handler was called. right?
> >
> >In other words, if 'x' was the value when signal handler was called, it
> >will be 'x' when return from the signal handler.
> >
> >If correct, than it is consistent with the behavior on POWER.
> 
> That's good to know.  I tended to implement the same semantics on x86.
> 
> >>I think we still should add a flag, so that applications can easily
> >>determine if a kernel has this patch.  Setting up a signal handler,
> >>sending the signal, and thus checking for inheritance is a bit
> >>involved, and we'd have to do this in the dynamic linker before we
> >>can use pkeys to harden lazy binding.  The flag could just be a
> >>no-op, apart from the lack of an EINVAL failure if it is specified.
> >
> >Sorry. I am little confused.  What should I implement on POWER?
> >PKEY_ALLOC_SETSIGNAL semantics?
> 
> No, we would add a flag, with a different name, and this patch only:
> 
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index ec39f73..021f1d4 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -523,14 +523,17 @@ static int do_mprotect_pkey(unsigned long
> start, size_t l
>         return do_mprotect_pkey(start, len, prot, pkey);
>  }
> 
> +#define PKEY_ALLOC_FLAGS ((unsigned long) (PKEY_ALLOC_SETSIGNAL))
> +
>  SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
>  {
>         int pkey;
>         int ret;
> 
> -       /* No flags supported yet. */
> -       if (flags)
> +       /* check for unsupported flags */
> +       if (flags & ~PKEY_ALLOC_FLAGS)
>                 return -EINVAL;
> +
>         /* check for unsupported init values */
>         if (init_val & ~PKEY_ACCESS_MASK)
>                 return -EINVAL;
> 
> 
> This way, an application can specify the flag during key allocation,
> and knows that if the allocation succeeds, the kernel implements
> access rights inheritance in signal handlers.  I think we need this
> so that applications which are incompatible with the earlier x86
> implementation of memory protection keys do not use them.
> 
> With my second patch (not the first one implementing
> PKEY_ALLOC_SETSIGNAL), no further changes to architecture=specific
> code are needed, except for the definition of the flag in the header
> files.

Ok. Sounds like I do not have much to do. My patches in its current form
will continue to work and provide the semantics you envision.


> 
> I'm open to a different way towards conveying this information to
> userspace.  I don't want to probe for the behavior by sending a
> signal because that is quite involved and would also be visible in
> debuggers, confusing programmers.

I am fine with your proposal.
RP

Re: pkeys: Support setting access rights for signal handlers

[Ram Pai]

On Sat, Dec 16, 2017 at 04:25:14PM +0100, Florian Weimer wrote:
> On 12/16/2017 04:09 PM, Ram Pai wrote:
> 
> >>It still restores the PKRU register value upon
> >>regular exit from the signal handler, which I think is something we
> >>should keep.
> >
> >On x86, the pkru value is restored, on return from the signal handler,
> >to the value before the signal handler was called. right?
> >
> >In other words, if 'x' was the value when signal handler was called, it
> >will be 'x' when return from the signal handler.
> >
> >If correct, than it is consistent with the behavior on POWER.
> 
> That's good to know.  I tended to implement the same semantics on x86.
> 
> >>I think we still should add a flag, so that applications can easily
> >>determine if a kernel has this patch.  Setting up a signal handler,
> >>sending the signal, and thus checking for inheritance is a bit
> >>involved, and we'd have to do this in the dynamic linker before we
> >>can use pkeys to harden lazy binding.  The flag could just be a
> >>no-op, apart from the lack of an EINVAL failure if it is specified.
> >
> >Sorry. I am little confused.  What should I implement on POWER?
> >PKEY_ALLOC_SETSIGNAL semantics?
> 
> No, we would add a flag, with a different name, and this patch only:
> 
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index ec39f73..021f1d4 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -523,14 +523,17 @@ static int do_mprotect_pkey(unsigned long
> start, size_t l
>         return do_mprotect_pkey(start, len, prot, pkey);
>  }
> 
> +#define PKEY_ALLOC_FLAGS ((unsigned long) (PKEY_ALLOC_SETSIGNAL))
> +
>  SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
>  {
>         int pkey;
>         int ret;
> 
> -       /* No flags supported yet. */
> -       if (flags)
> +       /* check for unsupported flags */
> +       if (flags & ~PKEY_ALLOC_FLAGS)
>                 return -EINVAL;
> +
>         /* check for unsupported init values */
>         if (init_val & ~PKEY_ACCESS_MASK)
>                 return -EINVAL;
> 
> 
> This way, an application can specify the flag during key allocation,
> and knows that if the allocation succeeds, the kernel implements
> access rights inheritance in signal handlers.  I think we need this
> so that applications which are incompatible with the earlier x86
> implementation of memory protection keys do not use them.
> 
> With my second patch (not the first one implementing
> PKEY_ALLOC_SETSIGNAL), no further changes to architecture=specific
> code are needed, except for the definition of the flag in the header
> files.

Ok. Sounds like I do not have much to do. My patches in its current form
will continue to work and provide the semantics you envision.


> 
> I'm open to a different way towards conveying this information to
> userspace.  I don't want to probe for the behavior by sending a
> signal because that is quite involved and would also be visible in
> debuggers, confusing programmers.

I am fine with your proposal.
RP

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: pkeys: Support setting access rights for signal handlers

[Ram Pai]

On Sat, Dec 16, 2017 at 04:25:14PM +0100, Florian Weimer wrote:
> On 12/16/2017 04:09 PM, Ram Pai wrote:
> 
> >>It still restores the PKRU register value upon
> >>regular exit from the signal handler, which I think is something we
> >>should keep.
> >
> >On x86, the pkru value is restored, on return from the signal handler,
> >to the value before the signal handler was called. right?
> >
> >In other words, if 'x' was the value when signal handler was called, it
> >will be 'x' when return from the signal handler.
> >
> >If correct, than it is consistent with the behavior on POWER.
> 
> That's good to know.  I tended to implement the same semantics on x86.
> 
> >>I think we still should add a flag, so that applications can easily
> >>determine if a kernel has this patch.  Setting up a signal handler,
> >>sending the signal, and thus checking for inheritance is a bit
> >>involved, and we'd have to do this in the dynamic linker before we
> >>can use pkeys to harden lazy binding.  The flag could just be a
> >>no-op, apart from the lack of an EINVAL failure if it is specified.
> >
> >Sorry. I am little confused.  What should I implement on POWER?
> >PKEY_ALLOC_SETSIGNAL semantics?
> 
> No, we would add a flag, with a different name, and this patch only:
> 
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index ec39f73..021f1d4 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -523,14 +523,17 @@ static int do_mprotect_pkey(unsigned long
> start, size_t l
>         return do_mprotect_pkey(start, len, prot, pkey);
>  }
> 
> +#define PKEY_ALLOC_FLAGS ((unsigned long) (PKEY_ALLOC_SETSIGNAL))
> +
>  SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
>  {
>         int pkey;
>         int ret;
> 
> -       /* No flags supported yet. */
> -       if (flags)
> +       /* check for unsupported flags */
> +       if (flags & ~PKEY_ALLOC_FLAGS)
>                 return -EINVAL;
> +
>         /* check for unsupported init values */
>         if (init_val & ~PKEY_ACCESS_MASK)
>                 return -EINVAL;
> 
> 
> This way, an application can specify the flag during key allocation,
> and knows that if the allocation succeeds, the kernel implements
> access rights inheritance in signal handlers.  I think we need this
> so that applications which are incompatible with the earlier x86
> implementation of memory protection keys do not use them.
> 
> With my second patch (not the first one implementing
> PKEY_ALLOC_SETSIGNAL), no further changes to architecture=specific
> code are needed, except for the definition of the flag in the header
> files.

Ok. Sounds like I do not have much to do. My patches in its current form
will continue to work and provide the semantics you envision.


> 
> I'm open to a different way towards conveying this information to
> userspace.  I don't want to probe for the behavior by sending a
> signal because that is quite involved and would also be visible in
> debuggers, confusing programmers.

I am fine with your proposal.
RP

Re: pkeys: Support setting access rights for signal handlers

[Florian Weimer]

On 12/16/2017 06:20 PM, Ram Pai wrote:
> Ok. Sounds like I do not have much to do. My patches in its current form
> will continue to work and provide the semantics you envision.

Thanks for confirming.

>> I'm open to a different way towards conveying this information to
>> userspace.  I don't want to probe for the behavior by sending a
>> signal because that is quite involved and would also be visible in
>> debuggers, confusing programmers.

> I am fine with your proposal.

So how can we move this forward?  Should I submit a single new patch 
with the new flag with a more appropriate name (PKEY_ALLOC_SIGNALINHERIT 
comes to my mind) and the signal inheritance change?

Dave, do you still want to wait for feedback from the x86 maintainer 
regarding a general interface?  Is this really feasible without detailed 
knowledge of the XSAVE output structure?  Otherwise, there probably 
isn't a way around code which explicitly copies the bits we want to 
preserve from the interrupted CPU context to the signal handler context.

Thanks,
Florian

Re: pkeys: Support setting access rights for signal handlers

[Florian Weimer]

On 12/16/2017 06:20 PM, Ram Pai wrote:
> Ok. Sounds like I do not have much to do. My patches in its current form
> will continue to work and provide the semantics you envision.

Thanks for confirming.

>> I'm open to a different way towards conveying this information to
>> userspace.  I don't want to probe for the behavior by sending a
>> signal because that is quite involved and would also be visible in
>> debuggers, confusing programmers.

> I am fine with your proposal.

So how can we move this forward?  Should I submit a single new patch 
with the new flag with a more appropriate name (PKEY_ALLOC_SIGNALINHERIT 
comes to my mind) and the signal inheritance change?

Dave, do you still want to wait for feedback from the x86 maintainer 
regarding a general interface?  Is this really feasible without detailed 
knowledge of the XSAVE output structure?  Otherwise, there probably 
isn't a way around code which explicitly copies the bits we want to 
preserve from the interrupted CPU context to the signal handler context.

Thanks,
Florian

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>