Re: [PATCH] Re: Exception due to PCI: Use pci_is_root_bus() to check for root bus

[Bjorn Helgaas <helgaas@kernel.org>]

Hi David,

On Mon, Apr 25, 2016 at 03:04:47PM +0200, David Engraf wrote:
> Hi Bjorn,
> 
> Am 06.04.2016 um 16:01 schrieb David Engraf:
> >Hi Bjorn,
> >
> >Am 06.04.2016 um 15:35 schrieb Bjorn Helgaas:
> >>Hi David,
> >>
> >>On Wed, Apr 06, 2016 at 01:51:55PM +0200, David Engraf wrote:
> >>>Hi,
> >>>
> >>>I have an exception in __pci_bus_size_bridges() when pci_is_root_bus
> >>>returns false but bus->self == NULL. My driver registers a virtual
> >>>bus, like virtfn_add_bus(). pci_add_new_bus() is called with a
> >>>parent but without a pci_dev. Thus bus->parent is set but bus->self
> >>>is NULL. When __pci_bus_size_bridges() is called I get an exception
> >>>at:
> >>>
> >>>switch (bus->self->class >> 8)
> >>>
> >>>The previous version of the code, checking for bus->self != NULL
> >>>worked for me. I think an additional check is required to make sure
> >>>we're not accessing a NULL pointer.
> >>
> >>When you say "previous version of the code," do you mean a previous
> >>version of Linux worked correctly but a newer version does not?  What
> >>versions are they?  Did you identify a commit that changed the
> >>behavior?  I assume you're talking about an out-of-tree driver that
> >>registers a virtual bus?  Can you point us to the code?  Can you share
> >>the dmesg log, including the backtrace?
> >
> >With previous version, I mean reverting commit
> >2ba29e270e977b213a7d58ae1152c23a1c3074a3. This will check for bus->self
> >instead of using pci_is_root_bus().
> >I'm working on a driver which is basically based on drivers/pci/iov.c.
> >
> >The code looks like this:
> >
> >child = pci_add_new_bus(parent, NULL, busnr);
> >
> >This will create child with child->parent = parent and child->self =
> >NULL. When the kernel calls __pci_bus_size_bridges(), accessing
> >bus->self->class crashes:
> >
> >/* The root bus? */
> >if (pci_is_root_bus(bus))
> >     return;
> >
> >switch (bus->self->class >> 8) { <- crash here because of bus->self == NULL
> 
> With this patch the system no longer crashes and the virtual PCI bus
> works. It returns on bus->self == NULL, thus the switch instruction
> does not access a NULL pointer.

This would need a changelog and Signed-off-by before I could apply it;
see
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches

The changelog should explain why this is the correct fix.  It's easy
to fix any NULL pointer dereference by adding a check for the NULL
pointer, but that's not always the correct fix.

This particular case is in the resource assignment path, so we need to
think about how resource assignment works for your virtual PCI bus.
What exactly is your PCI topology?  Apparently you have a virtual PCI
bus without a standard PCI-PCI bridge leading to it?  That sounds
similar in some ways to the virtual bus for SR-IOV VFs, and in some
ways to the virtual root bus constructed for drivers like
drivers/pci/host/pci-hyperv.c and arch/x86/pci/vmd.c.

There are already several special cases for the SR-IOV virtual buses
and for root buses, so I'd like to understand better how your bus fits
into the landscape.  Your driver apparently isn't part of the kernel
source, so if we add special cases for it, they're hard to maintain
because we can't see the code that relies on them.

Bjorn

> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 55641a3..ddb3381 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1245,6 +1245,10 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct list_head *realloc_head)
>  	if (pci_is_root_bus(bus))
>  		return;
>  
> +	/* bridge device available? */
> +	if (!bus->self)
> +		return;
> +
>  	switch (bus->self->class >> 8) {
>  	case PCI_CLASS_BRIDGE_CARDBUS:
>  		/* don't size cardbuses yet. */