LKML Archive on
help / color / mirror / Atom feed
From: Greg KH <>
To: Ingo Molnar <>
Cc:, Yinghai Lu <>,
	Jacob Shin <>,
	Linus Torvalds <>,
	Alexander Viro <>
Subject: Re: [PATCH 2/5] x86: fix runtime error in arch/x86/kernel/cpu/mcheck/mce_amd_64.c
Date: Mon, 28 Jan 2008 10:32:37 -0800	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Mon, Jan 28, 2008 at 06:57:36PM +0100, Ingo Molnar wrote:
> * Greg KH <> wrote:
> > I have added a lot more "robustness" checks to the kobject core now, 
> > see the lkml messages about the maple bus for examples of where it is 
> > catching real problems already.  And the kobject debugging code is now 
> > "unified", printing out everything in a standard, easy to understand 
> > manner.
> > 
> > I really don't want to get into adding a "magic value" to a kobject 
> > field, and then checking it for every call, that too would have died 
> > on this kind of bug, just like the debug printk did :)
> > 
> > But if there's anything that you think I can add to make it easier to 
> > understand, please let me know.
> anything that just causes the function to die on that bug reliably 
> during normal use, without having to enable DEBUG_KOBJECT which just 
> kills the system due to its verbosity. A magic value would be perfect. 

But for this case, how would we test that 0x18 is passed in for a
pointer to a kobject, without just blowing up instantly no matter what?

> Are kobjects protected against accidental copying? If not add &kobj to 
> the 'magic value' too, and check that - it becomes copying-resistent 
> that way and has the same cost to check. (which is negligible anyway)

Oh, that's a very cool idea, I like it :)

So I guess the complaint is that CONFIG_KOBJECT is too verbose?  I can
understand that, I'll look to clean things up as we have a lot of
"tracing" type functions in the debug output.  Hm, I wonder if the new
marker code could help with that...

> > The root problem of this bug was us using a goto to call forward into 
> > a major code block within a function.  Not just using a goto for error 
> > cleanup, like the kernel code "normally" does.  Because of that, I 
> > totally missed this code path when reading the function many times. 
> > It's nasty code complexity for no reason at all that causes more 
> > problems than is needed, combined with a total lack of documentation 
> > for how this kobject userspace interface is supposed to be used, that 
> > needs to be fixed.
> no argument about that at all! This is exactly the same problem that the 
> spinlock/mutex/rwlock/etc. APIs were facing: it's used everywhere, and 
> that means dubious places as well that are not that well-known and are 
> rarely used. The more widely used a piece of kernel infrastructure is, 
> the more 'hardened' it must be against intentional or accidental abuse. 
> (at least with certain magic debug options enabled)
> So please regard this a good thing - obscure APIs need no debugging 
> infrastructure - widely used ones do need quite extensive debugging 
> infrastructure.

I totally agree.

> For example locks currently have 4000 lines of code of debugging 
> infrastructure and 1500 lines of code of self-tests. The total amount of 
> core locking code is less 1000 lines of code. So for every line of 
> locking code there's more than 5 lines of debugging infrastructure (!). 

Heh, good point, I'll look into fixing this up to be more useful to the
"normal" developer, and not just the two of us doing kobject core
development :)


greg k-h

  reply	other threads:[~2008-01-28 18:35 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-01-27 23:37 [GIT PATCH] driver core fixes against 2.6.24 Greg KH
2008-01-27 23:38 ` [PATCH 1/5] Driver core: Fix up build when CONFIG_BLOCK=N Greg Kroah-Hartman
2008-01-27 23:38 ` [PATCH 2/5] x86: fix runtime error in arch/x86/kernel/cpu/mcheck/mce_amd_64.c Greg Kroah-Hartman
2008-01-28 12:24   ` Ingo Molnar
2008-01-28 17:37     ` Greg KH
2008-01-28 17:57       ` Ingo Molnar
2008-01-28 18:32         ` Greg KH [this message]
2008-01-28 19:01           ` Ingo Molnar
2008-01-28 19:29             ` Cyrill Gorcunov
2008-01-28 19:42             ` Cyrill Gorcunov
2008-01-27 23:38 ` [PATCH 3/5] Module: check to see if we have a built in module with the same name Greg Kroah-Hartman
2008-01-28 23:54   ` Jan Engelhardt
2008-01-29  6:20   ` Rusty Russell
2008-01-27 23:38 ` [PATCH 4/5] Driver core: add bus_find_device_by_name function Greg Kroah-Hartman
2008-01-27 23:38 ` [PATCH 5/5] PPC: Fix powerpc vio_find_name to not use devices_subsys Greg Kroah-Hartman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH 2/5] x86: fix runtime error in arch/x86/kernel/cpu/mcheck/mce_amd_64.c' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).