x86/traps: cleanup DO_ERROR*() to prepare for uprobes fixes

PATCH In review Oleg Nesterov 4 participants 15 messages May 8, 2014 – May 13, 2014

Subsystems (3)

x86 perf events uprobes

lore

Share & tools

lore

Oleg Nesterov

author maintainer

2014-05-08 19:12 UTC #0 lore raw

cover x86/traps: cleanup DO_ERROR*() to prepare for uprobes fixes

Hello.

Quoted 7 lines · Oleg Nesterov

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(-)

Oleg Nesterov

author maintainer

2014-05-08 19:12 UTC #1 lore raw

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

Trivial, make math_error() static.

Signed-off-by Oleg Nesterov <[email protected]>

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

arch/x86/include/asm/traps.h +0 -1

index 58d66fe..a7b212d 100644
@@ -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);

arch/x86/kernel/traps.c +1 -1

index 57409f6..8eddd62 100644
@@ -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;

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2014-05-08 19:13 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---
 arch/x86/kernel/traps.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

arch/x86/kernel/traps.c +3 -6

index 8eddd62..2cd4291 100644
@@ -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;
 	}

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2014-05-08 19:12 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 arch/x86/kernel/traps.c |   38 +++++++++++++++++---------------------
 1 files changed, 17 insertions(+), 21 deletions(-)

arch/x86/kernel/traps.c +17 ~~-21~~

index 2cd4291..ab8dad7 100644
@@ -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 )

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2014-05-08 19:12 UTC #4 lore raw

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

In reply to 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 <[email protected]>

---
 arch/x86/kernel/traps.c |   53 +++++++++++++++++++++++++++++++++-------------
 1 files changed, 38 insertions(+), 15 deletions(-)

arch/x86/kernel/traps.c +38 ~~-15~~

index ab8dad7..1cf8a4c 100644
@@ -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 */

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2014-05-08 19:13 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 arch/x86/kernel/traps.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

arch/x86/kernel/traps.c +12 -9

index 1cf8a4c..c9dd741 100644
@@ -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)

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2014-05-08 19:12 UTC #6 lore raw

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

In reply to 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 <[email protected]>

---
 arch/x86/kernel/traps.c |   24 +++++++++---------------
 1 files changed, 9 insertions(+), 15 deletions(-)

arch/x86/kernel/traps.c +9 ~~-15~~

index c9dd741..73b3ea3 100644
@@ -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 */

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2014-05-09 14:07 UTC #7 lore raw

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

In reply to Oleg Nesterov

Quoted 4 lines · Oleg Nesterov

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.

arch/x86/include/asm/kdebug.h +0 -1

@@ -15,7 +15,6 @@ enum die_val {
 	DIE_DIE,
 	DIE_KERNELDEBUG,
 	DIE_TRAP,
-	DIE_GPF,
 	DIE_CALL,
 	DIE_PAGE_FAULT,
 	DIE_NMIUNKNOWN,

arch/x86/kernel/traps.c +4 -4

@@ -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;

arch/x86/kernel/kprobes/core.c +3 -2

@@ -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;

Masami Hiramatsu

Acked-by

2014-05-13 06:07 UTC #8 lore raw

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

Quoted 10 lines · Oleg Nesterov

Quoted 4 lines · Oleg Nesterov
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 :)

Quoted 3 lines

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 <[email protected]>

Thanks a lot!

Quoted 65 lines

IOW, any reason why the patch below is wrong?

Oleg.

arch/x86/include/asm/kdebug.h +0 -1

@@ -15,7 +15,6 @@ enum die_val {
 	DIE_DIE,
 	DIE_KERNELDEBUG,
 	DIE_TRAP,
-	DIE_GPF,
 	DIE_CALL,
 	DIE_PAGE_FAULT,
 	DIE_NMIUNKNOWN,

arch/x86/kernel/traps.c +4 -4

@@ -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;

arch/x86/kernel/kprobes/core.c +3 -3

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 4 lines

Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

Oleg Nesterov

author maintainer

2014-05-13 17:12 UTC #9 lore raw

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

Quoted 17 lines · Masami Hiramatsu

Quoted 10 lines · Oleg Nesterov
Quoted 4 lines · Oleg Nesterov
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 :)
Quoted 1 line

Quoted 7 lines

Quoted 2 lines
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 <[email protected]>

Great, thanks!

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

Oleg.

Oleg Nesterov

author maintainer

2014-05-12 17:09 UTC #10 lore raw

cover (Was: cleanup DO_ERROR*() to prepare for uprobes fixes)

In reply to Oleg Nesterov

Quoted 4 lines · Oleg Nesterov

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.

Oleg Nesterov

author maintainer

2014-05-12 17:10 UTC #11 lore raw

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

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 <[email protected]>

---
 arch/x86/kernel/traps.c |    7 ++++---
 include/linux/uprobes.h |    4 ++++
 kernel/events/uprobes.c |   10 ++++++++++
 3 files changed, 18 insertions(+), 3 deletions(-)

arch/x86/kernel/traps.c +4 -3

index 73b3ea3..3fdb205 100644
@@ -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;
 		/*

include/linux/uprobes.h +4 -0

index edff2b9..88c3b7e 100644
@@ -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)
 {

kernel/events/uprobes.c +10 -0

index a13251e..3b02c72 100644
@@ -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.

Signature · 1 line

1.5.5.1

Masami Hiramatsu

Reviewed-by on v1 1/1

2014-05-13 06:23 UTC #12 lore raw

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

Quoted 42 lines · 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;
	}

Quoted 6 lines

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 <[email protected]>

Looks good to me :)

Reviewed-by Masami Hiramatsu <[email protected]>

Thanks,

Quoted 85 lines

---
 arch/x86/kernel/traps.c |    7 ++++---
 include/linux/uprobes.h |    4 ++++
 kernel/events/uprobes.c |   10 ++++++++++
 3 files changed, 18 insertions(+), 3 deletions(-)

arch/x86/kernel/traps.c +4 -3

index 73b3ea3..3fdb205 100644
@@ -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;
 		/*

include/linux/uprobes.h +4 -0

index edff2b9..88c3b7e 100644
@@ -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)
 {

kernel/events/uprobes.c +10 -0

index a13251e..3b02c72 100644
@@ -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.

Signature · 4 lines

Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

David Long

2014-05-12 19:39 UTC #13 lore raw

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

In reply to Oleg Nesterov

Quoted 10 lines · Oleg Nesterov

Quoted 4 lines · Oleg Nesterov
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.

Quoted 9 lines

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

Ananth N Mavinakayanahalli

2014-05-13 05:10 UTC #14 lore raw

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

In reply to Oleg Nesterov

Quoted 8 lines · Oleg Nesterov

Quoted 4 lines · Oleg Nesterov
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 ;)

Quoted 8 lines

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