Re: [PATCH 23/25] powerpc: Deliver SEGV signal on pkey violation

On Tue, Oct 24, 2017 at 05:46:38PM +0200, Michael Ellerman wrote:

Ram Pai [off-list ref] writes:

quoted

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index ec74e20..f2a310d 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c

@@ -265,6 +266,15 @@ void user_single_step_siginfo(struct task_struct *tsk,
 	info->si_addr = (void __user *)regs->nip;
 }
 
+#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
+static void fill_sig_info_pkey(int si_code, siginfo_t *info, unsigned long addr)
+{
+	if (info->si_signo != SIGSEGV || si_code != SEGV_PKUERR)

Just checking si_code is sufficient there I think.

quoted

+		return;
+	info->si_pkey = get_paca()->paca_pkey;
+}
+#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */

This should define an empty version in the #else case, so we don't need
the ifdef below.

quoted

@@ -292,6 +302,18 @@ void _exception(int signr, struct pt_regs *regs, int code, unsigned long addr)
 	info.si_signo = signr;
 	info.si_code = code;
 	info.si_addr = (void __user *) addr;
+
+#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
+	/*
+	 * update the thread's pkey related fields.
+	 * core-dump handlers and other sub-systems
+	 * depend on those values.
+	 */
+	thread_pkey_regs_save(&current->thread);

You shouldn't need to do this.

We're not putting any of the pkey regs in the signal frame, so you don't
need to save before we do that. [And if you did the right place to do it
would be in setup_sigcontext() (or the TM version).]

One of the comments in the past was the ability to capture AMR
register status in core-dumps and also the ability to modify the
AMR through ptrace. Thiago wrote a patch towards that. That patch
is one of the subsequent patches sent in this series.

I will move the code to pkey_get().

For ptrace and coredumps it should happen in pkey_get(), see eg.
fpr_get() which does flush_fp_to_thread() as an example.

Ok.

quoted

+	/* update the violated-key value */
+	fill_sig_info_pkey(code, &info, addr);
+#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */

quoted

+
 	force_sig_info(signr, &info, current);
 }

cheers

-- 
Ram Pai