lkml.org 
[lkml]   [2008]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: v2.6.28-rc7: error in panic code? (NULL pointer dereference at 0000004c)
On Sat, Dec 20, 2008 at 09:52:10AM +0100, Vegard Nossum wrote:
> On Sat, Dec 20, 2008 at 1:46 AM, Greg KH <gregkh@suse.de> wrote:
> > On Fri, Dec 19, 2008 at 10:35:57PM +0200, Pekka Enberg wrote:
> >> Hi Greg,
> >>
> >> On Fri, Dec 19, 2008 at 06:19:43PM +0100, Vegard Nossum wrote:
> >> > Fixes what? It might be quite difficult to revert that patch now, as
> >> > the infrastructure is no longer in place to use a private pci device
> >> > list, that code is long gone.
> >>
> >> Vegard forced one oops but got two! The first one is expected and but
> >> the second one shouldn't probably be there:
> >
> > "Second" oopses are known to not be reliable, I wouldn't count it as a
> > real problem unless it happens on its own.
>
> Yes, because usually it's a process that BUGed and was killed --
> perhaps with locks held or in the middle of some transaction that will
> never complete. But this one happens in the panic code itself...
>
> >
> >> >> > [ 0.040993] EIP: [<c13b41dc>] klist_next+0x10/0x8d SS:ESP 0068:c165dd48
> >>
> >> Looks like the patch Vegard identified breaks something in the oops path?
> >
> > Very wierd, I also don't understand how reverting the specific patch
> > would even make a buildable system.
>
> It was an unclean revert, here's the relevant resultant hunk:
>
> diff --cc drivers/pci/probe.c
> index 003a9b3,2db2e4b..0000000
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@@ -32,16 -29,55 +28,11 @@@ LIST_HEAD(pci_devices)
> */
> int no_pci_devices(void)
> {
> - struct device *dev;
> - int no_devices;
> -
> - dev = bus_find_device(&pci_bus_type, NULL, NULL, find_anything);
> - no_devices = (dev == NULL);
> - put_device(dev);
> - return no_devices;
> + return list_empty(&pci_devices);
> }
> +
> EXPORT_SYMBOL(no_pci_devices);
>
>
> ...and this builds.
>
> The problem seems to be that pci_bus_type.p->klist_devices is NULL. Because:
>
> Oops happens here:
>
> struct klist_node *klist_next(struct klist_iter *i)
> {
> void (*put)(struct klist_node *) = i->i_klist->put;
>
> So either i == NULL or i->i_klist == NULL. But i->i_klist was just
> before set here:
>
> void klist_iter_init_node(struct klist *k, struct klist_iter *i,
> struct klist_node *n)
> {
> i->i_klist = k;
> i->i_cur = n;
> if (n)
> kref_get(&n->n_ref);
> }
>
> so the klist passed must have been NULL, it came from bus_find_device():
>
> klist_iter_init_node(&bus->p->klist_devices, &i,
> (start ? &start->knode_bus : NULL));
>
> ...and indeed, printing bus->p here yields 00000000. This function was
> called from no_pci_devices(), so the bus variable was initialized from
> &pci_bus_type. So pci_bus_type.p == NULL.
>
> This should be initialized in bus_register() called from
> pci_driver_init(). Aha, this never gets called because initcalls did
> not yet run.
>
> A summary of the bug:
>
> 1. Sending panic=1 wants to reboot on panic.
> 2. If panic occurs before initcalls ran, pci_bus_type.p is not initialized.
> 3. mach_reboot_fixups() in x86 code calls pci_get_device()
> 4. New oops
>
> Maybe mach_reboot_fixups() should check to see if PCI bus is
> initialized before calling pci_get_device(), since obviously it can be
> called before it has been initialized too.
>
> The funny thing is that no_pci_devices() is what _used_ to guard
> against using pci_bus_type too early:
>
> /*
> * Some device drivers need know if pci is initiated.
> * Basically, we think pci is not initiated when there
> * is no device to be found on the pci_bus_type.
> */
> int no_pci_devices(void)
>
> ...and now it uses pci_bus_type itself. That is what makes commit
> 70308923d317f2ad4973c30d90bb48ae38761317 wrong, because there might be
> other users of no_pci_devices() too, which would now almost certainly
> result in an Oops if the pci bus hasn't been initialized.
>
> Please tell if any of the above is unclear, and I will try to explain
> more. Thanks,

No, that makes perfect sense.

Care to make a patch for no_pci_devices() to work properly in this kind
of situation?

thanks,

greg k-h


\
 
 \ /
  Last update: 2008-12-20 10:03    [W:0.052 / U:0.876 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site