This patch seems to have been overlooked. It also seems to have had
some kind of midair collision with a patch from Greg that ignored the
real bug I found.
Here's an updated version. I think it should also be applied to
-stable.
----
Subject: [PCI] Fix reference counting bug
pci_get_subsys() will decrement the reference count of the device that
it starts searching from. Unfortunately, the pci_find_device() interface
will already have decremented the reference count of the device earlier,
so the device will end up losing all reference counts and be freed.
We can fix this by incrementing the reference count of the device to
start searching from before calling pci_get_subsys().
Signed-off-by: Matthew Wilcox <[email protected]>
diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index 4edfc47..5af8bd5 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -166,6 +166,7 @@ struct pci_dev *pci_find_device(unsigned int vendor, unsigned int device,
{
struct pci_dev *pdev;
+ pci_dev_get(from);
pdev = pci_get_subsys(vendor, device, PCI_ANY_ID, PCI_ANY_ID, from);
pci_dev_put(pdev);
return pdev;
@@ -270,12 +271,8 @@ static struct pci_dev *pci_get_dev_by_id(const struct pci_device_id *id,
struct pci_dev *pdev = NULL;
WARN_ON(in_interrupt());
- if (from) {
- /* FIXME
- * take the cast off, when bus_find_device is made const.
- */
- dev_start = (struct device *)&from->dev;
- }
+ if (from)
+ dev_start = &from->dev;
dev = bus_find_device(&pci_bus_type, dev_start, (void *)id,
match_pci_dev_by_id);
if (dev)
On Sun, Sep 28, 2008 at 10:32:11AM -0600, Matthew Wilcox wrote:
> On Sun, Sep 28, 2008 at 09:16:45PM +0600, Rakib Mullick wrote:
> > drivers/pci/search.c: In function `pci_get_dev_by_id':
> > drivers/pci/search.c:284: warning: passing arg 1 of `pci_dev_put'
> > discards qualifiers from pointer target type
> >
> > The following patch removes the above compilation warning.
> > Thanks.
>
> Yes, but this compilation warning is pointing to a real problem.
> We've told the compiler that the pci_dev is const (ie we won't modify
> it), but pci_dev_put() is most assuredly going to modify and potentially
> can even free the struct pci_dev.
>
> In looking at this, I found another bug in the pci_find_device()
> rewrite. pci_get_subsys() will put the reference to 'from' (if
> non-NULL), but the reference was already put by pci_find_device(),
> so I suspect the reference count ends up going zero very quickly.
> We can fix this by calling pci_dev_get() before calling
> pci_get_subsys(), but then we also have to drop the const from
> pci_find_device()'s argument.
>
> Jesse, how does this patch look? I think it's worth including in -rc
> since it fixes a refcounting bug (admittedly one only triggered by
> drivers using the deprecated pci_find_device() interface).
>
> ---
>
> Subject: [PCI] Fix reference counting bug
>
> pci_get_subsys() will decrement the reference count of the device that
> it starts searching from. Unfortunately, the pci_find_device() interface
> will already have decremented the reference count of the device earlier,
> so the device will end up losing all reference counts and be freed.
>
> We can fix this by incrementing the reference count of the device to
> start searching from before calling pci_get_subsys(). Unfortunately,
> this means we have to lose the 'const' on the arguments of several
> functions.
>
> Signed-off-by: Matthew Wilcox <[email protected]>
>
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index 3b3b5f1..5af8bd5 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -162,10 +162,11 @@ EXPORT_SYMBOL(pci_find_slot);
> * time.
> */
> struct pci_dev *pci_find_device(unsigned int vendor, unsigned int device,
> - const struct pci_dev *from)
> + struct pci_dev *from)
> {
> struct pci_dev *pdev;
>
> + pci_dev_get(from);
> pdev = pci_get_subsys(vendor, device, PCI_ANY_ID, PCI_ANY_ID, from);
> pci_dev_put(pdev);
> return pdev;
> @@ -263,19 +264,15 @@ static int match_pci_dev_by_id(struct device *dev, void *data)
> * this file.
> */
> static struct pci_dev *pci_get_dev_by_id(const struct pci_device_id *id,
> - const struct pci_dev *from)
> + struct pci_dev *from)
> {
> struct device *dev;
> struct device *dev_start = NULL;
> struct pci_dev *pdev = NULL;
>
> WARN_ON(in_interrupt());
> - if (from) {
> - /* FIXME
> - * take the cast off, when bus_find_device is made const.
> - */
> - dev_start = (struct device *)&from->dev;
> - }
> + if (from)
> + dev_start = &from->dev;
> dev = bus_find_device(&pci_bus_type, dev_start, (void *)id,
> match_pci_dev_by_id);
> if (dev)
> @@ -303,7 +300,7 @@ static struct pci_dev *pci_get_dev_by_id(const struct pci_device_id *id,
> */
> struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device,
> unsigned int ss_vendor, unsigned int ss_device,
> - const struct pci_dev *from)
> + struct pci_dev *from)
> {
> struct pci_dev *pdev;
> struct pci_device_id *id;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index a27293a..b8a04c4 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -534,7 +534,7 @@ extern void pci_sort_breadthfirst(void);
> #ifdef CONFIG_PCI_LEGACY
> struct pci_dev __deprecated *pci_find_device(unsigned int vendor,
> unsigned int device,
> - const struct pci_dev *from);
> + struct pci_dev *from);
> struct pci_dev __deprecated *pci_find_slot(unsigned int bus,
> unsigned int devfn);
> #endif /* CONFIG_PCI_LEGACY */
> @@ -550,7 +550,7 @@ struct pci_dev *pci_get_device(unsigned int vendor, unsigned int device,
> struct pci_dev *from);
> struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device,
> unsigned int ss_vendor, unsigned int ss_device,
> - const struct pci_dev *from);
> + struct pci_dev *from);
> struct pci_dev *pci_get_slot(struct pci_bus *bus, unsigned int devfn);
> struct pci_dev *pci_get_bus_and_slot(unsigned int bus, unsigned int devfn);
> struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from);
>
>
>
> --
> Matthew Wilcox Intel Open Source Technology Centre
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours. We can't possibly take such
> a retrograde step."
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
On Monday, October 20, 2008 6:13 pm Matthew Wilcox wrote:
> This patch seems to have been overlooked. It also seems to have had
> some kind of midair collision with a patch from Greg that ignored the
> real bug I found.
>
> Here's an updated version. I think it should also be applied to
> -stable.
Ah I didn't get Cc'd but I should have seen it on linux-pci. Queued up for my
next pull, thanks Matthew.
Jesse
On Monday, October 20, 2008 6:13 pm Matthew Wilcox wrote:
> Subject: [PCI] Fix reference counting bug
>
> pci_get_subsys() will decrement the reference count of the device that
> it starts searching from. Unfortunately, the pci_find_device() interface
> will already have decremented the reference count of the device earlier,
> so the device will end up losing all reference counts and be freed.
>
> We can fix this by incrementing the reference count of the device to
> start searching from before calling pci_get_subsys().
>
> Signed-off-by: Matthew Wilcox <[email protected]>
Applied, thanks Matthew.
Jesse
Matthew Wilcox wrote:
> This patch seems to have been overlooked. It also seems to have had
> some kind of midair collision with a patch from Greg that ignored the
> real bug I found.
>
> Here's an updated version. I think it should also be applied to
> -stable.
>
> ----
>
> Subject: [PCI] Fix reference counting bug
>
> pci_get_subsys() will decrement the reference count of the device that
> it starts searching from. Unfortunately, the pci_find_device() interface
> will already have decremented the reference count of the device earlier,
> so the device will end up losing all reference counts and be freed.
>
> We can fix this by incrementing the reference count of the device to
> start searching from before calling pci_get_subsys().
>
> Signed-off-by: Matthew Wilcox <[email protected]>
>
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index 4edfc47..5af8bd5 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -166,6 +166,7 @@ struct pci_dev *pci_find_device(unsigned int vendor, unsigned int device,
> {
> struct pci_dev *pdev;
>
> + pci_dev_get(from);
> pdev = pci_get_subsys(vendor, device, PCI_ANY_ID, PCI_ANY_ID, from);
> pci_dev_put(pdev);
> return pdev;
> @@ -270,12 +271,8 @@ static struct pci_dev *pci_get_dev_by_id(const struct pci_device_id *id,
> struct pci_dev *pdev = NULL;
>
> WARN_ON(in_interrupt());
> - if (from) {
> - /* FIXME
> - * take the cast off, when bus_find_device is made const.
> - */
> - dev_start = (struct device *)&from->dev;
> - }
> + if (from)
> + dev_start = &from->dev;
> dev = bus_find_device(&pci_bus_type, dev_start, (void *)id,
> match_pci_dev_by_id);
> if (dev)
This reminds me of other problems of PCI search functions.
The 'dev_start' is passed to bus_find_device(), and its 'knode_bus'
reference count is decreased by klist_iter_init_node() in that function.
The problem is the reference count may be already decrease to 0 because
the PCI device 'from' is hot-plugged off (e.g., pci_remove_bus) when the
search goes. A warning is fired when klist_iter_init_node() detects the
reference count becomes 0.
Some code uses pci_find_device() in a way that is not safe with the
hotplug, because a device may be destroyed after bus_find_device()
returns it and before it's held by pci_dev_get() in the next round.
Following is an example from a random grep:
for ( ;; )
{
if ((dev_netjet = pci_find_device(PCI_VENDOR_ID_TIGERJET,
PCI_DEVICE_ID_TIGERJET_300, dev_netjet))) {
ret = njs_pci_probe(dev_netjet, cs);
...
}
...
}
And some others use pci_get_bus_and_slot(), which has similar problem as
above.
Thanks,
Yu
On Sun, Oct 26, 2008 at 08:51:25PM +0800, Yu Zhao wrote:
> This reminds me of other problems of PCI search functions.
>
> The 'dev_start' is passed to bus_find_device(), and its 'knode_bus'
> reference count is decreased by klist_iter_init_node() in that function.
> The problem is the reference count may be already decrease to 0 because
> the PCI device 'from' is hot-plugged off (e.g., pci_remove_bus) when the
> search goes. A warning is fired when klist_iter_init_node() detects the
> reference count becomes 0.
>
> Some code uses pci_find_device() in a way that is not safe with the
> hotplug, because a device may be destroyed after bus_find_device()
> returns it and before it's held by pci_dev_get() in the next round.
> Following is an example from a random grep:
Yes, that's why pci_find_device() is deprecated. But it doesn't also
need to be buggy ;-)
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
Matthew Wilcox wrote:
> On Sun, Oct 26, 2008 at 08:51:25PM +0800, Yu Zhao wrote:
>> This reminds me of other problems of PCI search functions.
>>
>> The 'dev_start' is passed to bus_find_device(), and its 'knode_bus'
>> reference count is decreased by klist_iter_init_node() in that function.
>> The problem is the reference count may be already decrease to 0 because
>> the PCI device 'from' is hot-plugged off (e.g., pci_remove_bus) when the
>> search goes. A warning is fired when klist_iter_init_node() detects the
>> reference count becomes 0.
>>
>> Some code uses pci_find_device() in a way that is not safe with the
>> hotplug, because a device may be destroyed after bus_find_device()
>> returns it and before it's held by pci_dev_get() in the next round.
>> Following is an example from a random grep:
>
> Yes, that's why pci_find_device() is deprecated. But it doesn't also
> need to be buggy ;-)
>
How about pci_get_bus_and_slot()? People would meet the problem with it
anyway.
Thanks,
Yu
On Mon, Oct 27, 2008 at 11:18:43AM +0800, Zhao, Yu wrote:
> Matthew Wilcox wrote:
> >Yes, that's why pci_find_device() is deprecated. But it doesn't also
> >need to be buggy ;-)
>
> How about pci_get_bus_and_slot()? People would meet the problem with it
> anyway.
What problem with it? It's documented to return the device with an
increased refcount, and the implementation appears to do exactly that:
struct pci_dev * pci_get_bus_and_slot(unsigned int bus, unsigned int devfn)
{
struct pci_dev *dev = NULL;
while ((dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev)) != NULL) {
if (pci_domain_nr(dev->bus) == 0 &&
(dev->bus->number == bus && dev->devfn == devfn))
return dev;
}
return NULL;
}
Are you saying some users of it neglect to decrement the refcount before
disposing of the device?
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
Matthew Wilcox wrote:
> On Mon, Oct 27, 2008 at 11:18:43AM +0800, Zhao, Yu wrote:
>> Matthew Wilcox wrote:
>>> Yes, that's why pci_find_device() is deprecated. But it doesn't also
>>> need to be buggy ;-)
>> How about pci_get_bus_and_slot()? People would meet the problem with it
>> anyway.
>
> What problem with it? It's documented to return the device with an
> increased refcount, and the implementation appears to do exactly that:
>
> struct pci_dev * pci_get_bus_and_slot(unsigned int bus, unsigned int devfn)
> {
> struct pci_dev *dev = NULL;
>
> while ((dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev)) != NULL) {
> if (pci_domain_nr(dev->bus) == 0 &&
> (dev->bus->number == bus && dev->devfn == devfn))
> return dev;
> }
> return NULL;
> }
>
> Are you saying some users of it neglect to decrement the refcount before
> disposing of the device?
>
The 'dev' returned by pci_get_device() may be destroyed by PCI hotplug.
I suppose that passing this 'dev' to pci_get_device() in the next loop
would crash the system, right?
On Mon, Oct 27, 2008 at 03:13:51PM +0800, Zhao, Yu wrote:
> Matthew Wilcox wrote:
> >On Mon, Oct 27, 2008 at 11:18:43AM +0800, Zhao, Yu wrote:
> >>Matthew Wilcox wrote:
> >>>Yes, that's why pci_find_device() is deprecated. But it doesn't also
> >>>need to be buggy ;-)
> >>How about pci_get_bus_and_slot()? People would meet the problem with it
> >>anyway.
> >
> >What problem with it? It's documented to return the device with an
> >increased refcount, and the implementation appears to do exactly that:
> >
>
> The 'dev' returned by pci_get_device() may be destroyed by PCI hotplug.
> I suppose that passing this 'dev' to pci_get_device() in the next loop
> would crash the system, right?
Erm, no, the 'dev' cannot be destroyed because the caller has a refcount
on it. The physical device backing it might have gone away. The dev
won't be destroyed until its reference count reaches zero, which could
be any time someone calls pci_dev_put() on it. In the scenario you're
postulating, it would happen in pci_get_dev_by_id():
if (from)
pci_dev_put(from);
which is the last time that 'from' is referred to in that callchain.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
Matthew Wilcox wrote:
> On Mon, Oct 27, 2008 at 03:13:51PM +0800, Zhao, Yu wrote:
>> Matthew Wilcox wrote:
>>> On Mon, Oct 27, 2008 at 11:18:43AM +0800, Zhao, Yu wrote:
>>>> Matthew Wilcox wrote:
>>>>> Yes, that's why pci_find_device() is deprecated. But it doesn't also
>>>>> need to be buggy ;-)
>>>> How about pci_get_bus_and_slot()? People would meet the problem with it
>>>> anyway.
>>> What problem with it? It's documented to return the device with an
>>> increased refcount, and the implementation appears to do exactly that:
>>>
>> The 'dev' returned by pci_get_device() may be destroyed by PCI hotplug.
>> I suppose that passing this 'dev' to pci_get_device() in the next loop
>> would crash the system, right?
>
> Erm, no, the 'dev' cannot be destroyed because the caller has a refcount
> on it. The physical device backing it might have gone away. The dev
Why does the caller have a reference count? I don't see we increase the
reference count after the 'dev' is returned by following in
pci_get_dev_by_id():
dev = bus_find_device(&pci_bus_type, dev_start, (void *)id,
match_pci_dev_by_id);
And this 'dev' becomes the 'from' in the next loop, but it may be
destroyed before the 'pci_dev_get(from)', isn't it?
> won't be destroyed until its reference count reaches zero, which could
> be any time someone calls pci_dev_put() on it. In the scenario you're
> postulating, it would happen in pci_get_dev_by_id():
>
> if (from)
> pci_dev_put(from);
>
> which is the last time that 'from' is referred to in that callchain.
>
Zhao, Yu wrote:
> Matthew Wilcox wrote:
>> On Mon, Oct 27, 2008 at 03:13:51PM +0800, Zhao, Yu wrote:
>>> Matthew Wilcox wrote:
>>>> On Mon, Oct 27, 2008 at 11:18:43AM +0800, Zhao, Yu wrote:
>>>>> Matthew Wilcox wrote:
>>>>>> Yes, that's why pci_find_device() is deprecated. But it doesn't also
>>>>>> need to be buggy ;-)
>>>>> How about pci_get_bus_and_slot()? People would meet the problem with it
>>>>> anyway.
>>>> What problem with it? It's documented to return the device with an
>>>> increased refcount, and the implementation appears to do exactly that:
>>>>
>>> The 'dev' returned by pci_get_device() may be destroyed by PCI hotplug.
>>> I suppose that passing this 'dev' to pci_get_device() in the next loop
>>> would crash the system, right?
>> Erm, no, the 'dev' cannot be destroyed because the caller has a refcount
>> on it. The physical device backing it might have gone away. The dev
>
> Why does the caller have a reference count? I don't see we increase the
> reference count after the 'dev' is returned by following in
> pci_get_dev_by_id():
>
> dev = bus_find_device(&pci_bus_type, dev_start, (void *)id,
> match_pci_dev_by_id);
>
> And this 'dev' becomes the 'from' in the next loop, but it may be
> destroyed before the 'pci_dev_get(from)', isn't it?
I checked the source code, there is no 'pci_dev_get(from)', the
reference count is increased in bus_find_device().
while ((dev = next_device(&i)))
if (match(dev, data) && get_device(dev))
But the essential problem is same: the reference count of 'dev' above
may be decreased before the 'get_device(dev)', I guess.
On Mon, Oct 27, 2008 at 03:34:14PM +0800, Zhao, Yu wrote:
> >Erm, no, the 'dev' cannot be destroyed because the caller has a refcount
> >on it. The physical device backing it might have gone away. The dev
>
> Why does the caller have a reference count? I don't see we increase the
> reference count after the 'dev' is returned by following in
> pci_get_dev_by_id():
>
> dev = bus_find_device(&pci_bus_type, dev_start, (void *)id,
> match_pci_dev_by_id);
>
> And this 'dev' becomes the 'from' in the next loop, but it may be
> destroyed before the 'pci_dev_get(from)', isn't it?
Either you've gone back to talking about pci_find_device() [which, yes, is
vulnerable to race conditions, we know that, it's not interesting to talk
about it], or you're refusing to take it on faith that pci_get_dev_by_id()
returns a device with an increased refcount. And if you don't believe
that is true, please follow the code in bus_find_device() to prove it.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."