LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Re: [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead
@ 2007-02-13  7:52 Jan Beulich
  2007-02-13 10:00 ` Andi Kleen
  2007-02-14 17:51 ` Jeff Dike
  0 siblings, 2 replies; 8+ messages in thread
From: Jan Beulich @ 2007-02-13  7:52 UTC (permalink / raw)
  To: Jeff Dike; +Cc: Andi Kleen, linux-kernel, patches

>Yup.  How does this patch look to you?  We set error_code and trap_no
>for userspace faults and kernel faults which call die().  We don't set
>them for kernelspace faults which are fixed up.

Actually, after a second round of thinking I believe there's still more to do
- your second patch missed fixing i386's do_trap() similarly to x86-64's
and, vice versa, x86-64's do_general_protection() similarly to i386's.

Jan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead
  2007-02-13  7:52 [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead Jan Beulich
@ 2007-02-13 10:00 ` Andi Kleen
  2007-02-14 17:51 ` Jeff Dike
  1 sibling, 0 replies; 8+ messages in thread
From: Andi Kleen @ 2007-02-13 10:00 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Jeff Dike, linux-kernel, patches

On Tuesday 13 February 2007 08:52, Jan Beulich wrote:
> >Yup.  How does this patch look to you?  We set error_code and trap_no
> >for userspace faults and kernel faults which call die().  We don't set
> >them for kernelspace faults which are fixed up.
> 
> Actually, after a second round of thinking I believe there's still more to do
> - your second patch missed fixing i386's do_trap() similarly to x86-64's
> and, vice versa, x86-64's do_general_protection() similarly to i386's.

I dropped the patch for now until that is all worked out

-Andi

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead
  2007-02-13  7:52 [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead Jan Beulich
  2007-02-13 10:00 ` Andi Kleen
@ 2007-02-14 17:51 ` Jeff Dike
  2007-02-15  8:01   ` Jan Beulich
  1 sibling, 1 reply; 8+ messages in thread
From: Jeff Dike @ 2007-02-14 17:51 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Andi Kleen, linux-kernel, patches

On Tue, Feb 13, 2007 at 07:52:54AM +0000, Jan Beulich wrote:
> Actually, after a second round of thinking I believe there's still more to do
> - your second patch missed fixing i386's do_trap() similarly to x86-64's
> and, vice versa, x86-64's do_general_protection() similarly to i386's.

Sigh, here's another go at it - the full patch instead of
incrementally fixing the old one:

Index: linux-2.6/arch/i386/kernel/traps.c
===================================================================
--- linux-2.6.orig/arch/i386/kernel/traps.c
+++ linux-2.6/arch/i386/kernel/traps.c
@@ -473,8 +473,6 @@ static void __kprobes do_trap(int trapnr
 			      siginfo_t *info)
 {
 	struct task_struct *tsk = current;
-	tsk->thread.error_code = error_code;
-	tsk->thread.trap_no = trapnr;
 
 	if (regs->eflags & VM_MASK) {
 		if (vm86)
@@ -486,6 +484,9 @@ static void __kprobes do_trap(int trapnr
 		goto kernel_trap;
 
 	trap_signal: {
+		tsk->thread.error_code = error_code;
+		tsk->thread.trap_no = trapnr;
+
 		if (info)
 			force_sig_info(signr, info, tsk);
 		else
@@ -494,8 +495,11 @@ static void __kprobes do_trap(int trapnr
 	}
 
 	kernel_trap: {
-		if (!fixup_exception(regs))
+		if (!fixup_exception(regs)) {
+			tsk->thread.error_code = error_code;
+			tsk->thread.trap_no = trapnr;
 			die(str, regs, error_code);
+		}
 		return;
 	}
 
@@ -600,15 +604,21 @@ fastcall void __kprobes do_general_prote
 	}
 	put_cpu();
 
-	current->thread.error_code = error_code;
-	current->thread.trap_no = 13;
-
 	if (regs->eflags & VM_MASK)
 		goto gp_in_vm86;
 
 	if (!user_mode(regs))
 		goto gp_in_kernel;
 
+	/*
+	 * We want error_code and trap_no 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, fault.
+	 */
 	current->thread.error_code = error_code;
 	current->thread.trap_no = 13;
 	force_sig(SIGSEGV, current);
@@ -621,6 +631,8 @@ gp_in_vm86:
 
 gp_in_kernel:
 	if (!fixup_exception(regs)) {
+		current->thread.error_code = error_code;
+		current->thread.trap_no = 13;
 		if (notify_die(DIE_GPF, "general protection fault", regs,
 				error_code, 13, SIGSEGV) == NOTIFY_STOP)
 			return;
Index: linux-2.6/arch/x86_64/kernel/traps.c
===================================================================
--- linux-2.6.orig/arch/x86_64/kernel/traps.c
+++ linux-2.6/arch/x86_64/kernel/traps.c
@@ -581,10 +581,19 @@ static void __kprobes do_trap(int trapnr
 {
 	struct task_struct *tsk = current;
 
-	tsk->thread.error_code = error_code;
-	tsk->thread.trap_no = trapnr;
-
 	if (user_mode(regs)) {
+		/*
+		 * We want error_code and trap_no 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, fault.
+		 */
+		tsk->thread.error_code = error_code;
+		tsk->thread.trap_no = trapnr;
+
 		if (exception_trace && unhandled_signal(tsk, signr))
 			printk(KERN_INFO
 			       "%s[%d] trap %s rip:%lx rsp:%lx error:%lx\n",
@@ -605,8 +614,11 @@ static void __kprobes do_trap(int trapnr
 		fixup = search_exception_tables(regs->rip);
 		if (fixup)
 			regs->rip = fixup->fixup;
-		else	
+		else {
+			tsk->thread.error_code = error_code;
+			tsk->thread.trap_no = trapnr;
 			die(str, regs, error_code);
+		}
 		return;
 	}
 }
@@ -682,10 +694,10 @@ asmlinkage void __kprobes do_general_pro
 
 	conditional_sti(regs);
 
-	tsk->thread.error_code = error_code;
-	tsk->thread.trap_no = 13;
-
 	if (user_mode(regs)) {
+		tsk->thread.error_code = error_code;
+		tsk->thread.trap_no = 13;
+
 		if (exception_trace && unhandled_signal(tsk, SIGSEGV))
 			printk(KERN_INFO
 		       "%s[%d] general protection rip:%lx rsp:%lx error:%lx\n",
@@ -704,6 +716,10 @@ asmlinkage void __kprobes do_general_pro
 			regs->rip = fixup->fixup;
 			return;
 		}
+
+
+		tsk->thread.error_code = error_code;
+		tsk->thread.trap_no = 13;
 		if (notify_die(DIE_GPF, "general protection fault", regs,
 					error_code, 13, SIGSEGV) == NOTIFY_STOP)
 			return;

-- 
Work email - jdike at linux dot intel dot com

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead
  2007-02-14 17:51 ` Jeff Dike
@ 2007-02-15  8:01   ` Jan Beulich
  2007-02-15 16:23     ` Jeff Dike
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2007-02-15  8:01 UTC (permalink / raw)
  To: Jeff Dike; +Cc: Andi Kleen, linux-kernel, patches

>>> Jeff Dike <jdike@addtoit.com> 14.02.07 18:51 >>>
>On Tue, Feb 13, 2007 at 07:52:54AM +0000, Jan Beulich wrote:
>> Actually, after a second round of thinking I believe there's still more to do
>> - your second patch missed fixing i386's do_trap() similarly to x86-64's
>> and, vice versa, x86-64's do_general_protection() similarly to i386's.
>
>Sigh, here's another go at it - the full patch instead of
>incrementally fixing the old one:

Ack.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead
  2007-02-15  8:01   ` Jan Beulich
@ 2007-02-15 16:23     ` Jeff Dike
  0 siblings, 0 replies; 8+ messages in thread
From: Jeff Dike @ 2007-02-15 16:23 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Andi Kleen, linux-kernel, patches

On Thu, Feb 15, 2007 at 08:01:20AM +0000, Jan Beulich wrote:
> Ack.

Great, thanks for your help.

			Jeff

-- 
Work email - jdike at linux dot intel dot com

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead
  2007-02-12 16:42     ` Jeff Dike
@ 2007-02-12 17:01       ` Jan Beulich
  0 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2007-02-12 17:01 UTC (permalink / raw)
  To: Jeff Dike; +Cc: Andi Kleen, linux-kernel, patches

>Yup.  How does this patch look to you?  We set error_code and trap_no
>for userspace faults and kernel faults which call die().  We don't set
>them for kernelspace faults which are fixed up.

That seems a reasonable approach.

Thanks, Jan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead
  2007-02-12  9:32   ` [patches] " Jan Beulich
@ 2007-02-12 16:42     ` Jeff Dike
  2007-02-12 17:01       ` Jan Beulich
  0 siblings, 1 reply; 8+ messages in thread
From: Jeff Dike @ 2007-02-12 16:42 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Andi Kleen, linux-kernel, patches

On Mon, Feb 12, 2007 at 09:32:10AM +0000, Jan Beulich wrote:
> This breaks consumers of notify_die() relying on the proper trap number being
> passed, as the call to notify_die() from die() currently reads
> current->thread.trap_no.

Rats, good point.

> Also, you seem to leave other places where trap_no gets set untouched -
> is this intentional (do_debug - probably correct here, kernel_math_error -
> probably incorrect here)?

I did check the other trap handlers.  kernel_math_error calls die,
which calls do_exit(SIGSEGV).  This doesn't seem to allow the process
the opportunity to trap the SIGSEGV and examine the fault information.

> >I looked at i386, and there is a similar situation.  In this case, there is
> >duplicate code setting task->thread.error_code and trapno.  I deleted one,
> >leaving the copy that runs in the case of a userspace fault.
> 
> Likewise.

Yup.  How does this patch look to you?  We set error_code and trap_no
for userspace faults and kernel faults which call die().  We don't set
them for kernelspace faults which are fixed up.

Index: linux-2.6/arch/i386/kernel/traps.c
===================================================================
--- linux-2.6.orig/arch/i386/kernel/traps.c
+++ linux-2.6/arch/i386/kernel/traps.c
@@ -619,6 +619,8 @@ gp_in_vm86:
 
 gp_in_kernel:
 	if (!fixup_exception(regs)) {
+		current->thread.error_code = error_code;
+		current->thread.trap_no = 13;
 		if (notify_die(DIE_GPF, "general protection fault", regs,
 				error_code, 13, SIGSEGV) == NOTIFY_STOP)
 			return;
Index: linux-2.6/arch/x86_64/kernel/traps.c
===================================================================
--- linux-2.6.orig/arch/x86_64/kernel/traps.c
+++ linux-2.6/arch/x86_64/kernel/traps.c
@@ -605,8 +605,11 @@ static void __kprobes do_trap(int trapnr
 		fixup = search_exception_tables(regs->rip);
 		if (fixup)
 			regs->rip = fixup->fixup;
-		else	
+		else {
+			tsk->thread.error_code = error_code;
+			tsk->thread.trap_no = trapnr;
 			die(str, regs, error_code);
+		}
 		return;
 	}
 }


-- 
Work email - jdike at linux dot intel dot com


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead
  2007-02-10 11:50 ` [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead Andi Kleen
@ 2007-02-12  9:32   ` Jan Beulich
  2007-02-12 16:42     ` Jeff Dike
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2007-02-12  9:32 UTC (permalink / raw)
  To: Jeff Dike, Andi Kleen; +Cc: linux-kernel, patches

>>> Andi Kleen <ak@suse.de> 10.02.07 12:50 >>>
>
>From: Jeff Dike <jdike@addtoit.com>
>
>Kernel-mode traps on x86_64 can pollute the trap information for a previous
>userspace trap for which the signal has not yet been delivered to the
>process.
>
>do_trap and do_general_protection set task->thread.error_code and .trapno
>for kernel traps.  If a kernel-mode trap arrives between the arrival of a
>userspace trap and the delivery of the associated SISGEGV to the process,
>the process will get the kernel trap information in its sigcontext.
>
>This causes UML process segfaults, as the trapno that the UML kernel sees
>is 13, rather than the 14 for normal page faults.  So, the UML kernel
>passes the SIGSEGV along to its process.
>
>I don't claim to fully understand the problem.  On the one hand, a check in
>do_general_protection for a pending SIGSEGV turned up nothing.  On the
>other hand, this patch fixed the UML process segfault problem.
>
>The patch below moves the setting of error_code and trapno so that that
>only happens in the case of userspace faults.  As a side-effect, this
>should speed up kernel-mode fault handling a tiny bit.

This breaks consumers of notify_die() relying on the proper trap number being
passed, as the call to notify_die() from die() currently reads
current->thread.trap_no.

Also, you seem to leave other places where trap_no gets set untouched -
is this intentional (do_debug - probably correct here, kernel_math_error -
probably incorrect here)?

>I looked at i386, and there is a similar situation.  In this case, there is
>duplicate code setting task->thread.error_code and trapno.  I deleted one,
>leaving the copy that runs in the case of a userspace fault.

Likewise.

Jan

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2007-02-15 16:31 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-02-13  7:52 [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead Jan Beulich
2007-02-13 10:00 ` Andi Kleen
2007-02-14 17:51 ` Jeff Dike
2007-02-15  8:01   ` Jan Beulich
2007-02-15 16:23     ` Jeff Dike
  -- strict thread matches above, loose matches on Subject: below --
2007-02-10 11:50 [PATCH 2.6.21 review I] [1/25] x86_64: Add __copy_from_user_nocache Andi Kleen
2007-02-10 11:50 ` [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead Andi Kleen
2007-02-12  9:32   ` [patches] " Jan Beulich
2007-02-12 16:42     ` Jeff Dike
2007-02-12 17:01       ` Jan Beulich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).