Exception due to PCI: Use pci_is_root_bus() to check for root bus

DISCUSSION David Engraf 2 participants 7 messages Apr 6, 2016 – May 11, 2016 PCI

Share & tools

David Engraf

author

2016-04-06 12:01 UTC #0 lore raw

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.

Best regards
David Engraf

Bjorn Helgaas

2016-04-06 13:35 UTC #1 lore raw

Hi David,

Quoted 9 lines · David Engraf

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:

Quoted 5 lines

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?

Bjorn

David Engraf

author

2016-04-06 14:01 UTC #2 lore raw

Hi Bjorn,

Am 06.04.2016 um 15:35 schrieb Bjorn Helgaas:

Quoted 18 lines

Hi David,
Quoted 9 lines · David Engraf
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:
Quoted 5 lines
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.

Quoted 6 lines

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

Best regards
David

David Engraf

author

2016-04-25 13:04 UTC #3 lore raw

Hi Bjorn,

Am 06.04.2016 um 16:01 schrieb David Engraf:

Quoted 41 lines

Hi Bjorn,
Am 06.04.2016 um 15:35 schrieb Bjorn Helgaas:
Quoted 18 lines
Hi David,
Quoted 9 lines · David Engraf
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:
Quoted 5 lines
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.
Quoted 6 lines
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:

Quoted 5 lines

/* 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.

Best regards
- David

drivers/pci/setup-bus.c +4 -0

index 55641a3..ddb3381 100644
@@ -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. */

Bjorn Helgaas

2016-04-25 17:00 UTC #4 lore raw

Hi David,

Quoted 48 lines · David Engraf

Hi Bjorn,
Am 06.04.2016 um 16:01 schrieb David Engraf:
Quoted 45 lines
Hi Bjorn,
Am 06.04.2016 um 15:35 schrieb Bjorn Helgaas:
Quoted 18 lines
Hi David,
Quoted 9 lines · David Engraf
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:
Quoted 5 lines
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.
Quoted 6 lines
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;

Quoted 5 lines

Quoted 1 line
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

Quoted 15 lines

drivers/pci/setup-bus.c +4 -0

index 55641a3..ddb3381 100644
@@ -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. */

David Engraf

author

2016-05-11 07:04 UTC #5 lore raw

Hi Bjorn,

Am 25.04.2016 um 18:59 schrieb Bjorn Helgaas:

Quoted 65 lines

Hi David,
Quoted 48 lines · David Engraf
Hi Bjorn,
Am 06.04.2016 um 16:01 schrieb David Engraf:
Quoted 45 lines
Hi Bjorn,
Am 06.04.2016 um 15:35 schrieb Bjorn Helgaas:
Quoted 18 lines
Hi David,
Quoted 9 lines · David Engraf
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:
Quoted 5 lines
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.
Quoted 6 lines
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;
Quoted 5 lines
Quoted 1 line
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.

Quoted 7 lines

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

Quoted 24 lines

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
Quoted 15 lines
drivers/pci/setup-bus.c +4 -0
index 55641a3..ddb3381 100644
@@ -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. */

Bjorn Helgaas

2016-05-11 14:53 UTC #6 lore raw

Hi David,

Quoted 76 lines · David Engraf

Hi Bjorn,
Am 25.04.2016 um 18:59 schrieb Bjorn Helgaas:
Quoted 65 lines
Hi David,
Quoted 48 lines · David Engraf
Hi Bjorn,
Am 06.04.2016 um 16:01 schrieb David Engraf:
Quoted 45 lines
Hi Bjorn,
Am 06.04.2016 um 15:35 schrieb Bjorn Helgaas:
Quoted 18 lines
Hi David,
Quoted 9 lines · David Engraf
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:
Quoted 5 lines
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.
Quoted 6 lines
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;
Quoted 5 lines
Quoted 1 line
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.
Quoted 7 lines
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.

Quoted 7 lines

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.

Just send the patch and description. If I don't agree or think it needs tweaking, we can iterate on it. There's not much point in me trying to evaluate a prose argument by itself: I might agree with the argument but still disagree with the implementation. Or, as often happens, I might agree with the implementation, but try to refine the description.

Quoted 5 lines

Quoted 5 lines
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.

I'd still like to see this part addressed so I can get a sense of how to integrate this into the SR-IOV, VMD, and Hyper-V landscape.

Quoted 17 lines

Quoted 16 lines

Quoted 15 lines

drivers/pci/setup-bus.c +4 -0

index 55641a3..ddb3381 100644
@@ -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. */