From: Joe P. <jo...@pe...> - 2012-12-12 18:19:41
|
Remove the somewhat awkward uses of print_symbol and convert all the existing uses to a new vsprintf pointer type of %pSR. print_symbol can be interleaved when it is used in a sequence like: printk("something: ..."); print_symbol("%s", addr); printk("\n"); Instead use: printk("something: %pSR\n", (void *)addr); Add a new %p[SsFf]R vsprintf extension that can perform the same symbol function/address/offset formatting as print_symbol to reduce the number and styles of message logging functions. print_symbol used __builtin_extract_return_addr for those architectures like S/390 and SPARC that have offset or masked addressing. %p[FfSs]R uses the same gcc __builtin Joe Perches (26): vsprintf: Add extension %pSR - print_symbol replacement alpha: Convert print_symbol to %pSR arm: Convert print_symbol to %pSR arm64: Convert print_symbol to %pSR avr32: Convert print_symbol to %pSR c6x: Convert print_symbol to %pSR ia64: Convert print_symbol to %pSR m32r: Convert print_symbol to %pSR mn10300: Convert print_symbol to %pSR openrisc: Convert print_symbol to %pSR powerpc: Convert print_symbol to %pSR s390: Convert print_symbol to %pSR sh: Convert print_symbol to %pSR um: Convert print_symbol to %pSR unicore32: Convert print_symbol to %pSR x86: Convert print_symbol to %pSR xtensa: Convert print_symbol to %pSR drivers: base: Convert print_symbol to %pSR gfs2: Convert print_symbol to %pSR sysfs: Convert print_symbol to %pSR irq: Convert print_symbol to %pSR smp_processor_id: Convert print_symbol to %pSR mm: Convert print_symbol to %pSR xtensa: Convert print_symbol to %pSR x86: head_64.S: Use vsprintf extension %pSR not print_symbol kallsyms: Remove print_symbol Documentation/filesystems/sysfs.txt | 4 +- Documentation/printk-formats.txt | 2 + Documentation/zh_CN/filesystems/sysfs.txt | 4 +- arch/alpha/kernel/traps.c | 8 ++---- arch/arm/kernel/process.c | 4 +- arch/arm64/kernel/process.c | 4 +- arch/avr32/kernel/process.c | 25 ++++++----------------- arch/c6x/kernel/traps.c | 3 +- arch/ia64/kernel/process.c | 13 ++++------- arch/m32r/kernel/traps.c | 6 +--- arch/mn10300/kernel/traps.c | 8 +++--- arch/openrisc/kernel/traps.c | 7 +---- arch/powerpc/platforms/cell/spu_callbacks.c | 12 ++++------ arch/s390/kernel/traps.c | 28 +++++++++++++++----------- arch/sh/kernel/process_32.c | 4 +- arch/um/kernel/sysrq.c | 6 +--- arch/unicore32/kernel/process.c | 5 ++- arch/x86/kernel/cpu/mcheck/mce.c | 13 ++++++----- arch/x86/kernel/dumpstack.c | 5 +-- arch/x86/kernel/head_64.S | 4 +- arch/x86/kernel/process_32.c | 2 +- arch/x86/mm/mmio-mod.c | 4 +- arch/x86/um/sysrq_32.c | 9 ++----- arch/xtensa/kernel/traps.c | 6 +--- drivers/base/core.c | 4 +- fs/gfs2/glock.c | 4 +- fs/gfs2/trans.c | 3 +- fs/sysfs/file.c | 4 +- include/linux/kallsyms.h | 18 ----------------- kernel/irq/debug.h | 15 ++++++------- kernel/kallsyms.c | 11 ---------- lib/smp_processor_id.c | 2 +- lib/vsprintf.c | 18 ++++++++++++---- mm/memory.c | 8 +++--- mm/slab.c | 8 ++---- 35 files changed, 117 insertions(+), 164 deletions(-) -- 1.7.8.112.g3fd21 |
From: Joe P. <jo...@pe...> - 2012-12-12 18:21:55
|
Use the new vsprintf extension to avoid any possible message interleaving. Signed-off-by: Joe Perches <jo...@pe...> --- arch/um/kernel/sysrq.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-) diff --git a/arch/um/kernel/sysrq.c b/arch/um/kernel/sysrq.c index e562ff8..6fa632c 100644 --- a/arch/um/kernel/sysrq.c +++ b/arch/um/kernel/sysrq.c @@ -24,10 +24,8 @@ void show_trace(struct task_struct *task, unsigned long * stack) while (((long) stack & (THREAD_SIZE-1)) != 0) { addr = *stack; if (__kernel_text_address(addr)) { - printk(KERN_INFO "%08lx: [<%08lx>]", - (unsigned long) stack, addr); - print_symbol(KERN_CONT " %s", addr); - printk(KERN_CONT "\n"); + printk(KERN_INFO "%08lx: [<%08lx>] %pSR\n", + (unsigned long)stack, addr, (void *)addr); } stack++; } -- 1.7.8.112.g3fd21 |
From: Joe P. <jo...@pe...> - 2012-12-12 18:22:15
|
Use the new vsprintf extension to avoid any possible message interleaving. Signed-off-by: Joe Perches <jo...@pe...> --- arch/x86/kernel/cpu/mcheck/mce.c | 13 +++++++------ arch/x86/kernel/dumpstack.c | 5 ++--- arch/x86/kernel/process_32.c | 2 +- arch/x86/mm/mmio-mod.c | 4 ++-- arch/x86/um/sysrq_32.c | 9 +++------ 5 files changed, 15 insertions(+), 18 deletions(-) diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c index 80dbda8..996f98a 100644 --- a/arch/x86/kernel/cpu/mcheck/mce.c +++ b/arch/x86/kernel/cpu/mcheck/mce.c @@ -242,13 +242,14 @@ static void print_mce(struct mce *m) m->extcpu, m->mcgstatus, m->bank, m->status); if (m->ip) { - pr_emerg(HW_ERR "RIP%s %02x:<%016Lx> ", - !(m->mcgstatus & MCG_STATUS_EIPV) ? " !INEXACT!" : "", - m->cs, m->ip); - if (m->cs == __KERNEL_CS) - print_symbol("{%s}", m->ip); - pr_cont("\n"); + pr_emerg(HW_ERR "RIP%s %02x:<%016Lx> {%pSR}\n", + !(m->mcgstatus & MCG_STATUS_EIPV) ? " !INEXACT!" : "", + m->cs, m->ip, (void *)m->ip); + else + pr_emerg(HW_ERR "RIP%s %02x:<%016Lx>\n", + !(m->mcgstatus & MCG_STATUS_EIPV) ? " !INEXACT!" : "", + m->cs, m->ip); } pr_emerg(HW_ERR "TSC %llx ", m->tsc); diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c index ae42418b..b6e5bdb 100644 --- a/arch/x86/kernel/dumpstack.c +++ b/arch/x86/kernel/dumpstack.c @@ -281,9 +281,8 @@ int __kprobes __die(const char *str, struct pt_regs *regs, long err) sp = kernel_stack_pointer(regs); savesegment(ss, ss); } - printk(KERN_EMERG "EIP: [<%08lx>] ", regs->ip); - print_symbol("%s", regs->ip); - printk(" SS:ESP %04x:%08lx\n", ss, sp); + printk(KERN_EMERG "EIP: [<%08lx>] %pSR SS:ESP %04x:%08lx\n", + regs->ip, (void *)regs->ip, ss, sp); #else /* Executive summary in case the oops scrolled away */ printk(KERN_ALERT "RIP "); diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c index b5a8905..0db77e0 100644 --- a/arch/x86/kernel/process_32.c +++ b/arch/x86/kernel/process_32.c @@ -89,7 +89,7 @@ void __show_regs(struct pt_regs *regs, int all) printk(KERN_DEFAULT "EIP: %04x:[<%08lx>] EFLAGS: %08lx CPU: %d\n", (u16)regs->cs, regs->ip, regs->flags, smp_processor_id()); - print_symbol("EIP is at %s\n", regs->ip); + printk(KERN_DEFAULT "EIP is at %pSR\n", (void *)regs->ip); printk(KERN_DEFAULT "EAX: %08lx EBX: %08lx ECX: %08lx EDX: %08lx\n", regs->ax, regs->bx, regs->cx, regs->dx); diff --git a/arch/x86/mm/mmio-mod.c b/arch/x86/mm/mmio-mod.c index dc0b727..c0ca484 100644 --- a/arch/x86/mm/mmio-mod.c +++ b/arch/x86/mm/mmio-mod.c @@ -123,8 +123,8 @@ static void die_kmmio_nesting_error(struct pt_regs *regs, unsigned long addr) pr_emerg("unexpected fault for address: 0x%08lx, last fault for address: 0x%08lx\n", addr, my_reason->addr); print_pte(addr); - print_symbol(KERN_EMERG "faulting IP is at %s\n", regs->ip); - print_symbol(KERN_EMERG "last faulting IP was at %s\n", my_reason->ip); + pr_emerg("faulting IP is at %pSR\n", (void *)regs->ip); + pr_emerg("last faulting IP was at %pSR\n", (void *)my_reason->ip); #ifdef __i386__ pr_emerg("eax: %08lx ebx: %08lx ecx: %08lx edx: %08lx\n", regs->ax, regs->bx, regs->cx, regs->dx); diff --git a/arch/x86/um/sysrq_32.c b/arch/x86/um/sysrq_32.c index c9bee5b..30b4200 100644 --- a/arch/x86/um/sysrq_32.c +++ b/arch/x86/um/sysrq_32.c @@ -50,18 +50,15 @@ static inline unsigned long print_context_stack(struct thread_info *tinfo, #ifdef CONFIG_FRAME_POINTER while (valid_stack_ptr(tinfo, (void *)ebp)) { addr = *(unsigned long *)(ebp + 4); - printk("%08lx: [<%08lx>]", ebp + 4, addr); - print_symbol(" %s", addr); - printk("\n"); + printk("%08lx: [<%08lx>] %pSR\n", ebp + 4, addr, (void *)addr); ebp = *(unsigned long *)ebp; } #else while (valid_stack_ptr(tinfo, stack)) { addr = *stack; if (__kernel_text_address(addr)) { - printk("%08lx: [<%08lx>]", (unsigned long) stack, addr); - print_symbol(" %s", addr); - printk("\n"); + printk("%08lx: [<%08lx>] %pSR\n", + (unsigned long)stack, addr, (void *)addr); } stack++; } -- 1.7.8.112.g3fd21 |
From: Borislav P. <bp...@al...> - 2012-12-12 21:28:16
|
On Wed, Dec 12, 2012 at 10:19:05AM -0800, Joe Perches wrote: > Use the new vsprintf extension to avoid any possible > message interleaving. > > Signed-off-by: Joe Perches <jo...@pe...> > --- > arch/x86/kernel/cpu/mcheck/mce.c | 13 +++++++------ > arch/x86/kernel/dumpstack.c | 5 ++--- > arch/x86/kernel/process_32.c | 2 +- > arch/x86/mm/mmio-mod.c | 4 ++-- > arch/x86/um/sysrq_32.c | 9 +++------ > 5 files changed, 15 insertions(+), 18 deletions(-) > > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c > index 80dbda8..996f98a 100644 > --- a/arch/x86/kernel/cpu/mcheck/mce.c > +++ b/arch/x86/kernel/cpu/mcheck/mce.c > @@ -242,13 +242,14 @@ static void print_mce(struct mce *m) > m->extcpu, m->mcgstatus, m->bank, m->status); > > if (m->ip) { > - pr_emerg(HW_ERR "RIP%s %02x:<%016Lx> ", > - !(m->mcgstatus & MCG_STATUS_EIPV) ? " !INEXACT!" : "", > - m->cs, m->ip); > - > if (m->cs == __KERNEL_CS) > - print_symbol("{%s}", m->ip); > - pr_cont("\n"); > + pr_emerg(HW_ERR "RIP%s %02x:<%016Lx> {%pSR}\n", > + !(m->mcgstatus & MCG_STATUS_EIPV) ? " !INEXACT!" : "", > + m->cs, m->ip, (void *)m->ip); > + else > + pr_emerg(HW_ERR "RIP%s %02x:<%016Lx>\n", > + !(m->mcgstatus & MCG_STATUS_EIPV) ? " !INEXACT!" : "", > + m->cs, m->ip); > } I think I'd go ahead and ACK this unless Tony has some comments. I'm not happy about the two pr_emerg calls based on the conditional. Or, Tony, what do you think, could we get away if we printed empty string for when m->cs != __KERNEL_CS? I'm thinking of not breaking any userspace which is parsing that output and seeing "... {}" in that case. This assumes that vsprintf can print (void *)0 when passed as an argument through %pSR. Joe? Thanks. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- |
From: Joe P. <jo...@pe...> - 2012-12-12 21:30:12
|
On Wed, 2012-12-12 at 22:09 +0100, Borislav Petkov wrote: > On Wed, Dec 12, 2012 at 10:19:05AM -0800, Joe Perches wrote: > > Use the new vsprintf extension to avoid any possible > > message interleaving. [] > > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c [] > > @@ -242,13 +242,14 @@ static void print_mce(struct mce *m) > > m->extcpu, m->mcgstatus, m->bank, m->status); > > > > if (m->ip) { > > - pr_emerg(HW_ERR "RIP%s %02x:<%016Lx> ", > > - !(m->mcgstatus & MCG_STATUS_EIPV) ? " !INEXACT!" : "", > > - m->cs, m->ip); > > - > > if (m->cs == __KERNEL_CS) > > - print_symbol("{%s}", m->ip); > > - pr_cont("\n"); > > + pr_emerg(HW_ERR "RIP%s %02x:<%016Lx> {%pSR}\n", > > + !(m->mcgstatus & MCG_STATUS_EIPV) ? " !INEXACT!" : "", > > + m->cs, m->ip, (void *)m->ip); > > + else > > + pr_emerg(HW_ERR "RIP%s %02x:<%016Lx>\n", > > + !(m->mcgstatus & MCG_STATUS_EIPV) ? " !INEXACT!" : "", > > + m->cs, m->ip); > > } > > I think I'd go ahead and ACK this unless Tony has some comments. I'm not > happy about the two pr_emerg calls based on the conditional. It was done to avoid interleaving. > Or, Tony, what do you think, could we get away if we printed empty > string for when m->cs != __KERNEL_CS? I'm thinking of not breaking any > userspace which is parsing that output and seeing "... {}" in that case. > > This assumes that vsprintf can print (void *)0 when passed as an > argument through %pSR. Joe? Definitely yes when not #defined CONFIG_KALLSYMS I believe no object exists at address 0 for all arches so I believe yes for CONFIG_KALLSYMS too, My preference is to eventually do away with all the "[%0(size)lx] %pSR", addr, (void *)addr uses and create another %pSx that emits the pointer address and the function/offset in one go in a standardized style without caring about the pointer size. Something like: "%pSp", (void *)addr would emit [<7def0123>] function_name+offset/size |
From: Borislav P. <bp...@al...> - 2012-12-12 21:49:07
|
On Wed, Dec 12, 2012 at 01:30:03PM -0800, Joe Perches wrote: > > I think I'd go ahead and ACK this unless Tony has some comments. I'm > > not happy about the two pr_emerg calls based on the conditional. > > It was done to avoid interleaving. Right. > > Or, Tony, what do you think, could we get away if we printed empty > > string for when m->cs != __KERNEL_CS? I'm thinking of not breaking any > > userspace which is parsing that output and seeing "... {}" in that case. > > > > This assumes that vsprintf can print (void *)0 when passed as an > > argument through %pSR. Joe? > > Definitely yes when not #defined CONFIG_KALLSYMS > > I believe no object exists at address 0 for all > arches so I believe yes for CONFIG_KALLSYMS too, Ok, here's what I'm thinking more specifically. Have single call like this: pr_emerg(HW_ERR "RIP%s %02x:<%016Lx> {%pSR}\n", !(m->mcgstatus & MCG_STATUS_EIPV) ? " !INEXACT!" : "", m->cs, m->ip, (m->cs == __KERNEL_CS) ? (void *)m->ip : (void *)0); and when vsprintf gets to it, it recognizes the special case of (void *)0 and dumps the empty string "" for that argument. Hmm. > My preference is to eventually do away with all the > "[%0(size)lx] %pSR", addr, (void *)addr > uses and create another %pSx that emits the pointer > address and the function/offset in one go in a > standardized style without caring about the pointer > size. > > Something like: > > "%pSp", (void *)addr > > would emit > > [<7def0123>] function_name+offset/size Right, this is another way of looking it. And in order to make it more robust, it should be able to handle the (void *)0 case so that callers don't have to check the arg. Thanks. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- |
From: Luck, T. <ton...@in...> - 2012-12-12 21:49:47
|
> I think I'd go ahead and ACK this unless Tony has some comments. I'm not > happy about the two pr_emerg calls based on the conditional. As written the patch has the nice property of not making any changes to the console output (except to eliminate the possibility of interleaved output that the original code had). Whereas making the change you suggest would end up with a useless "{}" in the usermode case (or worse if %pSR did something with m->ip .. which is most probably not NULL ... it should be a valid user mode instruction pointer when m->cs != KERNEL_CS). So Acked-by: Tony Luck <ton...@in...> |
From: Joe P. <jo...@pe...> - 2012-12-12 22:24:08
|
On Wed, 2012-12-12 at 21:49 +0000, Luck, Tony wrote: > > I think I'd go ahead and ACK this unless Tony has some comments. I'm not > > happy about the two pr_emerg calls based on the conditional. > > As written the patch has the nice property of not making any changes to the > console output (except to eliminate the possibility of interleaved output that > the original code had). Well, it does eliminate a trailing space when m->cs != KERNEL_CS. That probably won't hurt anything, but... |
From: Borislav P. <bp...@al...> - 2012-12-12 22:45:59
|
On Wed, Dec 12, 2012 at 02:23:59PM -0800, Joe Perches wrote: > On Wed, 2012-12-12 at 21:49 +0000, Luck, Tony wrote: > > > I think I'd go ahead and ACK this unless Tony has some comments. I'm not > > > happy about the two pr_emerg calls based on the conditional. > > > > As written the patch has the nice property of not making any changes to the > > console output (except to eliminate the possibility of interleaved output that > > the original code had). > > Well, it does eliminate a trailing space when m->cs != KERNEL_CS. > That probably won't hurt anything, but... Yeah, that won't be an issue. So let's take it as is, as Tony suggests. Acked-by: Borislav Petkov <bp...@al...> Thanks. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- |
From: Joe P. <jo...@pe...> - 2012-12-13 18:23:19
|
On Wed, 2012-12-12 at 23:45 +0100, Borislav Petkov wrote: > On Wed, Dec 12, 2012 at 02:23:59PM -0800, Joe Perches wrote: > > On Wed, 2012-12-12 at 21:49 +0000, Luck, Tony wrote: > > > > I think I'd go ahead and ACK this unless Tony has some comments. I'm not > > > > happy about the two pr_emerg calls based on the conditional. > > > > > > As written the patch has the nice property of not making any changes to the > > > console output (except to eliminate the possibility of interleaved output that > > > the original code had). > > > > Well, it does eliminate a trailing space when m->cs != KERNEL_CS. > > That probably won't hurt anything, but... > > Yeah, that won't be an issue. So let's take it as is, as Tony suggests. Hi again Boris. I should mention one more thing. m->ip is a u64 so, when compiling x86-32, there's a new warning "cast to pointer from integer of different size". This isn't new different behavior, just a new warning. The previous print_symbol took a ulong and the u64 was silently truncated. CC arch/x86/kernel/cpu/mcheck/mce.o arch/x86/kernel/cpu/mcheck/mce.c: In function ‘print_mce’: arch/x86/kernel/cpu/mcheck/mce.c:246:4: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] If appropriate, the code could be changed to (void *)(unsigned long)m->ip to shut the compiler up. |
From: Borislav P. <bp...@al...> - 2012-12-13 18:37:14
|
On Thu, Dec 13, 2012 at 10:23:10AM -0800, Joe Perches wrote: > m->ip is a u64 so, when compiling x86-32, there's a new warning > "cast to pointer from integer of different size". This isn't new > different behavior, just a new warning. The previous print_symbol > took a ulong and the u64 was silently truncated. > > CC arch/x86/kernel/cpu/mcheck/mce.o > arch/x86/kernel/cpu/mcheck/mce.c: In function ‘print_mce’: > arch/x86/kernel/cpu/mcheck/mce.c:246:4: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > > If appropriate, the code could be changed to > > (void *)(unsigned long)m->ip Can we explicitly cast it to what it is so that we can be explicit as to what we're casting it? IOW: (void *)(__u64)m->ip; Does that even work on 32bit? Also, does the compiler bitch about this useless cast when building with W=123? Thanks. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- |
From: Joe P. <jo...@pe...> - 2012-12-13 18:57:31
|
On Thu, 2012-12-13 at 19:37 +0100, Borislav Petkov wrote: > On Thu, Dec 13, 2012 at 10:23:10AM -0800, Joe Perches wrote: > > m->ip is a u64 so, when compiling x86-32, there's a new warning > > "cast to pointer from integer of different size". This isn't new > > different behavior, just a new warning. The previous print_symbol > > took a ulong and the u64 was silently truncated. > > > > CC arch/x86/kernel/cpu/mcheck/mce.o > > arch/x86/kernel/cpu/mcheck/mce.c: In function ‘print_mce’: > > arch/x86/kernel/cpu/mcheck/mce.c:246:4: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > > > > If appropriate, the code could be changed to > > > > (void *)(unsigned long)m->ip > > Can we explicitly cast it to what it is so that we can be explicit as to > what we're casting it? IOW: > > (void *)(__u64)m->ip; > > Does that even work on 32bit? No, it already is __u64. You need (unsigned long) or something else the same size as (void *) from: arch/x86/include/asm/mce.h /* Fields are zero when not available */ struct mce { ... __u64 ip; > Also, does the compiler bitch about this useless cast when building with > W=123? For x86-32: With the cast the line is silent. Without the cast, just the warning about cast. |
From: H. P. A. <hp...@zy...> - 2012-12-13 18:43:36
|
On 12/13/2012 10:37 AM, Borislav Petkov wrote: >> >> If appropriate, the code could be changed to >> >> (void *)(unsigned long)m->ip > > Can we explicitly cast it to what it is so that we can be explicit as to > what we're casting it? IOW: > > (void *)(__u64)m->ip; > > Does that even work on 32bit? > > Also, does the compiler bitch about this useless cast when building with > W=123? > Uh... no. The point is that (void *)(unsigned long) casts it to an integer of pointer size -- in userspace you would to (void *)(size_t) or (void *)(uintptr_t) -- so that the compiler knows "I meant to do that." -hpa |
From: Borislav P. <bp...@al...> - 2012-12-13 19:05:56
|
On Thu, Dec 13, 2012 at 10:42:28AM -0800, H. Peter Anvin wrote: > On 12/13/2012 10:37 AM, Borislav Petkov wrote: > >> > >> If appropriate, the code could be changed to > >> > >> (void *)(unsigned long)m->ip > > > > Can we explicitly cast it to what it is so that we can be explicit as to > > what we're casting it? IOW: > > > > (void *)(__u64)m->ip; > > > > Does that even work on 32bit? > > > > Also, does the compiler bitch about this useless cast when building with > > W=123? > > > > Uh... no. > > The point is that (void *)(unsigned long) casts it to an integer of > pointer size -- in userspace you would to (void *)(size_t) or (void > *)(uintptr_t) -- so that the compiler knows "I meant to do that." Ok, I grok it now. So, in most cases, mce.ip comes from pt_regs.ip which is unsigned long so we're fine for both 32- and 64-bit. There's only the case with precise RIP reporting where we get the rip from an MSR but even then, it should be 4 bytes on 32-bit since our addresses there are 4 bytes. So, actually struct mce should've had ->ip defined as an unsigned long from the very beginning. But we can't change that now because this struct is shared with userspace :( So yes Joe, make it (void *)(unsigned long). Thanks guys. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- |
From: H. P. A. <hp...@zy...> - 2012-12-13 21:36:38
|
On 12/13/2012 11:05 AM, Borislav Petkov wrote: > > Ok, I grok it now. So, in most cases, mce.ip comes from pt_regs.ip which > is unsigned long so we're fine for both 32- and 64-bit. There's only the > case with precise RIP reporting where we get the rip from an MSR but > even then, it should be 4 bytes on 32-bit since our addresses there are > 4 bytes. > > So, actually struct mce should've had ->ip defined as an unsigned long > from the very beginning. But we can't change that now because this > struct is shared with userspace :( > Good thing, then,. otherwise we'd have a compat headache. -hpa -- H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf. |
From: Joe P. <jo...@pe...> - 2012-12-13 19:13:37
|
Use the new vsprintf extension to avoid any possible message interleaving. Signed-off-by: Joe Perches <jo...@pe...> Acked-by: Tony Luck <ton...@in...> Acked-by: Borislav Petkov <bp...@al...> --- V2: mce.c: Add cast to (unsigned long) before cast to (void *) to quiet the compiler on x86-32 arch/x86/kernel/cpu/mcheck/mce.c | 13 +++++++------ arch/x86/kernel/dumpstack.c | 5 ++--- arch/x86/kernel/process_32.c | 2 +- arch/x86/mm/mmio-mod.c | 4 ++-- arch/x86/um/sysrq_32.c | 9 +++------ 5 files changed, 15 insertions(+), 18 deletions(-) diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c index 80dbda8..996f98a 100644 --- a/arch/x86/kernel/cpu/mcheck/mce.c +++ b/arch/x86/kernel/cpu/mcheck/mce.c @@ -242,13 +242,14 @@ static void print_mce(struct mce *m) m->extcpu, m->mcgstatus, m->bank, m->status); if (m->ip) { - pr_emerg(HW_ERR "RIP%s %02x:<%016Lx> ", - !(m->mcgstatus & MCG_STATUS_EIPV) ? " !INEXACT!" : "", - m->cs, m->ip); - if (m->cs == __KERNEL_CS) - print_symbol("{%s}", m->ip); - pr_cont("\n"); + pr_emerg(HW_ERR "RIP%s %02x:<%016Lx> {%pSR}\n", + !(m->mcgstatus & MCG_STATUS_EIPV) ? " !INEXACT!" : "", + m->cs, m->ip, (void *)(unsigned long)m->ip); + else + pr_emerg(HW_ERR "RIP%s %02x:<%016Lx>\n", + !(m->mcgstatus & MCG_STATUS_EIPV) ? " !INEXACT!" : "", + m->cs, m->ip); } pr_emerg(HW_ERR "TSC %llx ", m->tsc); diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c index ae42418b..b6e5bdb 100644 --- a/arch/x86/kernel/dumpstack.c +++ b/arch/x86/kernel/dumpstack.c @@ -281,9 +281,8 @@ int __kprobes __die(const char *str, struct pt_regs *regs, long err) sp = kernel_stack_pointer(regs); savesegment(ss, ss); } - printk(KERN_EMERG "EIP: [<%08lx>] ", regs->ip); - print_symbol("%s", regs->ip); - printk(" SS:ESP %04x:%08lx\n", ss, sp); + printk(KERN_EMERG "EIP: [<%08lx>] %pSR SS:ESP %04x:%08lx\n", + regs->ip, (void *)regs->ip, ss, sp); #else /* Executive summary in case the oops scrolled away */ printk(KERN_ALERT "RIP "); diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c index b5a8905..0db77e0 100644 --- a/arch/x86/kernel/process_32.c +++ b/arch/x86/kernel/process_32.c @@ -89,7 +89,7 @@ void __show_regs(struct pt_regs *regs, int all) printk(KERN_DEFAULT "EIP: %04x:[<%08lx>] EFLAGS: %08lx CPU: %d\n", (u16)regs->cs, regs->ip, regs->flags, smp_processor_id()); - print_symbol("EIP is at %s\n", regs->ip); + printk(KERN_DEFAULT "EIP is at %pSR\n", (void *)regs->ip); printk(KERN_DEFAULT "EAX: %08lx EBX: %08lx ECX: %08lx EDX: %08lx\n", regs->ax, regs->bx, regs->cx, regs->dx); diff --git a/arch/x86/mm/mmio-mod.c b/arch/x86/mm/mmio-mod.c index dc0b727..c0ca484 100644 --- a/arch/x86/mm/mmio-mod.c +++ b/arch/x86/mm/mmio-mod.c @@ -123,8 +123,8 @@ static void die_kmmio_nesting_error(struct pt_regs *regs, unsigned long addr) pr_emerg("unexpected fault for address: 0x%08lx, last fault for address: 0x%08lx\n", addr, my_reason->addr); print_pte(addr); - print_symbol(KERN_EMERG "faulting IP is at %s\n", regs->ip); - print_symbol(KERN_EMERG "last faulting IP was at %s\n", my_reason->ip); + pr_emerg("faulting IP is at %pSR\n", (void *)regs->ip); + pr_emerg("last faulting IP was at %pSR\n", (void *)my_reason->ip); #ifdef __i386__ pr_emerg("eax: %08lx ebx: %08lx ecx: %08lx edx: %08lx\n", regs->ax, regs->bx, regs->cx, regs->dx); diff --git a/arch/x86/um/sysrq_32.c b/arch/x86/um/sysrq_32.c index c9bee5b..30b4200 100644 --- a/arch/x86/um/sysrq_32.c +++ b/arch/x86/um/sysrq_32.c @@ -50,18 +50,15 @@ static inline unsigned long print_context_stack(struct thread_info *tinfo, #ifdef CONFIG_FRAME_POINTER while (valid_stack_ptr(tinfo, (void *)ebp)) { addr = *(unsigned long *)(ebp + 4); - printk("%08lx: [<%08lx>]", ebp + 4, addr); - print_symbol(" %s", addr); - printk("\n"); + printk("%08lx: [<%08lx>] %pSR\n", ebp + 4, addr, (void *)addr); ebp = *(unsigned long *)ebp; } #else while (valid_stack_ptr(tinfo, stack)) { addr = *stack; if (__kernel_text_address(addr)) { - printk("%08lx: [<%08lx>]", (unsigned long) stack, addr); - print_symbol(" %s", addr); - printk("\n"); + printk("%08lx: [<%08lx>] %pSR\n", + (unsigned long)stack, addr, (void *)addr); } stack++; } -- 1.7.8.112.g3fd21 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to maj...@vg... More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |