Re: [PATCH] pci: Account for virtual buses in pci_acs_path_enabled

[Bjorn Helgaas <bhelgaas@google.com>]

On Sat, Aug 4, 2012 at 12:19 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> It's possible to have buses without an associated bridge
> (bus->self == NULL).  SR-IOV can generate such buses.  When
> we find these, skip to the parent bus to look for the next
> ACS test.

To make sure I understand the problem here, I think you're referring
to the situation where an SR-IOV device can span several bus numbers,
e.g., the "VFs Spanning Multiple Bus Numbers" implementation note in
the SR-IOV 1.1 spec, sec. 2.1.2.

It says "All PFs must be located on the Device's captured Bus Number"
-- I think that means every PF will be directly on a bridge's
secondary bus and hence will have a valid dev->bus->self pointer.

However, VFs need not be on the same bus number.  If a VF is on
(captured Bus Number plus 1), I think we allocate a new struct pci_bus
for it, but there's no P2P bridge that leads to that bus, so the
bus->self pointer is probably NULL.

This makes me quite nervous, because I bet there are many places that
assume every non-root bus has a valid bus->self pointer  -- I know I
certainly had that assumption.

I looked at callers of pci_is_root_bus(), and at first glance, it seems like
iommu_init_device(), intel_iommu_add_device(), pci_acs_path_enabled(),
pci_get_interrupt_pin(), pci_common_swizzle(),
pci_find_upstream_pcie_bridge(), and
pci_bus_release_bridge_resources() all might have similar problems.

> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>
> David Ahern reported an oops from iommu drivers passing NULL into
> this function for the same mistake.  Harden this function against
> assuming bus->self is valid as well.  David, please include this
> patch as well as the iommu patches in your testing.
>
>  drivers/pci/pci.c |   22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f3ea977..e11a49c 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2486,18 +2486,30 @@ bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags)
>  bool pci_acs_path_enabled(struct pci_dev *start,
>                           struct pci_dev *end, u16 acs_flags)
>  {
> -       struct pci_dev *pdev, *parent = start;
> +       struct pci_dev *pdev = start;
> +       struct pci_bus *bus;
>
>         do {
> -               pdev = parent;
> -
>                 if (!pci_acs_enabled(pdev, acs_flags))
>                         return false;
>
> -               if (pci_is_root_bus(pdev->bus))
> +               bus = pdev->bus;
> +
> +               if (pci_is_root_bus(bus))
>                         return (end == NULL);
>
> -               parent = pdev->bus->self;
> +               /*
> +                * Skip buses without an associated bridge.  In this
> +                * case move to the parent and continue.
> +                */
> +               while (!bus->self) {
> +                       if (!pci_is_root_bus(bus))
> +                               bus = bus->parent;
> +                       else
> +                               return (end == NULL);
> +               }
> +
> +               pdev = bus->self;
>         } while (pdev != end);
>
>         return true;
>