[PATCH 0/6] x86/traps: cleanup DO_ERROR*() to prepare for uprobes fixes

[PATCH 0/6] x86/traps: cleanup DO_ERROR*() to prepare for uprobes fixes

[Oleg Nesterov]

Hello.

On 05/06, Oleg Nesterov wrote:
>
> Well, actually we are not 100% fine because si_addr can be wrong. But this is
> not invalid_op-specific, we need to fix this anyway and the fix is simple.
>
> I do not want to discuss this now, but I am going to make another series later
> which adds something like uprobe_instruction_pointer(regs). It can (should) be
> used by DO_ERROR_INFO() (perhaps by something else, not sure about math_error())

So we basically need a very simple fix,

	- DO_ERROR_INFO(..., regs->ip)
	+ DO_ERROR_INFO(..., uprobe_get_trap_addr(regs))

but it is not easy to change these define's, this series tries to cleanup and
simplify this code first.

Note: initially I was going to send more changes. For example, after this series
we can convert math_error() into the "normal" DO_ERROR() user, and most probably
we can do the same with do_general_protection().

But let me send the initial changes first for review. If they pass the review
and if nobody objects, I'd like to route them along with the pending uprobes
fixes.

Oleg.

 arch/x86/include/asm/traps.h |    1 -
 arch/x86/kernel/traps.c      |  107 +++++++++++++++++++++++------------------
 2 files changed, 60 insertions(+), 48 deletions(-)

[PATCH 1/6] x86/traps: Make math_error() static

[Oleg Nesterov]

Trivial, make math_error() static.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/include/asm/traps.h |    1 -
 arch/x86/kernel/traps.c      |    2 +-
 2 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/traps.h b/arch/x86/include/asm/traps.h
index 58d66fe..a7b212d 100644
--- a/arch/x86/include/asm/traps.h
+++ b/arch/x86/include/asm/traps.h
@@ -98,7 +98,6 @@ static inline int get_si_code(unsigned long condition)
 
 extern int panic_on_unrecovered_nmi;
 
-void math_error(struct pt_regs *, int, int);
 void math_emulate(struct math_emu_info *);
 #ifndef CONFIG_X86_32
 asmlinkage void smp_thermal_interrupt(void);
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 57409f6..8eddd62 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -488,7 +488,7 @@ exit:
  * the correct behaviour even in the presence of the asynchronous
  * IRQ13 behaviour
  */
-void math_error(struct pt_regs *regs, int error_code, int trapnr)
+static void math_error(struct pt_regs *regs, int error_code, int trapnr)
 {
 	struct task_struct *task = current;
 	siginfo_t info;
-- 
1.5.5.1

[PATCH 2/6] x86/traps: Use SEND_SIG_PRIV instead of force_sig()

[Oleg Nesterov]

force_sig() is just force_sig_info(SEND_SIG_PRIV). Imho it should die,
we have too many ugly "send signal" helpers.

And do_trap() looks just ugly because it uses force_sig_info() or
force_sig() depending on info != NULL.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/traps.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 8eddd62..2cd4291 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -168,10 +168,7 @@ do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
 	}
 #endif
 
-	if (info)
-		force_sig_info(signr, info, tsk);
-	else
-		force_sig(signr, tsk);
+	force_sig_info(signr, info ?: SEND_SIG_PRIV, tsk);
 }
 
 #define DO_ERROR(trapnr, signr, str, name)				\
@@ -305,7 +302,7 @@ do_general_protection(struct pt_regs *regs, long error_code)
 		pr_cont("\n");
 	}
 
-	force_sig(SIGSEGV, tsk);
+	force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk);
 exit:
 	exception_exit(prev_state);
 }
@@ -645,7 +642,7 @@ void math_state_restore(void)
 	 */
 	if (unlikely(restore_fpu_checking(tsk))) {
 		drop_init_fpu(tsk);
-		force_sig(SIGSEGV, tsk);
+		force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk);
 		return;
 	}
 
-- 
1.5.5.1

[PATCH 3/6] x86/traps: Introduce do_error_trap()

[Oleg Nesterov]

Move the common code from DO_ERROR() and DO_ERROR_INFO() into the new
helper, do_error_trap(). This simplifies define's and shaves 527 bytes
from traps.o.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/traps.c |   38 +++++++++++++++++---------------------
 1 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 2cd4291..ab8dad7 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -171,41 +171,37 @@ do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
 	force_sig_info(signr, info ?: SEND_SIG_PRIV, tsk);
 }
 
+static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
+			  unsigned long trapnr, int signr, siginfo_t *info)
+{
+	enum ctx_state prev_state = exception_enter();
+
+	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
+			NOTIFY_STOP) {
+		conditional_sti(regs);
+		do_trap(trapnr, signr, str, regs, error_code, info);
+	}
+
+	exception_exit(prev_state);
+}
+
 #define DO_ERROR(trapnr, signr, str, name)				\
 dotraplinkage void do_##name(struct pt_regs *regs, long error_code)	\
 {									\
-	enum ctx_state prev_state;					\
-									\
-	prev_state = exception_enter();					\
-	if (notify_die(DIE_TRAP, str, regs, error_code,			\
-			trapnr, signr) == NOTIFY_STOP) {		\
-		exception_exit(prev_state);				\
-		return;							\
-	}								\
-	conditional_sti(regs);						\
-	do_trap(trapnr, signr, str, regs, error_code, NULL);		\
-	exception_exit(prev_state);					\
+	do_error_trap(regs, error_code, str, trapnr, signr, NULL);	\
 }
 
 #define DO_ERROR_INFO(trapnr, signr, str, name, sicode, siaddr)		\
 dotraplinkage void do_##name(struct pt_regs *regs, long error_code)	\
 {									\
 	siginfo_t info;							\
-	enum ctx_state prev_state;					\
 									\
 	info.si_signo = signr;						\
 	info.si_errno = 0;						\
 	info.si_code = sicode;						\
 	info.si_addr = (void __user *)siaddr;				\
-	prev_state = exception_enter();					\
-	if (notify_die(DIE_TRAP, str, regs, error_code,			\
-			trapnr, signr) == NOTIFY_STOP) {		\
-		exception_exit(prev_state);				\
-		return;							\
-	}								\
-	conditional_sti(regs);						\
-	do_trap(trapnr, signr, str, regs, error_code, &info);		\
-	exception_exit(prev_state);					\
+									\
+	do_error_trap(regs, error_code, str, trapnr, signr, &info);	\
 }
 
 DO_ERROR_INFO(X86_TRAP_DE,     SIGFPE,  "divide error",			divide_error,		     FPE_INTDIV, regs->ip )
-- 
1.5.5.1

[PATCH 4/6] x86/traps: Introduce fill_trap_info(), simplify DO_ERROR_INFO()

[Oleg Nesterov]

Extract the fill-siginfo code from DO_ERROR_INFO() into the new helper,
fill_trap_info().

It can calculate si_code and si_addr looking at trapnr, so we can remove
these arguments from DO_ERROR_INFO() and simplify the source code. The
generated code is the same, __builtin_constant_p(trapnr) == T.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/traps.c |   53 +++++++++++++++++++++++++++++++++-------------
 1 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index ab8dad7..1cf8a4c 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -136,6 +136,33 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 	return -1;
 }
 
+static void fill_trap_info(struct pt_regs *regs, int signr, int trapnr,
+			   siginfo_t *info)
+{
+	unsigned long siaddr;
+	int sicode;
+
+	switch (trapnr) {
+	case X86_TRAP_DE:
+		sicode = FPE_INTDIV;
+		siaddr = regs->ip;
+		break;
+	case X86_TRAP_UD:
+		sicode = ILL_ILLOPN;
+		siaddr = regs->ip;
+		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;
+}
+
 static void __kprobes
 do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
 	long error_code, siginfo_t *info)
@@ -191,30 +218,26 @@ dotraplinkage void do_##name(struct pt_regs *regs, long error_code)	\
 	do_error_trap(regs, error_code, str, trapnr, signr, NULL);	\
 }
 
-#define DO_ERROR_INFO(trapnr, signr, str, name, sicode, siaddr)		\
+#define DO_ERROR_INFO(trapnr, signr, str, name)				\
 dotraplinkage void do_##name(struct pt_regs *regs, long error_code)	\
 {									\
 	siginfo_t info;							\
 									\
-	info.si_signo = signr;						\
-	info.si_errno = 0;						\
-	info.si_code = sicode;						\
-	info.si_addr = (void __user *)siaddr;				\
-									\
+	fill_trap_info(regs, signr, trapnr, &info);			\
 	do_error_trap(regs, error_code, str, trapnr, signr, &info);	\
 }
 
-DO_ERROR_INFO(X86_TRAP_DE,     SIGFPE,  "divide error",			divide_error,		     FPE_INTDIV, regs->ip )
-DO_ERROR     (X86_TRAP_OF,     SIGSEGV, "overflow",			overflow					  )
-DO_ERROR     (X86_TRAP_BR,     SIGSEGV, "bounds",			bounds						  )
-DO_ERROR_INFO(X86_TRAP_UD,     SIGILL,  "invalid opcode",		invalid_op,		     ILL_ILLOPN, regs->ip )
-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_INFO(X86_TRAP_DE,     SIGFPE,  "divide error",			divide_error)
+DO_ERROR     (X86_TRAP_OF,     SIGSEGV, "overflow",			overflow)
+DO_ERROR     (X86_TRAP_BR,     SIGSEGV, "bounds",			bounds)
+DO_ERROR_INFO(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)
 #ifdef CONFIG_X86_32
-DO_ERROR     (X86_TRAP_SS,     SIGBUS,  "stack segment",		stack_segment					  )
+DO_ERROR     (X86_TRAP_SS,     SIGBUS,  "stack segment",		stack_segment)
 #endif
-DO_ERROR_INFO(X86_TRAP_AC,     SIGBUS,  "alignment check",		alignment_check,	     BUS_ADRALN, 0	  )
+DO_ERROR_INFO(X86_TRAP_AC,     SIGBUS,  "alignment check",		alignment_check)
 
 #ifdef CONFIG_X86_64
 /* Runs on IST stack */
-- 
1.5.5.1

[PATCH 5/6] x86/traps: Shift fill_trap_info() from DO_ERROR_INFO() to do_error_trap()

[Oleg Nesterov]

Move the callsite of fill_trap_info() into do_error_trap() and remove
the "siginfo_t *info" argument.

This obviously breaks DO_ERROR() which passed info == NULL, we simply
change fill_trap_info() to return "siginfo_t *" and add the "default"
case which returns SEND_SIG_PRIV.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/traps.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 1cf8a4c..c9dd741 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -136,13 +136,16 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 	return -1;
 }
 
-static void fill_trap_info(struct pt_regs *regs, int signr, int trapnr,
-			   siginfo_t *info)
+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 = regs->ip;
@@ -161,6 +164,7 @@ static void fill_trap_info(struct pt_regs *regs, int signr, int trapnr,
 	info->si_errno = 0;
 	info->si_code = sicode;
 	info->si_addr = (void __user *)siaddr;
+	return info;
 }
 
 static void __kprobes
@@ -199,14 +203,16 @@ do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
 }
 
 static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
-			  unsigned long trapnr, int signr, siginfo_t *info)
+			  unsigned long trapnr, int signr)
 {
 	enum ctx_state prev_state = exception_enter();
+	siginfo_t info;
 
 	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
 			NOTIFY_STOP) {
 		conditional_sti(regs);
-		do_trap(trapnr, signr, str, regs, error_code, info);
+		do_trap(trapnr, signr, str, regs, error_code,
+			fill_trap_info(regs, signr, trapnr, &info));
 	}
 
 	exception_exit(prev_state);
@@ -215,16 +221,13 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
 #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, NULL);	\
+	do_error_trap(regs, error_code, str, trapnr, signr);		\
 }
 
 #define DO_ERROR_INFO(trapnr, signr, str, name)				\
 dotraplinkage void do_##name(struct pt_regs *regs, long error_code)	\
 {									\
-	siginfo_t info;							\
-									\
-	fill_trap_info(regs, signr, trapnr, &info);			\
-	do_error_trap(regs, error_code, str, trapnr, signr, &info);	\
+	do_error_trap(regs, error_code, str, trapnr, signr);		\
 }
 
 DO_ERROR_INFO(X86_TRAP_DE,     SIGFPE,  "divide error",			divide_error)
-- 
1.5.5.1

[PATCH 6/6] x86/traps: Kill DO_ERROR_INFO()

[Oleg Nesterov]

Now that DO_ERROR_INFO() doesn't differ from DO_ERROR() we can remove
it and use DO_ERROR() instead.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/traps.c |   24 +++++++++---------------
 1 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index c9dd741..73b3ea3 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -224,23 +224,17 @@ dotraplinkage void do_##name(struct pt_regs *regs, long error_code)	\
 	do_error_trap(regs, error_code, str, trapnr, signr);		\
 }
 
-#define DO_ERROR_INFO(trapnr, signr, str, name)				\
-dotraplinkage void do_##name(struct pt_regs *regs, long error_code)	\
-{									\
-	do_error_trap(regs, error_code, str, trapnr, signr);		\
-}
-
-DO_ERROR_INFO(X86_TRAP_DE,     SIGFPE,  "divide error",			divide_error)
-DO_ERROR     (X86_TRAP_OF,     SIGSEGV, "overflow",			overflow)
-DO_ERROR     (X86_TRAP_BR,     SIGSEGV, "bounds",			bounds)
-DO_ERROR_INFO(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_DE,     SIGFPE,  "divide error",		divide_error)
+DO_ERROR(X86_TRAP_OF,     SIGSEGV, "overflow",			overflow)
+DO_ERROR(X86_TRAP_BR,     SIGSEGV, "bounds",			bounds)
+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)
 #ifdef CONFIG_X86_32
-DO_ERROR     (X86_TRAP_SS,     SIGBUS,  "stack segment",		stack_segment)
+DO_ERROR(X86_TRAP_SS,     SIGBUS,  "stack segment",		stack_segment)
 #endif
-DO_ERROR_INFO(X86_TRAP_AC,     SIGBUS,  "alignment check",		alignment_check)
+DO_ERROR(X86_TRAP_AC,     SIGBUS,  "alignment check",		alignment_check)
 
 #ifdef CONFIG_X86_64
 /* Runs on IST stack */
-- 
1.5.5.1

can't we kill DIE_GPF ? (Was: x86/traps: cleanup DO_ERROR*() to prepare for uprobes fixes)

[Oleg Nesterov]

On 05/08, Oleg Nesterov wrote:
>
> For example, after this series
> we can convert math_error() into the "normal" DO_ERROR() user, and most probably
> we can do the same with do_general_protection().

As for do_general_protection(), the problem is DIE_GPF.

Masami, could you explain why it is needed ? kprobe_exceptions_notify()
is the only user, can't it use DIE_TRAP and check trapnr = X86_TRAP_GP ?

And if it can, probably we can do notify_die() at the start like other
DO_ERROR() functions do ?

IOW, any reason why the patch below is wrong?

Oleg.

--- x/arch/x86/include/asm/kdebug.h
+++ x/arch/x86/include/asm/kdebug.h
@@ -15,7 +15,6 @@ enum die_val {
 	DIE_DIE,
 	DIE_KERNELDEBUG,
 	DIE_TRAP,
-	DIE_GPF,
 	DIE_CALL,
 	DIE_PAGE_FAULT,
 	DIE_NMIUNKNOWN,
--- x/arch/x86/kernel/traps.c
+++ x/arch/x86/kernel/traps.c
@@ -283,6 +283,9 @@ do_general_protection(struct pt_regs *re
 	enum ctx_state prev_state;
 
 	prev_state = exception_enter();
+	if (notify_die(DIE_TRAP, "general protection fault", regs, error_code,
+			X86_TRAP_GP, SIGSEGV) == NOTIFY_STOP)
+		goto exit;
 	conditional_sti(regs);
 
 #ifdef CONFIG_X86_32
@@ -300,10 +303,7 @@ do_general_protection(struct pt_regs *re
 
 		tsk->thread.error_code = error_code;
 		tsk->thread.trap_nr = X86_TRAP_GP;
-		if (notify_die(DIE_GPF, "general protection fault", regs, error_code,
-			       X86_TRAP_GP, SIGSEGV) != NOTIFY_STOP)
-			die("general protection fault", regs, error_code);
-		goto exit;
+		die("general protection fault", regs, error_code);
 	}
 
 	tsk->thread.error_code = error_code;
--- x/arch/x86/kernel/kprobes/core.c
+++ x/arch/x86/kernel/kprobes/core.c
@@ -979,13 +979,14 @@ kprobe_exceptions_notify(struct notifier
 			ret = NOTIFY_STOP;
 		}
 		break;
-	case DIE_GPF:
+	case DIE_TRAP:
 		/*
 		 * To be potentially processing a kprobe fault and to
 		 * trust the result from kprobe_running(), we have
 		 * be non-preemptible.
 		 */
-		if (!preemptible() && kprobe_running() &&
+		if (args->trapnr == X86_TRAP_GP &&
+		    !preemptible() && kprobe_running() &&
 		    kprobe_fault_handler(args->regs, args->trapnr))
 			ret = NOTIFY_STOP;
 		break;

Re: can't we kill DIE_GPF ? (Was: x86/traps: cleanup DO_ERROR*() to prepare for uprobes fixes)

[Masami Hiramatsu]

(2014/05/09 23:07), Oleg Nesterov wrote:
> On 05/08, Oleg Nesterov wrote:
>>
>> For example, after this series
>> we can convert math_error() into the "normal" DO_ERROR() user, and most probably
>> we can do the same with do_general_protection().
> 
> As for do_general_protection(), the problem is DIE_GPF.
> 
> Masami, could you explain why it is needed ? kprobe_exceptions_notify()
> is the only user, can't it use DIE_TRAP and check trapnr = X86_TRAP_GP ?

Actually, this may be only for something which will happen on
single-stepping out-of-line. And yes, I can move it onto the DIE_TRAP :)

> 
> And if it can, probably we can do notify_die() at the start like other
> DO_ERROR() functions do ?

Agreed, it seems OK to me. (and seems better, since we can handle GPF
before changing task->thread struct)

Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Thanks a lot!

> 
> IOW, any reason why the patch below is wrong?
> 
> Oleg.
> 
> --- x/arch/x86/include/asm/kdebug.h
> +++ x/arch/x86/include/asm/kdebug.h
> @@ -15,7 +15,6 @@ enum die_val {
>  	DIE_DIE,
>  	DIE_KERNELDEBUG,
>  	DIE_TRAP,
> -	DIE_GPF,
>  	DIE_CALL,
>  	DIE_PAGE_FAULT,
>  	DIE_NMIUNKNOWN,
> --- x/arch/x86/kernel/traps.c
> +++ x/arch/x86/kernel/traps.c
> @@ -283,6 +283,9 @@ do_general_protection(struct pt_regs *re
>  	enum ctx_state prev_state;
>  
>  	prev_state = exception_enter();
> +	if (notify_die(DIE_TRAP, "general protection fault", regs, error_code,
> +			X86_TRAP_GP, SIGSEGV) == NOTIFY_STOP)
> +		goto exit;
>  	conditional_sti(regs);
>  
>  #ifdef CONFIG_X86_32
> @@ -300,10 +303,7 @@ do_general_protection(struct pt_regs *re
>  
>  		tsk->thread.error_code = error_code;
>  		tsk->thread.trap_nr = X86_TRAP_GP;
> -		if (notify_die(DIE_GPF, "general protection fault", regs, error_code,
> -			       X86_TRAP_GP, SIGSEGV) != NOTIFY_STOP)
> -			die("general protection fault", regs, error_code);
> -		goto exit;
> +		die("general protection fault", regs, error_code);
>  	}
>  
>  	tsk->thread.error_code = error_code;
> --- x/arch/x86/kernel/kprobes/core.c
> +++ x/arch/x86/kernel/kprobes/core.c
> @@ -979,13 +979,14 @@ kprobe_exceptions_notify(struct notifier
>  			ret = NOTIFY_STOP;
>  		}
>  		break;
> -	case DIE_GPF:
> +	case DIE_TRAP:
>  		/*
>  		 * To be potentially processing a kprobe fault and to
>  		 * trust the result from kprobe_running(), we have
>  		 * be non-preemptible.
>  		 */
> -		if (!preemptible() && kprobe_running() &&
> +		if (args->trapnr == X86_TRAP_GP &&
> +		    !preemptible() && kprobe_running() &&
>  		    kprobe_fault_handler(args->regs, args->trapnr))
>  			ret = NOTIFY_STOP;
>  		break;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: can't we kill DIE_GPF ? (Was: x86/traps: cleanup DO_ERROR*() to prepare for uprobes fixes)

[Oleg Nesterov]

On 05/13, Masami Hiramatsu wrote:
>
> (2014/05/09 23:07), Oleg Nesterov wrote:
> > On 05/08, Oleg Nesterov wrote:
> >>
> >> For example, after this series
> >> we can convert math_error() into the "normal" DO_ERROR() user, and most probably
> >> we can do the same with do_general_protection().
> >
> > As for do_general_protection(), the problem is DIE_GPF.
> >
> > Masami, could you explain why it is needed ? kprobe_exceptions_notify()
> > is the only user, can't it use DIE_TRAP and check trapnr = X86_TRAP_GP ?
>
> Actually, this may be only for something which will happen on
> single-stepping out-of-line. And yes, I can move it onto the DIE_TRAP :)
>
> >
> > And if it can, probably we can do notify_die() at the start like other
> > DO_ERROR() functions do ?
>
> Agreed, it seems OK to me. (and seems better, since we can handle GPF
> before changing task->thread struct)
>
> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Great, thanks!

I'll resend this patch with the changelog and other changes.

Oleg.

[PATCH 0/1] (Was: cleanup DO_ERROR*() to prepare for uprobes fixes)

[Oleg Nesterov]

On 05/08, Oleg Nesterov wrote:
>
> But let me send the initial changes first for review. If they pass the review
> and if nobody objects, I'd like to route them along with the pending uprobes
> fixes.

OK, nobody cares ;)

So I am going to ask Ingo to pull these changes plus the following minor fix.

To remind, I think that traps.c needs more cleanups: do_general_protection()
and math_error() should be converted into DO_ERROR(), DIE_GPF should die, and
the users of show_unhandled_signals() should be unified/cleanuped.

Ananth, David, this is x86 specific, but at first glance powerpc/arm might
want to use uprobe_get_trap_addr() too.

Oleg.

[PATCH 1/1] uprobes/x86: Fix the wrong ->si_addr when xol triggers a trap

[Oleg Nesterov]

If the probed insn triggers a trap, ->si_addr = regs->ip is technically
correct, but this is not what the signal handler wants; we need to pass
the address of the probed insn, not the address of xol slot.

Add the new arch-agnostic helper, uprobe_get_trap_addr(), and change
fill_trap_info() and math_error() to use it. !CONFIG_UPROBES case in
uprobes.h uses a macro to avoid include hell and ensure that it can be
compiled even if an architecture doesn't define instruction_pointer().

Test-case:

	#include <signal.h>
	#include <stdio.h>
	#include <unistd.h>

	extern void probe_div(void);

	void sigh(int sig, siginfo_t *info, void *c)
	{
		int passed = (info->si_addr == probe_div);
		printf(passed ? "PASS\n" : "FAIL\n");
		_exit(!passed);
	}

	int main(void)
	{
		struct sigaction sa = {
			.sa_sigaction	= sigh,
			.sa_flags	= SA_SIGINFO,
		};

		sigaction(SIGFPE, &sa, NULL);

		asm (
			"xor %ecx,%ecx\n"
			".globl probe_div; probe_div:\n"
			"idiv %ecx\n"
		);

		return 0;
	}

it fails if probe_div() is probed.

Note: show_unhandled_signals users should probably use this helper too,
but we need to cleanup them first.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/traps.c |    7 ++++---
 include/linux/uprobes.h |    4 ++++
 kernel/events/uprobes.c |   10 ++++++++++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 73b3ea3..3fdb205 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -23,6 +23,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/ptrace.h>
+#include <linux/uprobes.h>
 #include <linux/string.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
@@ -148,11 +149,11 @@ static siginfo_t *fill_trap_info(struct pt_regs *regs, int signr, int trapnr,
 
 	case X86_TRAP_DE:
 		sicode = FPE_INTDIV;
-		siaddr = regs->ip;
+		siaddr = uprobe_get_trap_addr(regs);
 		break;
 	case X86_TRAP_UD:
 		sicode = ILL_ILLOPN;
-		siaddr = regs->ip;
+		siaddr = uprobe_get_trap_addr(regs);
 		break;
 	case X86_TRAP_AC:
 		sicode = BUS_ADRALN;
@@ -531,7 +532,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)
 	task->thread.error_code = error_code;
 	info.si_signo = SIGFPE;
 	info.si_errno = 0;
-	info.si_addr = (void __user *)regs->ip;
+	info.si_addr = (void __user *)uprobe_get_trap_addr(regs);
 	if (trapnr == X86_TRAP_MF) {
 		unsigned short cwd, swd;
 		/*
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index edff2b9..88c3b7e 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -102,6 +102,7 @@ extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, u
 extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
 extern bool __weak is_trap_insn(uprobe_opcode_t *insn);
 extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
+extern unsigned long uprobe_get_trap_addr(struct pt_regs *regs);
 extern int uprobe_write_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t);
 extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
 extern int uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool);
@@ -130,6 +131,9 @@ extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *r
 #else /* !CONFIG_UPROBES */
 struct uprobes_state {
 };
+
+#define uprobe_get_trap_addr(regs)	instruction_pointer(regs)
+
 static inline int
 uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
 {
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index a13251e..3b02c72 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1351,6 +1351,16 @@ unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs)
 	return instruction_pointer(regs) - UPROBE_SWBP_INSN_SIZE;
 }
 
+unsigned long uprobe_get_trap_addr(struct pt_regs *regs)
+{
+	struct uprobe_task *utask = current->utask;
+
+	if (unlikely(utask && utask->active_uprobe))
+		return utask->vaddr;
+
+	return instruction_pointer(regs);
+}
+
 /*
  * Called with no locks held.
  * Called in context of a exiting or a exec-ing thread.
-- 
1.5.5.1

Re: [PATCH 1/1] uprobes/x86: Fix the wrong ->si_addr when xol triggers a trap

[Masami Hiramatsu]

(2014/05/13 2:08), Oleg Nesterov wrote:
> If the probed insn triggers a trap, ->si_addr = regs->ip is technically
> correct, but this is not what the signal handler wants; we need to pass
> the address of the probed insn, not the address of xol slot.
> 
> Add the new arch-agnostic helper, uprobe_get_trap_addr(), and change
> fill_trap_info() and math_error() to use it. !CONFIG_UPROBES case in
> uprobes.h uses a macro to avoid include hell and ensure that it can be
> compiled even if an architecture doesn't define instruction_pointer().
> 
> Test-case:
> 
> 	#include <signal.h>
> 	#include <stdio.h>
> 	#include <unistd.h>
> 
> 	extern void probe_div(void);
> 
> 	void sigh(int sig, siginfo_t *info, void *c)
> 	{
> 		int passed = (info->si_addr == probe_div);
> 		printf(passed ? "PASS\n" : "FAIL\n");
> 		_exit(!passed);
> 	}
> 
> 	int main(void)
> 	{
> 		struct sigaction sa = {
> 			.sa_sigaction	= sigh,
> 			.sa_flags	= SA_SIGINFO,
> 		};
> 
> 		sigaction(SIGFPE, &sa, NULL);
> 
> 		asm (
> 			"xor %ecx,%ecx\n"
> 			".globl probe_div; probe_div:\n"
> 			"idiv %ecx\n"
> 		);
> 
> 		return 0;
> 	}
> 
> it fails if probe_div() is probed.
> 
> Note: show_unhandled_signals users should probably use this helper too,
> but we need to cleanup them first.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Looks good to me :)

Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Thanks,

> ---
>  arch/x86/kernel/traps.c |    7 ++++---
>  include/linux/uprobes.h |    4 ++++
>  kernel/events/uprobes.c |   10 ++++++++++
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 73b3ea3..3fdb205 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -23,6 +23,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/ptrace.h>
> +#include <linux/uprobes.h>
>  #include <linux/string.h>
>  #include <linux/delay.h>
>  #include <linux/errno.h>
> @@ -148,11 +149,11 @@ static siginfo_t *fill_trap_info(struct pt_regs *regs, int signr, int trapnr,
>  
>  	case X86_TRAP_DE:
>  		sicode = FPE_INTDIV;
> -		siaddr = regs->ip;
> +		siaddr = uprobe_get_trap_addr(regs);
>  		break;
>  	case X86_TRAP_UD:
>  		sicode = ILL_ILLOPN;
> -		siaddr = regs->ip;
> +		siaddr = uprobe_get_trap_addr(regs);
>  		break;
>  	case X86_TRAP_AC:
>  		sicode = BUS_ADRALN;
> @@ -531,7 +532,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)
>  	task->thread.error_code = error_code;
>  	info.si_signo = SIGFPE;
>  	info.si_errno = 0;
> -	info.si_addr = (void __user *)regs->ip;
> +	info.si_addr = (void __user *)uprobe_get_trap_addr(regs);
>  	if (trapnr == X86_TRAP_MF) {
>  		unsigned short cwd, swd;
>  		/*
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index edff2b9..88c3b7e 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -102,6 +102,7 @@ extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, u
>  extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
>  extern bool __weak is_trap_insn(uprobe_opcode_t *insn);
>  extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
> +extern unsigned long uprobe_get_trap_addr(struct pt_regs *regs);
>  extern int uprobe_write_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t);
>  extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
>  extern int uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool);
> @@ -130,6 +131,9 @@ extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *r
>  #else /* !CONFIG_UPROBES */
>  struct uprobes_state {
>  };
> +
> +#define uprobe_get_trap_addr(regs)	instruction_pointer(regs)
> +
>  static inline int
>  uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
>  {
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index a13251e..3b02c72 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1351,6 +1351,16 @@ unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs)
>  	return instruction_pointer(regs) - UPROBE_SWBP_INSN_SIZE;
>  }
>  
> +unsigned long uprobe_get_trap_addr(struct pt_regs *regs)
> +{
> +	struct uprobe_task *utask = current->utask;
> +
> +	if (unlikely(utask && utask->active_uprobe))
> +		return utask->vaddr;
> +
> +	return instruction_pointer(regs);
> +}
> +
>  /*
>   * Called with no locks held.
>   * Called in context of a exiting or a exec-ing thread.
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 0/1] (Was: cleanup DO_ERROR*() to prepare for uprobes fixes)

[David Long]

On 05/12/14 13:08, Oleg Nesterov wrote:
> On 05/08, Oleg Nesterov wrote:
>>
>> But let me send the initial changes first for review. If they pass the review
>> and if nobody objects, I'd like to route them along with the pending uprobes
>> fixes.
>
> OK, nobody cares ;)
>
> So I am going to ask Ingo to pull these changes plus the following minor fix.
>
> To remind, I think that traps.c needs more cleanups: do_general_protection()
> and math_error() should be converted into DO_ERROR(), DIE_GPF should die, and
> the users of show_unhandled_signals() should be unified/cleanuped.
>
> Ananth, David, this is x86 specific, but at first glance powerpc/arm might
> want to use uprobe_get_trap_addr() too.
>
> Oleg.
>

Thanks for the heads-up.  I currently have a preliminary patch to use 
the PC LSB to identify Thumb-mode instructions on ARM but I don't think 
this adds any new hinderances to that in the intended use case.

-dl

Re: [PATCH 0/1] (Was: cleanup DO_ERROR*() to prepare for uprobes fixes)

[Ananth N Mavinakayanahalli]

On Mon, May 12, 2014 at 07:08:22PM +0200, Oleg Nesterov wrote:
> On 05/08, Oleg Nesterov wrote:
> >
> > But let me send the initial changes first for review. If they pass the review
> > and if nobody objects, I'd like to route them along with the pending uprobes
> > fixes.
> 
> OK, nobody cares ;)
> 
> So I am going to ask Ingo to pull these changes plus the following minor fix.
> 
> To remind, I think that traps.c needs more cleanups: do_general_protection()
> and math_error() should be converted into DO_ERROR(), DIE_GPF should die, and
> the users of show_unhandled_signals() should be unified/cleanuped.
> 
> Ananth, David, this is x86 specific, but at first glance powerpc/arm might
> want to use uprobe_get_trap_addr() too.

Thanks for the heads-up Oleg.

Ananth