Re: [PATCH] pci: Fixing drivers/pci/search.c compilation warning.

[Matthew Wilcox]

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 <willy@xxxxxxxxxxxxxxx>

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 <willy@xxxxxxxxxxxxxxx>
> 
> 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."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/