diff mbox

[RESEND] ACPICA: fix leak of acpi_os_validate_address

Message ID 1250131948.4044.5.camel@minggr.sh.intel.com (mailing list archive)
State RFC, archived
Headers show

Commit Message

Lin Ming Aug. 13, 2009, 2:52 a.m. UTC
From: Lin Ming <ming.m.lin@intel.com>
Date: Thu, 13 Aug 2009 10:43:27 +0800
Subject: [PATCH] ACPICA: fix leak of acpi_os_validate_address

http://bugzilla.kernel.org/show_bug.cgi?id=13620

If the dynamic region is created and added to resource list over and over again,
it has the potential to be a memory leak by growing the list every time.

This patch fixes the memory leak, as below

1) add a new field "count" to struct acpi_res_list.

   When inserting, if the region(addr, len) is already in the resource
   list, we just increase "count", otherwise, the region is inserted
   with count=1.

   When deleting, the "count" is decreased, if it's decreased to 0,
   the region is deleted from the resource list.

   With "count", the region with same address and length can only be
   inserted to the resource list once, so prevent potential memory leak.

2) add a new function acpi_os_invalidate_address, which is called when
   region is deleted.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/acpi/acpica/utdelete.c |    6 +++
 drivers/acpi/osl.c             |   94 ++++++++++++++++++++++++++++++++++++++-
 include/acpi/acpiosxf.h        |    4 ++
 3 files changed, 101 insertions(+), 3 deletions(-)



--
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

Comments

Lin Ming Aug. 13, 2009, 3:25 a.m. UTC | #1
On Thu, 2009-08-13 at 10:52 +0800, Lin Ming wrote:
> From: Lin Ming <ming.m.lin@intel.com>
> Date: Thu, 13 Aug 2009 10:43:27 +0800
> Subject: [PATCH] ACPICA: fix leak of acpi_os_validate_address
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=13620
> 
> If the dynamic region is created and added to resource list over and over again,
> it has the potential to be a memory leak by growing the list every time.
> 
> This patch fixes the memory leak, as below
> 
> 1) add a new field "count" to struct acpi_res_list.
> 
>    When inserting, if the region(addr, len) is already in the resource
>    list, we just increase "count", otherwise, the region is inserted
>    with count=1.
> 
>    When deleting, the "count" is decreased, if it's decreased to 0,
>    the region is deleted from the resource list.
> 
>    With "count", the region with same address and length can only be
>    inserted to the resource list once, so prevent potential memory leak.
> 
> 2) add a new function acpi_os_invalidate_address, which is called when
>    region is deleted.
> 
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>

Forgot to mention that it's applied on top of 2.6.31-rc5.

Lin Ming

--
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
Len Brown Aug. 27, 2009, 5:09 p.m. UTC | #2
it is easier to apply patches when they do not appear multiple times
in the e-mail message...

why does acpi_os_invalidate_address() need a return value other than void?

applied to acpi-test

thanks,
Len Brown, Intel Open Source Technology Center

--
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
Lin Ming Aug. 28, 2009, 1:17 a.m. UTC | #3
On Fri, 2009-08-28 at 01:09 +0800, Len Brown wrote:
> it is easier to apply patches when they do not appear multiple times
> in the e-mail message...
> 
> why does acpi_os_invalidate_address() need a return value other than void?

Just for extension, maybe some future usage.

Lin Ming

> 
> applied to acpi-test
> 
> thanks,
> Len Brown, Intel Open Source Technology Center
> 

--
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/acpica/utdelete.c b/drivers/acpi/acpica/utdelete.c
index bc17103..96e26e7 100644
--- a/drivers/acpi/acpica/utdelete.c
+++ b/drivers/acpi/acpica/utdelete.c
@@ -215,6 +215,12 @@  static void acpi_ut_delete_internal_obj(union acpi_operand_object *object)
 		ACPI_DEBUG_PRINT((ACPI_DB_ALLOCATIONS,
 				  "***** Region %p\n", object));
 
+		/* Invalidate the region address/length via the host OS */
+
+		acpi_os_invalidate_address(object->region.space_id,
+					  object->region.address,
+					  (acpi_size) object->region.length);
+
 		second_desc = acpi_ns_get_secondary_object(object);
 		if (second_desc) {
 			/*
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 7167071..348a846 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -88,6 +88,7 @@  struct acpi_res_list {
 	char name[5];   /* only can have a length of 4 chars, make use of this
 			   one instead of res->name, no need to kalloc then */
 	struct list_head resource_list;
+	int count;
 };
 
 static LIST_HEAD(resource_list_head);
@@ -1333,6 +1334,89 @@  acpi_os_validate_interface (char *interface)
 	return AE_SUPPORT;
 }
 
+static inline int acpi_res_list_add(struct acpi_res_list *res)
+{
+	struct acpi_res_list *res_list_elem;
+
+	list_for_each_entry(res_list_elem, &resource_list_head,
+			    resource_list) {
+
+		if (res->resource_type == res_list_elem->resource_type &&
+		    res->start == res_list_elem->start &&
+		    res->end == res_list_elem->end) {
+
+			/*
+			 * The Region(addr,len) already exist in the list,
+			 * just increase the count
+			 */
+
+			res_list_elem->count++;
+			return 0;
+		}
+	}
+
+	res->count = 1;
+	list_add(&res->resource_list, &resource_list_head);
+	return 1;
+}
+
+static inline void acpi_res_list_del(struct acpi_res_list *res)
+{
+	struct acpi_res_list *res_list_elem;
+
+	list_for_each_entry(res_list_elem, &resource_list_head,
+			    resource_list) {
+
+		if (res->resource_type == res_list_elem->resource_type &&
+		    res->start == res_list_elem->start &&
+		    res->end == res_list_elem->end) {
+
+			/*
+			 * If the res count is decreased to 0,
+			 * remove and free it
+			 */
+
+			if (--res_list_elem->count == 0) {
+				list_del(&res_list_elem->resource_list);
+				kfree(res_list_elem);
+			}
+			return;
+		}
+	}
+}
+
+acpi_status
+acpi_os_invalidate_address(
+    u8                   space_id,
+    acpi_physical_address   address,
+    acpi_size               length)
+{
+	struct acpi_res_list res;
+
+	switch (space_id) {
+	case ACPI_ADR_SPACE_SYSTEM_IO:
+	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
+		/* Only interference checks against SystemIO and SytemMemory
+		   are needed */
+		res.start = address;
+		res.end = address + length - 1;
+		res.resource_type = space_id;
+		spin_lock(&acpi_res_lock);
+		acpi_res_list_del(&res);
+		spin_unlock(&acpi_res_lock);
+		break;
+	case ACPI_ADR_SPACE_PCI_CONFIG:
+	case ACPI_ADR_SPACE_EC:
+	case ACPI_ADR_SPACE_SMBUS:
+	case ACPI_ADR_SPACE_CMOS:
+	case ACPI_ADR_SPACE_PCI_BAR_TARGET:
+	case ACPI_ADR_SPACE_DATA_TABLE:
+	case ACPI_ADR_SPACE_FIXED_HARDWARE:
+		break;
+	}
+	return AE_OK;
+}
+
 /******************************************************************************
  *
  * FUNCTION:    acpi_os_validate_address
@@ -1357,6 +1441,7 @@  acpi_os_validate_address (
     char *name)
 {
 	struct acpi_res_list *res;
+	int added;
 	if (acpi_enforce_resources == ENFORCE_RESOURCES_NO)
 		return AE_OK;
 
@@ -1374,14 +1459,17 @@  acpi_os_validate_address (
 		res->end = address + length - 1;
 		res->resource_type = space_id;
 		spin_lock(&acpi_res_lock);
-		list_add(&res->resource_list, &resource_list_head);
+		added = acpi_res_list_add(res);
 		spin_unlock(&acpi_res_lock);
-		pr_debug("Added %s resource: start: 0x%llx, end: 0x%llx, "
-			 "name: %s\n", (space_id == ACPI_ADR_SPACE_SYSTEM_IO)
+		pr_debug("%s %s resource: start: 0x%llx, end: 0x%llx, "
+			 "name: %s\n", added ? "Added" : "Already exist",
+			 (space_id == ACPI_ADR_SPACE_SYSTEM_IO)
 			 ? "SystemIO" : "System Memory",
 			 (unsigned long long)res->start,
 			 (unsigned long long)res->end,
 			 res->name);
+		if (!added)
+			kfree(res);
 		break;
 	case ACPI_ADR_SPACE_PCI_CONFIG:
 	case ACPI_ADR_SPACE_EC:
diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
index 3e79859..cb13260 100644
--- a/include/acpi/acpiosxf.h
+++ b/include/acpi/acpiosxf.h
@@ -242,6 +242,10 @@  acpi_os_derive_pci_id(acpi_handle rhandle,
 acpi_status acpi_os_validate_interface(char *interface);
 acpi_status acpi_osi_invalidate(char* interface);
 
+acpi_status
+acpi_os_invalidate_address(u8 space_id, acpi_physical_address address,
+			 acpi_size length);
+
 u64 acpi_os_get_timer(void);
 
 acpi_status acpi_os_signal(u32 function, void *info);