Machine check recovery when kernel accesses poison
Subsystems (5)
v7 cover Machine check recovery when kernel accesses poison
This series is initially targeted at the folks doing filesystems on top of NVDIMMs. They really want to be able to return -EIO when there is a h/w error (just like spinning rust, and SSD does).
I plan to use the same infrastructure to write a machine check aware "copy_from_user()" that will SIGBUS the calling application when a syscall touches poison in user space (just like we do when the application touches the poison itself).
Changes V6-V7:
Boris: Why add/subtract 0x20000000? Added better comment provided by Andy
Boris: Churn. Part2 changes things only introduced in part1.
Merged parts 1&2 into one patch.
Ingo: Missing my sign off on part1. Added.
Changes V5-V6
Andy: Provoked massive re-write by providing what is now part1 of this
patch series. This frees up two bits in the exception table fixup field that can be used to tag exception table entries as different "classes". This means we don't need my separate exception table fro machine checks. Also avoids duplicating fixup actions for #PF and #MC cases that were in version 5.
Andy: Use C99 array initializers to tie the various class fixup
functions back to the defintions of each class. Also give the functions meanningful names (not fixup_class0() etc.).
Boris: Cleaned up my lousy assembly code removing many spurious 'l'
modifiers on instructions.
Boris: Provided some helper functions for the machine check severity
calculation that make the code more readable.
Boris: Have __mcsafe_copy() return a structure with the 'remaining bytes'
in a separate field from the fault indicator. Boris had suggested Linux -EFAULT/-EINVAL ... but I thought it made more sense to return the exception number (X86_TRAP_MC, etc.) This finally kills off BIT(63) which has been controversial throughout all the early versions of this patch series.
Changes V4-V5
Tony: Extended __mcsafe_copy() to have fixup entries for both machine
check and page fault.
Changes V3-V4:
Andy: Simplify fixup_mcexception() by dropping used-once local variable Andy: "Reviewed-by" tag added to part1 Boris: Moved new functions to memcpy_64.S and declaration to asm/string_64.h Boris: Changed name s/mcsafe_memcpy/__mcsafe_copy/ to make it clear that this
is an internal function and that return value doesn't follow memcpy() semantics.
Boris: "Reviewed-by" tag added to parts 1&2
Changes V2-V3:
Andy: Don't hack "regs->ax = BIT(63) | addr;" in the machine check
handler. Now have better fixup code that computes the number of remaining bytes (just like page-fault fixup).
Andy: #define for BIT(63). Done, plus couple of extra macros using it. Boris: Don't clutter up generic code (like mm/extable.c) with this.
I moved everything under arch/x86 (the asm-generic change is a more generic #define).
Boris: Dependencies for CONFIG_MCE_KERNEL_RECOVERY are too generic.
I made it a real menu item with default "n". Dan Williams will use "select MCE_KERNEL_RECOVERY" from his persistent filesystem code.
Boris: Simplify conditionals in mce.c by moving tolerant/kill_it
checks earlier, with a skip to end if they aren't set.
Boris: Miscellaneous grammar/punctuation. Fixed. Boris: Don't leak spurious __start_mcextable symbols into kernels
that didn't configure MCE_KERNEL_RECOVERY. Done.
Tony: New code doesn't belong in user_copy_64.S/uaccess*.h. Moved
to new .S/.h files
Elliott:Cacheing behavior non-optimal. Could use movntdqa, vmovntdqa
or vmovntdqa on source addresses. I didn't fix this yet. Think of the current mcsafe_memcpy() as the first of several functions. This one is useful for small copies (meta-data) where the overhead of saving SSE/AVX state isn't justified.
Changes V1->V2:
0-day: Reported build errors and warnings on 32-bit systems. Fixed 0-day: Reported bloat to tinyconfig. Fixed Boris: Suggestions to use extra macros to reduce code duplication in _ASM_*EXTABLE. Done Boris: Re-write "tolerant==3" check to reduce indentation level. See below. Andy: Check IP is valid before searching kernel exception tables. Done. Andy: Explain use of BIT(63) on return value from mcsafe_memcpy(). Done (added decode macros). Andy: Untangle mess of code in tail of do_machine_check() to make it
clear what is going on (e.g. that we only enter the ist_begin_non_atomic() if we were called from user code, not from kernel!). Done.
Tony Luck (3):
x86: Add classes to exception tables
x86, mce: Check for faults tagged in EXTABLE_CLASS_FAULT exception
table entries
x86, mce: Add __mcsafe_copy()
arch/x86/Kconfig | 10 +++ arch/x86/include/asm/asm.h | 102 ++++++++++++++++------ arch/x86/include/asm/string_64.h | 10 +++ arch/x86/include/asm/uaccess.h | 17 +++- arch/x86/kernel/cpu/mcheck/mce-severity.c | 32 ++++++- arch/x86/kernel/cpu/mcheck/mce.c | 71 ++++++++-------- arch/x86/kernel/kprobes/core.c | 2 +- arch/x86/kernel/traps.c | 6 +- arch/x86/kernel/x8664_ksyms_64.c | 4 + arch/x86/lib/memcpy_64.S | 136 ++++++++++++++++++++++++++++++ arch/x86/mm/extable.c | 66 ++++++++++----- arch/x86/mm/fault.c | 2 +- 12 files changed, 369 insertions(+), 89 deletions(-)
Signature · 1 line
2.1.4
PATCH v7 1/3 x86: Add classes to exception tables
Starting with a patch from Andy Lutomirski <[email protected]> that used linker relocation trickery to free up a couple of bits in the "fixup" field of the exception table (and generalized the uaccess_err hack to use one of the classes).
This patch allocates another one of the classes to provide a mechanism to provide the fault number to the fixup code in %rax.
Still one free class for the next brilliant idea. If more are needed it should be possible to squeeze another bit or three extending the same technique.
Originally-from: Andy Lutomirski <[email protected]>
--- arch/x86/include/asm/asm.h | 102 +++++++++++++++++++++++++++++++---------- arch/x86/include/asm/uaccess.h | 17 +++++-- arch/x86/kernel/kprobes/core.c | 2 +- arch/x86/kernel/traps.c | 6 +-- arch/x86/mm/extable.c | 66 ++++++++++++++++++-------- arch/x86/mm/fault.c | 2 +- 6 files changed, 142 insertions(+), 53 deletions(-)
arch/x86/include/asm/asm.h +78 -24
@@ -43,19 +43,79 @@#define _ASM_DI __ASM_REG(di)/* Exception table entry */ -#ifdef __ASSEMBLY__ -# define _ASM_EXTABLE (from,to) \ - .pushsection "__ex_table","a" ; \ - .balign 8 ; \ - .long (from) - . ; \ - .long (to) - . ; \ - .popsection -# define _ASM_EXTABLE_EX (from,to) \ - .pushsection "__ex_table","a" ; \ - .balign 8 ; \ - .long (from) - . ; \ - .long (to) - . + 0x7ffffff0 ; \ +#define _EXTABLE_BIAS 0x20000000 +/* +* An exception table entry is 64 bits. The first 32 bits are the offset +* from that entry to the potentially faulting instruction. sortextable +* relies on that exact encoding. The second 32 bits encode the fault +* handler address. +* +* We want to stick two extra bits of handler class into the fault handler +* address. All of these are generated by relocations, so we can only +* rely on addition. +* +* The offset to the fixup is signed, and we're trying to use the high +* bits for a different purpose. In C, we could just do: +* +* u32 class_and_offset = ((target - here) & 0x3fffffff) | class; +* +* Then, to decode it, we'd mask off the class and sign-extend to recover +* the offset. +* +* In asm, we can't do that, because this all gets laundered through the +* linker, and there's no relocation type that supports this chicanery. +* Instead we cheat a bit. We first add a large number to the offset +* (0x20000000). The result is still nominally signed, but now it's +* always positive, and the two high bits are always clear. We can then +* set high bits by ordinary addition or subtraction instead of using +* bitwise operations. As far as the linker is concerned, all we're +* doing is adding a large constant to the difference between here (".") +* and the target, and that's a valid relocation type. +* +* We therefore emit: +* (target - here) + (class) + 0x20000000 +* +* This has the property that the two high bits are the class (see +* ex_class(). +* +* To get the offset back we just mask off the class bits and subtract +* 0x20000000. See ex_fixup_addr(). +*/ + +/* +* There are two bits of extable entry class giving four classes +*/ +#define EXTABLE_CLASS_DEFAULT 0/* standard uaccess fixup */ +#define EXTABLE_CLASS_FAULT 1/* provide fault number as well as fixup */ +#define EXTABLE_CLASS_EX 2/* uaccess + set uaccess_err */ +#define EXTABLE_CLASS_UNUSED 3/* available for something else */ + +/* +* The biases are the class constants + _EXTABLE_BIAS, as signed integers. +* This can't use ordinary arithmetic -- the assembler isn't that smart. +*/ +#define _EXTABLE_BIAS_DEFAULT _EXTABLE_BIAS +#define _EXTABLE_BIAS_FAULT _EXTABLE_BIAS + 0x40000000 +#define _EXTABLE_BIAS_EX _EXTABLE_BIAS - 0x80000000 +#define _EXTABLE_BIAS_UNUSED _EXTABLE_BIAS - 0x40000000 + +#define _ASM_EXTABLE (from,to) \ + _ASM_EXTABLE_CLASS(from, to, _EXTABLE_BIAS_DEFAULT) + +#define _ASM_EXTABLE_FAULT (from,to) \ + _ASM_EXTABLE_CLASS(from, to, _EXTABLE_BIAS_FAULT) + +#define _ASM_EXTABLE_EX (from,to) \ + _ASM_EXTABLE_CLASS(from, to, _EXTABLE_BIAS_EX) + +#ifdef __ASSEMBLY__ +# define _EXPAND_EXTABLE_BIAS (x) x +# define _ASM_EXTABLE_CLASS (from,to,bias) \ + .pushsection "__ex_table","a" ; \ + .balign 8 ; \ + .long (from) - . ; \ + .long (to) - . + _EXPAND_EXTABLE_BIAS(bias) ; \ .popsection# define _ASM_NOKPROBE (entry) \
@@ -89,18 +149,12 @@ .endm#else -# define _ASM_EXTABLE (from,to) \ - " .pushsection \"__ex_table\",\"a\"\n" \ - " .balign 8\n" \ - " .long (" #from ") - .\n" \ - " .long (" #to ") - .\n" \ - " .popsection\n" - -# define _ASM_EXTABLE_EX (from,to) \ - " .pushsection \"__ex_table\",\"a\"\n" \ - " .balign 8\n" \ - " .long (" #from ") - .\n" \ - " .long (" #to ") - . + 0x7ffffff0\n" \ +# define _EXPAND_EXTABLE_BIAS (x) #x +# define _ASM_EXTABLE_CLASS (from,to,bias) \ + " .pushsection \"__ex_table\",\"a\"\n" \ + " .balign 8\n" \ + " .long (" #from ") - .\n" \ + " .long (" #to ") - . + " _EXPAND_EXTABLE_BIAS(bias) "\n" \ " .popsection\n"/* For C file, we already have NOKPROBE_SYMBOL macro */ #endif
arch/x86/include/asm/uaccess.h +13 -4
@@ -93,9 +93,12 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un* The exception table consists of pairs of addresses relative to the * exception table enty itself: the first is the address of an * instruction that is allowed to fault, and the second is the address -* at which the program should continue. No registers are modified, -* so it is entirely up to the continuation code to figure out what to -* do. +* at which the program should continue. The exception table is linked +* soon after the fixup section, so we don't need a full 32-bit offset +* for the fixup. We steal the two upper bits so we can tag exception +* table entries with different classes. In the default class no registers +* are modified, so it is entirely up to the continuation code to figure +* out what to do. * * All the routines below use bits of fixup code that are out of line * with the main instruction path. This means when everything is well,
@@ -110,7 +113,13 @@ struct exception_table_entry {#define ARCH_HAS_SORT_EXTABLE #define ARCH_HAS_SEARCH_EXTABLE -extern int fixup_exception (struct pt_regs *regs); +static inline unsigned int +ex_class (const struct exception_table_entry *x) +{ +return (u32 )x->fixup >>30 ; +} + +extern int fixup_exception (struct pt_regs *regs,int trapnr);extern int early_fixup_exception (unsigned long *ip);/*
arch/x86/kernel/kprobes/core.c +1 -1
@@ -988,7 +988,7 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)* In case the user-specified fault handler returned * zero, try to fix up. */ -if (fixup_exception (regs)) +if (fixup_exception (regs, trapnr))return 1 ;/*
arch/x86/kernel/traps.c +3 -3
@@ -199,7 +199,7 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str, }if (!user_mode (regs)) { -if (!fixup_exception (regs)) { +if (!fixup_exception (regs, trapnr)) { tsk->thread.error_code = error_code; tsk->thread.trap_nr = trapnr;die (str, regs, error_code);
@@ -453,7 +453,7 @@ do_general_protection(struct pt_regs *regs, long error_code) tsk = current;if (!user_mode (regs)) { -if (fixup_exception (regs)) +if (fixup_exception (regs,X86_TRAP_GP ))return ; tsk->thread.error_code = error_code;
@@ -699,7 +699,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)conditional_sti (regs);if (!user_mode (regs)) { -if (!fixup_exception (regs)) { +if (!fixup_exception (regs,X86_TRAP_DE )) { task->thread.error_code = error_code; task->thread.trap_nr = trapnr;die (str, regs, error_code);
arch/x86/mm/extable.c +46 -20
94 lines not on this page · raw message
arch/x86/mm/fault.c +1 -1
@@ -656,7 +656,7 @@ no_context(struct pt_regs *regs, unsigned long error_code,int sig;/* Are we prepared to handle this kernel fault? */ -if (fixup_exception (regs)) { +if (fixup_exception (regs,X86_TRAP_PF )) {/* * Any interrupt that takes a fault gets the fixup. This makes * the below recursive fault logic only apply to a faults from
Signature · 7 lines
2.1.4 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Borislav Petkov
maintainerRe: [PATCH v7 1/3] x86: Add classes to exception tables
Quoted 4 lines · Tony Luck
Starting with a patch from Andy Lutomirski <[email protected]> that used linker relocation trickery to free up a couple of bits in the "fixup" field of the exception table (and generalized the
uaccess_errhack to use one of the classes).
So I still think that the other idea Andy gave with putting the handler in the exception table is much cleaner and straightforward.
Here's a totally untested patch which at least builds here. I think this approach is much more extensible and simpler for the price of a couple of KBs of __ex_table size.
---
arch/x86/include/asm/asm.h +6 -3
@@ -44,18 +44,20 @@/* Exception table entry */ #ifdef __ASSEMBLY__ -# define _ASM_EXTABLE (from,to) \ +# define _ASM_EXTABLE (from,to) \ .pushsection "__ex_table","a" ; \ .balign 8 ; \ .long (from) - . ; \ .long (to) - . ; \ + .long 0 - .; \ .popsection# define _ASM_EXTABLE_EX (from,to) \ .pushsection "__ex_table","a" ; \ .balign 8 ; \ .long (from) - . ; \ - .long (to) - . + 0x7ffffff0 ; \ + .long (to) - . ; \ + .long ex_handler_ext - . ; \ .popsection# define _ASM_NOKPROBE (entry) \
@@ -94,13 +96,14 @@" .balign 8\n" \ " .long (" #from ") - .\n" \ " .long (" #to ") - .\n" \ + " .long 0 - .\n" \ " .popsection\n"# define _ASM_EXTABLE_EX (from,to) \ " .pushsection \"__ex_table\",\"a\"\n" \ " .balign 8\n" \ " .long (" #from ") - .\n" \ - " .long (" #to ") - . + 0x7ffffff0\n" \ + " .long ex_handler_ext - .\n" \ " .popsection\n"/* For C file, we already have NOKPROBE_SYMBOL macro */ #endif
arch/x86/include/asm/uaccess.h +2 -2
@@ -104,13 +104,13 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un*/ struct exception_table_entry { -int insn, fixup; +int insn, fixup, handler; };/* This is not the generic standard exception_table_entry format */ #define ARCH_HAS_SORT_EXTABLE #define ARCH_HAS_SEARCH_EXTABLE -extern int fixup_exception (struct pt_regs *regs); +extern int fixup_exception (struct pt_regs *regs,int trapnr);extern int early_fixup_exception (unsigned long *ip);/*
arch/x86/kernel/kprobes/core.c +1 -1
@@ -988,7 +988,7 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)* In case the user-specified fault handler returned * zero, try to fix up. */ -if (fixup_exception (regs)) +if (fixup_exception (regs, trapnr))return 1 ;/*
arch/x86/kernel/traps.c +3 -3
@@ -199,7 +199,7 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str, }if (!user_mode (regs)) { -if (!fixup_exception (regs)) { +if (!fixup_exception (regs, trapnr)) { tsk->thread.error_code = error_code; tsk->thread.trap_nr = trapnr;die (str, regs, error_code);
@@ -453,7 +453,7 @@ do_general_protection(struct pt_regs *regs, long error_code) tsk = current;if (!user_mode (regs)) { -if (fixup_exception (regs)) +if (fixup_exception (regs,X86_TRAP_GP ))return ; tsk->thread.error_code = error_code;
@@ -699,7 +699,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)conditional_sti (regs);if (!user_mode (regs)) { -if (!fixup_exception (regs)) { +if (!fixup_exception (regs, trapnr)) { task->thread.error_code = error_code; task->thread.trap_nr = trapnr;die (str, regs, error_code);
arch/x86/mm/extable.c +56 -27
114 lines not on this page · raw message
arch/x86/mm/fault.c +1 -1
@@ -656,7 +656,7 @@ no_context(struct pt_regs *regs, unsigned long error_code,int sig;/* Are we prepared to handle this kernel fault? */ -if (fixup_exception (regs)) { +if (fixup_exception (regs,X86_TRAP_PF )) {/* * Any interrupt that takes a fault gets the fixup. This makes * the below recursive fault logic only apply to a faults from
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.Re: [PATCH v7 1/3] x86: Add classes to exception tables
Quoted 6 lines · Borislav Petkov
Quoted 4 lines · Tony Luck
Starting with a patch from Andy Lutomirski <[email protected]> that used linker relocation trickery to free up a couple of bits in the "fixup" field of the exception table (and generalized the
uaccess_errhack to use one of the classes).
Quoted 6 lines
So I still think that the other idea Andy gave with putting the handler in the exception table is much cleaner and straightforward.
Here's a totally untested patch which at least builds here. I think this approach is much more extensible and simpler for the price of a couple of KBs of
__ex_tablesize.
On my config (based on an enterprise distro config) the vmlinux ex_table would grow from 8k to 12k. There are a handful of modules that would also see 50% expansion (the size is the first hex field after the __ex_table):
arch/x86/kernel/test_nx.ko 4 __ex_table 00000008 0000000000000000 0000000000000000 000002a8 2**3 arch/x86/kvm/kvm-intel.ko 7 __ex_table 000009b8 0000000000000000 0000000000000000 0001ff60 2**3 arch/x86/kvm/kvm-amd.ko 6 __ex_table 00000070 0000000000000000 0000000000000000 0000a1e0 2**3 arch/x86/kvm/kvm.ko 23 __ex_table 00000080 0000000000000000 0000000000000000 00059940 2**3 drivers/char/ipmi/ipmi_devintf.ko 9 __ex_table 00000098 0000000000000000 0000000000000000 000017f0 2**3 drivers/gpu/drm/i915/i915.ko 20 __ex_table 00000060 0000000000000000 0000000000000000 000f8f10 2**3 drivers/gpu/drm/radeon/radeon.ko 19 __ex_table 000001d0 0000000000000000 0000000000000000 00145c88 2**3 drivers/gpu/drm/drm.ko 24 __ex_table 00000400 0000000000000000 0000000000000000 0003b688 2**3 drivers/media/usb/uvc/uvcvideo.ko 15 __ex_table 00000030 0000000000000000 0000000000000000 0000f100 2**3 drivers/scsi/sg.ko 12 __ex_table 00000060 0000000000000000 0000000000000000 00006548 2**3 drivers/vhost/vhost.ko 12 __ex_table 00000068 0000000000000000 0000000000000000 00003360 2**3 drivers/xen/xen-privcmd.ko 11 __ex_table 00000010 0000000000000000 0000000000000000 00000f70 2**3 net/sunrpc/sunrpc.ko 24 __ex_table 00000008 0000000000000000 0000000000000000 000321b0 2**3 sound/core/snd-pcm.ko 19 __ex_table 00000040 0000000000000000 0000000000000000 000108f0 2**3
Linus, Peter, Ingo, Thomas: Can we head this direction? The code is cleaner and more flexible. Or should we stick with Andy's clever way to squeeze a couple of "class" bits into the fixup field of the exception table?
-Tony
Quoted 249 lines
---
arch/x86/include/asm/asm.h +6
-3@@ -44,18 +44,20 @@/* Exception table entry */ #ifdef __ASSEMBLY__ -# define _ASM_EXTABLE (from,to) \ +# define _ASM_EXTABLE (from,to) \ .pushsection "__ex_table","a" ; \ .balign 8 ; \ .long (from) - . ; \ .long (to) - . ; \ + .long 0 - .; \ .popsection# define _ASM_EXTABLE_EX (from,to) \ .pushsection "__ex_table","a" ; \ .balign 8 ; \ .long (from) - . ; \ - .long (to) - . + 0x7ffffff0 ; \ + .long (to) - . ; \ + .long ex_handler_ext - . ; \ .popsection# define _ASM_NOKPROBE (entry) \@@ -94,13 +96,14 @@" .balign 8\n" \ " .long (" #from ") - .\n" \ " .long (" #to ") - .\n" \ + " .long 0 - .\n" \ " .popsection\n"# define _ASM_EXTABLE_EX (from,to) \ " .pushsection \"__ex_table\",\"a\"\n" \ " .balign 8\n" \ " .long (" #from ") - .\n" \ - " .long (" #to ") - . + 0x7ffffff0\n" \ + " .long ex_handler_ext - .\n" \ " .popsection\n"/* For C file, we already have NOKPROBE_SYMBOL macro */ #endif arch/x86/include/asm/uaccess.h +2
-2@@ -104,13 +104,13 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un*/ struct exception_table_entry { -int insn, fixup; +int insn, fixup, handler; };/* This is not the generic standard exception_table_entry format */ #define ARCH_HAS_SORT_EXTABLE #define ARCH_HAS_SEARCH_EXTABLE -extern int fixup_exception (struct pt_regs *regs); +extern int fixup_exception (struct pt_regs *regs,int trapnr);extern int early_fixup_exception (unsigned long *ip);/* arch/x86/kernel/kprobes/core.c +1
-1@@ -988,7 +988,7 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)* In case the user-specified fault handler returned * zero, try to fix up. */ -if (fixup_exception (regs)) +if (fixup_exception (regs, trapnr))return 1 ;/* arch/x86/kernel/traps.c +3
-3@@ -199,7 +199,7 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str, }if (!user_mode (regs)) { -if (!fixup_exception (regs)) { +if (!fixup_exception (regs, trapnr)) { tsk->thread.error_code = error_code; tsk->thread.trap_nr = trapnr;die (str, regs, error_code);@@ -453,7 +453,7 @@ do_general_protection(struct pt_regs *regs, long error_code) tsk = current;if (!user_mode (regs)) { -if (fixup_exception (regs)) +if (fixup_exception (regs,X86_TRAP_GP ))return ; tsk->thread.error_code = error_code;@@ -699,7 +699,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)conditional_sti (regs);if (!user_mode (regs)) { -if (!fixup_exception (regs)) { +if (!fixup_exception (regs, trapnr)) { task->thread.error_code = error_code; task->thread.trap_nr = trapnr;die (str, regs, error_code);arch/x86/mm/extable.c +56
-27@@ -3,6 +3,8 @@#include <linux/sort.h> #include <asm/uaccess.h> +typedef int (*ex_handler_t )(const struct exception_table_entry *,struct pt_regs *,int ); +static inline unsigned long ex_insn_addr (const struct exception_table_entry *x) {@@ -14,10 +16,39 @@ ex_fixup_addr(const struct exception_table_entry *x)return (unsigned long )&x->fixup + x->fixup; } -int fixup_exception (struct pt_regs *regs) +inline ex_handler_t ex_fixup_handler (const struct exception_table_entry *x) +{ +return (ex_handler_t )&x->handler + x->handler; +} + +int ex_handler_default (const struct exception_table_entry *fixup, +struct pt_regs *regs,int trapnr) +{ + regs->ip =ex_fixup_addr (fixup); +return 1 ; +} + +int ex_handler_fault (const struct exception_table_entry *fixup, +struct pt_regs *regs,int trapnr) +{ + regs->ip =ex_fixup_addr (fixup); + regs->ax = trapnr; +return 1 ; +} +int ex_handler_ext (const struct exception_table_entry *fixup, +struct pt_regs *regs,int trapnr) { -const struct exception_table_entry *fixup; +/* Special hack for uaccess_err */ +current_thread_info ()->uaccess_err =1 ; + regs->ip =ex_fixup_addr (fixup); +return 1 ; +} + +int fixup_exception (struct pt_regs *regs,int trapnr) +{ +const struct exception_table_entry *e;unsigned long new_ip; +ex_handler_t handler;#ifdef CONFIG_PNPBIOS if (unlikely (SEGMENT_IS_PNP_CODE (regs->cs))) {@@ -33,42 +64,40 @@ int fixup_exception(struct pt_regs *regs) }#endif - fixup =search_exception_tables (regs->ip); -if (fixup) { - new_ip =ex_fixup_addr (fixup); - -if (fixup->fixup - fixup->insn >=0x7ffffff0 -4 ) { -/* Special hack for uaccess_err */ -current_thread_info ()->uaccess_err =1 ; - new_ip -=0x7ffffff0 ; - } - regs->ip = new_ip; -return 1 ; - } + e =search_exception_tables (regs->ip); +if (!e) +return 0 ; + + new_ip =ex_fixup_addr (e); + handler =ex_fixup_handler (e); + +if (!handler) + handler = ex_handler_default; + +return handler (e, regs, trapnr); -return 0 ; }/* Restricted version used during very early boot */ int __init early_fixup_exception (unsigned long *ip) { -const struct exception_table_entry *fixup; +const struct exception_table_entry *e;unsigned long new_ip; +ex_handler_t handler; - fixup =search_exception_tables (*ip); -if (fixup) { - new_ip =ex_fixup_addr (fixup); + e =search_exception_tables (*ip); +if (!e) +return 0 ; -if (fixup->fixup - fixup->insn >=0x7ffffff0 -4 ) { -/* uaccess handling not supported during early boot */ -return 0 ; - } + new_ip =ex_fixup_addr (e); + handler =ex_fixup_handler (e); - *ip = new_ip; -return 1 ; - } +/* uaccess handling not supported during early boot */ +if (handler && handler == ex_handler_ext) +return 0 ; -return 0 ; + *ip = new_ip; +return 1 ; }/* arch/x86/mm/fault.c +1
-1@@ -656,7 +656,7 @@ no_context(struct pt_regs *regs, unsigned long error_code,int sig;/* Are we prepared to handle this kernel fault? */ -if (fixup_exception (regs)) { +if (fixup_exception (regs,X86_TRAP_PF )) {/* * Any interrupt that takes a fault gets the fixup. This makes * the below recursive fault logic only apply to a faults from Signature · 5 lines
-- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v7 1/3] x86: Add classes to exception tables
Quoted 4 lines · Tony
Linus, Peter, Ingo, Thomas: Can we head this direction? The code is cleaner and more flexible. Or should we stick with Andy's clever way to squeeze a couple of "class" bits into the fixup field of the exception table?
I'd rather not be clever in order to save just a tiny amount of space in the exception table, which isn't really criticial for anybody.
So I think Borislav's patch has the advantage of being pretty straightforward and allowing arbitrary fixups, in case we end up having localized special cases..
Linus
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v7 1/3] x86: Add classes to exception tables
In reply to Borislav PetkovQuoted 19 lines · Borislav Petkov
Quoted 4 lines · Tony Luck
Starting with a patch from Andy Lutomirski <[email protected]> that used linker relocation trickery to free up a couple of bits in the "fixup" field of the exception table (and generalized the
uaccess_errhack to use one of the classes).So I still think that the other idea Andy gave with putting the handler in the exception table is much cleaner and straightforward.
Here's a totally untested patch which at least builds here. I think this approach is much more extensible and simpler for the price of a couple of KBs of
__ex_tablesize.---
arch/x86/include/asm/asm.h +0
-0
arch/x86/include/asm/asm.h line 52
Quoted 10 lines
arch/x86/include/asm/asm.h +2
-1@@ -44,18 +44,20 @@/* Exception table entry */ #ifdef __ASSEMBLY__ -# define _ASM_EXTABLE (from,to) \ +# define _ASM_EXTABLE (from,to) \ .pushsection "__ex_table","a" ; \ .balign 8 ; \ .long (from) - . ; \ .long (to) - . ; \ + .long 0 - .; \
I assume that this zero is to save the couple of bytes for the relocation entry on relocatable kernels?
If so, ...
arch/x86/include/asm/asm.h line 55
Quoted 3 lines
arch/x86/include/asm/asm.h +3
-0+inline ex_handler_t ex_fixup_handler (const struct exception_table_entry *x) +{ +return (ex_handler_t )&x->handler + x->handler;
I would check for zero here, because...
arch/x86/include/asm/asm.h line 60
Quoted 5 lines
arch/x86/include/asm/asm.h +5
-0+ new_ip =ex_fixup_addr (e); + handler =ex_fixup_handler (e); + +if (!handler) + handler = ex_handler_default;
the !handler condition here will never trigger because the offset was already applied.
Otherwise this looks generally sane.
--Andy
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Borislav Petkov
maintainerRe: [PATCH v7 1/3] x86: Add classes to exception tables
Quoted 2 lines · Andy Lutomirski
I assume that this zero is to save the couple of bytes for the relocation entry on relocatable kernels?
I didn't want to touch all _ASM_EXTABLE() macro invocations by adding a third param @handler which is redundant as we know which it is.
Quoted 8 lines
arch/x86/mm/extable.c int fixup_exception(struct pt_regs *regs) · line 75
Quoted 5 lines
arch/x86/mm/extable.c +5
-0+ new_ip =ex_fixup_addr (e); + handler =ex_fixup_handler (e); + +if (!handler) + handler = ex_handler_default;the !handler condition here will never trigger because the offset was already applied.
Actually, if I do "0 - .", that would overflow the int because current location is virtual address and that's 64-bit. Or would gas simply truncate it? Lemme check...
Anyway, what we should do instead is simply
.long 0
to denote that the @handler is implicit.
Right?
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.Re: [PATCH v7 1/3] x86: Add classes to exception tables
Quoted 6 lines · Borislav Petkov
Quoted 2 lines · Andy Lutomirski
I assume that this zero is to save the couple of bytes for the relocation entry on relocatable kernels?
I didn't want to touch all
_ASM_EXTABLE()macro invocations by adding a third param @handler which is redundant as we know which it is.
I see. You could shove the .long ex_handler_default - . into the macro, but that would indeed bloat the kernel image a bit more (although not the in-memory size of the kernel).
Quoted 14 lines
Quoted 9 lines
arch/x86/mm/extable.c int fixup_exception(struct pt_regs *regs) · line 75
Quoted 6 lines
arch/x86/mm/extable.c +5
-0+ new_ip =ex_fixup_addr (e); + handler =ex_fixup_handler (e); + +if (!handler) + handler = ex_handler_default;the !handler condition here will never trigger because the offset was already applied.
Actually, if I do "0 - .", that would overflow the int because current location is virtual address and that's 64-bit. Or would gas simply truncate it? Lemme check...
Quoted 7 lines
Anyway, what we should do instead is simply
.long 0to denote that the @handler is implicit.
Right?
Agreed. I just think that your current fixup_ex_handler implementation needs adjustment if you do it that way.
--Andy
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Borislav Petkov
maintainerRe: [PATCH v7 1/3] x86: Add classes to exception tables
Quoted 2 lines · Andy Lutomirski
Agreed. I just think that your current
fixup_ex_handlerimplementation needs adjustment if you do it that way.
Right, and as you just mentioned on IRC, there's also sortextable.c which needs adjusting. So I'll go stare at that code first to try to figure out what exactly is being done there...
Thanks.
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.Borislav Petkov
maintainerRe: [PATCH v7 1/3] x86: Add classes to exception tables
Quoted 7 lines · Borislav Petkov
Quoted 2 lines · Andy Lutomirski
Agreed. I just think that your current
fixup_ex_handlerimplementation needs adjustment if you do it that way.Right, and as you just mentioned on IRC, there's also
sortextable.cwhich needs adjusting. So I'll go stare at that code first to try to figure out what exactly is being done there...
Sorry, it took me a while to figure out all that normalization and denormalization of the offsets sortextable.c does. And with the bigger table of 3 ints, we need to do the sorting a bit differently. Also, the alignment of 8 makes gas turn ".long 0" into 8 bytes worth of zeroes so I had to do ".balign 4". Is that going to be a problem...?
With it, the exception table looks ok to me:
8e9984 35 ae 91 ff 73 d5 ff ff 00 00 00 00 8e9990 3e af 91 ff 71 d5 ff ff 00 00 00 00 8e999c 4e af 91 ff 6c d5 ff ff 00 00 00 00 8e99a8 49 af 91 ff 67 d5 ff ff 00 00 00 00 8e99b4 ab b3 91 ff 62 d5 ff ff 00 00 00 00 8e99c0 b0 b3 91 ff 5d d5 ff ff 00 00 00 00 8e99cc e7 b3 91 ff 5b d5 ff ff 00 00 00 00 8e99d8 82 b4 91 ff 59 d5 ff ff 00 00 00 00 8e99e4 8e b9 91 ff 8e b9 91 ff d8 41 96 ff 8e99f0 8a b9 91 ff 8a b9 91 ff cc 41 96 ff 8e99fc 86 b9 91 ff 86 b9 91 ff c0 41 96 ff 8e9a08 82 b9 91 ff 82 b9 91 ff b4 41 96 ff 8e9a14 81 b9 91 ff 81 b9 91 ff a8 41 96 ff 8e9a20 7d b9 91 ff 7d b9 91 ff 9c 41 96 ff
...
the last int not 00 is the offset to ex_handler_ext(). Regarding that, I probably should change _ASM_EXTABLE_EX() to take a @handler argument so that Tony can pass in the ex_handler_fault() too.
Anyway, here's what I have, it boots fine in a guest.
Btw, it seems I'm coming down with the cold and all that above could be hallucinations so please double-check me.
Thanks.
---
arch/x86/include/asm/asm.h +10 -6
@@ -46,16 +46,18 @@#ifdef __ASSEMBLY__# define _ASM_EXTABLE (from,to) \ .pushsection "__ex_table","a" ; \ - .balign 8 ; \ + .balign 4 ; \ .long (from) - . ; \ .long (to) - . ; \ + .long 0; \ .popsection# define _ASM_EXTABLE_EX (from,to) \ .pushsection "__ex_table","a" ; \ - .balign 8 ; \ + .balign 4 ; \ .long (from) - . ; \ - .long (to) - . + 0x7ffffff0 ; \ + .long (to) - . ; \ + .long ex_handler_ext - . ; \ .popsection# define _ASM_NOKPROBE (entry) \
@@ -91,16 +93,18 @@#else # define _ASM_EXTABLE (from,to) \ " .pushsection \"__ex_table\",\"a\"\n" \ - " .balign 8\n" \ + " .balign 4\n" \ " .long (" #from ") - .\n" \ " .long (" #to ") - .\n" \ + " .long 0\n" \ " .popsection\n"# define _ASM_EXTABLE_EX (from,to) \ " .pushsection \"__ex_table\",\"a\"\n" \ - " .balign 8\n" \ + " .balign 4\n" \ " .long (" #from ") - .\n" \ - " .long (" #to ") - . + 0x7ffffff0\n" \ + " .long (" #to ") - .\n" \ + " .long ex_handler_ext - .\n" \ " .popsection\n"/* For C file, we already have NOKPROBE_SYMBOL macro */ #endif
arch/x86/include/asm/uaccess.h +2 -2
@@ -104,13 +104,13 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un*/ struct exception_table_entry { -int insn, fixup; +int insn, fixup, handler; };/* This is not the generic standard exception_table_entry format */ #define ARCH_HAS_SORT_EXTABLE #define ARCH_HAS_SEARCH_EXTABLE -extern int fixup_exception (struct pt_regs *regs); +extern int fixup_exception (struct pt_regs *regs,int trapnr);extern int early_fixup_exception (unsigned long *ip);/*
arch/x86/kernel/kprobes/core.c +1 -1
@@ -988,7 +988,7 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)* In case the user-specified fault handler returned * zero, try to fix up. */ -if (fixup_exception (regs)) +if (fixup_exception (regs, trapnr))return 1 ;/*
arch/x86/kernel/traps.c +3 -3
@@ -199,7 +199,7 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str, }if (!user_mode (regs)) { -if (!fixup_exception (regs)) { +if (!fixup_exception (regs, trapnr)) { tsk->thread.error_code = error_code; tsk->thread.trap_nr = trapnr;die (str, regs, error_code);
@@ -453,7 +453,7 @@ do_general_protection(struct pt_regs *regs, long error_code) tsk = current;if (!user_mode (regs)) { -if (fixup_exception (regs)) +if (fixup_exception (regs,X86_TRAP_GP ))return ; tsk->thread.error_code = error_code;
@@ -699,7 +699,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)conditional_sti (regs);if (!user_mode (regs)) { -if (!fixup_exception (regs)) { +if (!fixup_exception (regs, trapnr)) { task->thread.error_code = error_code; task->thread.trap_nr = trapnr;die (str, regs, error_code);
arch/x86/mm/extable.c +55 -27
113 lines not on this page · raw message
arch/x86/mm/fault.c +1 -1
@@ -670,7 +670,7 @@ no_context(struct pt_regs *regs, unsigned long error_code,int sig;/* Are we prepared to handle this kernel fault? */ -if (fixup_exception (regs)) { +if (fixup_exception (regs,X86_TRAP_PF )) {/* * Any interrupt that takes a fault gets the fixup. This makes * the below recursive fault logic only apply to a faults from
scripts/sortextable.c +30 -0
@@ -209,6 +209,33 @@ static int compare_relative_table(const void *a, const void *b)return 0 ; } +static void x86_sort_relative_table (char *extab_image,int image_size) +{ +int i; + + i =0 ; +while (i < image_size) { +uint32_t *loc = (uint32_t *)(extab_image + i); + +w (r (loc) + i, loc); +w (r (loc +1 ) + i +4 , loc +1 ); + + i +=sizeof (uint32_t ) *3 ; + } + +qsort (extab_image, image_size /12 ,12 , compare_relative_table); + + i =0 ; +while (i < image_size) { +uint32_t *loc = (uint32_t *)(extab_image + i); + +w (r (loc) - i, loc); +w (r (loc +1 ) - (i +4 ), loc +1 ); + + i +=sizeof (uint32_t ) *3 ; + } +} +static void sort_relative_table (char *extab_image,int image_size) {int i;
@@ -281,6 +308,9 @@ do_file(char const *const fname)break ;case EM_386 :case EM_X86_64 : + custom_sort = x86_sort_relative_table; +break ; +case EM_S390 : custom_sort = sort_relative_table;break ;
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.Re: [PATCH v7 1/3] x86: Add classes to exception tables
arch/x86/include/asm/asm.h line 52
Quoted 8 lines · Borislav Petkov
#ifdef
__ASSEMBLY__# define _ASM_EXTABLE(from,to) \arch/x86/include/asm/asm.h +2
-1.pushsection "__ex_table","a" ; \ - .balign 8 ; \ + .balign 4 ; \ .long (from) - . ; \ .long (to) - . ; \ + .long 0; \
Why not
.long ex_handler_default - . ;
Then you wouldn't have to special case the zero in the lookup (and in the sort, which you don't do now, but should)
Quoted 12 lines
arch/x86/mm/extable.c +0
-8@@ -33,42 +67,36 @@ int fixup_exception(struct pt_regs *regs) }#endif - fixup =search_exception_tables (regs->ip); -if (fixup) { - new_ip =ex_fixup_addr (fixup); - -if (fixup->fixup - fixup->insn >=0x7ffffff0 -4 ) { -/* Special hack for uaccess_err */ -current_thread_info ()->uaccess_err =1 ; - new_ip -=0x7ffffff0 ;
arch/x86/mm/extable.c int fixup_exception(struct pt_regs *regs) · line 74
Quoted 10 lines
arch/x86/mm/extable.c +4
-5- } - regs->ip = new_ip; -return 1 ; - } + e =search_exception_tables (regs->ip); +if (!e) +return 0 ; -return 0 ; + new_ip =ex_fixup_addr (e);
Assigned, but not used - delete the declaration above too.
scripts/sortextable.c static int compare_relative_table(const void *a, const void *b) · line 221
Quoted 10 lines
scripts/sortextable.c +10
-0+static void x86_sort_relative_table (char *extab_image,int image_size) +{ +int i; + + i =0 ; +while (i < image_size) { +uint32_t *loc = (uint32_t *)(extab_image + i); + +w (r (loc) + i, loc); +w (r (loc +1 ) + i +4 , loc +1 );
Need to twiddle the 'handler' field too (unless it is 0). If you give up on the magic zero and fill in the offset to ex_handler_default then I *think* you need:
w(r(loc + 2) + i + 8, loc + 2); the special case *might* be: if (r(loc + 2)) w(r(loc + 2) + i + 8, loc + 2);
scripts/sortextable.c static int compare_relative_table(const void *a, const void *b) · line 233
Quoted 12 lines
scripts/sortextable.c +12
-0+ + i +=sizeof (uint32_t ) *3 ; + } + +qsort (extab_image, image_size /12 ,12 , compare_relative_table); + + i =0 ; +while (i < image_size) { +uint32_t *loc = (uint32_t *)(extab_image + i); + +w (r (loc) - i, loc); +w (r (loc +1 ) - (i +4 ), loc +1 );
ditto, untwiddle the handler (unless it was zero)
w(r(loc + 2) - (i + 8), loc + 2);
There is also arch/x86/mm/extable.c:sort_extable() which will be used on the main kernel exception table if for some reason the build skipped using scripts/sortextable ... and is always used for exception tables in modules. It also needs to know about the new field (and would be another place to special case the zero fields for the default handler).
-Tony
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v7 1/3] x86: Add classes to exception tables
In reply to Borislav PetkovQuoted 4 lines · Borislav Petkov
Anyway, here's what I have, it boots fine in a guest.
Btw, it seems I'm coming down with the cold and all that above could be hallucinations so please double-check me.
Hardly any hallucinations ... here's an update with the changes I mentioned in earlier e-mail. Boots on actual h/w.
arch/x86/include/asm/asm.h +10 -6
@@ -46,16 +46,18 @@#ifdef __ASSEMBLY__# define _ASM_EXTABLE (from,to) \ .pushsection "__ex_table","a" ; \ - .balign 8 ; \ + .balign 4 ; \ .long (from) - . ; \ .long (to) - . ; \ + .long ex_handler_default - . ; \ .popsection# define _ASM_EXTABLE_EX (from,to) \ .pushsection "__ex_table","a" ; \ - .balign 8 ; \ + .balign 4 ; \ .long (from) - . ; \ - .long (to) - . + 0x7ffffff0 ; \ + .long (to) - . ; \ + .long ex_handler_ext - . ; \ .popsection# define _ASM_NOKPROBE (entry) \
@@ -91,16 +93,18 @@#else # define _ASM_EXTABLE (from,to) \ " .pushsection \"__ex_table\",\"a\"\n" \ - " .balign 8\n" \ + " .balign 4\n" \ " .long (" #from ") - .\n" \ " .long (" #to ") - .\n" \ + " .long ex_handler_default - .\n" \ " .popsection\n"# define _ASM_EXTABLE_EX (from,to) \ " .pushsection \"__ex_table\",\"a\"\n" \ - " .balign 8\n" \ + " .balign 4\n" \ " .long (" #from ") - .\n" \ - " .long (" #to ") - . + 0x7ffffff0\n" \ + " .long (" #to ") - .\n" \ + " .long ex_handler_ext - .\n" \ " .popsection\n"/* For C file, we already have NOKPROBE_SYMBOL macro */ #endif
arch/x86/include/asm/uaccess.h +2 -2
@@ -104,13 +104,13 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un*/ struct exception_table_entry { -int insn, fixup; +int insn, fixup, handler; };/* This is not the generic standard exception_table_entry format */ #define ARCH_HAS_SORT_EXTABLE #define ARCH_HAS_SEARCH_EXTABLE -extern int fixup_exception (struct pt_regs *regs); +extern int fixup_exception (struct pt_regs *regs,int trapnr);extern int early_fixup_exception (unsigned long *ip);/*
arch/x86/kernel/kprobes/core.c +1 -1
@@ -988,7 +988,7 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)* In case the user-specified fault handler returned * zero, try to fix up. */ -if (fixup_exception (regs)) +if (fixup_exception (regs, trapnr))return 1 ;/*
arch/x86/kernel/traps.c +3 -3
@@ -199,7 +199,7 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str, }if (!user_mode (regs)) { -if (!fixup_exception (regs)) { +if (!fixup_exception (regs, trapnr)) { tsk->thread.error_code = error_code; tsk->thread.trap_nr = trapnr;die (str, regs, error_code);
@@ -453,7 +453,7 @@ do_general_protection(struct pt_regs *regs, long error_code) tsk = current;if (!user_mode (regs)) { -if (fixup_exception (regs)) +if (fixup_exception (regs,X86_TRAP_GP ))return ; tsk->thread.error_code = error_code;
@@ -699,7 +699,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)conditional_sti (regs);if (!user_mode (regs)) { -if (!fixup_exception (regs)) { +if (!fixup_exception (regs, trapnr)) { task->thread.error_code = error_code; task->thread.trap_nr = trapnr;die (str, regs, error_code);
arch/x86/mm/extable.c +56 -28
126 lines not on this page · raw message
arch/x86/mm/fault.c +1 -1
@@ -656,7 +656,7 @@ no_context(struct pt_regs *regs, unsigned long error_code,int sig;/* Are we prepared to handle this kernel fault? */ -if (fixup_exception (regs)) { +if (fixup_exception (regs,X86_TRAP_PF )) {/* * Any interrupt that takes a fault gets the fixup. This makes * the below recursive fault logic only apply to a faults from
scripts/sortextable.c +32 -0
@@ -209,6 +209,35 @@ static int compare_relative_table(const void *a, const void *b)return 0 ; } +static void x86_sort_relative_table (char *extab_image,int image_size) +{ +int i; + + i =0 ; +while (i < image_size) { +uint32_t *loc = (uint32_t *)(extab_image + i); + +w (r (loc) + i, loc); +w (r (loc +1 ) + i +4 , loc +1 ); +w (r (loc +2 ) + i +8 , loc +2 ); + + i +=sizeof (uint32_t ) *3 ; + } + +qsort (extab_image, image_size /12 ,12 , compare_relative_table); + + i =0 ; +while (i < image_size) { +uint32_t *loc = (uint32_t *)(extab_image + i); + +w (r (loc) - i, loc); +w (r (loc +1 ) - (i +4 ), loc +1 ); +w (r (loc +2 ) - (i +8 ), loc +2 ); + + i +=sizeof (uint32_t ) *3 ; + } +} +static void sort_relative_table (char *extab_image,int image_size) {int i;
@@ -281,6 +310,9 @@ do_file(char const *const fname)break ;case EM_386 :case EM_X86_64 : + custom_sort = x86_sort_relative_table; +break ; +case EM_S390 : custom_sort = sort_relative_table;break ;
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Borislav Petkov
maintainerRe: [PATCH v7 1/3] x86: Add classes to exception tables
Quoted 8 lines · Tony
Quoted 4 lines · Borislav Petkov
Anyway, here's what I have, it boots fine in a guest.
Btw, it seems I'm coming down with the cold and all that above could be hallucinations so please double-check me.
Hardly any hallucinations ... here's an update with the changes I mentioned in earlier e-mail. Boots on actual h/w.
Cool, thanks for fixing it up. Looks good to me. Feel free to make it into a proper patch and add it to your series... unless you want me to do it.
Just one small question below:
Quoted 17 lines
arch/x86/mm/extable.c +2
-0@@ -3,6 +3,8 @@#include <linux/sort.h> #include <asm/uaccess.h> +typedef int (*ex_handler_t )(const struct exception_table_entry *,struct pt_regs *,int ); +static inline unsigned long ex_insn_addr (const struct exception_table_entry *x) {@@ -14,10 +16,39 @@ ex_fixup_addr(const struct exception_table_entry *x)return (unsigned long )&x->fixup + x->fixup; }
arch/x86/mm/extable.c ex_fixup_addr(const struct exception_table_entry *x) · line 25
Quoted 10 lines
arch/x86/mm/extable.c +6
-3-int fixup_exception (struct pt_regs *regs) +int ex_handler_default (const struct exception_table_entry *fixup, +struct pt_regs *regs,int trapnr) { -const struct exception_table_entry *fixup; -unsigned long new_ip; + regs->ip =ex_fixup_addr (fixup); +return 1 ; +} +EXPORT_SYMBOL (ex_handler_default);
Why not EXPORT_SYMBOL_GPL() ?
We do not care about external modules.
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.RE: [PATCH v7 1/3] x86: Add classes to exception tables
Quoted 5 lines
Quoted 1 line
+EXPORT_SYMBOL(ex_handler_default);
Why not
EXPORT_SYMBOL_GPL()?We do not care about external modules.
I thought the guideline was that new features are GPL, but changes to existing features shouldn't break by adding new GPL requirements.
The point is moot though because the shared hallucinations wore off this morning and I realized that having the "handler" be a pointer to a function can't work. We're storing the 32-bit signed offset from the extable to the target address. This is fine if the table and the address are close together. But for modules we have an exception table wherever vmalloc() loaded the module, and a function back in the base kernel.
So back to your ".long 0" for the default case. And if we want to allow modules to use any of the new handlers, then we can't use relative function pointers for them either.
So I'm looking at making the new field just a simple integer and using it to index an array of function pointers (like in v7).
Unless someone has a better idea?
-Tony
Borislav Petkov
maintainerRe: [PATCH v7 1/3] x86: Add classes to exception tables
Quoted 10 lines · Tony
I thought the guideline was that new features are GPL, but changes to existing features shouldn't break by adding new GPL requirements.
The point is moot though because the shared hallucinations wore off this morning and I realized that having the "handler" be a pointer to a function can't work. We're storing the 32-bit signed offset from the extable to the target address. This is fine if the table and the address are close together. But for modules we have an exception table wherever
vmalloc()loaded the module, and a function back in the base kernel.
Whoops, true story.
Quoted 6 lines
So back to your ".long 0" for the default case. And if we want to allow modules to use any of the new handlers, then we can't use relative function pointers for them either.
So I'm looking at making the new field just a simple integer and using it to index an array of function pointers (like in v7).
Right, that sounds good too. I guess we can even split the integer into
[0 ... 7][8 ... 31]
where slice [0:7] is an index into the handlers array and the remaining unused 24-bits could be used for other stuff later. Normal addition as a way to OR values should work.
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.Re: [PATCH v7 1/3] x86: Add classes to exception tables
In reply to Luck, TonyQuoted 17 lines · Tony
Quoted 5 lines
Quoted 1 line
+EXPORT_SYMBOL(ex_handler_default);
Why not
EXPORT_SYMBOL_GPL()?We do not care about external modules.
I thought the guideline was that new features are GPL, but changes to existing features shouldn't break by adding new GPL requirements.
The point is moot though because the shared hallucinations wore off this morning and I realized that having the "handler" be a pointer to a function can't work. We're storing the 32-bit signed offset from the extable to the target address. This is fine if the table and the address are close together. But for modules we have an exception table wherever
vmalloc()loaded the module, and a function back in the base kernel.
Quoted 8 lines
So back to your ".long 0" for the default case. And if we want to allow modules to use any of the new handlers, then we can't use relative function pointers for them either.
So I'm looking at making the new field just a simple integer and using it to index an array of function pointers (like in v7).
Unless someone has a better idea?
Aren't modules loaded in the top 2GB of address space like the main kernel? Otherwise rip-relative addressing wouldn't work and modules would have to be compiled as PIC.
--
Brian Gerst
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v7 1/3] x86: Add classes to exception tables
In reply to Borislav PetkovAlso need some comment and Documentation/ changes:
Documentation/x86/exception-tables.txt +34 -0
@@ -290,3 +290,37 @@ Due to the way that the exception table is built and needs to be ordered, only use exceptions for code in the .text section. Any other section will cause the exception table to not be sorted correctly, and the exceptions will fail. + +Things changed when 64-bit support was added to x86 Linux. Rather than +double the size of the exception table by expanding the two entries +from 32-bits to 64 bits, a clever trick was used to store addreesses +as relative offsets from the table itself. The assembly code changed +from: + .long 1b,3b +to: + .long (from) - . + .long (to) - . +and the C-code that uses these values converts back to absolute addresses +like this: + ex_insn_addr(const struct exception_table_entry *x) + { + return (unsigned long)&x->insn + x->insn; + } + +In v4.5 the exception table entry was given a new field "handler". +This is also 32-bits wide and contains a table entry relative address +of a handler function that can perform specific operations in addition +to re-writing the instruction pointer to jump to the fixup location. +Initially there are three such functions: + +1) int ex_handler_default(const struct exception_table_entry *fixup, + This is legacy case that just jumps to the fixup code +2) int ex_handler_fault(const struct exception_table_entry *fixup, + This case provides the fault number of the trap that occured at + entry->insn. It is used to distinguish page faults from machine + check. +3) int ex_handler_ext(const struct exception_table_entry *fixup, + This case is used to for uaccess_err ... we need to set a flag + in the task structure. Before the handler functions existed this + case was handled by adding a large offset to the fixup to tag + it as special.
arch/x86/include/asm/uaccess.h +5 -5
@@ -90,12 +90,12 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, unlikely (!__range_not_ok (addr, size,user_addr_max ()))/* -* The exception table consists of pairs of addresses relative to the +* The exception table consists of triples of addresses relative to the * exception table enty itself: the first is the address of an -* instruction that is allowed to fault, and the second is the address -* at which the program should continue. No registers are modified, -* so it is entirely up to the continuation code to figure out what to -* do. +* instruction that is allowed to fault, the second is the address +* at which the program should continue, the last is the address of +* a handler function to deal with the fault referenced by the instruction +* in the first field. * * All the routines below use bits of fixup code that are out of line * with the main instruction path. This means when everything is well,
arch/x86/lib/memcpy_64.S +0 -1
@@ -310,4 +310,3 @@ ENTRY(__mcsafe_copy) _ASM_EXTABLE_FAULT(12b,38b) _ASM_EXTABLE_FAULT(18b,39b) _ASM_EXTABLE_FAULT(21b,40b) -#endif
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v7 1/3] x86: Add classes to exception tables
Quoted 57 lines · Tony
Also need some comment and
Documentation/changes:Documentation/x86/exception-tables.txt +34
-0@@ -290,3 +290,37 @@ Due to the way that the exception table is built and needs to be ordered, only use exceptions for code in the .text section. Any other section will cause the exception table to not be sorted correctly, and the exceptions will fail. + +Things changed when 64-bit support was added to x86 Linux. Rather than +double the size of the exception table by expanding the two entries +from 32-bits to 64 bits, a clever trick was used to store addreesses +as relative offsets from the table itself. The assembly code changed +from: + .long 1b,3b +to: + .long (from) - . + .long (to) - . +and the C-code that uses these values converts back to absolute addresses +like this: + ex_insn_addr(const struct exception_table_entry *x) + { + return (unsigned long)&x->insn + x->insn; + } + +In v4.5 the exception table entry was given a new field "handler". +This is also 32-bits wide and contains a table entry relative address +of a handler function that can perform specific operations in addition +to re-writing the instruction pointer to jump to the fixup location. +Initially there are three such functions: + +1) int ex_handler_default(const struct exception_table_entry *fixup, + This is legacy case that just jumps to the fixup code +2) int ex_handler_fault(const struct exception_table_entry *fixup, + This case provides the fault number of the trap that occured at + entry->insn. It is used to distinguish page faults from machine + check. +3) int ex_handler_ext(const struct exception_table_entry *fixup, + This case is used to for uaccess_err ... we need to set a flag + in the task structure. Before the handler functions existed this + case was handled by adding a large offset to the fixup to tag + it as special.arch/x86/include/asm/uaccess.h +1
-2@@ -90,12 +90,12 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, unlikely (!__range_not_ok (addr, size,user_addr_max ()))/* -* The exception table consists of pairs of addresses relative to the +* The exception table consists of triples of addresses relative to the * exception table enty itself: the first is the address of an -* instruction that is allowed to fault, and the second is the address
arch/x86/include/asm/uaccess.h static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un · line 101
Quoted 10 lines
arch/x86/include/asm/uaccess.h +4
-3-* at which the program should continue. No registers are modified, -* so it is entirely up to the continuation code to figure out what to -* do. +* instruction that is allowed to fault, the second is the address +* at which the program should continue, the last is the address of +* a handler function to deal with the fault referenced by the instruction +* in the first field. * * All the routines below use bits of fixup code that are out of line * with the main instruction path. This means when everything is well,
Looks good. /me always likes patches adding more sensible documentation:
arch/x86/lib/memcpy_64.S ENTRY(__mcsafe_copy) · line 313
Quoted 9 lines
arch/x86/lib/memcpy_64.S +0
-1@@ -310,4 +310,3 @@ ENTRY(__mcsafe_copy) _ASM_EXTABLE_FAULT(12b,38b) _ASM_EXTABLE_FAULT(18b,39b) _ASM_EXTABLE_FAULT(21b,40b) -#endif
This looks like a stray change.
Thanks.
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.Borislav Petkov
maintainerRe: [PATCH v7 1/3] x86: Add classes to exception tables
In reply to Tony LuckQuoted 23 lines · Tony Luck
Starting with a patch from Andy Lutomirski <[email protected]> that used linker relocation trickery to free up a couple of bits in the "fixup" field of the exception table (and generalized the
uaccess_errhack to use one of the classes).This patch allocates another one of the classes to provide a mechanism to provide the fault number to the fixup code in %rax.
Still one free class for the next brilliant idea. If more are needed it should be possible to squeeze another bit or three extending the same technique.
Originally-from: Andy Lutomirski <[email protected]>
Signed-off-by Tony Luck <[email protected]>--- arch/x86/include/asm/asm.h | 102 +++++++++++++++++++++++++++++++---------- arch/x86/include/asm/uaccess.h | 17 +++++-- arch/x86/kernel/kprobes/core.c | 2 +- arch/x86/kernel/traps.c | 6 +-- arch/x86/mm/extable.c | 66 ++++++++++++++++++-------- arch/x86/mm/fault.c | 2 +- 6 files changed, 142 insertions(+), 53 deletions(-)
...
arch/x86/kernel/traps.c static void math_error(struct pt_regs *regs, int error_code, int trapnr) · line 702
Quoted 6 lines
arch/x86/kernel/traps.c +1
-1@@ -699,7 +699,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)conditional_sti (regs);if (!user_mode (regs)) { -if (!fixup_exception (regs)) { +if (!fixup_exception (regs,X86_TRAP_DE )) {
Whatever we end up doing, this needs to be trapnr above and not X86_TRAP_DE.
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.PATCH v7 2/3 x86, mce: Check for faults tagged in EXTABLE_CLASS_FAULT exception table entries
In reply to Tony LuckExtend the severity checking code to add a new context IN_KERN_RECOV which is used to indicate that the machine check was triggered by code in the kernel with a EXTABLE_CLASS_FAULT fixup entry.
Major re-work to the tail code in do_machine_check() to make all this readable/maintainable. One functional change is that tolerant=3 no longer stops recovery actions. Revert to only skipping sending SIGBUS to the current process.
--- arch/x86/kernel/cpu/mcheck/mce-severity.c | 32 +++++++++++++- arch/x86/kernel/cpu/mcheck/mce.c | 71 ++++++++++++++++--------------- 2 files changed, 67 insertions(+), 36 deletions(-)
arch/x86/kernel/cpu/mcheck/mce-severity.c +30 -2
@@ -13,7 +13,9 @@#include <linux/seq_file.h> #include <linux/init.h> #include <linux/debugfs.h> +#include <linux/module.h> #include <asm/mce.h> +#include <asm/uaccess.h> #include "mce-internal.h"
@@ -29,7 +31,7 @@* panic situations) */ -enum context {IN_KERNEL =1 ,IN_USER =2 }; +enum context {IN_KERNEL =1 ,IN_USER =2 ,IN_KERNEL_RECOV =3 };enum ser {SER_REQUIRED =1 ,NO_SER =2 };enum exception {EXCP_CONTEXT =1 ,NO_EXCP =2 };
@@ -48,6 +50,7 @@ static struct severity { #define MCESEV(s, m, c...) { .sev = MCE_ ## s ## _SEVERITY, .msg = m, ## c }#define KERNEL .context = IN_KERNEL#define USER .context = IN_USER +#define KERNEL_RECOV .context = IN_KERNEL_RECOV#define SER .ser = SER_REQUIRED#define NOSER .ser = NO_SER#define EXCP .excp = EXCP_CONTEXT
@@ -87,6 +90,10 @@ static struct severity { EXCP, KERNEL, MCGMASK(MCG_STATUS_RIPV, 0) ),MCESEV ( +PANIC ,"In kernel and no restart IP" , +EXCP ,KERNEL_RECOV ,MCGMASK (MCG_STATUS_RIPV ,0 ) + ), +MCESEV (DEFERRED ,"Deferred error" ,NOSER ,MASK (MCI_STATUS_UC |MCI_STATUS_DEFERRED |MCI_STATUS_POISON ,MCI_STATUS_DEFERRED ) ),
@@ -123,6 +130,11 @@ static struct severity {MCGMASK (MCG_STATUS_RIPV |MCG_STATUS_EIPV ,MCG_STATUS_RIPV ) ),MCESEV ( +AR ,"Action required: data load in error recoverable area of kernel" , +SER ,MASK (MCI_STATUS_OVER |MCI_UC_SAR |MCI_ADDR |MCACOD ,MCI_UC_SAR |MCI_ADDR |MCACOD_DATA ), +KERNEL_RECOV + ), +MCESEV (AR ,"Action required: data load error in a user process" ,SER ,MASK (MCI_STATUS_OVER |MCI_UC_SAR |MCI_ADDR |MCACOD ,MCI_UC_SAR |MCI_ADDR |MCACOD_DATA ), USER
@@ -170,6 +182,18 @@ static struct severity { ) /* always matches. keep at end */ }; +#define mc_recoverable (mcg) (((mcg) & (MCG_STATUS_RIPV|MCG_STATUS_EIPV)) == \ + (MCG_STATUS_RIPV|MCG_STATUS_EIPV)) + +static inline bool mce_in_kernel_recov (unsigned long addr) +{ +const struct exception_table_entry *fixup; + + fixup =search_exception_tables (addr); + +return fixup &&ex_class (fixup) ==EXTABLE_CLASS_FAULT ; +} +/* * If mcgstatus indicated that ip/cs on the stack were * no good, then "m->cs" will be zero and we will have
@@ -183,7 +207,11 @@ static struct severity {*/ static int error_context (struct mce *m) { -return ((m->cs &3 ) ==3 ) ?IN_USER :IN_KERNEL ; +if ((m->cs &3 ) ==3 ) +return IN_USER ; +if (mc_recoverable (m->mcgstatus) &&mce_in_kernel_recov (m->ip)) +return IN_KERNEL_RECOV ; +return IN_KERNEL ; }/*
arch/x86/kernel/cpu/mcheck/mce.c +37 -34
104 lines not on this page · raw message
Signature · 1 line
2.1.4
PATCH v7 3/3 x86, mce: Add __mcsafe_copy()
In reply to Tony LuckMake use of the EXTABLE_FAULT exception table entries. This routine returns a structure to indicate the result of the copy:
struct mcsafe_ret {
u64 trapnr;
u64 remain;
};If the copy is successful, then both 'trapnr' and 'remain' are zero.
If we faulted during the copy, then 'trapnr' will say which type of trap (X86_TRAP_PF or X86_TRAP_MC) and 'remain' says how many bytes were not copied.
--- arch/x86/Kconfig | 10 +++ arch/x86/include/asm/string_64.h | 10 +++ arch/x86/kernel/x8664_ksyms_64.c | 4 ++ arch/x86/lib/memcpy_64.S | 136 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 160 insertions(+)
arch/x86/Kconfig +10 -0
@@ -1001,6 +1001,16 @@ config X86_MCE_INJECT If you don't know what a machine check is and you don't do kernel QA it is safe to say n. +config MCE_KERNEL_RECOVERY + bool "Recovery from machine checks in special kernel memory copy functions" + default n + depends on X86_MCE && X86_64 + ---help--- + This option provides a new memory copy function mcsafe_memcpy() + that is annotated to allow the machine check handler to return + to an alternate code path to return an error to the caller instead + of crashing the system. Say yes if you have a driver that uses this. + config X86_THERMAL_VECTOR def_bool y depends on X86_MCE_INTEL
arch/x86/include/asm/string_64.h +10 -0
@@ -78,6 +78,16 @@ int strcmp(const char *cs, const char *ct);#define memset (s, c, n) __memset(s, c, n)#endif +#ifdef CONFIG_MCE_KERNEL_RECOVERY +struct mcsafe_ret { +u64 trapnr; +u64 remain; +}; + +struct mcsafe_ret __mcsafe_copy (void *dst,const void __user *src,size_t cnt); +extern void __mcsafe_copy_end (void ); +#endif +#endif /* __KERNEL__ */ #endif /* _ASM_X86_STRING_64_H */
arch/x86/kernel/x8664_ksyms_64.c +4 -0
@@ -37,6 +37,10 @@ EXPORT_SYMBOL(__copy_user_nocache);EXPORT_SYMBOL (_copy_from_user);EXPORT_SYMBOL (_copy_to_user); +#ifdef CONFIG_MCE_KERNEL_RECOVERY +EXPORT_SYMBOL (__mcsafe_copy); +#endif +EXPORT_SYMBOL (copy_page);EXPORT_SYMBOL (clear_page);
arch/x86/lib/memcpy_64.S +136 -0
139 lines not on this page · raw message
Signature · 7 lines
2.1.4 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v7 3/3] x86, mce: Add __mcsafe_copy()
Quoted 48 lines · Tony Luck
Make use of the
EXTABLE_FAULTexception table entries. This routine returns a structure to indicate the result of the copy:struct mcsafe_ret { u64 trapnr; u64 remain; };If the copy is successful, then both 'trapnr' and 'remain' are zero.
If we faulted during the copy, then 'trapnr' will say which type of trap (
X86_TRAP_PForX86_TRAP_MC) and 'remain' says how many bytes were not copied.Signed-off-by Tony Luck <[email protected]>--- arch/x86/Kconfig | 10 +++ arch/x86/include/asm/string_64.h | 10 +++ arch/x86/kernel/x8664_ksyms_64.c | 4 ++ arch/x86/lib/memcpy_64.S | 136 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 160 insertions(+)arch/x86/Kconfig +10
-0@@ -1001,6 +1001,16 @@ config X86_MCE_INJECT If you don't know what a machine check is and you don't do kernel QA it is safe to say n. +config MCE_KERNEL_RECOVERY + bool "Recovery from machine checks in special kernel memory copy functions" + default n + depends on X86_MCE && X86_64 + ---help--- + This option provides a new memory copy function mcsafe_memcpy() + that is annotated to allow the machine check handler to return + to an alternate code path to return an error to the caller instead + of crashing the system. Say yes if you have a driver that uses this. + config X86_THERMAL_VECTOR def_bool y depends on X86_MCE_INTELarch/x86/include/asm/string_64.h +0
-0
arch/x86/include/asm/string_64.h int strcmp(const char *cs, const char *ct); · line 85
Quoted 8 lines
arch/x86/include/asm/string_64.h +5
-0@@ -78,6 +78,16 @@ int strcmp(const char *cs, const char *ct);#define memset (s, c, n) __memset(s, c, n)#endif +#ifdef CONFIG_MCE_KERNEL_RECOVERY +struct mcsafe_ret { +u64 trapnr; +u64 remain; +};
Can we move this definition outside of the CONFIG_MCE_KERNEL_RECOVERY ifdef guard? On a test integration branch the kbuild robot caught the following:
In file included from include/linux/pmem.h:21:0,
from drivers/acpi/nfit.c:22:
arch/x86/include/asm/pmem.h: In function 'arch_memcpy_from_pmem':
Quoted 1 line
Quoted 1 line
arch/x86/include/asm/pmem.h:55:21: error: storage size of 'ret' isn't known
struct mcsafe_ret ret;
^Quoted 1 line
Quoted 1 line
arch/x86/include/asm/pmem.h:57:9: error: implicit declaration of function '__mcsafe_copy' [-Werror=implicit-function-declaration]
ret = __mcsafe_copy(dst, (void __force *) src, n);
^Quoted 1 line
Quoted 1 line
arch/x86/include/asm/pmem.h:55:21: warning: unused variable 'ret' [-Wunused-variable]
struct mcsafe_ret ret;
^cc1: some warnings being treated as errors
vim +55 arch/x86/include/asm/pmem.h
49 }
50
51 static inline int arch_memcpy_from_pmem(void *dst, const void
__pmem *src,
52 size_t n)
53 {
54 if (IS_ENABLED(CONFIG_MCE_KERNEL_RECOVERY)) {Quoted 1 line
55 struct
mcsafe_retret;
56
Quoted 1 line
57 ret =
__mcsafe_copy(dst, (void __force *) src, n);
58 if (ret.remain)
59 return -EIO;
60 return 0;--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v7 3/3] x86, mce: Add __mcsafe_copy()
You were heading towards:
ld: undefined __mcsafe_copy
since that is also inside the #ifdef.
Weren't you going to "select" this?
I'm seriously wondering whether the ifdef still makes sense. Now I don't have an extra exception table and routines to sort/search/fixup, it doesn't seem as useful as it was a few iterations ago.
Sent from my iPhone
Quoted 87 lines · Dan Williams
Quoted 50 lines · Tony Luck
Make use of the
EXTABLE_FAULTexception table entries. This routine returns a structure to indicate the result of the copy:struct mcsafe_ret { u64 trapnr; u64 remain; };If the copy is successful, then both 'trapnr' and 'remain' are zero.
If we faulted during the copy, then 'trapnr' will say which type of trap (
X86_TRAP_PForX86_TRAP_MC) and 'remain' says how many bytes were not copied.Signed-off-by Tony Luck <[email protected]>---
arch/x86/Kconfig | 10 +++ arch/x86/include/asm/string_64.h | 10 +++ arch/x86/kernel/x8664_ksyms_64.c | 4 ++ arch/x86/lib/memcpy_64.S | 136 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 160 insertions(+)arch/x86/Kconfig +10
-0@@ -1001,6 +1001,16 @@ config X86_MCE_INJECT If you don't know what a machine check is and you don't do kernel QA it is safe to say n. +config MCE_KERNEL_RECOVERY + bool "Recovery from machine checks in special kernel memory copy functions" + default n + depends on X86_MCE && X86_64 + ---help--- + This option provides a new memory copy function mcsafe_memcpy() + that is annotated to allow the machine check handler to return + to an alternate code path to return an error to the caller instead + of crashing the system. Say yes if you have a driver that uses this. +config
X86_THERMAL_VECTOR
def_booly
depends onX86_MCE_INTELarch/x86/include/asm/string_64.h +0
-0arch/x86/include/asm/string_64.h int strcmp(const char *cs, const char *ct); · line 85
Quoted 8 lines
arch/x86/include/asm/string_64.h +5
-0@@ -78,6 +78,16 @@ int strcmp(const char *cs, const char *ct);#define memset (s, c, n) __memset(s, c, n)#endif +#ifdef CONFIG_MCE_KERNEL_RECOVERY +struct mcsafe_ret { +u64 trapnr; +u64 remain; +};Can we move this definition outside of the
CONFIG_MCE_KERNEL_RECOVERYifdef guard? On a test integration branch the kbuild robot caught the following:In file included from
include/linux/pmem.h:21:0,
fromdrivers/acpi/nfit.c:22:
arch/x86/include/asm/pmem.h: In function 'arch_memcpy_from_pmem':Quoted 1 line
Quoted 1 line
arch/x86/include/asm/pmem.h:55:21: error: storage size of 'ret' isn't knownstruct mcsafe_ret ret; ^Quoted 1 line
Quoted 1 line
arch/x86/include/asm/pmem.h:57:9: error: implicit declaration of function '__mcsafe_copy' [-Werror=implicit-function-declaration]ret = __mcsafe_copy(dst, (void __force *) src, n); ^Quoted 1 line
Quoted 1 line
arch/x86/include/asm/pmem.h:55:21: warning: unused variable 'ret' [-Wunused-variable]struct mcsafe_ret ret; ^cc1: some warnings being treated as errors
vim +55
arch/x86/include/asm/pmem.h49 } 50 51 static inline int arch_memcpy_from_pmem(void *dst, const void __pmem *src, 52 size_t n) 53 { 54 if (IS_ENABLED(CONFIG_MCE_KERNEL_RECOVERY)) {
Quoted 6 lines
Quoted 1 line
55 struct
mcsafe_retret;56
Quoted 1 line
57 ret =
__mcsafe_copy(dst, (void __force *) src, n);58 if (ret.remain)
59 return -EIO; 60 return 0;
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v7 3/3] x86, mce: Add __mcsafe_copy()
Quoted 3 lines · Tony
You were heading towards:
ld: undefined
__mcsafe_copy
True, we'd also need a dummy mcsafe_copy() definition to compile it out in the disabled case.
Quoted 4 lines
since that is also inside the #ifdef.
Weren't you going to "select" this?
I do select it, but by randconfig I still need to handle the CONFIG_X86_MCE=n case.
Quoted 1 line
I'm seriously wondering whether the ifdef still makes sense. Now I don't have an extra exception table and routines to sort/search/fixup, it doesn't seem as useful as it was a few iterations ago.
Either way is ok with me. That said, the extra definitions to allow it compile out when not enabled don't seem too onerous.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v7 3/3] x86, mce: Add __mcsafe_copy()
Quoted 13 lines · Dan Williams
Quoted 3 lines · Tony
You were heading towards:
ld: undefined
__mcsafe_copyTrue, we'd also need a dummy
mcsafe_copy()definition to compile it out in the disabled case.Quoted 4 lines
since that is also inside the #ifdef.
Weren't you going to "select" this?
Quoted 7 lines
I do select it, but by randconfig I still need to handle the
CONFIG_X86_MCE=ncase.Quoted 1 line
I'm seriously wondering whether the ifdef still makes sense. Now I don't have an extra exception table and routines to sort/search/fixup, it doesn't seem as useful as it was a few iterations ago.
Either way is ok with me. That said, the extra definitions to allow it compile out when not enabled don't seem too onerous.
This works for me, because all we need is the definitions. As long as we don't attempt to link to mcsafe_copy() we get the benefit of compiling this out when de-selected:
arch/x86/include/asm/string_64.h +0 -0
@@ -78,7 +78,6 @@ int strcmp(const char *cs, const char *ct);
#define memset(s, c, n) __memset(s, c, n)
#endif
-#ifdef CONFIG_MCE_KERNEL_RECOVERY
struct mcsafe_ret {
u64 trapnr;
u64 remain;@@ -86,7 +85,6 @@ struct mcsafe_ret {
struct mcsafe_ret __mcsafe_copy(void *dst, const void __user *src, size_t cnt); extern void __mcsafe_copy_end(void);
-#endif
#endif /* __KERNEL__ */
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
RE: [PATCH v7 3/3] x86, mce: Add __mcsafe_copy()
Pj4gSSBkbyBzZWxlY3QgaXQsIGJ1dCBieSByYW5kY29uZmlnIEkgc3RpbGwgbmVlZCB0byBoYW5k bGUgdGhlDQo+PiBDT05GSUdfWDg2X01DRT1uIGNhc2UuDQo+Pg0KPj4+IEknbSBzZXJpb3VzbHkg d29uZGVyaW5nIHdoZXRoZXIgdGhlIGlmZGVmIHN0aWxsIG1ha2VzIHNlbnNlLiBOb3cgSSBkb24n dCBoYXZlIGFuIGV4dHJhIGV4Y2VwdGlvbiB0YWJsZSBhbmQgcm91dGluZXMgdG8gc29ydC9zZWFy Y2gvZml4dXAsIGl0IGRvZXNuJ3Qgc2VlbSBhcyB1c2VmdWwgYXMgaXQgd2FzIGEgZmV3IGl0ZXJh dGlvbnMgYWdvLg0KPj4NCj4+IEVpdGhlciB3YXkgaXMgb2sgd2l0aCBtZS4gIFRoYXQgc2FpZCwg dGhlIGV4dHJhIGRlZmluaXRpb25zIHRvIGFsbG93DQo+PiBpdCBjb21waWxlIG91dCB3aGVuIG5v dCBlbmFibGVkIGRvbid0IHNlZW0gdG9vIG9uZXJvdXMuDQo+DQo+IFRoaXMgd29ya3MgZm9yIG1l LCBiZWNhdXNlIGFsbCB3ZSBuZWVkIGlzIHRoZSBkZWZpbml0aW9ucy4gIEFzIGxvbmcgYXMNCj4g d2UgZG9uJ3QgYXR0ZW1wdCB0byBsaW5rIHRvIG1jc2FmZV9jb3B5KCkgd2UgZ2V0IHRoZSBiZW5l Zml0IG9mDQo+IGNvbXBpbGluZyB0aGlzIG91dCB3aGVuIGRlLXNlbGVjdGVkOg0KDQpJdCBzZWVt cyAgdGhhdCBLY29uZmlnJ3MgInNlbGVjdCIgc3RhdGVtZW50IGRvZXNuJ3QgYXV0by1zZWxlY3Qg b3RoZXIgdGhpbmdzDQp0aGF0IGFyZSBkZXBlbmRlbmNpZXMgb2YgdGhlIHN5bWJvbCB5b3UgY2hv b3NlLg0KDQpDT05GSUdfTUNFX0tFUk5FTF9SRUNPVkVSWSByZWFsbHkgaXMgZGVwZW5kZW50IG9u DQpDT05GSUdfWDg2X01DRSAuLi4gaGF2aW5nIHRoZSBjb2RlIGZvciB0aGUgX19tY3NhZmVfY29w eSgpDQpsaW5rZWQgaW50byB0aGUga2VybmVsIHdvbid0IGRvIHlvdSBhbnkgZ29vZCB3aXRob3V0 IGEgbWFjaGluZQ0KY2hlY2sgaGFuZGxlciB0aGF0IGp1bXBzIHRvIHRoZSBmaXh1cCBjb2RlLg0K DQpTbyBJIHRoaW5rIHlvdSBoYXZlIHRvIHNlbGVjdCBYODZfTUNFIGFzIHdlbGwgKG9yIEtjb25m aWcgbmVlZHMNCnRvIGJlIHRhdWdodCB0byBkbyBpdCBhdXRvbWF0aWNhbGx5IC4uLiBidXQgSSBo YXZlIGEgbmFnZ2luZyBmZWVsaW5nDQp0aGF0IHRoaXMgaXMga25vd24gYmVoYXZpb3IpLg0KDQot VG9ueQ0K
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v7 3/3] x86, mce: Add __mcsafe_copy()
Quoted 15 lines · Tony
Quoted 11 lines
Quoted 7 lines
I do select it, but by randconfig I still need to handle the
CONFIG_X86_MCE=ncase.Quoted 1 line
I'm seriously wondering whether the ifdef still makes sense. Now I don't have an extra exception table and routines to sort/search/fixup, it doesn't seem as useful as it was a few iterations ago.
Either way is ok with me. That said, the extra definitions to allow it compile out when not enabled don't seem too onerous.
This works for me, because all we need is the definitions. As long as we don't attempt to link to
mcsafe_copy()we get the benefit of compiling this out when de-selected:It seems that Kconfig's "select" statement doesn't auto-select other things that are dependencies of the symbol you choose.
Quoted 9 lines
CONFIG_MCE_KERNEL_RECOVERYreally is dependent onCONFIG_X86_MCE... having the code for the__mcsafe_copy()linked into the kernel won't do you any good without a machine check handler that jumps to the fixup code.So I think you have to select
X86_MCEas well (orKconfigneeds to be taught to do it automatically ... but I have a nagging feeling that this is known behavior).
I don't want to force it on, otherwise we might as well remove the ability to configure it. Instead I have this:
config BLK_DEV_PMEM
select MCE_KERNEL_RECOVERY if X86_MCE && X86_64
...that way if you turn on X86_MCE and BLK_DEV_PMEM you get MCE_KERNEL_RECOVERY by default.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>