* [TRIVIAL PATCH 14/26] um: Convert print_symbol to %pSR
2012-12-12 18:18 [TRIVIAL PATCH 00/26] treewide: Add and use vsprintf extension %pSR Joe Perches
@ 2012-12-12 18:19 ` Joe Perches
2012-12-12 18:19 ` [TRIVIAL PATCH 16/26] x86: " Joe Perches
2012-12-13 19:13 ` [TRIVIAL PATCH V2 " Joe Perches
2 siblings, 0 replies; 16+ messages in thread
From: Joe Perches @ 2012-12-12 18:19 UTC (permalink / raw)
To: Jiri Kosina
Cc: Jeff Dike, Richard Weinberger, user-mode-linux-devel,
user-mode-linux-user, linux-kernel
Use the new vsprintf extension to avoid any possible
message interleaving.
Signed-off-by: Joe Perches <joe@perches.com>
---
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [TRIVIAL PATCH 16/26] x86: Convert print_symbol to %pSR
2012-12-12 18:18 [TRIVIAL PATCH 00/26] treewide: Add and use vsprintf extension %pSR Joe Perches
2012-12-12 18:19 ` [TRIVIAL PATCH 14/26] um: Convert print_symbol to %pSR Joe Perches
@ 2012-12-12 18:19 ` Joe Perches
2012-12-12 21:09 ` Borislav Petkov
2012-12-13 19:13 ` [TRIVIAL PATCH V2 " Joe Perches
2 siblings, 1 reply; 16+ messages in thread
From: Joe Perches @ 2012-12-12 18:19 UTC (permalink / raw)
To: Jiri Kosina, Tony Luck, Borislav Petkov, Jeff Dike,
Richard Weinberger
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-edac,
linux-kernel, user-mode-linux-devel, user-mode-linux-user
Use the new vsprintf extension to avoid any possible
message interleaving.
Signed-off-by: Joe Perches <joe@perches.com>
---
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [TRIVIAL PATCH 16/26] x86: Convert print_symbol to %pSR
2012-12-12 18:19 ` [TRIVIAL PATCH 16/26] x86: " Joe Perches
@ 2012-12-12 21:09 ` Borislav Petkov
2012-12-12 21:30 ` Joe Perches
0 siblings, 1 reply; 16+ messages in thread
From: Borislav Petkov @ 2012-12-12 21:09 UTC (permalink / raw)
To: Joe Perches
Cc: Jiri Kosina, Tony Luck, Jeff Dike, Richard Weinberger,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-edac,
linux-kernel, user-mode-linux-devel, user-mode-linux-user
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 <joe@perches.com>
> ---
> 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.
--
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [TRIVIAL PATCH 16/26] x86: Convert print_symbol to %pSR
2012-12-12 21:09 ` Borislav Petkov
@ 2012-12-12 21:30 ` Joe Perches
2012-12-12 21:48 ` Borislav Petkov
2012-12-12 21:49 ` Luck, Tony
0 siblings, 2 replies; 16+ messages in thread
From: Joe Perches @ 2012-12-12 21:30 UTC (permalink / raw)
To: Borislav Petkov
Cc: Jiri Kosina, Tony Luck, Jeff Dike, Richard Weinberger,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-edac,
linux-kernel, user-mode-linux-devel, user-mode-linux-user
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [TRIVIAL PATCH 16/26] x86: Convert print_symbol to %pSR
2012-12-12 21:30 ` Joe Perches
@ 2012-12-12 21:48 ` Borislav Petkov
2012-12-12 21:49 ` Luck, Tony
1 sibling, 0 replies; 16+ messages in thread
From: Borislav Petkov @ 2012-12-12 21:48 UTC (permalink / raw)
To: Joe Perches
Cc: Jiri Kosina, Tony Luck, Jeff Dike, Richard Weinberger,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-edac,
linux-kernel, user-mode-linux-devel, user-mode-linux-user
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.
--
^ permalink raw reply [flat|nested] 16+ messages in thread* RE: [TRIVIAL PATCH 16/26] x86: Convert print_symbol to %pSR
2012-12-12 21:30 ` Joe Perches
2012-12-12 21:48 ` Borislav Petkov
@ 2012-12-12 21:49 ` Luck, Tony
2012-12-12 22:23 ` Joe Perches
1 sibling, 1 reply; 16+ messages in thread
From: Luck, Tony @ 2012-12-12 21:49 UTC (permalink / raw)
To: Joe Perches, Borislav Petkov
Cc: Jiri Kosina, Jeff Dike, Richard Weinberger, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, x86@kernel.org,
linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
user-mode-linux-devel@lists.sourceforge.net,
user-mode-linux-user@lists.sourceforge.net
> 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 <tony.luck@intel.com>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [TRIVIAL PATCH 16/26] x86: Convert print_symbol to %pSR
2012-12-12 21:49 ` Luck, Tony
@ 2012-12-12 22:23 ` Joe Perches
2012-12-12 22:45 ` Borislav Petkov
0 siblings, 1 reply; 16+ messages in thread
From: Joe Perches @ 2012-12-12 22:23 UTC (permalink / raw)
To: Luck, Tony
Cc: Borislav Petkov, Jiri Kosina, Jeff Dike, Richard Weinberger,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86@kernel.org,
linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
user-mode-linux-devel@lists.sourceforge.net,
user-mode-linux-user@lists.sourceforge.net
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...
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [TRIVIAL PATCH 16/26] x86: Convert print_symbol to %pSR
2012-12-12 22:23 ` Joe Perches
@ 2012-12-12 22:45 ` Borislav Petkov
2012-12-13 18:23 ` Joe Perches
0 siblings, 1 reply; 16+ messages in thread
From: Borislav Petkov @ 2012-12-12 22:45 UTC (permalink / raw)
To: Joe Perches
Cc: Luck, Tony, Jiri Kosina, Jeff Dike, Richard Weinberger,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86@kernel.org,
linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
user-mode-linux-devel@lists.sourceforge.net,
user-mode-linux-user@lists.sourceforge.net
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@alien8.de>
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [TRIVIAL PATCH 16/26] x86: Convert print_symbol to %pSR
2012-12-12 22:45 ` Borislav Petkov
@ 2012-12-13 18:23 ` Joe Perches
2012-12-13 18:37 ` Borislav Petkov
0 siblings, 1 reply; 16+ messages in thread
From: Joe Perches @ 2012-12-13 18:23 UTC (permalink / raw)
To: Borislav Petkov
Cc: Luck, Tony, Jiri Kosina, Jeff Dike, Richard Weinberger,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86@kernel.org,
linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
user-mode-linux-devel@lists.sourceforge.net,
user-mode-linux-user@lists.sourceforge.net
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [TRIVIAL PATCH 16/26] x86: Convert print_symbol to %pSR
2012-12-13 18:23 ` Joe Perches
@ 2012-12-13 18:37 ` Borislav Petkov
2012-12-13 18:42 ` H. Peter Anvin
2012-12-13 18:57 ` Joe Perches
0 siblings, 2 replies; 16+ messages in thread
From: Borislav Petkov @ 2012-12-13 18:37 UTC (permalink / raw)
To: Joe Perches
Cc: Luck, Tony, Jiri Kosina, Jeff Dike, Richard Weinberger,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86@kernel.org,
linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
user-mode-linux-devel@lists.sourceforge.net,
user-mode-linux-user@lists.sourceforge.net
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.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [TRIVIAL PATCH 16/26] x86: Convert print_symbol to %pSR
2012-12-13 18:37 ` Borislav Petkov
@ 2012-12-13 18:42 ` H. Peter Anvin
2012-12-13 19:05 ` Borislav Petkov
2012-12-13 18:57 ` Joe Perches
1 sibling, 1 reply; 16+ messages in thread
From: H. Peter Anvin @ 2012-12-13 18:42 UTC (permalink / raw)
To: Borislav Petkov, Joe Perches, Luck, Tony, Jiri Kosina, Jeff Dike,
Richard Weinberger, Thomas Gleixner, Ingo Molnar, x86@kernel.org,
linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
user-mode-linux-devel@lists.sourceforge.net,
user-mode-linux-user@lists.sourceforge.net
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [TRIVIAL PATCH 16/26] x86: Convert print_symbol to %pSR
2012-12-13 18:42 ` H. Peter Anvin
@ 2012-12-13 19:05 ` Borislav Petkov
2012-12-13 21:35 ` H. Peter Anvin
0 siblings, 1 reply; 16+ messages in thread
From: Borislav Petkov @ 2012-12-13 19:05 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Joe Perches, Luck, Tony, Jiri Kosina, Jeff Dike,
Richard Weinberger, Thomas Gleixner, Ingo Molnar, x86@kernel.org,
linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
user-mode-linux-devel@lists.sourceforge.net,
user-mode-linux-user@lists.sourceforge.net
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.
--
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [TRIVIAL PATCH 16/26] x86: Convert print_symbol to %pSR
2012-12-13 19:05 ` Borislav Petkov
@ 2012-12-13 21:35 ` H. Peter Anvin
0 siblings, 0 replies; 16+ messages in thread
From: H. Peter Anvin @ 2012-12-13 21:35 UTC (permalink / raw)
To: Borislav Petkov, Joe Perches, Luck, Tony, Jiri Kosina, Jeff Dike,
Richard Weinberger, Thomas Gleixner, Ingo Molnar, x86@kernel.org,
linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
user-mode-linux-devel@lists.sourceforge.net,
user-mode-linux-user@lists.sourceforge.net
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [TRIVIAL PATCH 16/26] x86: Convert print_symbol to %pSR
2012-12-13 18:37 ` Borislav Petkov
2012-12-13 18:42 ` H. Peter Anvin
@ 2012-12-13 18:57 ` Joe Perches
1 sibling, 0 replies; 16+ messages in thread
From: Joe Perches @ 2012-12-13 18:57 UTC (permalink / raw)
To: Borislav Petkov
Cc: Luck, Tony, Jiri Kosina, Jeff Dike, Richard Weinberger,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86@kernel.org,
linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
user-mode-linux-devel@lists.sourceforge.net,
user-mode-linux-user@lists.sourceforge.net
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 16+ messages in thread
* [TRIVIAL PATCH V2 16/26] x86: Convert print_symbol to %pSR
2012-12-12 18:18 [TRIVIAL PATCH 00/26] treewide: Add and use vsprintf extension %pSR Joe Perches
2012-12-12 18:19 ` [TRIVIAL PATCH 14/26] um: Convert print_symbol to %pSR Joe Perches
2012-12-12 18:19 ` [TRIVIAL PATCH 16/26] x86: " Joe Perches
@ 2012-12-13 19:13 ` Joe Perches
2 siblings, 0 replies; 16+ messages in thread
From: Joe Perches @ 2012-12-13 19:13 UTC (permalink / raw)
To: Jiri Kosina, Tony Luck, Borislav Petkov, Jeff Dike,
Richard Weinberger
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-edac,
linux-kernel, user-mode-linux-devel, user-mode-linux-user
Use the new vsprintf extension to avoid any possible
message interleaving.
Signed-off-by: Joe Perches <joe@perches.com>
Acked-by: Tony Luck <tony.luck@intel.com>
Acked-by: Borislav Petkov <bp@alien8.de>
---
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply related [flat|nested] 16+ messages in thread