[REVIEW][PATCH 00/20] siginfo cleanups for x86

[REVIEW][PATCH 00/20] siginfo cleanups for x86

[Eric W. Biederman]

I have been slowly going thought and reworking the arch specific
functions that generate siginfo.  The problems I have been addressing
is that using siginfo directly is error prone.  Using siginfo directly
makes it easy to leave fields initialized, and get confused about
which fields need to be filled in.

To address this I have added a series of helper functions to
kernel/signal.c, that are specific to exactly one use of struct siginfo
and take the parameters they need.

To use these functions the x86 signal handling needs some cleanups but
the net result appears to be less code that is easier to follow.

If while looking over these patches you see anything please let me know.
I don't think I missed something but to err is human.

Likewise if you would like to merge these patches via the tip tree
let me know.  Otherwise after the review is complete I plan on merging
these into my siginfo tree.  At this point I believe all of the
prerequisite patches are merged so it should not make a difference.

Eric W. Biederman (20):
      signal: Simplify tracehook_report_syscall_exit
      signal/x86: Inline fill_sigtrap_info in it's only caller send_sigtrap
      signal/x86: Move MCE error reporting out of force_sig_info_fault
      signal/x86: Use send_sig_mceerr as apropriate
      signal/x86: In trace_mpx_bounds_register_exception add __user annotations
      signal/x86: Move mpx siginfo generation into do_bounds
      signal/x86/traps: Factor out show_signal
      signal/x86/traps: Move setting error_code and trap_nr into do_trap_no_signal
      signal/x86/traps: Use force_sig_bnderr
      signal/x86/traps: Use force_sig instead of open coding it.
      signal/x86/traps: Simplify trap generation
      signal/x86: Remove pkey parameter from bad_area_nosemaphore
      signal/x86: Remove the pkey parameter from do_sigbus
      signal/x86: Remove pkey parameter from mm_fault_error
      signal/x86: Don't compute pkey in __do_page_fault
      signal/x86: Pass pkey not vma into __bad_area
      signal/x86: Call force_sig_pkuerr from __bad_area_nosemaphore
      signal/x86: Replace force_sig_info_fault with force_sig_fault
      signal/x86: Pass pkey by value
      signal/x86: Use force_sig_fault where appropriate

 arch/powerpc/include/asm/ptrace.h     |   2 +-
 arch/powerpc/kernel/traps.c           |   7 +-
 arch/x86/entry/vsyscall/vsyscall_64.c |   9 +-
 arch/x86/include/asm/mpx.h            |  13 ++-
 arch/x86/include/asm/ptrace.h         |   2 +-
 arch/x86/include/asm/trace/mpx.h      |   4 +-
 arch/x86/kernel/ptrace.c              |  29 ++----
 arch/x86/kernel/traps.c               | 175 ++++++++++++++--------------------
 arch/x86/kernel/umip.c                |   8 +-
 arch/x86/kvm/mmu.c                    |  11 +--
 arch/x86/mm/fault.c                   | 161 +++++++++++--------------------
 arch/x86/mm/mpx.c                     |  30 ++----
 include/linux/ptrace.h                |  17 ++--
 include/linux/tracehook.h             |  13 +--
 14 files changed, 175 insertions(+), 306 deletions(-)

Eric

[REVIEW][PATCH 01/20] signal: Simplify tracehook_report_syscall_exit

[Eric W. Biederman]

Replace user_single_step_siginfo with user_single_step_report
that allocates siginfo structure on the stack and sends it.

This allows tracehook_report_syscall_exit to become a simple
if statement that calls user_single_step_report or ptrace_report_syscall
depending on the value of step.

Update the default helper function now called user_single_step_report
to explicitly set si_code to SI_USER and to set si_uid and si_pid to 0.
The default helper has always been doing this (using memset) but it
was far from obvious.

The powerpc helper can now just call force_sig_fault.
The x86 helper can now just call send_sigtrap.

Unfortunately the default implementation of user_single_step_report
can not use force_sig_fault as it does not use a SIGTRAP si_code.
So it has to carefully setup the siginfo and use use force_sig_info.

The net result is code that is easier to understand and simpler
to maintain.

Ref: 85ec7fd9f8e5 ("ptrace: introduce user_single_step_siginfo() helper")
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/powerpc/include/asm/ptrace.h |  2 +-
 arch/powerpc/kernel/traps.c       |  7 ++-----
 arch/x86/include/asm/ptrace.h     |  2 +-
 arch/x86/kernel/ptrace.c          | 11 +++++------
 include/linux/ptrace.h            | 17 +++++++++++------
 include/linux/tracehook.h         | 13 ++++---------
 6 files changed, 24 insertions(+), 28 deletions(-)

diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 447cbd1bee99..5b480e1d5909 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -149,7 +149,7 @@ do {									      \
 
 #define arch_has_single_step()	(1)
 #define arch_has_block_step()	(!cpu_has_feature(CPU_FTR_601))
-#define ARCH_HAS_USER_SINGLE_STEP_INFO
+#define ARCH_HAS_USER_SINGLE_STEP_REPORT
 
 /*
  * kprobe-based event tracer support
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index c85adb858271..f651fa91cdc9 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -307,12 +307,9 @@ void die(const char *str, struct pt_regs *regs, long err)
 }
 NOKPROBE_SYMBOL(die);
 
-void user_single_step_siginfo(struct task_struct *tsk,
-				struct pt_regs *regs, siginfo_t *info)
+void user_single_step_report(struct pt_regs *regs)
 {
-	info->si_signo = SIGTRAP;
-	info->si_code = TRAP_TRACE;
-	info->si_addr = (void __user *)regs->nip;
+	force_sig_fault(SIGTRAP, TRAP_TRACE, (void __user *)regs->nip, current);
 }
 
 static void show_signal_msg(int signr, struct pt_regs *regs, int code,
diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
index 6de1fd3d0097..e353f08b7fe2 100644
--- a/arch/x86/include/asm/ptrace.h
+++ b/arch/x86/include/asm/ptrace.h
@@ -263,7 +263,7 @@ static inline unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs,
 #define arch_has_block_step()	(boot_cpu_data.x86 >= 6)
 #endif
 
-#define ARCH_HAS_USER_SINGLE_STEP_INFO
+#define ARCH_HAS_USER_SINGLE_STEP_REPORT
 
 /*
  * When hitting ptrace_stop(), we cannot return using SYSRET because
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index e2ee403865eb..94bd6e89129a 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -1382,12 +1382,6 @@ static void fill_sigtrap_info(struct task_struct *tsk,
 	info->si_addr = user_mode(regs) ? (void __user *)regs->ip : NULL;
 }
 
-void user_single_step_siginfo(struct task_struct *tsk,
-				struct pt_regs *regs,
-				struct siginfo *info)
-{
-	fill_sigtrap_info(tsk, regs, 0, TRAP_BRKPT, info);
-}
 
 void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs,
 					 int error_code, int si_code)
@@ -1399,3 +1393,8 @@ void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs,
 	/* Send us the fake SIGTRAP */
 	force_sig_info(SIGTRAP, &info, tsk);
 }
+
+void user_single_step_report(struct pt_regs *regs)
+{
+	send_sigtrap(current, regs, 0, TRAP_BRKPT);
+}
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 4f36431c380b..1de2235511c8 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -336,14 +336,19 @@ static inline void user_enable_block_step(struct task_struct *task)
 extern void user_enable_block_step(struct task_struct *);
 #endif	/* arch_has_block_step */
 
-#ifdef ARCH_HAS_USER_SINGLE_STEP_INFO
-extern void user_single_step_siginfo(struct task_struct *tsk,
-				struct pt_regs *regs, siginfo_t *info);
+#ifdef ARCH_HAS_USER_SINGLE_STEP_REPORT
+extern void user_single_step_report(struct pt_regs *regs);
 #else
-static inline void user_single_step_siginfo(struct task_struct *tsk,
-				struct pt_regs *regs, siginfo_t *info)
+static inline void user_single_step_report(struct pt_regs *regs)
 {
-	info->si_signo = SIGTRAP;
+	siginfo_t info;
+	clear_siginfo(&info);
+	info.si_signo = SIGTRAP;
+	info.si_errno = 0;
+	info.si_code = SI_USER;
+	info.si_pid = 0;
+	info.si_uid = 0;
+	force_sig_info(info.si_signo, &info, current);
 }
 #endif
 
diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
index 05589a3e37f4..40b0b4c1bf7b 100644
--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -123,15 +123,10 @@ static inline __must_check int tracehook_report_syscall_entry(
  */
 static inline void tracehook_report_syscall_exit(struct pt_regs *regs, int step)
 {
-	if (step) {
-		siginfo_t info;
-		clear_siginfo(&info);
-		user_single_step_siginfo(current, regs, &info);
-		force_sig_info(SIGTRAP, &info, current);
-		return;
-	}
-
-	ptrace_report_syscall(regs);
+	if (step)
+		user_single_step_report(regs);
+	else
+		ptrace_report_syscall(regs);
 }
 
 /**
-- 
2.17.1

Re: [REVIEW][PATCH 01/20] signal: Simplify tracehook_report_syscall_exit

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> Replace user_single_step_siginfo with user_single_step_report
> that allocates siginfo structure on the stack and sends it.
> 
> This allows tracehook_report_syscall_exit to become a simple
> if statement that calls user_single_step_report or ptrace_report_syscall
> depending on the value of step.
> 
> Update the default helper function now called user_single_step_report
> to explicitly set si_code to SI_USER and to set si_uid and si_pid to 0.
> The default helper has always been doing this (using memset) but it
> was far from obvious.
> 
> The powerpc helper can now just call force_sig_fault.
> The x86 helper can now just call send_sigtrap.
> 
> Unfortunately the default implementation of user_single_step_report
> can not use force_sig_fault as it does not use a SIGTRAP si_code.
> So it has to carefully setup the siginfo and use use force_sig_info.
> 
> The net result is code that is easier to understand and simpler
> to maintain.
> 
> Ref: 85ec7fd9f8e5 ("ptrace: introduce user_single_step_siginfo() helper")
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[REVIEW][PATCH 02/20] signal/x86: Inline fill_sigtrap_info in it's only caller send_sigtrap

[Eric W. Biederman]

The function fill_sigtrap_info now only has one caller so remove
it and put it's contents in it's caller.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/kernel/ptrace.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 94bd6e89129a..511ea0f16078 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -1369,27 +1369,19 @@ const struct user_regset_view *task_user_regset_view(struct task_struct *task)
 #endif
 }
 
-static void fill_sigtrap_info(struct task_struct *tsk,
-				struct pt_regs *regs,
-				int error_code, int si_code,
-				struct siginfo *info)
-{
-	tsk->thread.trap_nr = X86_TRAP_DB;
-	tsk->thread.error_code = error_code;
-
-	info->si_signo = SIGTRAP;
-	info->si_code = si_code;
-	info->si_addr = user_mode(regs) ? (void __user *)regs->ip : NULL;
-}
-
-
 void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs,
 					 int error_code, int si_code)
 {
 	struct siginfo info;
 
 	clear_siginfo(&info);
-	fill_sigtrap_info(tsk, regs, error_code, si_code, &info);
+	tsk->thread.trap_nr = X86_TRAP_DB;
+	tsk->thread.error_code = error_code;
+
+	info.si_signo = SIGTRAP;
+	info.si_code = si_code;
+	info.si_addr = user_mode(regs) ? (void __user *)regs->ip : NULL;
+
 	/* Send us the fake SIGTRAP */
 	force_sig_info(SIGTRAP, &info, tsk);
 }
-- 
2.17.1

Re: [REVIEW][PATCH 02/20] signal/x86: Inline fill_sigtrap_info in it's only caller send_sigtrap

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> The function fill_sigtrap_info now only has one caller so remove
> it and put it's contents in it's caller.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [REVIEW][PATCH 02/20] signal/x86: Inline fill_sigtrap_info in it's only caller send_sigtrap

[Christoph Hellwig]

>  
>  	clear_siginfo(&info);
> -	fill_sigtrap_info(tsk, regs, error_code, si_code, &info);
> +	tsk->thread.trap_nr = X86_TRAP_DB;
> +	tsk->thread.error_code = error_code;
> +
> +	info.si_signo = SIGTRAP;
> +	info.si_code = si_code;
> +	info.si_addr = user_mode(regs) ? (void __user *)regs->ip : NULL;

clear_siginfo already zeroes the whole structure, so this could be
written more clearly as:

	if (user_mode(regs)
		info.si_addr = (void __user *)regs->ip;

Re: [REVIEW][PATCH 02/20] signal/x86: Inline fill_sigtrap_info in it's only caller send_sigtrap

[Eric W. Biederman]

Christoph Hellwig <hch@infradead.org> writes:

>>  
>>  	clear_siginfo(&info);
>> -	fill_sigtrap_info(tsk, regs, error_code, si_code, &info);
>> +	tsk->thread.trap_nr = X86_TRAP_DB;
>> +	tsk->thread.error_code = error_code;
>> +
>> +	info.si_signo = SIGTRAP;
>> +	info.si_code = si_code;
>> +	info.si_addr = user_mode(regs) ? (void __user *)regs->ip : NULL;
>
> clear_siginfo already zeroes the whole structure, so this could be
> written more clearly as:
>
> 	if (user_mode(regs)
> 		info.si_addr = (void __user *)regs->ip;

That change does not make sense in this particular patch as it is just
code motion.

It also does not make sense in the final version of the code at
the end of the patch series which is:

void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs,
					 int error_code, int si_code)
{
	tsk->thread.trap_nr = X86_TRAP_DB;
	tsk->thread.error_code = error_code;

	/* Send us the fake SIGTRAP */
	force_sig_fault(SIGTRAP, si_code,
			user_mode(regs) ? (void __user *)regs->ip : NULL, tsk);
}

In this version the test also makes sense because struct siginfo is
gone because manually filling it out results in more bugs than
necessary.  That is now left up to force_sig_fault.

I was hoping that we could show that user_mode(regs) is always true.
But according to arch/x86/kernel/traps.c:do_debug watch points that will
trigger even when the kernel accesses user space addresses.

Eric

[REVIEW][PATCH 03/20] signal/x86: Move MCE error reporting out of force_sig_info_fault

[Eric W. Biederman]

Only the call from do_sigbus will send SIGBUS due to a memory machine
check error.  Consolidate all of the machine check signal generation
code in do_sigbus and remove the now unnecessary fault parameter from
force_sig_info_fault.

Explicitly use the now constant si_code BUS_ADRERR in the call
to force_sig_info_fault from do_sigbus.

This makes the code in arch/x86/mm/fault.c easier to follower and
simpler to maintain.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/mm/fault.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index b9123c497e0a..4112b05d0dec 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -205,9 +205,8 @@ static void fill_sig_info_pkey(int si_signo, int si_code, siginfo_t *info,
 
 static void
 force_sig_info_fault(int si_signo, int si_code, unsigned long address,
-		     struct task_struct *tsk, u32 *pkey, int fault)
+		     struct task_struct *tsk, u32 *pkey)
 {
-	unsigned lsb = 0;
 	siginfo_t info;
 
 	clear_siginfo(&info);
@@ -215,11 +214,6 @@ force_sig_info_fault(int si_signo, int si_code, unsigned long address,
 	info.si_errno	= 0;
 	info.si_code	= si_code;
 	info.si_addr	= (void __user *)address;
-	if (fault & VM_FAULT_HWPOISON_LARGE)
-		lsb = hstate_index_to_shift(VM_FAULT_GET_HINDEX(fault)); 
-	if (fault & VM_FAULT_HWPOISON)
-		lsb = PAGE_SHIFT;
-	info.si_addr_lsb = lsb;
 
 	fill_sig_info_pkey(si_signo, si_code, &info, pkey);
 
@@ -731,7 +725,7 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 
 			/* XXX: hwpoison faults will set the wrong code. */
 			force_sig_info_fault(signal, si_code, address,
-					     tsk, NULL, 0);
+					     tsk, NULL);
 		}
 
 		/*
@@ -890,7 +884,7 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 		tsk->thread.error_code	= error_code;
 		tsk->thread.trap_nr	= X86_TRAP_PF;
 
-		force_sig_info_fault(SIGSEGV, si_code, address, tsk, pkey, 0);
+		force_sig_info_fault(SIGSEGV, si_code, address, tsk, pkey);
 
 		return;
 	}
@@ -971,7 +965,6 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
 	  u32 *pkey, unsigned int fault)
 {
 	struct task_struct *tsk = current;
-	int code = BUS_ADRERR;
 
 	/* Kernel mode? Handle exceptions or die: */
 	if (!(error_code & X86_PF_USER)) {
@@ -989,13 +982,19 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
 
 #ifdef CONFIG_MEMORY_FAILURE
 	if (fault & (VM_FAULT_HWPOISON|VM_FAULT_HWPOISON_LARGE)) {
+		unsigned lsb = 0;
 		printk(KERN_ERR
 	"MCE: Killing %s:%d due to hardware memory corruption fault at %lx\n",
 			tsk->comm, tsk->pid, address);
-		code = BUS_MCEERR_AR;
+		if (fault & VM_FAULT_HWPOISON_LARGE)
+			lsb = hstate_index_to_shift(VM_FAULT_GET_HINDEX(fault));
+		if (fault & VM_FAULT_HWPOISON)
+			lsb = PAGE_SHIFT;
+		force_sig_mceerr(BUS_MCEERR_AR, (void __user *)address, lsb, tsk);
+		return;
 	}
 #endif
-	force_sig_info_fault(SIGBUS, code, address, tsk, pkey, fault);
+	force_sig_info_fault(SIGBUS, BUS_ADRERR, address, tsk, pkey);
 }
 
 static noinline void
-- 
2.17.1

Re: [REVIEW][PATCH 03/20] signal/x86: Move MCE error reporting out of force_sig_info_fault

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:
>  #ifdef CONFIG_MEMORY_FAILURE
>  	if (fault & (VM_FAULT_HWPOISON|VM_FAULT_HWPOISON_LARGE)) {
> +		unsigned lsb = 0;

Newline between variable declaration and code please.

>  		printk(KERN_ERR
>  	"MCE: Killing %s:%d due to hardware memory corruption fault at %lx\n",
>  			tsk->comm, tsk->pid, address);

Can you please convert that to pr_err() while at it?

> -		code = BUS_MCEERR_AR;
> +		if (fault & VM_FAULT_HWPOISON_LARGE)
> +			lsb = hstate_index_to_shift(VM_FAULT_GET_HINDEX(fault));
> +		if (fault & VM_FAULT_HWPOISON)
> +			lsb = PAGE_SHIFT;
> +		force_sig_mceerr(BUS_MCEERR_AR, (void __user *)address, lsb, tsk);
> +		return;
>  	}

With that fixed:

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [REVIEW][PATCH 03/20] signal/x86: Move MCE error reporting out of force_sig_info_fault

[Eric W. Biederman]

Thomas Gleixner <tglx@linutronix.de> writes:

> On Tue, 18 Sep 2018, Eric W. Biederman wrote:
>>  #ifdef CONFIG_MEMORY_FAILURE
>>  	if (fault & (VM_FAULT_HWPOISON|VM_FAULT_HWPOISON_LARGE)) {
>> +		unsigned lsb = 0;
>
> Newline between variable declaration and code please.
>
>>  		printk(KERN_ERR
>>  	"MCE: Killing %s:%d due to hardware memory corruption fault at %lx\n",
>>  			tsk->comm, tsk->pid, address);
>
> Can you please convert that to pr_err() while at it?
>
>> -		code = BUS_MCEERR_AR;
>> +		if (fault & VM_FAULT_HWPOISON_LARGE)
>> +			lsb = hstate_index_to_shift(VM_FAULT_GET_HINDEX(fault));
>> +		if (fault & VM_FAULT_HWPOISON)
>> +			lsb = PAGE_SHIFT;
>> +		force_sig_mceerr(BUS_MCEERR_AR, (void __user *)address, lsb, tsk);
>> +		return;
>>  	}
>
> With that fixed:
>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Fixes applied.

Eric

[REVIEW][PATCH 04/20] signal/x86: Use send_sig_mceerr as apropriate

[Eric W. Biederman]

This simplifies the code making it clearer what is going on.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/kvm/mmu.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index a282321329b5..95349bfe3b59 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3114,16 +3114,7 @@ static int __direct_map(struct kvm_vcpu *vcpu, int write, int map_writable,
 
 static void kvm_send_hwpoison_signal(unsigned long address, struct task_struct *tsk)
 {
-	siginfo_t info;
-
-	clear_siginfo(&info);
-	info.si_signo	= SIGBUS;
-	info.si_errno	= 0;
-	info.si_code	= BUS_MCEERR_AR;
-	info.si_addr	= (void __user *)address;
-	info.si_addr_lsb = PAGE_SHIFT;
-
-	send_sig_info(SIGBUS, &info, tsk);
+	send_sig_mceerr(BUS_MCEERR_AR, (void __user *)address, PAGE_SHIFT, tsk);
 }
 
 static int kvm_handle_bad_page(struct kvm_vcpu *vcpu, gfn_t gfn, kvm_pfn_t pfn)
-- 
2.17.1

Re: [REVIEW][PATCH 04/20] signal/x86: Use send_sig_mceerr as apropriate

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

CC+ kvm folks

> This simplifies the code making it clearer what is going on.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

> ---
>  arch/x86/kvm/mmu.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index a282321329b5..95349bfe3b59 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -3114,16 +3114,7 @@ static int __direct_map(struct kvm_vcpu *vcpu, int write, int map_writable,
>  
>  static void kvm_send_hwpoison_signal(unsigned long address, struct task_struct *tsk)
>  {
> -	siginfo_t info;
> -
> -	clear_siginfo(&info);
> -	info.si_signo	= SIGBUS;
> -	info.si_errno	= 0;
> -	info.si_code	= BUS_MCEERR_AR;
> -	info.si_addr	= (void __user *)address;
> -	info.si_addr_lsb = PAGE_SHIFT;
> -
> -	send_sig_info(SIGBUS, &info, tsk);
> +	send_sig_mceerr(BUS_MCEERR_AR, (void __user *)address, PAGE_SHIFT, tsk);
>  }
>  
>  static int kvm_handle_bad_page(struct kvm_vcpu *vcpu, gfn_t gfn, kvm_pfn_t pfn)
> -- 
> 2.17.1
> 
> 

Re: [REVIEW][PATCH 04/20] signal/x86: Use send_sig_mceerr as apropriate

[Paolo Bonzini]

On 18/09/2018 22:21, Thomas Gleixner wrote:
> On Tue, 18 Sep 2018, Eric W. Biederman wrote:
> 
> CC+ kvm folks
> 
>> This simplifies the code making it clearer what is going on.
>>
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> 
>> ---
>>  arch/x86/kvm/mmu.c | 11 +----------
>>  1 file changed, 1 insertion(+), 10 deletions(-)
>>
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index a282321329b5..95349bfe3b59 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -3114,16 +3114,7 @@ static int __direct_map(struct kvm_vcpu *vcpu, int write, int map_writable,
>>  
>>  static void kvm_send_hwpoison_signal(unsigned long address, struct task_struct *tsk)
>>  {
>> -	siginfo_t info;
>> -
>> -	clear_siginfo(&info);
>> -	info.si_signo	= SIGBUS;
>> -	info.si_errno	= 0;
>> -	info.si_code	= BUS_MCEERR_AR;
>> -	info.si_addr	= (void __user *)address;
>> -	info.si_addr_lsb = PAGE_SHIFT;
>> -
>> -	send_sig_info(SIGBUS, &info, tsk);
>> +	send_sig_mceerr(BUS_MCEERR_AR, (void __user *)address, PAGE_SHIFT, tsk);
>>  }
>>  
>>  static int kvm_handle_bad_page(struct kvm_vcpu *vcpu, gfn_t gfn, kvm_pfn_t pfn)
>> -- 
>> 2.17.1
>>
>>

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

[REVIEW][PATCH 05/20] signal/x86: In trace_mpx_bounds_register_exception add __user annotations

[Eric W. Biederman]

The value passed in to addr_referenced is of type void __user *, so update
the addr_referenced parameter in trace_mpx_bounds_register_exception to match.

Also update the addr_referenced paramater in TP_STRUCT__entry as it again
holdes the same value.

I don't know why this was missed earlier but sparse was complaining when
testing test branch so fix this now.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/include/asm/trace/mpx.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/trace/mpx.h b/arch/x86/include/asm/trace/mpx.h
index 7bd92db09e8d..54133017267c 100644
--- a/arch/x86/include/asm/trace/mpx.h
+++ b/arch/x86/include/asm/trace/mpx.h
@@ -11,12 +11,12 @@
 
 TRACE_EVENT(mpx_bounds_register_exception,
 
-	TP_PROTO(void *addr_referenced,
+	TP_PROTO(void __user *addr_referenced,
 		 const struct mpx_bndreg *bndreg),
 	TP_ARGS(addr_referenced, bndreg),
 
 	TP_STRUCT__entry(
-		__field(void *, addr_referenced)
+		__field(void __user *, addr_referenced)
 		__field(u64, lower_bound)
 		__field(u64, upper_bound)
 	),
-- 
2.17.1

Re: [REVIEW][PATCH 05/20] signal/x86: In trace_mpx_bounds_register_exception add __user annotations

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> The value passed in to addr_referenced is of type void __user *, so update
> the addr_referenced parameter in trace_mpx_bounds_register_exception to match.
> 
> Also update the addr_referenced paramater in TP_STRUCT__entry as it again
> holdes the same value.
> 
> I don't know why this was missed earlier but sparse was complaining when
> testing test branch so fix this now.

Good question.

> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[REVIEW][PATCH 06/20] signal/x86: Move mpx siginfo generation into do_bounds

[Eric W. Biederman]

This separates the logic of generating the signal from the logic of
gathering the information about the bounds violation.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/include/asm/mpx.h | 13 ++++++++++---
 arch/x86/kernel/traps.c    | 19 ++++++++++++++-----
 arch/x86/mm/mpx.c          | 30 +++++++++---------------------
 3 files changed, 33 insertions(+), 29 deletions(-)

diff --git a/arch/x86/include/asm/mpx.h b/arch/x86/include/asm/mpx.h
index 61eb4b63c5ec..8f55ea0cd7da 100644
--- a/arch/x86/include/asm/mpx.h
+++ b/arch/x86/include/asm/mpx.h
@@ -57,8 +57,15 @@
 #define MPX_BNDCFG_ADDR_MASK	(~((1UL<<MPX_BNDCFG_TAIL)-1))
 #define MPX_BNDSTA_ERROR_CODE	0x3
 
+struct mpx_fault_info
+{
+	void __user *addr;
+	void __user *lower;
+	void __user *upper;
+};
+
 #ifdef CONFIG_X86_INTEL_MPX
-siginfo_t *mpx_generate_siginfo(struct pt_regs *regs);
+int mpx_fault_info(struct mpx_fault_info *info, struct pt_regs *regs);
 int mpx_handle_bd_fault(void);
 static inline int kernel_managing_mpx_tables(struct mm_struct *mm)
 {
@@ -78,9 +85,9 @@ void mpx_notify_unmap(struct mm_struct *mm, struct vm_area_struct *vma,
 unsigned long mpx_unmapped_area_check(unsigned long addr, unsigned long len,
 		unsigned long flags);
 #else
-static inline siginfo_t *mpx_generate_siginfo(struct pt_regs *regs)
+static inline int mpx_fault_info(struct mpx_fault_info *info, struct pt_regs *regs)
 {
-	return NULL;
+	return -EINVAL;
 }
 static inline int mpx_handle_bd_fault(void)
 {
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index e6db475164ed..2155d2c7f49b 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -455,7 +455,6 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
 dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
 {
 	const struct mpx_bndcsr *bndcsr;
-	siginfo_t *info;
 
 	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
 	if (notify_die(DIE_TRAP, "bounds", regs, error_code,
@@ -493,8 +492,11 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
 			goto exit_trap;
 		break; /* Success, it was handled */
 	case 1: /* Bound violation. */
-		info = mpx_generate_siginfo(regs);
-		if (IS_ERR(info)) {
+	{
+		struct mpx_fault_info mpx;
+		struct siginfo info;
+
+		if (mpx_fault_info(&mpx, regs)) {
 			/*
 			 * We failed to decode the MPX instruction.  Act as if
 			 * the exception was not caused by MPX.
@@ -508,9 +510,16 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
 		 * allows and application to possibly handle the
 		 * #BR exception itself.
 		 */
-		do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, info);
-		kfree(info);
+		clear_siginfo(&info);
+		info.si_signo = SIGSEGV;
+		info.si_errno = 0;
+		info.si_code  = SEGV_BNDERR;
+		info.si_addr  = mpx.addr;
+		info.si_lower = mpx.lower;
+		info.si_upper = mpx.upper;
+		do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, &info);
 		break;
+	}
 	case 0: /* No exception caused by Intel MPX operations. */
 		goto exit_trap;
 	default:
diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
index e500949bae24..2385538e8065 100644
--- a/arch/x86/mm/mpx.c
+++ b/arch/x86/mm/mpx.c
@@ -118,14 +118,11 @@ static int mpx_insn_decode(struct insn *insn,
  * anything it wants in to the instructions.  We can not
  * trust anything about it.  They might not be valid
  * instructions or might encode invalid registers, etc...
- *
- * The caller is expected to kfree() the returned siginfo_t.
  */
-siginfo_t *mpx_generate_siginfo(struct pt_regs *regs)
+int mpx_fault_info(struct mpx_fault_info *info, struct pt_regs *regs)
 {
 	const struct mpx_bndreg_state *bndregs;
 	const struct mpx_bndreg *bndreg;
-	siginfo_t *info = NULL;
 	struct insn insn;
 	uint8_t bndregno;
 	int err;
@@ -153,11 +150,6 @@ siginfo_t *mpx_generate_siginfo(struct pt_regs *regs)
 	/* now go select the individual register in the set of 4 */
 	bndreg = &bndregs->bndreg[bndregno];
 
-	info = kzalloc(sizeof(*info), GFP_KERNEL);
-	if (!info) {
-		err = -ENOMEM;
-		goto err_out;
-	}
 	/*
 	 * The registers are always 64-bit, but the upper 32
 	 * bits are ignored in 32-bit mode.  Also, note that the
@@ -168,27 +160,23 @@ siginfo_t *mpx_generate_siginfo(struct pt_regs *regs)
 	 * complains when casting from integers to different-size
 	 * pointers.
 	 */
-	info->si_lower = (void __user *)(unsigned long)bndreg->lower_bound;
-	info->si_upper = (void __user *)(unsigned long)~bndreg->upper_bound;
-	info->si_addr_lsb = 0;
-	info->si_signo = SIGSEGV;
-	info->si_errno = 0;
-	info->si_code = SEGV_BNDERR;
-	info->si_addr = insn_get_addr_ref(&insn, regs);
+	info->lower = (void __user *)(unsigned long)bndreg->lower_bound;
+	info->upper = (void __user *)(unsigned long)~bndreg->upper_bound;
+	info->addr  = insn_get_addr_ref(&insn, regs);
+
 	/*
 	 * We were not able to extract an address from the instruction,
 	 * probably because there was something invalid in it.
 	 */
-	if (info->si_addr == (void __user *)-1) {
+	if (info->addr == (void __user *)-1) {
 		err = -EINVAL;
 		goto err_out;
 	}
-	trace_mpx_bounds_register_exception(info->si_addr, bndreg);
-	return info;
+	trace_mpx_bounds_register_exception(info->addr, bndreg);
+	return 0;
 err_out:
 	/* info might be NULL, but kfree() handles that */
-	kfree(info);
-	return ERR_PTR(err);
+	return err;
 }
 
 static __user void *mpx_get_bounds_dir(void)
-- 
2.17.1

Re: [REVIEW][PATCH 06/20] signal/x86: Move mpx siginfo generation into do_bounds

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> This separates the logic of generating the signal from the logic of
> gathering the information about the bounds violation.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [REVIEW][PATCH 06/20] signal/x86: Move mpx siginfo generation into do_bounds

[Christoph Hellwig]

> +struct mpx_fault_info
> +{

Normal kernel style would be:

struct mpx_fault_info {

Re: [REVIEW][PATCH 06/20] signal/x86: Move mpx siginfo generation into do_bounds

[Eric W. Biederman]

Christoph Hellwig <hch@infradead.org> writes:

>> +struct mpx_fault_info
>> +{
>
> Normal kernel style would be:
>
> struct mpx_fault_info {


Good point.  Fixed.

Eric

[REVIEW][PATCH 07/20] signal/x86/traps: Factor out show_signal

[Eric W. Biederman]

The code for conditionally printing unhanded signals is duplicated twice
in arch/x86/kernel/traps.c.  Factor it out into it's own subroutine
called show_signal to make the code clearer and easier to maintain.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/kernel/traps.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 2155d2c7f49b..31a689b67be3 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -217,6 +217,20 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 	return -1;
 }
 
+static void show_signal(struct task_struct *tsk, int signr,
+			const char *type, const char *desc,
+			struct pt_regs *regs, long error_code)
+{
+	if (show_unhandled_signals && unhandled_signal(tsk, signr) &&
+	    printk_ratelimit()) {
+		pr_info("%s[%d] %s%s ip:%lx sp:%lx error:%lx",
+			tsk->comm, task_pid_nr(tsk), type, desc,
+			regs->ip, regs->sp, error_code);
+		print_vma_addr(KERN_CONT " in ", regs->ip);
+		pr_cont("\n");
+	}
+}
+
 static siginfo_t *fill_trap_info(struct pt_regs *regs, int signr, int trapnr,
 				siginfo_t *info)
 {
@@ -269,14 +283,7 @@ do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
 	tsk->thread.error_code = error_code;
 	tsk->thread.trap_nr = trapnr;
 
-	if (show_unhandled_signals && unhandled_signal(tsk, signr) &&
-	    printk_ratelimit()) {
-		pr_info("%s[%d] trap %s ip:%lx sp:%lx error:%lx",
-			tsk->comm, tsk->pid, str,
-			regs->ip, regs->sp, error_code);
-		print_vma_addr(KERN_CONT " in ", regs->ip);
-		pr_cont("\n");
-	}
+	show_signal(tsk, signr, "trap ", str, regs, error_code);
 
 	force_sig_info(signr, info ?: SEND_SIG_PRIV, tsk);
 }
@@ -542,6 +549,7 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
 dotraplinkage void
 do_general_protection(struct pt_regs *regs, long error_code)
 {
+	const char *desc = "general protection fault";
 	struct task_struct *tsk;
 
 	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
@@ -565,23 +573,16 @@ do_general_protection(struct pt_regs *regs, long error_code)
 
 		tsk->thread.error_code = error_code;
 		tsk->thread.trap_nr = X86_TRAP_GP;
-		if (notify_die(DIE_GPF, "general protection fault", regs, error_code,
+		if (notify_die(DIE_GPF, desc, regs, error_code,
 			       X86_TRAP_GP, SIGSEGV) != NOTIFY_STOP)
-			die("general protection fault", regs, error_code);
+			die(desc, regs, error_code);
 		return;
 	}
 
 	tsk->thread.error_code = error_code;
 	tsk->thread.trap_nr = X86_TRAP_GP;
 
-	if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
-			printk_ratelimit()) {
-		pr_info("%s[%d] general protection ip:%lx sp:%lx error:%lx",
-			tsk->comm, task_pid_nr(tsk),
-			regs->ip, regs->sp, error_code);
-		print_vma_addr(KERN_CONT " in ", regs->ip);
-		pr_cont("\n");
-	}
+	show_signal(tsk, SIGSEGV, "", desc, regs, error_code);
 
 	force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk);
 }
-- 
2.17.1

Re: [REVIEW][PATCH 07/20] signal/x86/traps: Factor out show_signal

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> The code for conditionally printing unhanded signals is duplicated twice
> in arch/x86/kernel/traps.c.  Factor it out into it's own subroutine
> called show_signal to make the code clearer and easier to maintain.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[REVIEW][PATCH 08/20] signal/x86/traps: Move setting error_code and trap_nr into do_trap_no_signal

[Eric W. Biederman]

Half of the times when error_code and trap_nr are set are already in
do_trap_no_signal.  Move the other time these are set into do_trap_no_signal,
and give do_trap_no_signal a single exit where a signals are sent.

After the move the comment documeting this all is much easier to follow
as everything is together in one place.

Also update the string that is passed in to const.  The only user of that
str is die and it already takes a const string, so this just makes it explicit
that the string won't change.

This prepares the way for adding a second caller to do_trap_no_signal.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/kernel/traps.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 31a689b67be3..f31c0ddee278 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -189,7 +189,7 @@ int fixup_bug(struct pt_regs *regs, int trapnr)
 }
 
 static nokprobe_inline int
-do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
+do_trap_no_signal(struct task_struct *tsk, int trapnr, const char *str,
 		  struct pt_regs *regs,	long error_code)
 {
 	if (v8086_mode(regs)) {
@@ -202,10 +202,8 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 						error_code, trapnr))
 				return 0;
 		}
-		return -1;
 	}
-
-	if (!user_mode(regs)) {
+	else if (!user_mode(regs)) {
 		if (fixup_exception(regs, trapnr))
 			return 0;
 
@@ -214,6 +212,18 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 		die(str, regs, error_code);
 	}
 
+	/*
+	 * We want error_code and trap_nr set for userspace faults and
+	 * kernelspace faults which result in die(), but not
+	 * kernelspace faults which are fixed up.  die() gives the
+	 * process no chance to handle the signal and notice the
+	 * kernel fault information, so that won't result in polluting
+	 * the information about previously queued, but not yet
+	 * delivered, faults.  See also do_general_protection below.
+	 */
+	tsk->thread.error_code = error_code;
+	tsk->thread.trap_nr = trapnr;
+
 	return -1;
 }
 
@@ -271,17 +281,6 @@ do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
 
 	if (!do_trap_no_signal(tsk, trapnr, str, regs, error_code))
 		return;
-	/*
-	 * We want error_code and trap_nr set for userspace faults and
-	 * kernelspace faults which result in die(), but not
-	 * kernelspace faults which are fixed up.  die() gives the
-	 * process no chance to handle the signal and notice the
-	 * kernel fault information, so that won't result in polluting
-	 * the information about previously queued, but not yet
-	 * delivered, faults.  See also do_general_protection below.
-	 */
-	tsk->thread.error_code = error_code;
-	tsk->thread.trap_nr = trapnr;
 
 	show_signal(tsk, signr, "trap ", str, regs, error_code);
 
-- 
2.17.1

Re: [REVIEW][PATCH 08/20] signal/x86/traps: Move setting error_code and trap_nr into do_trap_no_signal

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> Half of the times when error_code and trap_nr are set are already in

s/when// I think

> do_trap_no_signal.  Move the other time these are set into do_trap_no_signal,

Please write function as do_trap_no_signal() so it's more obvious what you
are talking about.

> and give do_trap_no_signal a single exit where a signals are sent.

s/a/// or s/a/all/ Not sure what you wanted to do here.

> After the move the comment documeting this all is much easier to follow
> as everything is together in one place.
> 
> Also update the string that is passed in to const.  The only user of that
> str is die and it already takes a const string, so this just makes it explicit

string

> that the string won't change.
> 
> This prepares the way for adding a second caller to do_trap_no_signal.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  arch/x86/kernel/traps.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 31a689b67be3..f31c0ddee278 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -189,7 +189,7 @@ int fixup_bug(struct pt_regs *regs, int trapnr)
>  }
>  
>  static nokprobe_inline int
> -do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
> +do_trap_no_signal(struct task_struct *tsk, int trapnr, const char *str,
>  		  struct pt_regs *regs,	long error_code)
>  {
>  	if (v8086_mode(regs)) {
> @@ -202,10 +202,8 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
>  						error_code, trapnr))
>  				return 0;
>  		}
> -		return -1;
>  	}
> -
> -	if (!user_mode(regs)) {
> +	else if (!user_mode(regs)) {

Please put that 'else if' right after the closing bracket, i.e.

	} else if (...) {

>  		if (fixup_exception(regs, trapnr))
>  			return 0;

With that fixed:

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [REVIEW][PATCH 08/20] signal/x86/traps: Move setting error_code and trap_nr into do_trap_no_signal

[Thomas Gleixner]

On Tue, 18 Sep 2018, Thomas Gleixner wrote:

> On Tue, 18 Sep 2018, Eric W. Biederman wrote:
> 
> > Half of the times when error_code and trap_nr are set are already in
> 
> s/when// I think
> 
> > do_trap_no_signal.  Move the other time these are set into do_trap_no_signal,
> 
> Please write function as do_trap_no_signal() so it's more obvious what you
> are talking about.

That applies to the other changelogs as well.

Thanks,

	tglx

Re: [REVIEW][PATCH 08/20] signal/x86/traps: Move setting error_code and trap_nr into do_trap_no_signal

[Eric W. Biederman]

Thomas Gleixner <tglx@linutronix.de> writes:

> On Tue, 18 Sep 2018, Eric W. Biederman wrote:
>
>> Half of the times when error_code and trap_nr are set are already in
>
> s/when// I think

Yes that changelog was not a good description of why I was changing
things.

>> do_trap_no_signal.  Move the other time these are set into do_trap_no_signal,
>
> Please write function as do_trap_no_signal() so it's more obvious what you
> are talking about.

That feels quite unnatural to me.

>> and give do_trap_no_signal a single exit where a signals are sent.
>
> s/a/// or s/a/all/ Not sure what you wanted to do here.
>
>> After the move the comment documeting this all is much easier to follow
>> as everything is together in one place.
>> 
>> Also update the string that is passed in to const.  The only user of that
>> str is die and it already takes a const string, so this just makes it explicit
>
> string
>
>> that the string won't change.
>> 
>> This prepares the way for adding a second caller to do_trap_no_signal.
>> 
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>>  arch/x86/kernel/traps.c | 29 ++++++++++++++---------------
>>  1 file changed, 14 insertions(+), 15 deletions(-)
>> 
>> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
>> index 31a689b67be3..f31c0ddee278 100644
>> --- a/arch/x86/kernel/traps.c
>> +++ b/arch/x86/kernel/traps.c
>> @@ -189,7 +189,7 @@ int fixup_bug(struct pt_regs *regs, int trapnr)
>>  }
>>  
>>  static nokprobe_inline int
>> -do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
>> +do_trap_no_signal(struct task_struct *tsk, int trapnr, const char *str,
>>  		  struct pt_regs *regs,	long error_code)
>>  {
>>  	if (v8086_mode(regs)) {
>> @@ -202,10 +202,8 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
>>  						error_code, trapnr))
>>  				return 0;
>>  		}
>> -		return -1;
>>  	}
>> -
>> -	if (!user_mode(regs)) {
>> +	else if (!user_mode(regs)) {
>
> Please put that 'else if' right after the closing bracket, i.e.
>
> 	} else if (...) {
>
>>  		if (fixup_exception(regs, trapnr))
>>  			return 0;
>
> With that fixed:
>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Done.  The upated patch is below.

I also realized for clarity of the code I could have moved show_signal
into do_trap_no_signal as well.  I will post a follow up change where I
do that.

From: "Eric W. Biederman" <ebiederm@xmission.com>
Date: Fri, 4 Aug 2017 14:01:50 -0500
Subject: [PATCH] signal/x86/traps: Move more code into do_trap_no_signal so it can be reused

The function do_trap_no_signal embodies almost all of the work of the
function do_trap.  The exceptions are setting of thread.error_code and
thread.trap_nr in the case when the signal will be sent, and reporting
which signal will be sent with show_signal.

Filling in struct siginfo and then calling do_trap is problematic as
filling in struct siginfo is an fiddly process that can through
inattention has resulted in fields not initialized and the wrong
fields being filled in.

To avoid this error prone situation I am replacing force_sig_info with
a set of functions that take as arguments the information needed to
send a specific kind of signal.

The function do_trap is called in the context of several different
kinds of signals today.  Having a solid do_trap_no_signal that
can be reused allows call sites that send different kinds of
signals to reuse all of the code in do_trap_no_signal.

Modify do_trap_no_signal to have a single exit there signals
where be sent (aka returning -1) to allow more of the signal
sending path to be moved to from do_trap to do_trap_no_signal.

Move setting thread.trap_nr and thread.error_code into do_trap_no_signal
so the code does not need to be duplicated.

Make the type of the string that is passed into do_trap_no_signal to
const.  The only user of that str is die and it already takes a const
string, so this just makes it explicit that the string won't change.

All of this prepares the way for using do_trap_no_signal outside
of do_trap.

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/kernel/traps.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 31a689b67be3..f31c0ddee278 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -189,7 +189,7 @@ int fixup_bug(struct pt_regs *regs, int trapnr)
 }
 
 static nokprobe_inline int
-do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
+do_trap_no_signal(struct task_struct *tsk, int trapnr, const char *str,
 		  struct pt_regs *regs,	long error_code)
 {
 	if (v8086_mode(regs)) {
@@ -202,10 +202,8 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 						error_code, trapnr))
 				return 0;
 		}
-		return -1;
 	}
-
-	if (!user_mode(regs)) {
+	else if (!user_mode(regs)) {
 		if (fixup_exception(regs, trapnr))
 			return 0;
 
@@ -214,6 +212,18 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 		die(str, regs, error_code);
 	}
 
+	/*
+	 * We want error_code and trap_nr set for userspace faults and
+	 * kernelspace faults which result in die(), but not
+	 * kernelspace faults which are fixed up.  die() gives the
+	 * process no chance to handle the signal and notice the
+	 * kernel fault information, so that won't result in polluting
+	 * the information about previously queued, but not yet
+	 * delivered, faults.  See also do_general_protection below.
+	 */
+	tsk->thread.error_code = error_code;
+	tsk->thread.trap_nr = trapnr;
+
 	return -1;
 }
 
@@ -271,17 +281,6 @@ do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
 
 	if (!do_trap_no_signal(tsk, trapnr, str, regs, error_code))
 		return;
-	/*
-	 * We want error_code and trap_nr set for userspace faults and
-	 * kernelspace faults which result in die(), but not
-	 * kernelspace faults which are fixed up.  die() gives the
-	 * process no chance to handle the signal and notice the
-	 * kernel fault information, so that won't result in polluting
-	 * the information about previously queued, but not yet
-	 * delivered, faults.  See also do_general_protection below.
-	 */
-	tsk->thread.error_code = error_code;
-	tsk->thread.trap_nr = trapnr;
 
 	show_signal(tsk, signr, "trap ", str, regs, error_code);
 
-- 
2.17.1

Re: [REVIEW][PATCH 08/20] signal/x86/traps: Move setting error_code and trap_nr into do_trap_no_signal

[Eric W. Biederman]

ebiederm@xmission.com (Eric W. Biederman) writes:

> I also realized for clarity of the code I could have moved show_signal
> into do_trap_no_signal as well.  I will post a follow up change where I
> do that.

Except of course do_trap_no_signal does not take a signal number show
it can't call show signal.  On second glance I don't see enough gain
to make it worth moving show_signal into do_trap_no_signal.  So I am
just going to skip that for now.

Eric

[REVIEW][PATCH 09/20] signal/x86/traps: Use force_sig_bnderr

[Eric W. Biederman]

Instead of generating the siginfo in x86 specific code use the new
helper function force_sig_bnderr to separate the concerns of
collecting the information and generating a proper siginfo.

Making the code easier to understand and maintain.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/kernel/traps.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index f31c0ddee278..9f7b3831fb7e 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -499,8 +499,8 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
 		break; /* Success, it was handled */
 	case 1: /* Bound violation. */
 	{
+		struct task_struct *tsk = current;
 		struct mpx_fault_info mpx;
-		struct siginfo info;
 
 		if (mpx_fault_info(&mpx, regs)) {
 			/*
@@ -511,19 +511,18 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
 		}
 		/*
 		 * Success, we decoded the instruction and retrieved
-		 * an 'info' containing the address being accessed
+		 * an 'mpx' containing the address being accessed
 		 * which caused the exception.  This information
 		 * allows and application to possibly handle the
 		 * #BR exception itself.
 		 */
-		clear_siginfo(&info);
-		info.si_signo = SIGSEGV;
-		info.si_errno = 0;
-		info.si_code  = SEGV_BNDERR;
-		info.si_addr  = mpx.addr;
-		info.si_lower = mpx.lower;
-		info.si_upper = mpx.upper;
-		do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, &info);
+		if (!do_trap_no_signal(tsk, X86_TRAP_BR, "bounds", regs,
+				       error_code))
+			break;
+
+		show_signal(tsk, SIGSEGV, "trap ", "bounds", regs, error_code);
+
+		force_sig_bnderr(mpx.addr, mpx.lower, mpx.upper);
 		break;
 	}
 	case 0: /* No exception caused by Intel MPX operations. */
-- 
2.17.1

Re: [REVIEW][PATCH 09/20] signal/x86/traps: Use force_sig_bnderr

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> Instead of generating the siginfo in x86 specific code use the new
> helper function force_sig_bnderr to separate the concerns of
> collecting the information and generating a proper siginfo.
> 
> Making the code easier to understand and maintain.

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[REVIEW][PATCH 10/20] signal/x86/traps: Use force_sig instead of open coding it.

[Eric W. Biederman]

The function "force_sig(sig, tsk)" is equivalent to "
force_sig_info(sig, SEND_SIG_PRIV, tsk)".  Using the siginfo variants can
be error prone so use the simpler old fashioned force_sig variant,
and with luck the force_sig_info variant can go away.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/kernel/traps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 9f7b3831fb7e..3e4a3172d711 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -582,7 +582,7 @@ do_general_protection(struct pt_regs *regs, long error_code)
 
 	show_signal(tsk, SIGSEGV, "", desc, regs, error_code);
 
-	force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk);
+	force_sig(SIGSEGV, tsk);
 }
 NOKPROBE_SYMBOL(do_general_protection);
 
-- 
2.17.1

Re: [REVIEW][PATCH 10/20] signal/x86/traps: Use force_sig instead of open coding it.

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> The function "force_sig(sig, tsk)" is equivalent to "
> force_sig_info(sig, SEND_SIG_PRIV, tsk)".  Using the siginfo variants can
> be error prone so use the simpler old fashioned force_sig variant,
> and with luck the force_sig_info variant can go away.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[REVIEW][PATCH 11/20] signal/x86/traps: Simplify trap generation

[Eric W. Biederman]

Update the DO_ERROR macro to take si_code and si_addr values for a siginfo,
removing the need for the fill_trap_info function.

Update do_trap to also take the sicode and si_addr values for a sigininfo
and modify the code to call force_sig when a sicode is not passed in
and to call force_sig_fault when all of the information is present.

Making this a more obvious, simpler and less error prone construction.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/kernel/traps.c | 85 ++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 61 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 3e4a3172d711..eb54f1964252 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -241,40 +241,9 @@ static void show_signal(struct task_struct *tsk, int signr,
 	}
 }
 
-static siginfo_t *fill_trap_info(struct pt_regs *regs, int signr, int trapnr,
-				siginfo_t *info)
-{
-	unsigned long siaddr;
-	int sicode;
-
-	switch (trapnr) {
-	default:
-		return SEND_SIG_PRIV;
-
-	case X86_TRAP_DE:
-		sicode = FPE_INTDIV;
-		siaddr = uprobe_get_trap_addr(regs);
-		break;
-	case X86_TRAP_UD:
-		sicode = ILL_ILLOPN;
-		siaddr = uprobe_get_trap_addr(regs);
-		break;
-	case X86_TRAP_AC:
-		sicode = BUS_ADRALN;
-		siaddr = 0;
-		break;
-	}
-
-	info->si_signo = signr;
-	info->si_errno = 0;
-	info->si_code = sicode;
-	info->si_addr = (void __user *)siaddr;
-	return info;
-}
-
 static void
 do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
-	long error_code, siginfo_t *info)
+	long error_code, int sicode, void __user *addr)
 {
 	struct task_struct *tsk = current;
 
@@ -284,15 +253,16 @@ do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
 
 	show_signal(tsk, signr, "trap ", str, regs, error_code);
 
-	force_sig_info(signr, info ?: SEND_SIG_PRIV, tsk);
+	if (!sicode)
+		force_sig(signr, tsk);
+	else
+		force_sig_fault(signr, sicode, addr, tsk);
 }
 NOKPROBE_SYMBOL(do_trap);
 
 static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
-			  unsigned long trapnr, int signr)
+	unsigned long trapnr, int signr, int sicode, void __user *addr)
 {
-	siginfo_t info;
-
 	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
 
 	/*
@@ -305,26 +275,26 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
 	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
 			NOTIFY_STOP) {
 		cond_local_irq_enable(regs);
-		clear_siginfo(&info);
-		do_trap(trapnr, signr, str, regs, error_code,
-			fill_trap_info(regs, signr, trapnr, &info));
+		do_trap(trapnr, signr, str, regs, error_code, sicode, addr);
 	}
 }
 
-#define DO_ERROR(trapnr, signr, str, name)				\
-dotraplinkage void do_##name(struct pt_regs *regs, long error_code)	\
-{									\
-	do_error_trap(regs, error_code, str, trapnr, signr);		\
+#define IP ((void __user *)uprobe_get_trap_addr(regs))
+#define DO_ERROR(trapnr, signr, sicode, addr, str, name)		   \
+dotraplinkage void do_##name(struct pt_regs *regs, long error_code)	   \
+{									   \
+	do_error_trap(regs, error_code, str, trapnr, signr, sicode, addr); \
 }
 
-DO_ERROR(X86_TRAP_DE,     SIGFPE,  "divide error",		divide_error)
-DO_ERROR(X86_TRAP_OF,     SIGSEGV, "overflow",			overflow)
-DO_ERROR(X86_TRAP_UD,     SIGILL,  "invalid opcode",		invalid_op)
-DO_ERROR(X86_TRAP_OLD_MF, SIGFPE,  "coprocessor segment overrun",coprocessor_segment_overrun)
-DO_ERROR(X86_TRAP_TS,     SIGSEGV, "invalid TSS",		invalid_TSS)
-DO_ERROR(X86_TRAP_NP,     SIGBUS,  "segment not present",	segment_not_present)
-DO_ERROR(X86_TRAP_SS,     SIGBUS,  "stack segment",		stack_segment)
-DO_ERROR(X86_TRAP_AC,     SIGBUS,  "alignment check",		alignment_check)
+DO_ERROR(X86_TRAP_DE,     SIGFPE,  FPE_INTDIV,   IP, "divide error",        divide_error)
+DO_ERROR(X86_TRAP_OF,     SIGSEGV,          0, NULL, "overflow",            overflow)
+DO_ERROR(X86_TRAP_UD,     SIGILL,  ILL_ILLOPN,   IP, "invalid opcode",      invalid_op)
+DO_ERROR(X86_TRAP_OLD_MF, SIGFPE,           0, NULL, "coprocessor segment overrun", coprocessor_segment_overrun)
+DO_ERROR(X86_TRAP_TS,     SIGSEGV,          0, NULL, "invalid TSS",         invalid_TSS)
+DO_ERROR(X86_TRAP_NP,     SIGBUS,           0, NULL, "segment not present", segment_not_present)
+DO_ERROR(X86_TRAP_SS,     SIGBUS,           0, NULL, "stack segment",       stack_segment)
+DO_ERROR(X86_TRAP_AC,     SIGBUS,  BUS_ADRALN, NULL, "alignment check",     alignment_check)
+#undef IP
 
 #ifdef CONFIG_VMAP_STACK
 __visible void __noreturn handle_stack_overflow(const char *message,
@@ -541,7 +511,7 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
 	 * up here if the kernel has MPX turned off at compile
 	 * time..
 	 */
-	do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, NULL);
+	do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, 0, NULL);
 }
 
 dotraplinkage void
@@ -625,7 +595,7 @@ dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
 		goto exit;
 
 	cond_local_irq_enable(regs);
-	do_trap(X86_TRAP_BP, SIGTRAP, "int3", regs, error_code, NULL);
+	do_trap(X86_TRAP_BP, SIGTRAP, "int3", regs, error_code, 0, NULL);
 	cond_local_irq_disable(regs);
 
 exit:
@@ -936,20 +906,13 @@ NOKPROBE_SYMBOL(do_device_not_available);
 #ifdef CONFIG_X86_32
 dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
 {
-	siginfo_t info;
-
 	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
 	local_irq_enable();
 
-	clear_siginfo(&info);
-	info.si_signo = SIGILL;
-	info.si_errno = 0;
-	info.si_code = ILL_BADSTK;
-	info.si_addr = NULL;
 	if (notify_die(DIE_TRAP, "iret exception", regs, error_code,
 			X86_TRAP_IRET, SIGILL) != NOTIFY_STOP) {
 		do_trap(X86_TRAP_IRET, SIGILL, "iret exception", regs, error_code,
-			&info);
+			ILL_BADSTK, (void __user *)NULL);
 	}
 }
 #endif
-- 
2.17.1

Re: [REVIEW][PATCH 11/20] signal/x86/traps: Simplify trap generation

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> Update the DO_ERROR macro to take si_code and si_addr values for a siginfo,
> removing the need for the fill_trap_info function.
> 
> Update do_trap to also take the sicode and si_addr values for a sigininfo
> and modify the code to call force_sig when a sicode is not passed in
> and to call force_sig_fault when all of the information is present.
> 
> Making this a more obvious, simpler and less error prone construction.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[REVIEW][PATCH 12/20] signal/x86: Remove pkey parameter from bad_area_nosemaphore

[Eric W. Biederman]

The function bad_area_nosemaphore always sets si_code to SEGV_MAPERR
and as such can never return a pkey parameter.  Therefore remove the
unusable pkey parameter from bad_area_nosemaphore.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/mm/fault.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 4112b05d0dec..cfc88920716f 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -897,9 +897,9 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 
 static noinline void
 bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
-		     unsigned long address, u32 *pkey)
+		     unsigned long address)
 {
-	__bad_area_nosemaphore(regs, error_code, address, pkey, SEGV_MAPERR);
+	__bad_area_nosemaphore(regs, error_code, address, NULL, SEGV_MAPERR);
 }
 
 static void
@@ -1025,7 +1025,7 @@ mm_fault_error(struct pt_regs *regs, unsigned long error_code,
 			     VM_FAULT_HWPOISON_LARGE))
 			do_sigbus(regs, error_code, address, pkey, fault);
 		else if (fault & VM_FAULT_SIGSEGV)
-			bad_area_nosemaphore(regs, error_code, address, pkey);
+			bad_area_nosemaphore(regs, error_code, address);
 		else
 			BUG();
 	}
@@ -1255,7 +1255,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
 		 * Don't take the mm semaphore here. If we fixup a prefetch
 		 * fault we could otherwise deadlock:
 		 */
-		bad_area_nosemaphore(regs, error_code, address, NULL);
+		bad_area_nosemaphore(regs, error_code, address);
 
 		return;
 	}
@@ -1268,7 +1268,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
 		pgtable_bad(regs, error_code, address);
 
 	if (unlikely(smap_violation(error_code, regs))) {
-		bad_area_nosemaphore(regs, error_code, address, NULL);
+		bad_area_nosemaphore(regs, error_code, address);
 		return;
 	}
 
@@ -1277,7 +1277,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
 	 * in a region with pagefaults disabled then we must not take the fault
 	 */
 	if (unlikely(faulthandler_disabled() || !mm)) {
-		bad_area_nosemaphore(regs, error_code, address, NULL);
+		bad_area_nosemaphore(regs, error_code, address);
 		return;
 	}
 
@@ -1323,7 +1323,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
 	if (unlikely(!down_read_trylock(&mm->mmap_sem))) {
 		if (!(error_code & X86_PF_USER) &&
 		    !search_exception_tables(regs->ip)) {
-			bad_area_nosemaphore(regs, error_code, address, NULL);
+			bad_area_nosemaphore(regs, error_code, address);
 			return;
 		}
 retry:
-- 
2.17.1

Re: [REVIEW][PATCH 12/20] signal/x86: Remove pkey parameter from bad_area_nosemaphore

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:
> The function bad_area_nosemaphore always sets si_code to SEGV_MAPERR
> and as such can never return a pkey parameter.  Therefore remove the
> unusable pkey parameter from bad_area_nosemaphore.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Keeping patch for Dave...

> ---
>  arch/x86/mm/fault.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 4112b05d0dec..cfc88920716f 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -897,9 +897,9 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
>  
>  static noinline void
>  bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
> -		     unsigned long address, u32 *pkey)
> +		     unsigned long address)
>  {
> -	__bad_area_nosemaphore(regs, error_code, address, pkey, SEGV_MAPERR);
> +	__bad_area_nosemaphore(regs, error_code, address, NULL, SEGV_MAPERR);
>  }
>  
>  static void
> @@ -1025,7 +1025,7 @@ mm_fault_error(struct pt_regs *regs, unsigned long error_code,
>  			     VM_FAULT_HWPOISON_LARGE))
>  			do_sigbus(regs, error_code, address, pkey, fault);
>  		else if (fault & VM_FAULT_SIGSEGV)
> -			bad_area_nosemaphore(regs, error_code, address, pkey);
> +			bad_area_nosemaphore(regs, error_code, address);
>  		else
>  			BUG();
>  	}
> @@ -1255,7 +1255,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
>  		 * Don't take the mm semaphore here. If we fixup a prefetch
>  		 * fault we could otherwise deadlock:
>  		 */
> -		bad_area_nosemaphore(regs, error_code, address, NULL);
> +		bad_area_nosemaphore(regs, error_code, address);
>  
>  		return;
>  	}
> @@ -1268,7 +1268,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
>  		pgtable_bad(regs, error_code, address);
>  
>  	if (unlikely(smap_violation(error_code, regs))) {
> -		bad_area_nosemaphore(regs, error_code, address, NULL);
> +		bad_area_nosemaphore(regs, error_code, address);
>  		return;
>  	}
>  
> @@ -1277,7 +1277,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
>  	 * in a region with pagefaults disabled then we must not take the fault
>  	 */
>  	if (unlikely(faulthandler_disabled() || !mm)) {
> -		bad_area_nosemaphore(regs, error_code, address, NULL);
> +		bad_area_nosemaphore(regs, error_code, address);
>  		return;
>  	}
>  
> @@ -1323,7 +1323,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
>  	if (unlikely(!down_read_trylock(&mm->mmap_sem))) {
>  		if (!(error_code & X86_PF_USER) &&
>  		    !search_exception_tables(regs->ip)) {
> -			bad_area_nosemaphore(regs, error_code, address, NULL);
> +			bad_area_nosemaphore(regs, error_code, address);
>  			return;
>  		}
>  retry:
> -- 
> 2.17.1
> 
> 

Re: [REVIEW][PATCH 12/20] signal/x86: Remove pkey parameter from bad_area_nosemaphore

[Dave Hansen]

On 09/18/2018 01:44 PM, Thomas Gleixner wrote:
> On Tue, 18 Sep 2018, Eric W. Biederman wrote:
>> The function bad_area_nosemaphore always sets si_code to SEGV_MAPERR
>> and as such can never return a pkey parameter.  Therefore remove the
>> unusable pkey parameter from bad_area_nosemaphore.

The result of this looks nice to me.  It's especially nice to see the
scope where we have to pass around the pkey get reduced.

I also ran the pkey selftests against the current version of your
siginfo-testing branch.  Everything passes fine on my pkey-enabled system.

Re: [REVIEW][PATCH 12/20] signal/x86: Remove pkey parameter from bad_area_nosemaphore

[Eric W. Biederman]

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

> On 09/18/2018 01:44 PM, Thomas Gleixner wrote:
>> On Tue, 18 Sep 2018, Eric W. Biederman wrote:
>>> The function bad_area_nosemaphore always sets si_code to SEGV_MAPERR
>>> and as such can never return a pkey parameter.  Therefore remove the
>>> unusable pkey parameter from bad_area_nosemaphore.
>
> The result of this looks nice to me.  It's especially nice to see the
> scope where we have to pass around the pkey get reduced.
>
> I also ran the pkey selftests against the current version of your
> siginfo-testing branch.  Everything passes fine on my pkey-enabled
> system.

Thank you for testing.  I was quite pleased myself when I saw that the
pkey scope was reduced.

Did you by any chance test powerpc?  As I also reduced the scope of pkey
there as well and that is also in my siginfo-testing branch.

Eric

[REVIEW][PATCH 13/20] signal/x86: Remove the pkey parameter from do_sigbus

[Eric W. Biederman]

The function do_sigbus never sets si_code to PKUERR so it can never
return a pkey to userspace.  Therefore remove the unusable pkey
parameter from do_sigbus.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/mm/fault.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index cfc88920716f..6886866c072d 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -962,7 +962,7 @@ bad_area_access_error(struct pt_regs *regs, unsigned long error_code,
 
 static void
 do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
-	  u32 *pkey, unsigned int fault)
+	  unsigned int fault)
 {
 	struct task_struct *tsk = current;
 
@@ -994,7 +994,7 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
 		return;
 	}
 #endif
-	force_sig_info_fault(SIGBUS, BUS_ADRERR, address, tsk, pkey);
+	force_sig_info_fault(SIGBUS, BUS_ADRERR, address, tsk, NULL);
 }
 
 static noinline void
@@ -1023,7 +1023,7 @@ mm_fault_error(struct pt_regs *regs, unsigned long error_code,
 	} else {
 		if (fault & (VM_FAULT_SIGBUS|VM_FAULT_HWPOISON|
 			     VM_FAULT_HWPOISON_LARGE))
-			do_sigbus(regs, error_code, address, pkey, fault);
+			do_sigbus(regs, error_code, address, fault);
 		else if (fault & VM_FAULT_SIGSEGV)
 			bad_area_nosemaphore(regs, error_code, address);
 		else
-- 
2.17.1

Re: [REVIEW][PATCH 13/20] signal/x86: Remove the pkey parameter from do_sigbus

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> The function do_sigbus never sets si_code to PKUERR so it can never
> return a pkey to userspace.  Therefore remove the unusable pkey
> parameter from do_sigbus.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

> ---
>  arch/x86/mm/fault.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index cfc88920716f..6886866c072d 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -962,7 +962,7 @@ bad_area_access_error(struct pt_regs *regs, unsigned long error_code,
>  
>  static void
>  do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
> -	  u32 *pkey, unsigned int fault)
> +	  unsigned int fault)
>  {
>  	struct task_struct *tsk = current;
>  
> @@ -994,7 +994,7 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
>  		return;
>  	}
>  #endif
> -	force_sig_info_fault(SIGBUS, BUS_ADRERR, address, tsk, pkey);
> +	force_sig_info_fault(SIGBUS, BUS_ADRERR, address, tsk, NULL);
>  }
>  
>  static noinline void
> @@ -1023,7 +1023,7 @@ mm_fault_error(struct pt_regs *regs, unsigned long error_code,
>  	} else {
>  		if (fault & (VM_FAULT_SIGBUS|VM_FAULT_HWPOISON|
>  			     VM_FAULT_HWPOISON_LARGE))
> -			do_sigbus(regs, error_code, address, pkey, fault);
> +			do_sigbus(regs, error_code, address, fault);
>  		else if (fault & VM_FAULT_SIGSEGV)
>  			bad_area_nosemaphore(regs, error_code, address);
>  		else
> -- 
> 2.17.1
> 
> 

[REVIEW][PATCH 14/20] signal/x86: Remove pkey parameter from mm_fault_error

[Eric W. Biederman]

After the previous cleanups to do_sigbus and and bad_area_nosemaphore
mm_fault_error no now longer uses it's pkey parameter.  Therefore
remove the unused parameter.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/mm/fault.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 6886866c072d..7ba00519fa5d 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -999,7 +999,7 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
 
 static noinline void
 mm_fault_error(struct pt_regs *regs, unsigned long error_code,
-	       unsigned long address, u32 *pkey, vm_fault_t fault)
+	       unsigned long address, vm_fault_t fault)
 {
 	if (fatal_signal_pending(current) && !(error_code & X86_PF_USER)) {
 		no_context(regs, error_code, address, 0, 0);
@@ -1419,7 +1419,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
 
 	up_read(&mm->mmap_sem);
 	if (unlikely(fault & VM_FAULT_ERROR)) {
-		mm_fault_error(regs, error_code, address, &pkey, fault);
+		mm_fault_error(regs, error_code, address, fault);
 		return;
 	}
 
-- 
2.17.1

Re: [REVIEW][PATCH 14/20] signal/x86: Remove pkey parameter from mm_fault_error

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> After the previous cleanups to do_sigbus and and bad_area_nosemaphore
> mm_fault_error no now longer uses it's pkey parameter.  Therefore
> remove the unused parameter.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

> ---
>  arch/x86/mm/fault.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 6886866c072d..7ba00519fa5d 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -999,7 +999,7 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
>  
>  static noinline void
>  mm_fault_error(struct pt_regs *regs, unsigned long error_code,
> -	       unsigned long address, u32 *pkey, vm_fault_t fault)
> +	       unsigned long address, vm_fault_t fault)
>  {
>  	if (fatal_signal_pending(current) && !(error_code & X86_PF_USER)) {
>  		no_context(regs, error_code, address, 0, 0);
> @@ -1419,7 +1419,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
>  
>  	up_read(&mm->mmap_sem);
>  	if (unlikely(fault & VM_FAULT_ERROR)) {
> -		mm_fault_error(regs, error_code, address, &pkey, fault);
> +		mm_fault_error(regs, error_code, address, fault);
>  		return;
>  	}
>  
> -- 
> 2.17.1
> 
> 

[REVIEW][PATCH 15/20] signal/x86: Don't compute pkey in __do_page_fault

[Eric W. Biederman]

There are no more users of the computed pkey value in __do_page_fault
so stop computing the value.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/mm/fault.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 7ba00519fa5d..f82106578364 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -1215,7 +1215,6 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
 	struct mm_struct *mm;
 	vm_fault_t fault, major = 0;
 	unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
-	u32 pkey;
 
 	tsk = current;
 	mm = tsk->mm;
@@ -1387,10 +1386,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
 	 * (potentially after handling any pending signal during the return to
 	 * userland). The return to userland is identified whenever
 	 * FAULT_FLAG_USER|FAULT_FLAG_KILLABLE are both set in flags.
-	 * Thus we have to be careful about not touching vma after handling the
-	 * fault, so we read the pkey beforehand.
 	 */
-	pkey = vma_pkey(vma);
 	fault = handle_mm_fault(vma, address, flags);
 	major |= fault & VM_FAULT_MAJOR;
 
-- 
2.17.1

Re: [REVIEW][PATCH 15/20] signal/x86: Don't compute pkey in __do_page_fault

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> There are no more users of the computed pkey value in __do_page_fault
> so stop computing the value.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

> ---
>  arch/x86/mm/fault.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 7ba00519fa5d..f82106578364 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1215,7 +1215,6 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
>  	struct mm_struct *mm;
>  	vm_fault_t fault, major = 0;
>  	unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
> -	u32 pkey;
>  
>  	tsk = current;
>  	mm = tsk->mm;
> @@ -1387,10 +1386,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
>  	 * (potentially after handling any pending signal during the return to
>  	 * userland). The return to userland is identified whenever
>  	 * FAULT_FLAG_USER|FAULT_FLAG_KILLABLE are both set in flags.
> -	 * Thus we have to be careful about not touching vma after handling the
> -	 * fault, so we read the pkey beforehand.
>  	 */
> -	pkey = vma_pkey(vma);
>  	fault = handle_mm_fault(vma, address, flags);
>  	major |= fault & VM_FAULT_MAJOR;
>  
> -- 
> 2.17.1
> 
> 

[REVIEW][PATCH 16/20] signal/x86: Pass pkey not vma into __bad_area

[Eric W. Biederman]

There is only one caller of __bad_area that passes in PKUERR and thus
will generate a siginfo with si_pkey set.  Therefore simplify the
logic and hoist reading of vma_pkey up into that caller, and just
pass *pkey into __bad_area.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/mm/fault.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index f82106578364..11a93f14a674 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -904,22 +904,16 @@ bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 
 static void
 __bad_area(struct pt_regs *regs, unsigned long error_code,
-	   unsigned long address,  struct vm_area_struct *vma, int si_code)
+	   unsigned long address, u32 *pkey, int si_code)
 {
 	struct mm_struct *mm = current->mm;
-	u32 pkey;
-
-	if (vma)
-		pkey = vma_pkey(vma);
-
 	/*
 	 * Something tried to access memory that isn't in our memory map..
 	 * Fix it, but check if it's kernel or user first..
 	 */
 	up_read(&mm->mmap_sem);
 
-	__bad_area_nosemaphore(regs, error_code, address,
-			       (vma) ? &pkey : NULL, si_code);
+	__bad_area_nosemaphore(regs, error_code, address, pkey, si_code);
 }
 
 static noinline void
@@ -954,10 +948,12 @@ bad_area_access_error(struct pt_regs *regs, unsigned long error_code,
 	 * But, doing it this way allows compiler optimizations
 	 * if pkeys are compiled out.
 	 */
-	if (bad_area_access_from_pkeys(error_code, vma))
-		__bad_area(regs, error_code, address, vma, SEGV_PKUERR);
+	if (bad_area_access_from_pkeys(error_code, vma)) {
+		u32 pkey = vma_pkey(vma);
+		__bad_area(regs, error_code, address, &pkey, SEGV_PKUERR);
+	}
 	else
-		__bad_area(regs, error_code, address, vma, SEGV_ACCERR);
+		__bad_area(regs, error_code, address, NULL, SEGV_ACCERR);
 }
 
 static void
-- 
2.17.1

Re: [REVIEW][PATCH 16/20] signal/x86: Pass pkey not vma into __bad_area

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> There is only one caller of __bad_area that passes in PKUERR and thus
> will generate a siginfo with si_pkey set.  Therefore simplify the
> logic and hoist reading of vma_pkey up into that caller, and just
> pass *pkey into __bad_area.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  arch/x86/mm/fault.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index f82106578364..11a93f14a674 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -904,22 +904,16 @@ bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
>  
>  static void
>  __bad_area(struct pt_regs *regs, unsigned long error_code,
> -	   unsigned long address,  struct vm_area_struct *vma, int si_code)
> +	   unsigned long address, u32 *pkey, int si_code)
>  {
>  	struct mm_struct *mm = current->mm;
> -	u32 pkey;
> -
> -	if (vma)
> -		pkey = vma_pkey(vma);
> -
>  	/*
>  	 * Something tried to access memory that isn't in our memory map..
>  	 * Fix it, but check if it's kernel or user first..
>  	 */
>  	up_read(&mm->mmap_sem);
>  
> -	__bad_area_nosemaphore(regs, error_code, address,
> -			       (vma) ? &pkey : NULL, si_code);
> +	__bad_area_nosemaphore(regs, error_code, address, pkey, si_code);
>  }
>  
>  static noinline void
> @@ -954,10 +948,12 @@ bad_area_access_error(struct pt_regs *regs, unsigned long error_code,
>  	 * But, doing it this way allows compiler optimizations
>  	 * if pkeys are compiled out.
>  	 */
> -	if (bad_area_access_from_pkeys(error_code, vma))
> -		__bad_area(regs, error_code, address, vma, SEGV_PKUERR);
> +	if (bad_area_access_from_pkeys(error_code, vma)) {
> +		u32 pkey = vma_pkey(vma);
> +		__bad_area(regs, error_code, address, &pkey, SEGV_PKUERR);
> +	}
>  	else
> -		__bad_area(regs, error_code, address, vma, SEGV_ACCERR);
> +		__bad_area(regs, error_code, address, NULL, SEGV_ACCERR);

Please make that:

	} else {
		__bad_area(regs, error_code, address, NULL, SEGV_ACCERR);
	}
       
With that fixed:

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[REVIEW][PATCH 17/20] signal/x86: Call force_sig_pkuerr from __bad_area_nosemaphore

[Eric W. Biederman]

There is only one code path that can generate a pkuerr signal.  That
code path calls __bad_area_nosemaphore and can be dectected by testing
if si_code == SEGV_PKUERR.  It can be seen from inspection that all of
the other tests in fill_sig_info_pkey are unnecessary.

Therefore call force_sig_pkuerr directly from __bad_area_semaphore
and remove fill_sig_info_pkey.

At the same time move the comment above force_sig_info_pkey into
bad_area_access_error, so that the documentation of about pkey
generation races is not lost.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/mm/fault.c | 75 ++++++++++++++-------------------------------
 1 file changed, 23 insertions(+), 52 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 11a93f14a674..ccfeed902eee 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -153,56 +153,6 @@ is_prefetch(struct pt_regs *regs, unsigned long error_code, unsigned long addr)
 	return prefetch;
 }
 
-/*
- * A protection key fault means that the PKRU value did not allow
- * access to some PTE.  Userspace can figure out what PKRU was
- * from the XSAVE state, and this function fills out a field in
- * siginfo so userspace can discover which protection key was set
- * on the PTE.
- *
- * If we get here, we know that the hardware signaled a X86_PF_PK
- * fault and that there was a VMA once we got in the fault
- * handler.  It does *not* guarantee that the VMA we find here
- * was the one that we faulted on.
- *
- * 1. T1   : mprotect_key(foo, PAGE_SIZE, pkey=4);
- * 2. T1   : set PKRU to deny access to pkey=4, touches page
- * 3. T1   : faults...
- * 4.    T2: mprotect_key(foo, PAGE_SIZE, pkey=5);
- * 5. T1   : enters fault handler, takes mmap_sem, etc...
- * 6. T1   : reaches here, sees vma_pkey(vma)=5, when we really
- *	     faulted on a pte with its pkey=4.
- */
-static void fill_sig_info_pkey(int si_signo, int si_code, siginfo_t *info,
-		u32 *pkey)
-{
-	/* This is effectively an #ifdef */
-	if (!boot_cpu_has(X86_FEATURE_OSPKE))
-		return;
-
-	/* Fault not from Protection Keys: nothing to do */
-	if ((si_code != SEGV_PKUERR) || (si_signo != SIGSEGV))
-		return;
-	/*
-	 * force_sig_info_fault() is called from a number of
-	 * contexts, some of which have a VMA and some of which
-	 * do not.  The X86_PF_PK handing happens after we have a
-	 * valid VMA, so we should never reach this without a
-	 * valid VMA.
-	 */
-	if (!pkey) {
-		WARN_ONCE(1, "PKU fault with no VMA passed in");
-		info->si_pkey = 0;
-		return;
-	}
-	/*
-	 * si_pkey should be thought of as a strong hint, but not
-	 * absolutely guranteed to be 100% accurate because of
-	 * the race explained above.
-	 */
-	info->si_pkey = *pkey;
-}
-
 static void
 force_sig_info_fault(int si_signo, int si_code, unsigned long address,
 		     struct task_struct *tsk, u32 *pkey)
@@ -215,8 +165,6 @@ force_sig_info_fault(int si_signo, int si_code, unsigned long address,
 	info.si_code	= si_code;
 	info.si_addr	= (void __user *)address;
 
-	fill_sig_info_pkey(si_signo, si_code, &info, pkey);
-
 	force_sig_info(si_signo, &info, tsk);
 }
 
@@ -884,6 +832,9 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 		tsk->thread.error_code	= error_code;
 		tsk->thread.trap_nr	= X86_TRAP_PF;
 
+		if (si_code == SEGV_PKUERR)
+			force_sig_pkuerr((void __user *)address, *pkey);
+
 		force_sig_info_fault(SIGSEGV, si_code, address, tsk, pkey);
 
 		return;
@@ -949,6 +900,26 @@ bad_area_access_error(struct pt_regs *regs, unsigned long error_code,
 	 * if pkeys are compiled out.
 	 */
 	if (bad_area_access_from_pkeys(error_code, vma)) {
+		/*
+		 * A protection key fault means that the PKRU value did not allow
+		 * access to some PTE.  Userspace can figure out what PKRU was
+		 * from the XSAVE state.  This function captures the pkey from
+		 * the vma and passes it to userspace so userspace can discover
+		 * which protection key was set on the PTE.
+		 *
+		 * If we get here, we know that the hardware signaled a X86_PF_PK
+		 * fault and that there was a VMA once we got in the fault
+		 * handler.  It does *not* guarantee that the VMA we find here
+		 * was the one that we faulted on.
+		 *
+		 * 1. T1   : mprotect_key(foo, PAGE_SIZE, pkey=4);
+		 * 2. T1   : set PKRU to deny access to pkey=4, touches page
+		 * 3. T1   : faults...
+		 * 4.    T2: mprotect_key(foo, PAGE_SIZE, pkey=5);
+		 * 5. T1   : enters fault handler, takes mmap_sem, etc...
+		 * 6. T1   : reaches here, sees vma_pkey(vma)=5, when we really
+		 *	     faulted on a pte with its pkey=4.
+		 */
 		u32 pkey = vma_pkey(vma);
 		__bad_area(regs, error_code, address, &pkey, SEGV_PKUERR);
 	}
-- 
2.17.1

Re: [REVIEW][PATCH 17/20] signal/x86: Call force_sig_pkuerr from __bad_area_nosemaphore

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> There is only one code path that can generate a pkuerr signal.  That
> code path calls __bad_area_nosemaphore and can be dectected by testing
> if si_code == SEGV_PKUERR.  It can be seen from inspection that all of
> the other tests in fill_sig_info_pkey are unnecessary.
> 
> Therefore call force_sig_pkuerr directly from __bad_area_semaphore
> and remove fill_sig_info_pkey.
> 
> At the same time move the comment above force_sig_info_pkey into
> bad_area_access_error, so that the documentation of about pkey

of about? Pick one please

> generation races is not lost.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  arch/x86/mm/fault.c | 75 ++++++++++++++-------------------------------
>  1 file changed, 23 insertions(+), 52 deletions(-)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 11a93f14a674..ccfeed902eee 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -153,56 +153,6 @@ is_prefetch(struct pt_regs *regs, unsigned long error_code, unsigned long addr)
>  	return prefetch;
>  }
>  
> -/*
> - * A protection key fault means that the PKRU value did not allow
> - * access to some PTE.  Userspace can figure out what PKRU was
> - * from the XSAVE state, and this function fills out a field in
> - * siginfo so userspace can discover which protection key was set
> - * on the PTE.
> - *
> - * If we get here, we know that the hardware signaled a X86_PF_PK
> - * fault and that there was a VMA once we got in the fault
> - * handler.  It does *not* guarantee that the VMA we find here
> - * was the one that we faulted on.
> - *
> - * 1. T1   : mprotect_key(foo, PAGE_SIZE, pkey=4);
> - * 2. T1   : set PKRU to deny access to pkey=4, touches page
> - * 3. T1   : faults...
> - * 4.    T2: mprotect_key(foo, PAGE_SIZE, pkey=5);
> - * 5. T1   : enters fault handler, takes mmap_sem, etc...
> - * 6. T1   : reaches here, sees vma_pkey(vma)=5, when we really
> - *	     faulted on a pte with its pkey=4.
> - */
> -static void fill_sig_info_pkey(int si_signo, int si_code, siginfo_t *info,
> -		u32 *pkey)
> -{
> -	/* This is effectively an #ifdef */
> -	if (!boot_cpu_has(X86_FEATURE_OSPKE))
> -		return;
> -
> -	/* Fault not from Protection Keys: nothing to do */
> -	if ((si_code != SEGV_PKUERR) || (si_signo != SIGSEGV))
> -		return;
> -	/*
> -	 * force_sig_info_fault() is called from a number of
> -	 * contexts, some of which have a VMA and some of which
> -	 * do not.  The X86_PF_PK handing happens after we have a
> -	 * valid VMA, so we should never reach this without a
> -	 * valid VMA.
> -	 */
> -	if (!pkey) {
> -		WARN_ONCE(1, "PKU fault with no VMA passed in");
> -		info->si_pkey = 0;
> -		return;
> -	}
> -	/*
> -	 * si_pkey should be thought of as a strong hint, but not
> -	 * absolutely guranteed to be 100% accurate because of
> -	 * the race explained above.
> -	 */
> -	info->si_pkey = *pkey;
> -}
> -
>  static void
>  force_sig_info_fault(int si_signo, int si_code, unsigned long address,
>  		     struct task_struct *tsk, u32 *pkey)
> @@ -215,8 +165,6 @@ force_sig_info_fault(int si_signo, int si_code, unsigned long address,
>  	info.si_code	= si_code;
>  	info.si_addr	= (void __user *)address;
>  
> -	fill_sig_info_pkey(si_signo, si_code, &info, pkey);
> -
>  	force_sig_info(si_signo, &info, tsk);
>  }
>  
> @@ -884,6 +832,9 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
>  		tsk->thread.error_code	= error_code;
>  		tsk->thread.trap_nr	= X86_TRAP_PF;
>  
> +		if (si_code == SEGV_PKUERR)
> +			force_sig_pkuerr((void __user *)address, *pkey);
> +
>  		force_sig_info_fault(SIGSEGV, si_code, address, tsk, pkey);
>  
>  		return;
> @@ -949,6 +900,26 @@ bad_area_access_error(struct pt_regs *regs, unsigned long error_code,
>  	 * if pkeys are compiled out.
>  	 */
>  	if (bad_area_access_from_pkeys(error_code, vma)) {
> +		/*
> +		 * A protection key fault means that the PKRU value did not allow
> +		 * access to some PTE.  Userspace can figure out what PKRU was
> +		 * from the XSAVE state.  This function captures the pkey from
> +		 * the vma and passes it to userspace so userspace can discover
> +		 * which protection key was set on the PTE.
> +		 *
> +		 * If we get here, we know that the hardware signaled a X86_PF_PK
> +		 * fault and that there was a VMA once we got in the fault
> +		 * handler.  It does *not* guarantee that the VMA we find here
> +		 * was the one that we faulted on.
> +		 *
> +		 * 1. T1   : mprotect_key(foo, PAGE_SIZE, pkey=4);
> +		 * 2. T1   : set PKRU to deny access to pkey=4, touches page
> +		 * 3. T1   : faults...
> +		 * 4.    T2: mprotect_key(foo, PAGE_SIZE, pkey=5);
> +		 * 5. T1   : enters fault handler, takes mmap_sem, etc...
> +		 * 6. T1   : reaches here, sees vma_pkey(vma)=5, when we really
> +		 *	     faulted on a pte with its pkey=4.
> +		 */
>  		u32 pkey = vma_pkey(vma);
>  		__bad_area(regs, error_code, address, &pkey, SEGV_PKUERR);

Newline between variable declaration and code please.

With that fixed:

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[REVIEW][PATCH 18/20] signal/x86: Replace force_sig_info_fault with force_sig_fault

[Eric W. Biederman]

Now that the pkey handling has been removed force_sig_info_fault and
force_sig_fault perform identical work.  Just the type of the address
paramter is different.  So replace calls to force_sig_info_fault with
calls to force_sig_fault, and remove force_sig_info_fault.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/mm/fault.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index ccfeed902eee..548396aa03e1 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -153,21 +153,6 @@ is_prefetch(struct pt_regs *regs, unsigned long error_code, unsigned long addr)
 	return prefetch;
 }
 
-static void
-force_sig_info_fault(int si_signo, int si_code, unsigned long address,
-		     struct task_struct *tsk, u32 *pkey)
-{
-	siginfo_t info;
-
-	clear_siginfo(&info);
-	info.si_signo	= si_signo;
-	info.si_errno	= 0;
-	info.si_code	= si_code;
-	info.si_addr	= (void __user *)address;
-
-	force_sig_info(si_signo, &info, tsk);
-}
-
 DEFINE_SPINLOCK(pgd_lock);
 LIST_HEAD(pgd_list);
 
@@ -672,8 +657,8 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 			tsk->thread.cr2 = address;
 
 			/* XXX: hwpoison faults will set the wrong code. */
-			force_sig_info_fault(signal, si_code, address,
-					     tsk, NULL);
+			force_sig_fault(signal, si_code, (void __user *)address,
+					tsk);
 		}
 
 		/*
@@ -835,7 +820,7 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 		if (si_code == SEGV_PKUERR)
 			force_sig_pkuerr((void __user *)address, *pkey);
 
-		force_sig_info_fault(SIGSEGV, si_code, address, tsk, pkey);
+		force_sig_fault(SIGSEGV, si_code, (void __user *)address, tsk);
 
 		return;
 	}
@@ -961,7 +946,7 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
 		return;
 	}
 #endif
-	force_sig_info_fault(SIGBUS, BUS_ADRERR, address, tsk, NULL);
+	force_sig_fault(SIGBUS, BUS_ADRERR, (void __user *)address, tsk);
 }
 
 static noinline void
-- 
2.17.1

Re: [REVIEW][PATCH 18/20] signal/x86: Replace force_sig_info_fault with force_sig_fault

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> Now that the pkey handling has been removed force_sig_info_fault and
> force_sig_fault perform identical work.  Just the type of the address
> paramter is different.  So replace calls to force_sig_info_fault with
> calls to force_sig_fault, and remove force_sig_info_fault.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[REVIEW][PATCH 19/20] signal/x86: Pass pkey by value

[Eric W. Biederman]

Now that si_code == SEGV_PKUERR is the flag indicating that a pkey
is present there is no longer a need to pass a pointer to a local
pkey value, instead pkey can be passed more efficiently by value.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/mm/fault.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 548396aa03e1..c09a0c2fa614 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -769,7 +769,7 @@ show_signal_msg(struct pt_regs *regs, unsigned long error_code,
 
 static void
 __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
-		       unsigned long address, u32 *pkey, int si_code)
+		       unsigned long address, u32 pkey, int si_code)
 {
 	struct task_struct *tsk = current;
 
@@ -818,7 +818,7 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 		tsk->thread.trap_nr	= X86_TRAP_PF;
 
 		if (si_code == SEGV_PKUERR)
-			force_sig_pkuerr((void __user *)address, *pkey);
+			force_sig_pkuerr((void __user *)address, pkey);
 
 		force_sig_fault(SIGSEGV, si_code, (void __user *)address, tsk);
 
@@ -835,12 +835,12 @@ static noinline void
 bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 		     unsigned long address)
 {
-	__bad_area_nosemaphore(regs, error_code, address, NULL, SEGV_MAPERR);
+	__bad_area_nosemaphore(regs, error_code, address, 0, SEGV_MAPERR);
 }
 
 static void
 __bad_area(struct pt_regs *regs, unsigned long error_code,
-	   unsigned long address, u32 *pkey, int si_code)
+	   unsigned long address, u32 pkey, int si_code)
 {
 	struct mm_struct *mm = current->mm;
 	/*
@@ -855,7 +855,7 @@ __bad_area(struct pt_regs *regs, unsigned long error_code,
 static noinline void
 bad_area(struct pt_regs *regs, unsigned long error_code, unsigned long address)
 {
-	__bad_area(regs, error_code, address, NULL, SEGV_MAPERR);
+	__bad_area(regs, error_code, address, 0, SEGV_MAPERR);
 }
 
 static inline bool bad_area_access_from_pkeys(unsigned long error_code,
@@ -906,10 +906,10 @@ bad_area_access_error(struct pt_regs *regs, unsigned long error_code,
 		 *	     faulted on a pte with its pkey=4.
 		 */
 		u32 pkey = vma_pkey(vma);
-		__bad_area(regs, error_code, address, &pkey, SEGV_PKUERR);
+		__bad_area(regs, error_code, address, pkey, SEGV_PKUERR);
 	}
 	else
-		__bad_area(regs, error_code, address, NULL, SEGV_ACCERR);
+		__bad_area(regs, error_code, address, 0, SEGV_ACCERR);
 }
 
 static void
-- 
2.17.1

Re: [REVIEW][PATCH 19/20] signal/x86: Pass pkey by value

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> Now that si_code == SEGV_PKUERR is the flag indicating that a pkey
> is present there is no longer a need to pass a pointer to a local
> pkey value, instead pkey can be passed more efficiently by value.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

> ---
>  arch/x86/mm/fault.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 548396aa03e1..c09a0c2fa614 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -769,7 +769,7 @@ show_signal_msg(struct pt_regs *regs, unsigned long error_code,
>  
>  static void
>  __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
> -		       unsigned long address, u32 *pkey, int si_code)
> +		       unsigned long address, u32 pkey, int si_code)
>  {
>  	struct task_struct *tsk = current;
>  
> @@ -818,7 +818,7 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
>  		tsk->thread.trap_nr	= X86_TRAP_PF;
>  
>  		if (si_code == SEGV_PKUERR)
> -			force_sig_pkuerr((void __user *)address, *pkey);
> +			force_sig_pkuerr((void __user *)address, pkey);
>  
>  		force_sig_fault(SIGSEGV, si_code, (void __user *)address, tsk);
>  
> @@ -835,12 +835,12 @@ static noinline void
>  bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
>  		     unsigned long address)
>  {
> -	__bad_area_nosemaphore(regs, error_code, address, NULL, SEGV_MAPERR);
> +	__bad_area_nosemaphore(regs, error_code, address, 0, SEGV_MAPERR);
>  }
>  
>  static void
>  __bad_area(struct pt_regs *regs, unsigned long error_code,
> -	   unsigned long address, u32 *pkey, int si_code)
> +	   unsigned long address, u32 pkey, int si_code)
>  {
>  	struct mm_struct *mm = current->mm;
>  	/*
> @@ -855,7 +855,7 @@ __bad_area(struct pt_regs *regs, unsigned long error_code,
>  static noinline void
>  bad_area(struct pt_regs *regs, unsigned long error_code, unsigned long address)
>  {
> -	__bad_area(regs, error_code, address, NULL, SEGV_MAPERR);
> +	__bad_area(regs, error_code, address, 0, SEGV_MAPERR);
>  }
>  
>  static inline bool bad_area_access_from_pkeys(unsigned long error_code,
> @@ -906,10 +906,10 @@ bad_area_access_error(struct pt_regs *regs, unsigned long error_code,
>  		 *	     faulted on a pte with its pkey=4.
>  		 */
>  		u32 pkey = vma_pkey(vma);
> -		__bad_area(regs, error_code, address, &pkey, SEGV_PKUERR);
> +		__bad_area(regs, error_code, address, pkey, SEGV_PKUERR);
>  	}
>  	else
> -		__bad_area(regs, error_code, address, NULL, SEGV_ACCERR);
> +		__bad_area(regs, error_code, address, 0, SEGV_ACCERR);
>  }
>  
>  static void
> -- 
> 2.17.1
> 
> 

[REVIEW][PATCH 20/20] signal/x86: Use force_sig_fault where appropriate

[Eric W. Biederman]

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/x86/entry/vsyscall/vsyscall_64.c |  9 +--------
 arch/x86/kernel/ptrace.c              | 10 ++--------
 arch/x86/kernel/traps.c               | 14 +++++---------
 arch/x86/kernel/umip.c                |  8 +-------
 4 files changed, 9 insertions(+), 32 deletions(-)

diff --git a/arch/x86/entry/vsyscall/vsyscall_64.c b/arch/x86/entry/vsyscall/vsyscall_64.c
index 82ed001e8909..85fd85d52ffd 100644
--- a/arch/x86/entry/vsyscall/vsyscall_64.c
+++ b/arch/x86/entry/vsyscall/vsyscall_64.c
@@ -100,20 +100,13 @@ static bool write_ok_or_segv(unsigned long ptr, size_t size)
 	 */
 
 	if (!access_ok(VERIFY_WRITE, (void __user *)ptr, size)) {
-		siginfo_t info;
 		struct thread_struct *thread = &current->thread;
 
 		thread->error_code	= 6;  /* user fault, no page, write */
 		thread->cr2		= ptr;
 		thread->trap_nr		= X86_TRAP_PF;
 
-		clear_siginfo(&info);
-		info.si_signo		= SIGSEGV;
-		info.si_errno		= 0;
-		info.si_code		= SEGV_MAPERR;
-		info.si_addr		= (void __user *)ptr;
-
-		force_sig_info(SIGSEGV, &info, current);
+		force_sig_fault(SIGSEGV, SEGV_MAPERR, (void __user *)ptr, current);
 		return false;
 	} else {
 		return true;
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 511ea0f16078..a78fff5b3384 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -1372,18 +1372,12 @@ const struct user_regset_view *task_user_regset_view(struct task_struct *task)
 void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs,
 					 int error_code, int si_code)
 {
-	struct siginfo info;
-
-	clear_siginfo(&info);
 	tsk->thread.trap_nr = X86_TRAP_DB;
 	tsk->thread.error_code = error_code;
 
-	info.si_signo = SIGTRAP;
-	info.si_code = si_code;
-	info.si_addr = user_mode(regs) ? (void __user *)regs->ip : NULL;
-
 	/* Send us the fake SIGTRAP */
-	force_sig_info(SIGTRAP, &info, tsk);
+	force_sig_fault(SIGTRAP, si_code,
+			user_mode(regs) ? (void __user *)regs->ip : NULL, tsk);
 }
 
 void user_single_step_report(struct pt_regs *regs)
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index eb54f1964252..b3c5708b6d31 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -809,7 +809,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)
 {
 	struct task_struct *task = current;
 	struct fpu *fpu = &task->thread.fpu;
-	siginfo_t info;
+	int si_code;
 	char *str = (trapnr == X86_TRAP_MF) ? "fpu exception" :
 						"simd exception";
 
@@ -835,18 +835,14 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)
 
 	task->thread.trap_nr	= trapnr;
 	task->thread.error_code = error_code;
-	clear_siginfo(&info);
-	info.si_signo		= SIGFPE;
-	info.si_errno		= 0;
-	info.si_addr		= (void __user *)uprobe_get_trap_addr(regs);
-
-	info.si_code = fpu__exception_code(fpu, trapnr);
 
+	si_code = fpu__exception_code(fpu, trapnr);
 	/* Retry when we get spurious exceptions: */
-	if (!info.si_code)
+	if (!si_code)
 		return;
 
-	force_sig_info(SIGFPE, &info, task);
+	force_sig_fault(SIGFPE, si_code,
+			(void __user *)uprobe_get_trap_addr(regs), task);
 }
 
 dotraplinkage void do_coprocessor_error(struct pt_regs *regs, long error_code)
diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
index ff20b35e98dd..f8f3cfda01ae 100644
--- a/arch/x86/kernel/umip.c
+++ b/arch/x86/kernel/umip.c
@@ -271,19 +271,13 @@ static int emulate_umip_insn(struct insn *insn, int umip_inst,
  */
 static void force_sig_info_umip_fault(void __user *addr, struct pt_regs *regs)
 {
-	siginfo_t info;
 	struct task_struct *tsk = current;
 
 	tsk->thread.cr2		= (unsigned long)addr;
 	tsk->thread.error_code	= X86_PF_USER | X86_PF_WRITE;
 	tsk->thread.trap_nr	= X86_TRAP_PF;
 
-	clear_siginfo(&info);
-	info.si_signo	= SIGSEGV;
-	info.si_errno	= 0;
-	info.si_code	= SEGV_MAPERR;
-	info.si_addr	= addr;
-	force_sig_info(SIGSEGV, &info, tsk);
+	force_sig_fault(SIGSEGV, SEGV_MAPERR, addr, tsk);
 
 	if (!(show_unhandled_signals && unhandled_signal(tsk, SIGSEGV)))
 		return;
-- 
2.17.1

Re: [REVIEW][PATCH 20/20] signal/x86: Use force_sig_fault where appropriate

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:

> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [REVIEW][PATCH 00/20] siginfo cleanups for x86

[Thomas Gleixner]

On Tue, 18 Sep 2018, Eric W. Biederman wrote:
> I have been slowly going thought and reworking the arch specific
> functions that generate siginfo.  The problems I have been addressing
> is that using siginfo directly is error prone.  Using siginfo directly
> makes it easy to leave fields initialized, and get confused about
> which fields need to be filled in.
> 
> To address this I have added a series of helper functions to
> kernel/signal.c, that are specific to exactly one use of struct siginfo
> and take the parameters they need.
> 
> To use these functions the x86 signal handling needs some cleanups but
> the net result appears to be less code that is easier to follow.
> 
> If while looking over these patches you see anything please let me know.

Only nitpicks.

> I don't think I missed something but to err is human.

I went through the changes a couple of times, but failed to spot
something. Was pleasure to read that set!

> Likewise if you would like to merge these patches via the tip tree
> let me know.  Otherwise after the review is complete I plan on merging
> these into my siginfo tree.  At this point I believe all of the
> prerequisite patches are merged so it should not make a difference.

Works either way. Ingo?

Thanks,

	tglx

Re: [REVIEW][PATCH 00/20] siginfo cleanups for x86

[Eric W. Biederman]

Thomas Gleixner <tglx@linutronix.de> writes:

> On Tue, 18 Sep 2018, Eric W. Biederman wrote:
>> I have been slowly going thought and reworking the arch specific
>> functions that generate siginfo.  The problems I have been addressing
>> is that using siginfo directly is error prone.  Using siginfo directly
>> makes it easy to leave fields initialized, and get confused about
>> which fields need to be filled in.
>> 
>> To address this I have added a series of helper functions to
>> kernel/signal.c, that are specific to exactly one use of struct siginfo
>> and take the parameters they need.
>> 
>> To use these functions the x86 signal handling needs some cleanups but
>> the net result appears to be less code that is easier to follow.
>> 
>> If while looking over these patches you see anything please let me know.
>
> Only nitpicks.
>
>> I don't think I missed something but to err is human.
>
> I went through the changes a couple of times, but failed to spot
> something. Was pleasure to read that set!
>
>> Likewise if you would like to merge these patches via the tip tree
>> let me know.  Otherwise after the review is complete I plan on merging
>> these into my siginfo tree.  At this point I believe all of the
>> prerequisite patches are merged so it should not make a difference.
>
> Works either way. Ingo?

If I manage to get through all of the architecture specific code I can
shrink the in-kernel version of struct siginfo.    So there is a slight
advantage to having it all in my tree.  But worst case I just have to
wait another cycle which doesn't look like a particularly long wait at
this point.

Eric