Traps enable interrupts conditionally but rely on the ASM return code to
disable them again. That results in redundant interrupt disable and trace
calls.
Make the trap handlers disable interrupts before returning to avoid that,
which allows simplification of the ASM entry code.
Originally-by: Peter Zijlstra <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/kernel/traps.c | 28 +++++++++++++++++++---------
arch/x86/mm/fault.c | 15 +++++++++++++--
2 files changed, 32 insertions(+), 11 deletions(-)
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -201,6 +201,7 @@ static void do_error_trap(struct pt_regs
NOTIFY_STOP) {
cond_local_irq_enable(regs);
do_trap(trapnr, signr, str, regs, error_code, sicode, addr);
+ cond_local_irq_disable(regs);
}
}
@@ -399,6 +400,8 @@ dotraplinkage void do_bounds(struct pt_r
die("bounds", regs, error_code);
do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, 0, NULL);
+
+ cond_local_irq_disable(regs);
}
enum kernel_gp_hint {
@@ -458,12 +461,13 @@ dotraplinkage void do_general_protection
if (static_cpu_has(X86_FEATURE_UMIP)) {
if (user_mode(regs) && fixup_umip_exception(regs))
- return;
+ goto exit;
}
if (v8086_mode(regs)) {
local_irq_enable();
handle_vm86_fault((struct kernel_vm86_regs *) regs, error_code);
+ local_irq_disable();
return;
}
@@ -475,12 +479,11 @@ dotraplinkage void do_general_protection
show_signal(tsk, SIGSEGV, "", desc, regs, error_code);
force_sig(SIGSEGV);
-
- return;
+ goto exit;
}
if (fixup_exception(regs, X86_TRAP_GP, error_code, 0))
- return;
+ goto exit;
tsk->thread.error_code = error_code;
tsk->thread.trap_nr = X86_TRAP_GP;
@@ -492,11 +495,11 @@ dotraplinkage void do_general_protection
if (!preemptible() &&
kprobe_running() &&
kprobe_fault_handler(regs, X86_TRAP_GP))
- return;
+ goto exit;
ret = notify_die(DIE_GPF, desc, regs, error_code, X86_TRAP_GP, SIGSEGV);
if (ret == NOTIFY_STOP)
- return;
+ goto exit;
if (error_code)
snprintf(desc, sizeof(desc), "segment-related " GPFSTR);
@@ -518,6 +521,8 @@ dotraplinkage void do_general_protection
die_addr(desc, regs, error_code, gp_addr);
+exit:
+ cond_local_irq_disable(regs);
}
NOKPROBE_SYMBOL(do_general_protection);
@@ -775,7 +780,7 @@ static void math_error(struct pt_regs *r
if (!user_mode(regs)) {
if (fixup_exception(regs, trapnr, error_code, 0))
- return;
+ goto exit;
task->thread.error_code = error_code;
task->thread.trap_nr = trapnr;
@@ -783,7 +788,7 @@ static void math_error(struct pt_regs *r
if (notify_die(DIE_TRAP, str, regs, error_code,
trapnr, SIGFPE) != NOTIFY_STOP)
die(str, regs, error_code);
- return;
+ goto exit;
}
/*
@@ -797,10 +802,12 @@ static void math_error(struct pt_regs *r
si_code = fpu__exception_code(fpu, trapnr);
/* Retry when we get spurious exceptions: */
if (!si_code)
- return;
+ goto exit;
force_sig_fault(SIGFPE, si_code,
(void __user *)uprobe_get_trap_addr(regs));
+exit:
+ cond_local_irq_disable(regs);
}
dotraplinkage void do_coprocessor_error(struct pt_regs *regs, long error_code)
@@ -855,6 +862,8 @@ do_device_not_available(struct pt_regs *
info.regs = regs;
math_emulate(&info);
+
+ cond_local_irq_disable(regs);
return;
}
#endif
@@ -885,6 +894,7 @@ dotraplinkage void do_iret_error(struct
do_trap(X86_TRAP_IRET, SIGILL, "iret exception", regs, error_code,
ILL_BADSTK, (void __user *)NULL);
}
+ local_irq_disable();
}
#endif
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -927,6 +927,8 @@ static void
force_sig_fault(SIGSEGV, si_code, (void __user *)address);
+ local_irq_disable();
+
return;
}
@@ -1548,9 +1550,18 @@ do_page_fault(struct pt_regs *regs, unsi
return;
/* Was the fault on kernel-controlled part of the address space? */
- if (unlikely(fault_in_kernel_space(address)))
+ if (unlikely(fault_in_kernel_space(address))) {
do_kern_addr_fault(regs, hw_error_code, address);
- else
+ } else {
do_user_addr_fault(regs, hw_error_code, address);
+ /*
+ * User address page fault handling might have reenabled
+ * interrupts. Fixing up all potential exit points of
+ * do_user_addr_fault() and its leaf functions is just not
+ * doable w/o creating an unholy mess or turning the code
+ * upside down.
+ */
+ local_irq_disable();
+ }
}
NOKPROBE_SYMBOL(do_page_fault);
On 5/5/20 3:43 PM, Thomas Gleixner wrote:
> Traps enable interrupts conditionally but rely on the ASM return code to
> disable them again. That results in redundant interrupt disable and trace
> calls.
>
> Make the trap handlers disable interrupts before returning to avoid that,
> which allows simplification of the ASM entry code.
>
> Originally-by: Peter Zijlstra <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
>
> ---
> arch/x86/kernel/traps.c | 28 +++++++++++++++++++---------
> arch/x86/mm/fault.c | 15 +++++++++++++--
> 2 files changed, 32 insertions(+), 11 deletions(-)
>
So this patch makes C trap handlers disable interrupts on return but there's no
change to the ASM entry code, which will still (also) disable interrupts. I suppose
this is cleaned up in a next patch. So it's worth mentioning that the "simplification
of the ASM entry code" is not in this patch.
alex.
Alexandre Chartre <[email protected]> writes:
> On 5/5/20 3:43 PM, Thomas Gleixner wrote:
>> Traps enable interrupts conditionally but rely on the ASM return code to
>> disable them again. That results in redundant interrupt disable and trace
>> calls.
>>
>> Make the trap handlers disable interrupts before returning to avoid that,
>> which allows simplification of the ASM entry code.
>>
>> Originally-by: Peter Zijlstra <[email protected]>
>> Signed-off-by: Thomas Gleixner <[email protected]>
>>
>> ---
>> arch/x86/kernel/traps.c | 28 +++++++++++++++++++---------
>> arch/x86/mm/fault.c | 15 +++++++++++++--
>> 2 files changed, 32 insertions(+), 11 deletions(-)
>>
>
> So this patch makes C trap handlers disable interrupts on return but there's no
> change to the ASM entry code, which will still (also) disable interrupts. I suppose
> this is cleaned up in a next patch. So it's worth mentioning that the "simplification
> of the ASM entry code" is not in this patch.
I thought that was expressed by:
>> which allows simplification of the ASM entry code.
but yeah it's ambigous. Will clarify.
Thanks,
tglx
On Tue, May 5, 2020 at 7:15 AM Thomas Gleixner <[email protected]> wrote:
>
> Traps enable interrupts conditionally but rely on the ASM return code to
> disable them again. That results in redundant interrupt disable and trace
> calls.
>
> Make the trap handlers disable interrupts before returning to avoid that,
> which allows simplification of the ASM entry code.
Acked-by: Andy Lutomirski <[email protected]>
And thanks! This has bothered me forever.
The following commit has been merged into the x86/entry branch of tip:
Commit-ID: 1b208500418d351c476e173f2e4bebaaf0959ef0
Gitweb: https://git.kernel.org/tip/1b208500418d351c476e173f2e4bebaaf0959ef0
Author: Thomas Gleixner <[email protected]>
AuthorDate: Wed, 23 Oct 2019 14:27:10 +02:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Tue, 19 May 2020 16:03:54 +02:00
x86/traps: Make interrupt enable/disable symmetric in C code
Traps enable interrupts conditionally but rely on the ASM return code to
disable them again. That results in redundant interrupt disable and trace
calls.
Make the trap handlers disable interrupts before returning to avoid that,
which allows simplification of the ASM entry code in follow up changes.
Originally-by: Peter Zijlstra <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Alexandre Chartre <[email protected]>
Acked-by: Peter Zijlstra <[email protected]>
Acked-by: Andy Lutomirski <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/kernel/traps.c | 28 +++++++++++++++++++---------
arch/x86/mm/fault.c | 15 +++++++++++++--
2 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index adcc623..f5f4a76 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -201,6 +201,7 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
NOTIFY_STOP) {
cond_local_irq_enable(regs);
do_trap(trapnr, signr, str, regs, error_code, sicode, addr);
+ cond_local_irq_disable(regs);
}
}
@@ -397,6 +398,8 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
die("bounds", regs, error_code);
do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, 0, NULL);
+
+ cond_local_irq_disable(regs);
}
enum kernel_gp_hint {
@@ -456,12 +459,13 @@ dotraplinkage void do_general_protection(struct pt_regs *regs, long error_code)
if (static_cpu_has(X86_FEATURE_UMIP)) {
if (user_mode(regs) && fixup_umip_exception(regs))
- return;
+ goto exit;
}
if (v8086_mode(regs)) {
local_irq_enable();
handle_vm86_fault((struct kernel_vm86_regs *) regs, error_code);
+ local_irq_disable();
return;
}
@@ -473,12 +477,11 @@ dotraplinkage void do_general_protection(struct pt_regs *regs, long error_code)
show_signal(tsk, SIGSEGV, "", desc, regs, error_code);
force_sig(SIGSEGV);
-
- return;
+ goto exit;
}
if (fixup_exception(regs, X86_TRAP_GP, error_code, 0))
- return;
+ goto exit;
tsk->thread.error_code = error_code;
tsk->thread.trap_nr = X86_TRAP_GP;
@@ -490,11 +493,11 @@ dotraplinkage void do_general_protection(struct pt_regs *regs, long error_code)
if (!preemptible() &&
kprobe_running() &&
kprobe_fault_handler(regs, X86_TRAP_GP))
- return;
+ goto exit;
ret = notify_die(DIE_GPF, desc, regs, error_code, X86_TRAP_GP, SIGSEGV);
if (ret == NOTIFY_STOP)
- return;
+ goto exit;
if (error_code)
snprintf(desc, sizeof(desc), "segment-related " GPFSTR);
@@ -516,6 +519,8 @@ dotraplinkage void do_general_protection(struct pt_regs *regs, long error_code)
die_addr(desc, regs, error_code, gp_addr);
+exit:
+ cond_local_irq_disable(regs);
}
NOKPROBE_SYMBOL(do_general_protection);
@@ -773,7 +778,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)
if (!user_mode(regs)) {
if (fixup_exception(regs, trapnr, error_code, 0))
- return;
+ goto exit;
task->thread.error_code = error_code;
task->thread.trap_nr = trapnr;
@@ -781,7 +786,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)
if (notify_die(DIE_TRAP, str, regs, error_code,
trapnr, SIGFPE) != NOTIFY_STOP)
die(str, regs, error_code);
- return;
+ goto exit;
}
/*
@@ -795,10 +800,12 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)
si_code = fpu__exception_code(fpu, trapnr);
/* Retry when we get spurious exceptions: */
if (!si_code)
- return;
+ goto exit;
force_sig_fault(SIGFPE, si_code,
(void __user *)uprobe_get_trap_addr(regs));
+exit:
+ cond_local_irq_disable(regs);
}
dotraplinkage void do_coprocessor_error(struct pt_regs *regs, long error_code)
@@ -853,6 +860,8 @@ do_device_not_available(struct pt_regs *regs, long error_code)
info.regs = regs;
math_emulate(&info);
+
+ cond_local_irq_disable(regs);
return;
}
#endif
@@ -883,6 +892,7 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
do_trap(X86_TRAP_IRET, SIGILL, "iret exception", regs, error_code,
ILL_BADSTK, (void __user *)NULL);
}
+ local_irq_disable();
}
#endif
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 6486cce..0715720 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -927,6 +927,8 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
force_sig_fault(SIGSEGV, si_code, (void __user *)address);
+ local_irq_disable();
+
return;
}
@@ -1548,9 +1550,18 @@ do_page_fault(struct pt_regs *regs, unsigned long hw_error_code,
return;
/* Was the fault on kernel-controlled part of the address space? */
- if (unlikely(fault_in_kernel_space(address)))
+ if (unlikely(fault_in_kernel_space(address))) {
do_kern_addr_fault(regs, hw_error_code, address);
- else
+ } else {
do_user_addr_fault(regs, hw_error_code, address);
+ /*
+ * User address page fault handling might have reenabled
+ * interrupts. Fixing up all potential exit points of
+ * do_user_addr_fault() and its leaf functions is just not
+ * doable w/o creating an unholy mess or turning the code
+ * upside down.
+ */
+ local_irq_disable();
+ }
}
NOKPROBE_SYMBOL(do_page_fault);