Re: PCIe enable device races (Was: [PATCH v3] PCI: Data corruption happening due to race condition)

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Fri, 2018-08-17 at 09:17 +1000, Benjamin Herrenschmidt wrote:
> 
> > What is your rationale to introduce an additional mutex instead if
> > utilizing the existing mutex in struct device via device_lock() /
> > device_unlock() or alternatively pci_dev_lock() / pci_dev_unlock()?
> > 
> > This is also what Bjorn had suggested here:
> > https://lore.kernel.org/lkml/20170816134354.GV32525@bhelgaas-glaptop.roam.corp.google.com/
> 
> The device_lock() is really meant for the core device model
> (drivers/base/*) internal synchronization. I'm rather weary of
> extending its use to drivers.
> 
> In the specific PCI case, the obvious reason is that probe() is already
> called with that held. Thus we have an immediate deadlock if we try to
> take it in pci_enable_device() for example on the device itself.
> 
> We could require that pci_enable_device() is always called with the
> lock already held, and only take it for the parents when walking the
> bus hierarchy but frankly, this sort of implicit locking requirements
> are a rabbit hole and will lead to all sort of interesting bugs and
> extra driver auditing requirements.

In addition, there are additional uncertainties about whether
the device_lock of the parent is held or not.

Generally this only happens for USB but there are some cases when
the device has busy consumer links where remove() will hold it too.

That would make the walking-up-the-bus for pci_enable_bridge()
rather tricky.

That leads to crazy things like pci_reset_function() vs
pci_reset_function_locked(). Whenever we end up doing that, that's
usually a sign that we didn't think things through.

I think we would benefit greatly from the clarity of having a dedicated
mutex for pci_dev that handles all of our state management. We can
start with the enable_cnt (and as a result stop using atomics) and
is_busmaster, we can extend to is_added thus making Hari's patch
unnecessary, and then start looking at everything else.

We still need the actual device_lock for some things, like the whole
pci_bus_reset() business which we really want to exclude from
concurrent driver binding/unbinding I suppose.

We mostly get away with the lack of locking today because the bulk of
the :1 fields in there tend to be only use either at discovery time
or by the driver init, so in a single threaded manner, but as we saw
already, that assumption breaks on bridges and is generally rather
unsafe.

Cheers,
Ben.