Message-ID: <CABawtvO91R0wBL5XZf6xM8Rmray50Fjc74=iA9Y+XdeD7FsNwg@mail.gmail.com>
Date:	Mon, 9 Dec 2013 23:24:04 +0800
From:	Ethan Zhao <ethan.kernel@...il.com>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	Yinghai Lu <yinghai@...nel.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Gu Zheng <guz.fnst@...fujitsu.com>,
	Guo Chao <yan@...ux.vnet.ibm.com>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Myron Stowe <myron.stowe@...il.com>
Subject: Re: [PATCH v2 04/10] PCI: Destroy pci dev only once

On Sun, Dec 8, 2013 at 11:50 AM, Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
> On Sat, Dec 07, 2013 at 07:31:21PM -0800, Yinghai Lu wrote:
>> [+ GregKH]
>>
>> On Fri, Dec 6, 2013 at 5:27 PM, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
>> > On Thursday, December 05, 2013 10:52:36 PM Yinghai Lu wrote:
>> >> On Mon, Dec 2, 2013 at 6:49 AM, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
>> >> >
>> >> > Scenario 5: pci_stop_and_remove_bus_device() is run concurrently
>> >> >   for a device and its parent bridge via remove_callback().
>> >> >
>> >> >   In that case both code paths attempt to acquire
>> >> >   pci_remove_rescan_mutex.  If the child device removal acquires
>> >> >   it first, there will be no problems.  However, if the parent
>> >> >   bridge removal acquires it first, it will eventually execute
>> >> >   pci_destroy_dev() for the child device, but that device will
>> >> >   not be freed yet due to the reference held by the concurrent
>> >> >   child removal.  Consequently, both pci_stop_bus_device() and
>> >> >   pci_remove_bus_device() will be executed for that device
>> >> >   unnecessarily and pci_destroy_dev() will see a corrupted list
>> >> >   head in that object.  Moreover, an excess put_device() will
>> >> >   be executed for that device in that case which may lead to a
>> >> >   use-after-free in the final kobject_put() done by
>> >> >   sysfs_schedule_callback_work().
>> >> >
>> >> > Index: linux-pm/include/linux/pci.h
>> >> > ===================================================================
>> >> > --- linux-pm.orig/include/linux/pci.h
>> >> > +++ linux-pm/include/linux/pci.h
>> >> > @@ -321,6 +321,7 @@ struct pci_dev {
>> >> >         unsigned int    multifunction:1;/* Part of multi-function device */
>> >> >         /* keep track of device state */
>> >> >         unsigned int    is_added:1;
>> >> > +       unsigned int    is_gone:1;
>> >> >         unsigned int    is_busmaster:1; /* device is busmaster */
>> >> >         unsigned int    no_msi:1;       /* device may not use msi */
>> >> >         unsigned int    block_cfg_access:1;     /* config space access is blocked */
>> >> > Index: linux-pm/drivers/pci/remove.c
>> >> > ===================================================================
>> >> > --- linux-pm.orig/drivers/pci/remove.c
>> >> > +++ linux-pm/drivers/pci/remove.c
>> >> > @@ -34,6 +34,7 @@ static void pci_stop_dev(struct pci_dev
>> >> >
>> >> >  static void pci_destroy_dev(struct pci_dev *dev)
>> >> >  {
>> >> > +       dev->is_gone = 1;
>> >> >         device_del(&dev->dev);
>> >> >
>> >> >         down_write(&pci_bus_sem);
>> >> > @@ -109,8 +110,10 @@ static void pci_remove_bus_device(struct
>> >> >   */
>> >> >  void pci_stop_and_remove_bus_device(struct pci_dev *dev)
>> >> >  {
>> >> > -       pci_stop_bus_device(dev);
>> >> > -       pci_remove_bus_device(dev);
>> >> > +       if (!dev->is_gone) {
>> >> > +               pci_stop_bus_device(dev);
>> >> > +               pci_remove_bus_device(dev);
>> >> > +       }
>> >> >  }
>> >> >  EXPORT_SYMBOL(pci_stop_and_remove_bus_device);
>> >> >
>> >>
>> >> Yes, above change should address sys double remove problem.
>> >
>> > I've just realized that we don't need a new flag for that, though.
>> >
>> > It looks like we only need to check dev->dev.kobj.parent and return if that is
>> > NULL, because that means pci_destroy_dev() has run for that device already
>> > (I'm wondering why device_del() doesn't clear dev->parent, BTW, it looks like
>> > it should do that?).
>> >
>> > Of course, that still is going to be racy if we don't hold
>> > pci_remove_rescan_mutex around pci_stop_and_remove_bus_device() in every code
>> > path using it (or use another similar synchronization mechanism).
>>
>> Wonder if we can have safe way to check if device_del() is called already.
>
> Nope.
>
>> And those access_after_free should be addressed by driver core instead
>> of pci code?
>
> Nope, it's up to the bus to handle this.  It shouldn't be hard, you
> shouldn't actually care about this, if you do, something is wrong.
>
> How is this PCI code so hard to get right?  Look at USB for devices that
> disappear from anywhere at anytime as an example for how to handle
> this.  PCI should be doing the same thing, no need for this "is_gone"
> stuff.
Greg,

  Don't agree USB is a good example to follow, do you never hit panic
when you pull out USB device from anywhere at anytime without unmount
or stop it via command ? that is not truth.  the truth is none regards
it as enterprise level interface to attach devices.
  Is there a feature for an USB disk to tell the host you want to pull
out it and should sync all the data in cache and unmount the files
system then power it off ?
  What USB could drive for us ? 40GB nic ? infiniband ? High end graphic card ?

Thanks,
Ethan
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists