Re: [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead
On Thu, Feb 15, 2007 at 08:01:20AM +, Jan Beulich wrote: > Ack. Great, thanks for your help. Jeff -- Work email - jdike at linux dot intel dot com - 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/
Re: [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead
>>> Jeff Dike <[EMAIL PROTECTED]> 14.02.07 18:51 >>> >On Tue, Feb 13, 2007 at 07:52:54AM +, 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. - 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/
Re: [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead
On Tue, Feb 13, 2007 at 07:52:54AM +, 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_I
Re: [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead
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 - 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/
Re: [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead
>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 - 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/
Re: [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead
>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 - 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/
Re: [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead
On Mon, Feb 12, 2007 at 09:32:10AM +, 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
-
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/
Re: [patches] [PATCH 2.6.21 review I] [4/25] x86: kernel-mode faults pollute current->thead
>>> Andi Kleen <[EMAIL PROTECTED]> 10.02.07 12:50 >>> > >From: Jeff Dike <[EMAIL PROTECTED]> > >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 - 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/
