linux-kernel - Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 6 Feb 2013 13:50:49 -0700
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Yinghai Lu <yinghai@...nel.org>
Cc:	Jiang Liu <jiang.liu@...wei.com>,
	Mauro Carvalho Chehab <mchehab@...hat.com>,
	Tony Luck <tony.luck@...el.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Taku Izumi <izumi.taku@...fujitsu.com>,
	Toshi Kani <toshi.kani@...com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-pci@...r.kernel.org, Russell King <linux@....linux.org.uk>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

On Wed, Feb 6, 2013 at 11:59 AM, Yinghai Lu <yinghai@...nel.org> wrote:
> On Wed, Feb 6, 2013 at 9:54 AM, Bjorn Helgaas <bhelgaas@...gle.com> wrote:
>> I think you're missing the point.
>>
>> Search the tree for uses of "for_each_pci_dev()."  Almost every
>> occurrence is a bug because that code doesn't work correctly for
>> hot-added devices.  That code gets run for devices that are present at
>> boot, but not for devices hot-added later.
>>
>> You're proposing to add "for_each_pci_host_bridge()."  That will have
>> the exact same problem as for_each_pci_dev() already does.  Code that
>> uses it will work for host bridges present at boot, but not for
>> bridges hot-added later.
>>
>> Why would we want to add an interface when every use of that interface
>> will be a design bug?  I think we should fix the existing users of
>> pci_root_buses by changing their designs so they will work correctly
>> with host bridge hotplug.
>
> I'm a little confused about what you want.
>
> In boot stage using for_each_pci_host_bridge or pci_root_buses is fine.

No, it's not.  Boot-time should work exactly the same way as hot-add
time, unless there's a reason it can't.  There might be corner cases
where we can't do that yet, e.g., the pcibios_resource_survey stuff,
but in general I don't think there's anything that *forces* us to
iterate through host bridges at boot-time.

> For those cases that it should support host-bridge by nature.
> there are two solutions:
> 1. use for_each_pci_host_bridge, and it is hotplug-safe.

I'm trying to tell you that for_each_pci_host_bridge() is NOT
hotplug-safe.  Your series makes it safer than it was, in the sense
that it probably fixes stray pointer references when a host bridge
hotplug happens while somebody's traversing pci_root_buses.  But the
whole point of for_each_pci_host_bridge() is to run some code for
every bridge we know about *right now*.  If a host bridge is added
right after the for_each_pci_host_bridge() loop exits, that code
doesn't get run.  In most cases, that's a bug.  The only exception I
know about is the /sys/.../rescan interface.

> and make sgi_hotplug to use acpi_pci_driver interface.
> and acpi_pci_root_add() will call .add in the acpi_pci_driver.
>
> 2. make them all to be built-in, and  those acpi_pci_driver should be registered
> much early before acpi_pci_root_add is called.
> then we don't need to call for_each_host_bridge for it.
>
> So difference between them:
> 1. still keep the module support, and register acpi_pci_driver later.
> 2. built-in support only, and need to register acpi_pci_driver early.

acpi_pci_driver is going away, so I don't want to add any more uses of
it.  Obviously it's only relevant to x86 and ia64 anyway.

What I'd like is a change where for_each_pci_host_bridge() is used
only inside the PCI core and defined somewhere like drivers/pci/pci.h
so it's not even available outside.

The other uses should be changed so they use
pcibios_root_bridge_prepare() or something similar.  That way, it will
be obvious that the code supports hot-added bridges, and when it gets
copied to other places, we'll be copying a working design instead of a
broken one.

I don't want to have to audit these places and figure out "this is
safe because this arch doesn't support host bridge hotplug" or "this
is safe because this CPU doesn't support XYZ."  That's not a
maintainable solution.  The safety should be apparent from the code
itself, without requiring extra platform-specific knowledge.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/