Linux kernel development

Alex Williamson | 4 Aug 2012 20:19

Picon

[PATCH] pci: Account for virtual buses in pci_acs_path_enabled

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.

Signed-off-by: Alex Williamson <alex.williamson <at> 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
 <at>  <at>  -2486,18 +2486,30  <at>  <at>  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;

(Continue reading)

David Ahern | 6 Aug 2012 06:51

Picon

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

On 8/4/12 12:19 PM, Alex Williamson 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.
>
> Signed-off-by: Alex Williamson <alex.williamson <at> 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.

Tested-by: David Ahern <dsahern <at> gmail.com>

Bjorn Helgaas | 6 Aug 2012 07:30

Picon

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

On Sat, Aug 4, 2012 at 12:19 PM, Alex Williamson
<alex.williamson <at> 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.

(Continue reading)

Alex Williamson | 6 Aug 2012 07:55

Picon

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

On Sun, 2012-08-05 at 23:30 -0600, Bjorn Helgaas wrote:
> On Sat, Aug 4, 2012 at 12:19 PM, Alex Williamson
> <alex.williamson <at> 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.

Yes, exactly.  virtfn_add_bus() is where we're creating this new bus.

> 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(),

(Continue reading)

Bjorn Helgaas | 6 Aug 2012 22:47

Picon

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

On Sun, Aug 5, 2012 at 11:55 PM, Alex Williamson
<alex.williamson <at> redhat.com> wrote:
> On Sun, 2012-08-05 at 23:30 -0600, Bjorn Helgaas wrote:
>> On Sat, Aug 4, 2012 at 12:19 PM, Alex Williamson
>> <alex.williamson <at> 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.
>
> Yes, exactly.  virtfn_add_bus() is where we're creating this new bus.
>
>> 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.
>>

(Continue reading)

Don Dutile | 7 Aug 2012 23:50

Picon

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

On 08/06/2012 04:47 PM, Bjorn Helgaas wrote:
> On Sun, Aug 5, 2012 at 11:55 PM, Alex Williamson
> <alex.williamson <at> redhat.com>  wrote:
>> On Sun, 2012-08-05 at 23:30 -0600, Bjorn Helgaas wrote:
>>> On Sat, Aug 4, 2012 at 12:19 PM, Alex Williamson
>>> <alex.williamson <at> 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.
>>
>> Yes, exactly.  virtfn_add_bus() is where we're creating this new bus.
>>
>>> 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.

(Continue reading)

Bjorn Helgaas | 8 Aug 2012 08:00

Picon

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

On Tue, Aug 7, 2012 at 2:50 PM, Don Dutile <ddutile <at> redhat.com> wrote:
> On 08/06/2012 04:47 PM, Bjorn Helgaas wrote:
>>
>> On Sun, Aug 5, 2012 at 11:55 PM, Alex Williamson
>> <alex.williamson <at> redhat.com>  wrote:
>>>
>>> On Sun, 2012-08-05 at 23:30 -0600, Bjorn Helgaas wrote:
>>>>
>>>> On Sat, Aug 4, 2012 at 12:19 PM, Alex Williamson
>>>> <alex.williamson <at> 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.

(Continue reading)

David Ahern | 8 Aug 2012 15:51

Picon

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

On 8/8/12 12:00 AM, Bjorn Helgaas wrote:
> On Tue, Aug 7, 2012 at 2:50 PM, Don Dutile <ddutile <at> redhat.com> wrote:
>> On 08/06/2012 04:47 PM, Bjorn Helgaas wrote:
>>>
>>> On Sun, Aug 5, 2012 at 11:55 PM, Alex Williamson
>>> <alex.williamson <at> redhat.com>  wrote:
>>>>
>>>> On Sun, 2012-08-05 at 23:30 -0600, Bjorn Helgaas wrote:
>>>>>
>>>>> On Sat, Aug 4, 2012 at 12:19 PM, Alex Williamson
>>>>> <alex.williamson <at> 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

(Continue reading)

Alex Williamson | 8 Aug 2012 17:24

Picon

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

On Tue, 2012-08-07 at 23:00 -0700, Bjorn Helgaas wrote:
> On Tue, Aug 7, 2012 at 2:50 PM, Don Dutile <ddutile <at> redhat.com> wrote:
> > On 08/06/2012 04:47 PM, Bjorn Helgaas wrote:
> >>
> >> On Sun, Aug 5, 2012 at 11:55 PM, Alex Williamson
> >> <alex.williamson <at> redhat.com>  wrote:
> >>>
> >>> On Sun, 2012-08-05 at 23:30 -0600, Bjorn Helgaas wrote:
> >>>>
> >>>> On Sat, Aug 4, 2012 at 12:19 PM, Alex Williamson
> >>>> <alex.williamson <at> 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

(Continue reading)

Don Dutile | 8 Aug 2012 21:33

Picon

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

On 08/08/2012 11:24 AM, Alex Williamson wrote:
> On Tue, 2012-08-07 at 23:00 -0700, Bjorn Helgaas wrote:
>> On Tue, Aug 7, 2012 at 2:50 PM, Don Dutile<ddutile <at> redhat.com>  wrote:
>>> On 08/06/2012 04:47 PM, Bjorn Helgaas wrote:
>>>>
>>>> On Sun, Aug 5, 2012 at 11:55 PM, Alex Williamson
>>>> <alex.williamson <at> redhat.com>   wrote:
>>>>>
>>>>> On Sun, 2012-08-05 at 23:30 -0600, Bjorn Helgaas wrote:
>>>>>>
>>>>>> On Sat, Aug 4, 2012 at 12:19 PM, Alex Williamson
>>>>>> <alex.williamson <at> 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

(Continue reading)

Return

Return to gmane.linux.kernel.

Advertisement

Search Archive

Language

Change language

Options

Current view: Threads only / Showing only 20 lines / Not hiding cited text.
Change to All messages, whole messages, or hide cited text.

Post a message
NNTP Newsgroup
Classic Gmane web interface
XML

XML

RSS Feed
List Information

About Gmane