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

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

On Thu, 2018-08-16 at 12:26 +0200, Lukas Wunner wrote:
> On Thu, Aug 16, 2018 at 08:10:28PM +1000, Benjamin Herrenschmidt wrote:
> > On Thu, 2018-08-16 at 14:52 +0530, Hari Vyas wrote:
> > > There was an issue reported by my colleague srinath while enabling pci
> > > bridge and a race condition was happening while setting memory and
> > > master bits i.e. bits were over-written.
> > > As per my understanding is_busmaster and is_added bit race issue was
> > > at internal data management and is quite different from pci bridge
> > > enabling issue.
> > > Am I missing some thing ? Would be interested to know what exactly was
> > > affected due to is_busmaster fix.
> > 
> > The is_busmaster fix isn't I think affecting anything, however I don't
> > like the use of atomics for these things. It's a band-aid. If we grow a
> > proper pci_dev mutex, which is what I'm introducing here, it should be
> > able to also handle the is_added race etc..
> 
> 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.

Cheers,
Ben.