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

[Ray Jui <ray.jui@xxxxxxxxxxxx>]

On 6/27/2018 9:32 AM, Hari Vyas wrote:
> On Wed, Jun 27, 2018 at 9:57 PM, Ray Jui <ray.jui@xxxxxxxxxxxx> wrote:
> > Are you re-sending v1 version of this patch or this is actually v2?
> This is actually v2. Forgot to give version in first patch.

You may choose to specify [PATCH v1]: or simply [PATCH]: on your first 
version of the patch, so what you did in the first version was okay.

Now, prefixing the 2nd version of the patch with v1 is just wrong and 
causes confusions. Can you fix that?

In addition, the Reviewed-by filed on me needs to be removed on v2 of 
this patch, since I've never reviewed since you changed it from v1.

> There was spin lock initialization issue with v1 which was not caught
> in our environment.
> In any case, I am trying out suggested priv_flag approach and testing it.
> Hopefully that should be final version.
> > Thanks,
> >
> > Ray
> >
> >
> > On 6/27/2018 2:38 AM, Hari Vyas wrote:
> > > When a pci device is detected, a variable is_added is set to
> > > 1 in pci device structure and proc, sys entries are created.
> > >
> > > When a pci device is removed, first is_added is checked for one
> > > and then device is detached with clearing of proc and sys
> > > entries and at end, is_added is set to 0.
> > >
> > > is_added and is_busmaster are bit fields in pci_dev structure
> > > sharing same memory location.
> > >
> > > A strange issue was observed with multiple times removal and
> > > rescan of a pcie nvme device using sysfs commands where is_added
> > > flag was observed as zero instead of one while removing device
> > > and proc,sys entries are not cleared.  This causes issue in
> > > later device addition with warning message "proc_dir_entry"
> > > already registered.
> > >
> > > Debugging revealed a race condition between pcie core driver
> > > enabling is_added bit(pci_bus_add_device()) and nvme driver
> > > reset work-queue enabling is_busmaster bit (by pci_set_master()).
> > > As both fields are not handled in atomic manner and that clears
> > > is_added bit.
> > >
> > > Fix protects is_added and is_busmaster bits updation by a spin
> > > locking mechanism.
> > >
> > > Signed-off-by: Hari Vyas <hari.vyas@xxxxxxxxxxxx>
> > > Reviewed-by: Ray Jui <ray.jui@xxxxxxxxxxxx>
> > > ---
> > >    drivers/pci/bus.c        | 3 +++
> > >    drivers/pci/pci-driver.c | 2 ++
> > >    drivers/pci/pci.c        | 7 +++++++
> > >    drivers/pci/probe.c      | 1 +
> > >    drivers/pci/remove.c     | 5 +++++
> > >    include/linux/pci.h      | 1 +
> > >    6 files changed, 19 insertions(+)
> > >
> > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > > index 35b7fc8..61d389d 100644
> > > --- a/drivers/pci/bus.c
> > > +++ b/drivers/pci/bus.c
> > > @@ -310,6 +310,7 @@ void __weak pcibios_bus_add_device(struct pci_dev
> > > *pdev) { }
> > >    void pci_bus_add_device(struct pci_dev *dev)
> > >    {
> > >          int retval;
> > > +       unsigned long flags;
> > >          /*
> > >           * Can not put in pci_device_add yet because resources
> > > @@ -330,7 +331,9 @@ void pci_bus_add_device(struct pci_dev *dev)
> > >                  return;
> > >          }
> > >    +     spin_lock_irqsave(&dev->lock, flags);
> > >          dev->is_added = 1;
> > > +       spin_unlock_irqrestore(&dev->lock, flags);
> > >    }
> > >    EXPORT_SYMBOL_GPL(pci_bus_add_device);
> > >    diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > index c125d53..547bcd7 100644
> > > --- a/drivers/pci/pci-driver.c
> > > +++ b/drivers/pci/pci-driver.c
> > > @@ -123,6 +123,8 @@ static ssize_t new_id_store(struct device_driver
> > > *driver, const char *buf,
> > >                  pdev->subsystem_device = subdevice;
> > >                  pdev->class = class;
> > >    +             spin_lock_init(&pdev->lock);
> > > +
> > >                  if (pci_match_id(pdrv->id_table, pdev))
> > >                          retval = -EEXIST;
> > >    diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 97acba7..bcb1f96 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -1649,6 +1649,7 @@ void pci_disable_enabled_device(struct pci_dev *dev)
> > >    void pci_disable_device(struct pci_dev *dev)
> > >    {
> > >          struct pci_devres *dr;
> > > +       unsigned long flags;
> > >          dr = find_pci_dr(dev);
> > >          if (dr)
> > > @@ -1662,7 +1663,9 @@ void pci_disable_device(struct pci_dev *dev)
> > >          do_pci_disable_device(dev);
> > >    +     spin_lock_irqsave(&dev->lock, flags);
> > >          dev->is_busmaster = 0;
> > > +       spin_unlock_irqrestore(&dev->lock, flags);
> > >    }
> > >    EXPORT_SYMBOL(pci_disable_device);
> > >    @@ -3664,6 +3667,7 @@ void __iomem *devm_pci_remap_cfg_resource(struct
> > > device *dev,
> > >    static void __pci_set_master(struct pci_dev *dev, bool enable)
> > >    {
> > >          u16 old_cmd, cmd;
> > > +       unsigned long flags;
> > >          pci_read_config_word(dev, PCI_COMMAND, &old_cmd);
> > >          if (enable)
> > > @@ -3675,7 +3679,10 @@ static void __pci_set_master(struct pci_dev *dev,
> > > bool enable)
> > >                          enable ? "enabling" : "disabling");
> > >                  pci_write_config_word(dev, PCI_COMMAND, cmd);
> > >          }
> > > +
> > > +       spin_lock_irqsave(&dev->lock, flags);
> > >          dev->is_busmaster = enable;
> > > +       spin_unlock_irqrestore(&dev->lock, flags);
> > >    }
> > >      /**
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index ac876e3..9203b88 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -2102,6 +2102,7 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
> > >          INIT_LIST_HEAD(&dev->bus_list);
> > >          dev->dev.type = &pci_dev_type;
> > >          dev->bus = pci_bus_get(bus);
> > > +       spin_lock_init(&dev->lock);
> > >          return dev;
> > >    }
> > > diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> > > index 6f072ea..ff57bd6 100644
> > > --- a/drivers/pci/remove.c
> > > +++ b/drivers/pci/remove.c
> > > @@ -17,13 +17,18 @@ static void pci_free_resources(struct pci_dev *dev)
> > >      static void pci_stop_dev(struct pci_dev *dev)
> > >    {
> > > +       unsigned long flags;
> > > +
> > >          pci_pme_active(dev, false);
> > >          if (dev->is_added) {
> > >                  device_release_driver(&dev->dev);
> > >                  pci_proc_detach_device(dev);
> > >                  pci_remove_sysfs_dev_files(dev);
> > > +
> > > +               spin_lock_irqsave(&dev->lock, flags);
> > >                  dev->is_added = 0;
> > > +               spin_unlock_irqrestore(&dev->lock, flags);
> > >          }
> > >          if (dev->bus->self)
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index 340029b..6940825 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -365,6 +365,7 @@ struct pci_dev {
> > >          bool            match_driver;           /* Skip attaching driver
> > > */
> > >    +     spinlock_t      lock;                   /* Protect is_added and
> > > is_busmaster */
> > >          unsigned int    transparent:1;          /* Subtractive decode
> > > bridge */
> > >          unsigned int    multifunction:1;        /* Multi-function device
> > > */