Hi Bjorn,
Am 25.04.2016 um 18:59 schrieb Bjorn Helgaas:
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.
Please have a look at drivers/pci/iov.c:virtfn_add_bus(). This is the
code I'm using for adding a virtual PCI bus. It calls pci_add_new_bus()
with dev = NULL, thus the new child sets the member "self" to NULL. We
could also forbid using NULL for the second parameter of
pci_add_new_bus() but I have no idea how a virtual PCI bus without a
real device will work in this case. If you agree I will resend my patch
with a description.
David
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. */
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
