diff mbox

PCI/ACPI: fix wrong ref count handling in acpi_pci_bind()

Message ID alpine.LFD.2.00.0905271756060.25587@localhost.localdomain (mailing list archive)
State Accepted
Headers show

Commit Message

Len Brown May 27, 2009, 9:56 p.m. UTC
From dacd2549ca61ddbdd1ed62a76ca95dea3f0e02c6 Mon Sep 17 00:00:00 2001
From: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Date: Tue, 26 May 2009 09:08:03 +0900
Subject: [PATCH] PCI/ACPI: fix wrong ref count handling in acpi_pci_bind()

The 'dev' field of struct acpi_pci_data is having a pointer to struct
pci_dev without incrementing the reference counter. Because of this, I
got the following kernel oops when I was doing some pci hotplug
operations. This patch fixes this bug by replacing wrong hand-made
pci_find_slot() with pci_get_slot() in acpi_pci_bind().

 BUG: unable to handle kernel NULL pointer dereference at 00000000000000e8
 IP: [<ffffffff803f0e9b>] acpi_pci_unbind+0xb1/0xdd

 Call Trace:
  [<ffffffff803ecee4>] acpi_bus_remove+0x54/0x68
  [<ffffffff803ecf6d>] acpi_bus_trim+0x75/0xe3
  [<ffffffffa0345ddd>] acpiphp_disable_slot+0x16d/0x1e0 [acpiphp]
  [<ffffffffa03441f0>] disable_slot+0x20/0x60 [acpiphp]
  [<ffffffff803cfc18>] power_write_file+0xc8/0x110
  [<ffffffff803c6a54>] pci_slot_attr_store+0x24/0x30
  [<ffffffff803469ce>] sysfs_write_file+0xce/0x140
  [<ffffffff802e94e7>] vfs_write+0xc7/0x170
  [<ffffffff802e9aa0>] sys_write+0x50/0x90
  [<ffffffff8020bd6b>] system_call_fastpath+0x16/0x1b

Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Reviewed-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
Reviewed-by: Alex Chiang <achiang@hp.com>
Tested-by: Alex Chiang <achiang@hp.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---

as applied to the acpi tree...

cheers,
-Len

 drivers/acpi/pci_bind.c |   24 ++++++------------------
 1 files changed, 6 insertions(+), 18 deletions(-)

Comments

Kenji Kaneshige May 28, 2009, 12:09 a.m. UTC | #1
Thank you for updating the patch.
It looks good to me.

Thanks,
Kenji Kaneshige


Len Brown wrote:
>>From dacd2549ca61ddbdd1ed62a76ca95dea3f0e02c6 Mon Sep 17 00:00:00 2001
> From: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> Date: Tue, 26 May 2009 09:08:03 +0900
> Subject: [PATCH] PCI/ACPI: fix wrong ref count handling in acpi_pci_bind()
> 
> The 'dev' field of struct acpi_pci_data is having a pointer to struct
> pci_dev without incrementing the reference counter. Because of this, I
> got the following kernel oops when I was doing some pci hotplug
> operations. This patch fixes this bug by replacing wrong hand-made
> pci_find_slot() with pci_get_slot() in acpi_pci_bind().
> 
>  BUG: unable to handle kernel NULL pointer dereference at 00000000000000e8
>  IP: [<ffffffff803f0e9b>] acpi_pci_unbind+0xb1/0xdd
> 
>  Call Trace:
>   [<ffffffff803ecee4>] acpi_bus_remove+0x54/0x68
>   [<ffffffff803ecf6d>] acpi_bus_trim+0x75/0xe3
>   [<ffffffffa0345ddd>] acpiphp_disable_slot+0x16d/0x1e0 [acpiphp]
>   [<ffffffffa03441f0>] disable_slot+0x20/0x60 [acpiphp]
>   [<ffffffff803cfc18>] power_write_file+0xc8/0x110
>   [<ffffffff803c6a54>] pci_slot_attr_store+0x24/0x30
>   [<ffffffff803469ce>] sysfs_write_file+0xce/0x140
>   [<ffffffff802e94e7>] vfs_write+0xc7/0x170
>   [<ffffffff802e9aa0>] sys_write+0x50/0x90
>   [<ffffffff8020bd6b>] system_call_fastpath+0x16/0x1b
> 
> Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> Reviewed-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> Reviewed-by: Alex Chiang <achiang@hp.com>
> Tested-by: Alex Chiang <achiang@hp.com>
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
> 
> as applied to the acpi tree...
> 
> cheers,
> -Len
> 
>  drivers/acpi/pci_bind.c |   24 ++++++------------------
>  1 files changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/acpi/pci_bind.c b/drivers/acpi/pci_bind.c
> index 95650f8..bc46de3 100644
> --- a/drivers/acpi/pci_bind.c
> +++ b/drivers/acpi/pci_bind.c
> @@ -116,9 +116,6 @@ int acpi_pci_bind(struct acpi_device *device)
>  	struct acpi_pci_data *pdata;
>  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>  	acpi_handle handle;
> -	struct pci_dev *dev;
> -	struct pci_bus *bus;
> -
>  
>  	if (!device || !device->parent)
>  		return -EINVAL;
> @@ -176,20 +173,9 @@ int acpi_pci_bind(struct acpi_device *device)
>  	 * Locate matching device in PCI namespace.  If it doesn't exist
>  	 * this typically means that the device isn't currently inserted
>  	 * (e.g. docking station, port replicator, etc.).
> -	 * We cannot simply search the global pci device list, since
> -	 * PCI devices are added to the global pci list when the root
> -	 * bridge start ops are run, which may not have happened yet.
>  	 */
> -	bus = pci_find_bus(data->id.segment, data->id.bus);
> -	if (bus) {
> -		list_for_each_entry(dev, &bus->devices, bus_list) {
> -			if (dev->devfn == PCI_DEVFN(data->id.device,
> -						    data->id.function)) {
> -				data->dev = dev;
> -				break;
> -			}
> -		}
> -	}
> +	data->dev = pci_get_slot(pdata->bus,
> +				PCI_DEVFN(data->id.device, data->id.function));
>  	if (!data->dev) {
>  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>  				  "Device %04x:%02x:%02x.%d not present in PCI namespace\n",
> @@ -259,9 +245,10 @@ int acpi_pci_bind(struct acpi_device *device)
>  
>        end:
>  	kfree(buffer.pointer);
> -	if (result)
> +	if (result) {
> +		pci_dev_put(data->dev);
>  		kfree(data);
> -
> +	}
>  	return result;
>  }
>  
> @@ -303,6 +290,7 @@ static int acpi_pci_unbind(struct acpi_device *device)
>  	if (data->dev->subordinate) {
>  		acpi_pci_irq_del_prt(data->id.segment, data->bus->number);
>  	}
> +	pci_dev_put(data->dev);
>  	kfree(data);
>  
>        end:


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/acpi/pci_bind.c b/drivers/acpi/pci_bind.c
index 95650f8..bc46de3 100644
--- a/drivers/acpi/pci_bind.c
+++ b/drivers/acpi/pci_bind.c
@@ -116,9 +116,6 @@  int acpi_pci_bind(struct acpi_device *device)
 	struct acpi_pci_data *pdata;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	acpi_handle handle;
-	struct pci_dev *dev;
-	struct pci_bus *bus;
-
 
 	if (!device || !device->parent)
 		return -EINVAL;
@@ -176,20 +173,9 @@  int acpi_pci_bind(struct acpi_device *device)
 	 * Locate matching device in PCI namespace.  If it doesn't exist
 	 * this typically means that the device isn't currently inserted
 	 * (e.g. docking station, port replicator, etc.).
-	 * We cannot simply search the global pci device list, since
-	 * PCI devices are added to the global pci list when the root
-	 * bridge start ops are run, which may not have happened yet.
 	 */
-	bus = pci_find_bus(data->id.segment, data->id.bus);
-	if (bus) {
-		list_for_each_entry(dev, &bus->devices, bus_list) {
-			if (dev->devfn == PCI_DEVFN(data->id.device,
-						    data->id.function)) {
-				data->dev = dev;
-				break;
-			}
-		}
-	}
+	data->dev = pci_get_slot(pdata->bus,
+				PCI_DEVFN(data->id.device, data->id.function));
 	if (!data->dev) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "Device %04x:%02x:%02x.%d not present in PCI namespace\n",
@@ -259,9 +245,10 @@  int acpi_pci_bind(struct acpi_device *device)
 
       end:
 	kfree(buffer.pointer);
-	if (result)
+	if (result) {
+		pci_dev_put(data->dev);
 		kfree(data);
-
+	}
 	return result;
 }
 
@@ -303,6 +290,7 @@  static int acpi_pci_unbind(struct acpi_device *device)
 	if (data->dev->subordinate) {
 		acpi_pci_irq_del_prt(data->id.segment, data->bus->number);
 	}
+	pci_dev_put(data->dev);
 	kfree(data);
 
       end: