Hi David,
On Wed, May 11, 2016 at 09:04:44AM +0200, David Engraf wrote:
> 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.
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.
> >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.
> >>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
