On Wed, Sep 12, 2012 at 10:51:05AM -0600, Bjorn Helgaas wrote: > On Wed, Sep 12, 2012 at 9:42 AM, Jiang Liu <liuj97@xxxxxxxxx> wrote: > > On 09/12/2012 06:57 AM, Bjorn Helgaas wrote: > >> On Tue, Aug 7, 2012 at 10:10 AM, Jiang Liu <liuj97@xxxxxxxxx> wrote: > >>> Currently there's no mechanism to protect the global pci_root_buses list > >>> from dynamic change at runtime. That means, PCI root bridge hotplug > >>> operations, which dynamically change the pci_root_buses list, may cause > >>> invalid memory accesses. > >>> > >>> So introduce a global lock to serialize accesses to the pci_root_buses > >>> list and serialize PCI host bridge hotplug operations. > > >>> @@ -463,6 +463,8 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device) > >>> if (!root) > >>> return -ENOMEM; > >>> > >>> + pci_host_bridge_hotplug_lock(); > >> > >> Here's where I get lost. This is an ACPI driver's .add() routine, > >> which is analogous to a PCI driver's .probe() routine. PCI driver > >> .probe() routines don't need to be concerned with PCI device hotplug. > >> All the hotplug-related locking is handled by the PCI core, not by > >> individual drivers. So why do we need it here? > >> > >> I'm not suggesting that the existing locking is correct. I'm just not > >> convinced this is the right way to fix it. > >> > >> The commit log says we need protection for the global pci_root_buses > >> list. But even with this whole series, we still traverse the list > >> without protection in places like pcibios_resource_survey() and > >> pci_assign_unassigned_resources(). > >> > >> Maybe we can make progress on this by identifying specific failures > >> that can happen in a couple of these paths, e.g., acpi_pci_root_add() > >> and i7core_xeon_pci_fixup(). If we look at those paths, we might a > >> way to fix this in a more general fashion than throwing in lock/unlock > >> pairs. > >> > >> It might also help to know what the rule is for when we need to use > >> pci_host_bridge_hotplug_lock() and pci_host_bridge_hotplug_unlock(). > >> Apparently it is not as simple as protecting every reference to the > >> pci_root_buses list. > > Hi Bjorn, > > It's really a challenge work to protect the pci_root_buses list:) > > Yes. IIRC, your last patch was to unexport pci_root_buses, which I > think is a great idea. > > > All evils are caused by the pci_find_next_bus() interface, which is designed > > to be called at boot time only. I have tried several other solutions but > > failed. > > First I tried "pci_get_next_bus()" which holds a reference to the > > returned root bus "pci_bus". But that doesn't help because pci_bus could > > be removed from the pci_root_buses list even you hold a reference to > > pci_bus. And it will cause trouble when you call pci_get_next_bus(pci_bus) > > again because pci_bus->node.next is invalid now. > > That sounds like a bug. If an interface returns a structure after > acquiring a reference, the caller should be able to rely on the > structure remaining valid. Adding extra locks doesn't feel like the > right solution for that problem. > > In the big picture, I'm not sure how much sense all the > pci_find_bus(), pci_find_next_bus(), pci_get_bus(), > pci_get_next_bus(), etc., interfaces really make. There really aren't > very many callers, and most of them look a bit hacky to me. Usually > they're quirks trying to locate a device or drivers for device A > trying to locate companion device B or something similar. I wonder if > we could figure out some entirely new interface that wouldn't involve > traversing so much of the hierarchy and therefore could be safer. > > > Then I tried RCU and also failed because caller of pci_get_next_bus() > > may sleep. On the unlikely off-chance that it helps, SRCU does allow sleeping readers. Thanx, Paul > > And at last the global host bridge hotplug lock solution. The rules > > for locking are: > > 1) No need for locking when accessing the pci_root_buses list at > > system initialization stages. (It's system initialization instead of driver > > initialization here because driver's initialization code may be called > > at runtime when loading the driver.) It's single-threaded and no hotplug > > during system initialization stages. > > 2) Should acquire the global lock when accessing the pci_root_buses > > list at runtime. > > > > I have done several rounds of scanning to identify accessing to > > the pci_root_buses list at runtime. But there may still be something missed:( > > That's part of what makes me uneasy. We have to look at a lot of code > outside drivers/pci to analyze correctness, which is difficult. It > would be much better if we could do something in the core, where we > only have to analyze drivers/pci. I know this is probably much harder > and probably involves replacing or removing some of these interfaces > that cause problems. > > > I think the best solution is to get rid of the pci_find_next_bus(). > > but not sure whether we could achieve that. > > >> Actually, I looked at the callers of pci_find_next_bus(), and most of > >> them are unsafe in an even deeper way: they're doing device setup in > >> initcalls, so that setup won't be done for hot-added devices. For > >> example, I can pick on sba_init() because I think I wrote it back in > >> the dark ages. sba_init() is a subsys_initcall that calls > >> sba_connect_bus() for every bus we know about at boot-time, and it > >> sets the host bridge's iommu pointer. If we were to hot-add a host > >> bridge, we would never set the iommu pointer. > > > That's a more fundamental issue, another big topic for us:( > > >> I'm not sure why you didn't add a pci_host_bridge_hotplug_lock() in > >> the sba_init() path, since it looks similar to the drm_open_helper() > >> path above. But in any case, I think that would be the wrong thing to > >> do because it would fix the superficial problem while leaving the > >> deeper problem of host bridge hot-add not setting the iommu pointer. > > > sba_init is called during system initialization stages through subsys_initcall, > > so no extra protection for it. > > OK, I see your reasoning. But I don't agree :) All the users of an > interface should use the same locking scheme, even if they're at > boot-time where we "know" we don't need it. It's too hard to analyze > differences, and code gets copied from one place to somewhere else > where it might not be appropriate. > > Bjorn > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- 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
