[PATCH] Driver core: treat unregistered bus_types as having no devices

[PATCH] Driver core: treat unregistered bus_types as having no devices

[Bjorn Helgaas]

A bus_type has a list of devices (klist_devices), but the list and the
subsys_private structure that contains it are not initialized until the
bus_type is registered with bus_register().

The panic/reboot path has fixups that look up devices in pci_bus_type.  If
we panic before registering pci_bus_type, the bus_type exists but the list
does not, so mach_reboot_fixups() trips over a null pointer and panics
again:

    mach_reboot_fixups
      pci_get_device
        ..
          bus_find_device(&pci_bus_type, ...)
            bus->p is NULL

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/base/bus.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 24eb078..6856303 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -290,7 +290,7 @@ int bus_for_each_dev(struct bus_type *bus, struct device *start,
 	struct device *dev;
 	int error = 0;
 
-	if (!bus)
+	if (!bus || !bus->p)
 		return -EINVAL;
 
 	klist_iter_init_node(&bus->p->klist_devices, &i,
@@ -324,7 +324,7 @@ struct device *bus_find_device(struct bus_type *bus,
 	struct klist_iter i;
 	struct device *dev;
 
-	if (!bus)
+	if (!bus || !bus->p)
 		return NULL;
 
 	klist_iter_init_node(&bus->p->klist_devices, &i,

Re: [PATCH] Driver core: treat unregistered bus_types as having no devices

[Bjorn Helgaas]

On Tue, Jan 29, 2013 at 4:44 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> A bus_type has a list of devices (klist_devices), but the list and the
> subsys_private structure that contains it are not initialized until the
> bus_type is registered with bus_register().
>
> The panic/reboot path has fixups that look up devices in pci_bus_type.  If
> we panic before registering pci_bus_type, the bus_type exists but the list
> does not, so mach_reboot_fixups() trips over a null pointer and panics
> again:
>
>     mach_reboot_fixups
>       pci_get_device
>         ..
>           bus_find_device(&pci_bus_type, ...)
>             bus->p is NULL
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/base/bus.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 24eb078..6856303 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -290,7 +290,7 @@ int bus_for_each_dev(struct bus_type *bus, struct device *start,
>         struct device *dev;
>         int error = 0;
>
> -       if (!bus)
> +       if (!bus || !bus->p)
>                 return -EINVAL;
>
>         klist_iter_init_node(&bus->p->klist_devices, &i,
> @@ -324,7 +324,7 @@ struct device *bus_find_device(struct bus_type *bus,
>         struct klist_iter i;
>         struct device *dev;
>
> -       if (!bus)
> +       if (!bus || !bus->p)
>                 return NULL;
>
>         klist_iter_init_node(&bus->p->klist_devices, &i,
>

Sorry, I meant to include this in the original post:

Joonsoo reported a problem when panicking before PCI was initialized.
I think this patch should be sufficient to replace the patch he posted
here: https://lkml.org/lkml/2012/12/28/75 ("[PATCH] x86, reboot: skip
reboot_fixups in early boot phase")

Re: [PATCH] Driver core: treat unregistered bus_types as having no devices

[Greg Kroah-Hartman]

On Tue, Jan 29, 2013 at 04:47:13PM -0700, Bjorn Helgaas wrote:
> On Tue, Jan 29, 2013 at 4:44 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > A bus_type has a list of devices (klist_devices), but the list and the
> > subsys_private structure that contains it are not initialized until the
> > bus_type is registered with bus_register().
> >
> > The panic/reboot path has fixups that look up devices in pci_bus_type.  If
> > we panic before registering pci_bus_type, the bus_type exists but the list
> > does not, so mach_reboot_fixups() trips over a null pointer and panics
> > again:
> >
> >     mach_reboot_fixups
> >       pci_get_device
> >         ..
> >           bus_find_device(&pci_bus_type, ...)
> >             bus->p is NULL
> >
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/base/bus.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > index 24eb078..6856303 100644
> > --- a/drivers/base/bus.c
> > +++ b/drivers/base/bus.c
> > @@ -290,7 +290,7 @@ int bus_for_each_dev(struct bus_type *bus, struct device *start,
> >         struct device *dev;
> >         int error = 0;
> >
> > -       if (!bus)
> > +       if (!bus || !bus->p)
> >                 return -EINVAL;
> >
> >         klist_iter_init_node(&bus->p->klist_devices, &i,
> > @@ -324,7 +324,7 @@ struct device *bus_find_device(struct bus_type *bus,
> >         struct klist_iter i;
> >         struct device *dev;
> >
> > -       if (!bus)
> > +       if (!bus || !bus->p)
> >                 return NULL;
> >
> >         klist_iter_init_node(&bus->p->klist_devices, &i,
> >
> 
> Sorry, I meant to include this in the original post:
> 
> Joonsoo reported a problem when panicking before PCI was initialized.
> I think this patch should be sufficient to replace the patch he posted
> here: https://lkml.org/lkml/2012/12/28/75 ("[PATCH] x86, reboot: skip
> reboot_fixups in early boot phase")

Don't you think that should be applied as well?  That makes things a bit
more explicit in the boot process as to exactly what is going on.  The
driver core changes is good to have if other people mess things up like
this in the future, don't rely on it to protect you from foolish things.

thanks,

greg k-h

Re: [PATCH] Driver core: treat unregistered bus_types as having no devices

[Bjorn Helgaas]

On Wed, Jan 30, 2013 at 1:09 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Jan 29, 2013 at 04:47:13PM -0700, Bjorn Helgaas wrote:
>> On Tue, Jan 29, 2013 at 4:44 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> > A bus_type has a list of devices (klist_devices), but the list and the
>> > subsys_private structure that contains it are not initialized until the
>> > bus_type is registered with bus_register().
>> >
>> > The panic/reboot path has fixups that look up devices in pci_bus_type.  If
>> > we panic before registering pci_bus_type, the bus_type exists but the list
>> > does not, so mach_reboot_fixups() trips over a null pointer and panics
>> > again:
>> >
>> >     mach_reboot_fixups
>> >       pci_get_device
>> >         ..
>> >           bus_find_device(&pci_bus_type, ...)
>> >             bus->p is NULL
>> >
>> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> > ---
>> >  drivers/base/bus.c |    4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
>> > index 24eb078..6856303 100644
>> > --- a/drivers/base/bus.c
>> > +++ b/drivers/base/bus.c
>> > @@ -290,7 +290,7 @@ int bus_for_each_dev(struct bus_type *bus, struct device *start,
>> >         struct device *dev;
>> >         int error = 0;
>> >
>> > -       if (!bus)
>> > +       if (!bus || !bus->p)
>> >                 return -EINVAL;
>> >
>> >         klist_iter_init_node(&bus->p->klist_devices, &i,
>> > @@ -324,7 +324,7 @@ struct device *bus_find_device(struct bus_type *bus,
>> >         struct klist_iter i;
>> >         struct device *dev;
>> >
>> > -       if (!bus)
>> > +       if (!bus || !bus->p)
>> >                 return NULL;
>> >
>> >         klist_iter_init_node(&bus->p->klist_devices, &i,
>> >
>>
>> Sorry, I meant to include this in the original post:
>>
>> Joonsoo reported a problem when panicking before PCI was initialized.
>> I think this patch should be sufficient to replace the patch he posted
>> here: https://lkml.org/lkml/2012/12/28/75 ("[PATCH] x86, reboot: skip
>> reboot_fixups in early boot phase")
>
> Don't you think that should be applied as well?  That makes things a bit
> more explicit in the boot process as to exactly what is going on.  The
> driver core changes is good to have if other people mess things up like
> this in the future, don't rely on it to protect you from foolish things.

mach_reboot_fixups() is not part of the boot process at all.  That's
why it seems ugly to me to clutter it with a system_state check.  I
don't really think of it as having messed up by calling
pci_get_device() without checking system_state.  Certainly we don't
expect *every* caller to check system_state, and it's not trivial to
determine at every call site whether pci_bus has been registered or
not.

But if anybody wants to apply the https://lkml.org/lkml/2012/12/28/75
too, it's OK with me.

Tangential question ... I was curious as to why bus_for_each_dev() and
bus_find_device() (and other driver core things) check for "!bus" in
the first place.  For all the callers I looked at, it's obvious at
compile-time that bus can't be NULL.  The question of whether "bus->p"
is null can't be answered at compile-time because it depends on
whether the bus has been registered.

Bjorn