Re: [PATCH RFC 1/4] Generic BUG handling.

Re: [PATCH RFC 1/4] Generic BUG handling.

[Andrew Morton]

On Thu, 28 Sep 2006 15:54:45 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> This patch adds common handling for kernel BUGs, for use by
> architectures as they wish.  The code is derived from arch/powerpc.
> 
> The advantages of having common BUG handling are:
>  - consistent BUG reporting across architectures
>  - shared implementation of out-of-line file/line data
> 
> This means that in inline impact of BUG is just the illegal
> instruction itself, which is an improvement for i386 and x86-64.
> 
> A BUG is represented in the instruction stream as an illegal
> instruction, which has file/line/function information associated with
> it.  This extra information is stored in the __bug_table section in
> the ELF file.
> 
> When the kernel gets an illegal instruction, it first confirms it
> might possibly be from a BUG (ie, in kernel mode, the right illegal
> instruction).  It then calls report_bug().  This searches __bug_table
> for a matching instruction pointer, and if found, prints the
> corresponding file/line/function information.
> 
> Some architectures (powerpc) implement WARN using the same mechanism;
> if the illegal instruction was the result of a WARN, then report_bug()
> returns 1; otherwise it returns 0.

Neato.

> lib/bug.c keeps a list of loaded modules which can be searched for
> __bug_table entries.  The architecture must call
> module_bug_finalize()/module_bug_cleanup() from its corresponding
> module_finalize/cleanup functions.

What is the locking for these lists?  I don't see much in here.  It has
implications for code which wants to do BUG while holding that lock..

> This patch also converts i386, x86-64 and powerpc to use this
> infrastructure.  I have only tested i386; x86-64 and powerpc are not
> even compile-tested in this patch.
> 
> Because powerpc also records the function name, I added this to i386
> and x86-64 for consistency.  Strictly speaking the function name is
> redundant with kallsyms, so perhaps it can be dropped from powerpc.

I agree that the function name is a rather gratuitous space-consumer.

> +#ifdef CONFIG_GENERIC_BUG
> +	/* Support for BUG */
> +	struct list_head bug_list;
> +	struct bug_entry *bug_table;
> +	unsigned num_bugs;

Shouldn't this be u64? ;)

Re: [PATCH RFC 1/4] Generic BUG handling.

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> What is the locking for these lists?  I don't see much in here.  It has
> implications for code which wants to do BUG while holding that lock..
>   

There's no locking.  This is a direct copy of the original powerpc 
code.  I assume, but haven't checked, that there's a lock to serialize 
module loading/unloading, so the insertion/deletion is all properly 
synchronized. 

The only other user is traversal when actually handling a bug; if you're 
very unlucky this could happen while you're actually loading/unloading 
and you would see the list in an inconsistent state.  I guess we could 
put a lock there, and trylock it on traversal; at least that would stop 
a concurrent modload/unload from getting in there while we're trying to 
walk the list.

> Shouldn't this be u64? ;)
>   

I'll get right on that.  And perhaps it should be signed if people 
overshoot and introduce a negative number of BUGs.

    J

Re: [PATCH RFC 1/4] Generic BUG handling.

[Andrew Morton]

On Thu, 28 Sep 2006 16:43:55 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Andrew Morton wrote:
> > What is the locking for these lists?  I don't see much in here.  It has
> > implications for code which wants to do BUG while holding that lock..
> >   
> 
> There's no locking.  This is a direct copy of the original powerpc 
> code.  I assume, but haven't checked, that there's a lock to serialize 
> module loading/unloading, so the insertion/deletion is all properly 
> synchronized. 
> 
> The only other user is traversal when actually handling a bug; if you're 
> very unlucky this could happen while you're actually loading/unloading 
> and you would see the list in an inconsistent state.  I guess we could 
> put a lock there, and trylock it on traversal; at least that would stop 
> a concurrent modload/unload from getting in there while we're trying to 
> walk the list.

The module_bug_cleanup() code is in a stop_machine_run() callback, so
that's all OK.

I _think_ your module_bug_finalize()'s list_add() could race with another
CPU's BUG_ON().  We can live with that.

Re: [PATCH RFC 1/4] Generic BUG handling.

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 3943 bytes --]

On Thu, 2006-09-28 at 15:54 -0700, Jeremy Fitzhardinge wrote:
> plain text document attachment (generic-bug.patch)
> This patch adds common handling for kernel BUGs, for use by
> architectures as they wish.  The code is derived from arch/powerpc.
> 
> The advantages of having common BUG handling are:
>  - consistent BUG reporting across architectures
>  - shared implementation of out-of-line file/line data

Nice work.

> +       printk(KERN_EMERG "------------[ cut here ]------------\n");

I'm not sure I'm big on the cut here marker.

> i386 implements CONFIG_DEBUG_BUGVERBOSE, but x86-64 and powerpc do
> not.  This should probably be made more consistent.

It looks like if you do this you _might_ be able to share struct
bug_entry, or at least have consistent members for each arch. Which
would eliminate some of the inlines you have for accessing the bug
struct.

It needed a bit of work to get going on powerpc:

Generic BUG handling, Powerpc fixups

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>

Index: to-merge/arch/powerpc/kernel/traps.c
===================================================================
--- to-merge.orig/arch/powerpc/kernel/traps.c
+++ to-merge/arch/powerpc/kernel/traps.c
@@ -731,32 +731,9 @@ static int emulate_instruction(struct pt
 	return -EINVAL;
 }
 
-/*
- * Look through the list of trap instructions that are used for BUG(),
- * BUG_ON() and WARN_ON() and see if we hit one.  At this point we know
- * that the exception was caused by a trap instruction of some kind.
- * Returns 1 if we should continue (i.e. it was a WARN_ON) or 0
- * otherwise.
- */
-extern struct bug_entry __start___bug_table[], __stop___bug_table[];
-
-#ifndef CONFIG_MODULES
-#define module_find_bug(x)	NULL
-#endif
-
-struct bug_entry *find_bug(unsigned long bugaddr)
-{
-	struct bug_entry *bug;
-
-	for (bug = __start___bug_table; bug < __stop___bug_table; ++bug)
-		if (bugaddr == bug->bug_addr)
-			return bug;
-	return module_find_bug(bugaddr);
-}
-
 int is_valid_bugaddr(unsigned long addr)
 {
-	return addr >= PAGE_OFFSET;
+	return is_kernel_addr(addr);
 }
 
 void __kprobes program_check_exception(struct pt_regs *regs)
Index: to-merge/include/asm-powerpc/bug.h
===================================================================
--- to-merge.orig/include/asm-powerpc/bug.h
+++ to-merge/include/asm-powerpc/bug.h
@@ -20,8 +20,6 @@ struct bug_entry {
 	const char	*function;
 };
 
-struct bug_entry *find_bug(unsigned long bugaddr);
-
 /*
  * If this bit is set in the line number it means that the trap
  * is for WARN_ON rather than BUG or BUG_ON.
Index: to-merge/include/linux/bug.h
===================================================================
--- to-merge.orig/include/linux/bug.h
+++ to-merge/include/linux/bug.h
@@ -6,14 +6,16 @@
 #ifdef CONFIG_GENERIC_BUG
 #include <linux/module.h>
 
-int report_bug(unsigned long bug_addr);
+extern int report_bug(unsigned long bug_addr);
 
-int  module_bug_finalize(const Elf_Ehdr *, const Elf_Shdr *,
+extern int  module_bug_finalize(const Elf_Ehdr *, const Elf_Shdr *,
 			 struct module *);
-void module_bug_cleanup(struct module *);
+extern void module_bug_cleanup(struct module *);
+
+extern const struct bug_entry *find_bug(unsigned long bugaddr);
 
 /* These are defined by the architecture */
-int is_valid_bugaddr(unsigned long addr);
+extern int is_valid_bugaddr(unsigned long addr);
 
 #endif	/* CONFIG_GENERIC_BUG */
 #endif	/* _LINUX_BUG_H */
Index: to-merge/lib/bug.c
===================================================================
--- to-merge.orig/lib/bug.c
+++ to-merge/lib/bug.c
@@ -21,7 +21,7 @@ static const struct bug_entry *module_fi
 	return NULL;
 }
 
-static const struct bug_entry *find_bug(unsigned long bugaddr)
+const struct bug_entry *find_bug(unsigned long bugaddr)
 {
 	const struct bug_entry *bug;
 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 191 bytes --]

Re: [PATCH RFC 1/4] Generic BUG handling.

[Jeremy Fitzhardinge]

Michael Ellerman wrote:
>> +       printk(KERN_EMERG "------------[ cut here ]------------\n");
>>     
>
> I'm not sure I'm big on the cut here marker.
>   

x86 has it.  I figured its more important to not change x86 output than 
powerpc.

>> i386 implements CONFIG_DEBUG_BUGVERBOSE, but x86-64 and powerpc do
>> not.  This should probably be made more consistent.
>>     
>
> It looks like if you do this you _might_ be able to share struct
> bug_entry, or at least have consistent members for each arch. Which
> would eliminate some of the inlines you have for accessing the bug
> struct.
>   
Yeah, its a bit of a toss-up.  powerpc wants to hide the warn flag 
somewhere, which either means having a different structure, or using the 
fields differently.  CONFIG_DEBUG_BUGVERBOSE supporters (ie, i386) want 
to make the structure completely empty in the !DEBUG_BUGVERBOSE case 
(which doesn't currently happen).
> It needed a bit of work to get going on powerpc:
>   

Thanks.  I'll try to fold all this together into a new patch when things 
settle down.

    J

Re: [PATCH RFC 1/4] Generic BUG handling.

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]

On Fri, 2006-09-29 at 01:41 -0700, Jeremy Fitzhardinge wrote:
> Michael Ellerman wrote:
> >> +       printk(KERN_EMERG "------------[ cut here ]------------\n");
> >>     
> >
> > I'm not sure I'm big on the cut here marker.
> >   
> 
> x86 has it.  I figured its more important to not change x86 output than 
> powerpc.

Yeah, you don't want to go messing up legacy architectures.

> >> i386 implements CONFIG_DEBUG_BUGVERBOSE, but x86-64 and powerpc do
> >> not.  This should probably be made more consistent.
> >>     
> >
> > It looks like if you do this you _might_ be able to share struct
> > bug_entry, or at least have consistent members for each arch. Which
> > would eliminate some of the inlines you have for accessing the bug
> > struct.
> >   
> Yeah, its a bit of a toss-up.  powerpc wants to hide the warn flag 
> somewhere, which either means having a different structure, or using the 
> fields differently.  CONFIG_DEBUG_BUGVERBOSE supporters (ie, i386) want 
> to make the structure completely empty in the !DEBUG_BUGVERBOSE case 
> (which doesn't currently happen).
> > It needed a bit of work to get going on powerpc:
> >   
> 
> Thanks.  I'll try to fold all this together into a new patch when things 
> settle down.

Yeah ok there's a few competing concerns there, it's a good start
though.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 191 bytes --]

Re: [PATCH RFC 1/4] Generic BUG handling.

[Andrew Morton]

On Fri, 29 Sep 2006 01:41:21 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Michael Ellerman wrote:
> >> +       printk(KERN_EMERG "------------[ cut here ]------------\n");
> >>     
> >
> > I'm not sure I'm big on the cut here marker.
> >   
> 
> x86 has it.  I figured its more important to not change x86 output than 
> powerpc.

We need to clean that output up a bit.  For a while x86 was printing "BUG:"
in front of both warnings and BUGs because Ingo through it made things
clearer - we've lost that.

> >> i386 implements CONFIG_DEBUG_BUGVERBOSE, but x86-64 and powerpc do
> >> not.  This should probably be made more consistent.
> >>     
> >
> > It looks like if you do this you _might_ be able to share struct
> > bug_entry, or at least have consistent members for each arch. Which
> > would eliminate some of the inlines you have for accessing the bug
> > struct.
> >   
> Yeah, its a bit of a toss-up.  powerpc wants to hide the warn flag 
> somewhere, which either means having a different structure, or using the 
> fields differently.  CONFIG_DEBUG_BUGVERBOSE supporters (ie, i386) want 
> to make the structure completely empty in the !DEBUG_BUGVERBOSE case 
> (which doesn't currently happen).
> > It needed a bit of work to get going on powerpc:
> >   
> 
> Thanks.  I'll try to fold all this together into a new patch when things 
> settle down.

Is OK - I'm pretty happy with what I have now.  I'll clump various patches
together and we can take another look at it.  I guess I'll merge the core
and x86, send x86_64 to Andi, let the ppc guys worry about the powerpc
bits.

Re: [PATCH RFC 1/4] Generic BUG handling.

[Andi Kleen]

> Some architectures (powerpc) implement WARN using the same mechanism;
> if the illegal instruction was the result of a WARN, then report_bug()
> returns 1; otherwise it returns 0.

In theory we could do that on x86 too (and skipping the instruction), 
the only problem 
is that the only guaranteed to fault opcode is ud2 :/. Ok maybe we could
reserve some int XXX vector.

% gid WARN_ON | grep -v arch | wc -l
299

-Andi

Re: [PATCH RFC 1/4] Generic BUG handling.

[Andrew Morton]

On 29 Sep 2006 10:57:45 +0200
Andi Kleen <ak@muc.de> wrote:

> > Some architectures (powerpc) implement WARN using the same mechanism;
> > if the illegal instruction was the result of a WARN, then report_bug()
> > returns 1; otherwise it returns 0.
> 
> In theory we could do that on x86 too (and skipping the instruction), 
> the only problem 
> is that the only guaranteed to fault opcode is ud2 :/. Ok maybe we could
> reserve some int XXX vector.
> 
> % gid WARN_ON | grep -v arch | wc -l
> 299

powerpc sets a bit in the __LINE__ number to indicate that it was a
WARN_ON.  That'll work on all architectures.

Re: [PATCH RFC 1/4] Generic BUG handling.

[Andi Kleen]

On Fri, Sep 29, 2006 at 02:10:19AM -0700, Andrew Morton wrote:
> On 29 Sep 2006 10:57:45 +0200
> Andi Kleen <ak@muc.de> wrote:
> 
> > > Some architectures (powerpc) implement WARN using the same mechanism;
> > > if the illegal instruction was the result of a WARN, then report_bug()
> > > returns 1; otherwise it returns 0.
> > 
> > In theory we could do that on x86 too (and skipping the instruction), 
> > the only problem 
> > is that the only guaranteed to fault opcode is ud2 :/. Ok maybe we could
> > reserve some int XXX vector.
> > 
> > % gid WARN_ON | grep -v arch | wc -l
> > 299
> 
> powerpc sets a bit in the __LINE__ number to indicate that it was a
> WARN_ON.  That'll work on all architectures.

We still would need an architecture dependent way to skip the opcode
though (just returning would raise it again). On x86

regs->eip += 2     (rip on x86-64) 

should be enough 

-Andi

Re: [PATCH RFC 1/4] Generic BUG handling.

[Andrew Morton]

On Thu, 28 Sep 2006 15:54:45 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> This patch adds common handling for kernel BUGs, for use by
> architectures as they wish.  The code is derived from arch/powerpc.

For my x86_64 usualconfig .text (from objdump --headers) went from
0x002c55c7 down to 0x002c2bda, which is 10.5k saved.

According to /usr/bin/size, vmlinux got bigger:

box:/usr/src/25> size vmlinux 
   text    data     bss     dec     hex filename
3597448  716340  510456 4824244  499cb4 vmlinux-before
3640604  716228  510456 4867288  4a44d8 vmlinux-after

But that's because size(1) is too blunt an instrument: the sum of .text and
the new bug section got larger.

I think we need to thank the powerpc guys, then take away their function
name printing ;)

Re: [PATCH RFC 1/4] Generic BUG handling.

[Andrew Morton]

On 29 Sep 2006 11:13:19 +0200
Andi Kleen <ak@muc.de> wrote:

> On Fri, Sep 29, 2006 at 02:10:19AM -0700, Andrew Morton wrote:
> > On 29 Sep 2006 10:57:45 +0200
> > Andi Kleen <ak@muc.de> wrote:
> > 
> > > > Some architectures (powerpc) implement WARN using the same mechanism;
> > > > if the illegal instruction was the result of a WARN, then report_bug()
> > > > returns 1; otherwise it returns 0.
> > > 
> > > In theory we could do that on x86 too (and skipping the instruction), 
> > > the only problem 
> > > is that the only guaranteed to fault opcode is ud2 :/. Ok maybe we could
> > > reserve some int XXX vector.
> > > 
> > > % gid WARN_ON | grep -v arch | wc -l
> > > 299
> > 
> > powerpc sets a bit in the __LINE__ number to indicate that it was a
> > WARN_ON.  That'll work on all architectures.
> 
> We still would need an architecture dependent way to skip the opcode
> though (just returning would raise it again). On x86
> 
> regs->eip += 2     (rip on x86-64) 
> 
> should be enough 
> 

We have all that now.  Do:

	if (report_bug(regs->eip) == BUG_TRAP_TYPE_WARN)
		regs>eip += 2;

(The powerpc is_warning_bug() implementation needs to be hoisted into
generic code)

Re: [PATCH RFC 1/4] Generic BUG handling.

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> For my x86_64 usualconfig .text (from objdump --headers) went from
> 0x002c55c7 down to 0x002c2bda, which is 10.5k saved.
>
> According to /usr/bin/size, vmlinux got bigger:
>
> box:/usr/src/25> size vmlinux 
>    text    data     bss     dec     hex filename
> 3597448  716340  510456 4824244  499cb4 vmlinux-before
> 3640604  716228  510456 4867288  4a44d8 vmlinux-after
>   

Good, that's what we'd hope for.  It's going to be a bigger overall than 
the previous i386 code, because it's now saving away the EIP as well as 
filename* and line for each BUG.

> But that's because size(1) is too blunt an instrument: the sum of .text and
> the new bug section got larger.
>   

size -A will tell you everything you ever wanted to know.

> I think we need to thank the powerpc guys, then take away their function
> name printing ;)
>   

I think so...

    J

Re: [PATCH RFC 1/4] Generic BUG handling.

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> I think we need to thank the powerpc guys, then take away their function
> name printing ;)
>   

Also, I think !CONFIG_DEBUG_BUGVERBOSE shouldn't store anything other 
than the ud2a instructions, to keep the embedded people happy.

    J

Re: [PATCH RFC 1/4] Generic BUG handling.

[Jeremy Fitzhardinge]

Michael Ellerman wrote:
> It needed a bit of work to get going on powerpc:
>
> Generic BUG handling, Powerpc fixups
>   

BTW, powerpc doesn't seem to be using BUG_OPCODE or 
BUG_ILLEGAL_INSTRUCTION for actual BUGs any more (I presume they were 
once used).  There are still a couple of uses of those macros elsewhere 
(kernel/prom_init.c and kernel/head_64.S); should be converted to "twi 
31,0,0" as well?

    J

Re: [PATCH RFC 1/4] Generic BUG handling.

[Andrew Morton]

On Fri, 29 Sep 2006 12:44:37 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Michael Ellerman wrote:
> > It needed a bit of work to get going on powerpc:
> >
> > Generic BUG handling, Powerpc fixups
> >   
> 
> BTW, powerpc doesn't seem to be using BUG_OPCODE or 
> BUG_ILLEGAL_INSTRUCTION for actual BUGs any more (I presume they were 
> once used).  There are still a couple of uses of those macros elsewhere 
> (kernel/prom_init.c and kernel/head_64.S); should be converted to "twi 
> 31,0,0" as well?
> 

I added that to the changelog.

I'll collapse all the patches I have back into a sane series and I'll send
them back at you, in case you feel inspired to improve them ;)