Re: [PATCH] PCI: Data corruption happening due to race condition

[Lukas Wunner <lukas@wunner.de>]

On Tue, Jun 26, 2018 at 03:47:43PM +0530, Hari Vyas wrote:
> On Mon, Jun 25, 2018 at 4:45 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > On Mon, Jun 25, 2018 at 04:27:37PM +0530, Hari Vyas wrote:
> >>       This issue is happening  with multiple times device removal and
> >> rescan from sysfs. Card is not removed physically.
> >>       Is_added bit is set after device attach which probe nvme driver.
> >> NVMe driver starts one workqueue and that one is calling pci_set_master()
> >> to set is_busmaster bit.
> >>       With multiple times device removal and rescan from sysfs,  race
> >> condition is observed and is_added bit is over-written to 0 from workqueue
> >> started by NVMe driver.
> >
> > Could you add a dump_stack() to pci_bus_add_device() and pci_stop_dev()
> > where the is_added bit is modified, reproduce the issue and attach the
> > resulting dmesg output to a newly opened bug on bugzilla.kernel.org?
> >
> 
> I have raised a Bug 200283 - PCI: Data corruption happening due to a
> race condition.

Thanks for taking the time to open the bug and provide more detailed
information.

So the upshot seems to be that is_added and is_busmaster end up in
the same word and two CPUs perform a read-modify-write wherein one
CPU clobbers the result of the other CPU.

While a spinlock may do the job, I think a better solution would be
to move is_added to the priv_flags bitmap in struct pci_dev.  The
is_added flag is internal to the PCI core and anything outside has
no business dealing with it.

(Assuming arch/powerpc/kernel/pci-common.c can also be considered
part of the PCI core.)

The flags in priv_flags are defined in drivers/pci/pci.h, so far
there's only one for PCI_DEV_DISCONNECTED which was introduced by
89ee9f768.  That commit also introduced accessors, personally I
don't think that's necessary for the few places in the PCI core
that the new PCI_DEV_ADDED flag would be used and I'd just update
those sites to set or test the bit directly.

Moving the is_added flag should already fix the race with is_busmaster.
It may be worth making is_busmaster a bitmap flag as well, but
priv_flags might not be suitable because the flag is also queried
by various drivers.  I'll defer that decision to Bjorn.

HTH,

Lukas