Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1231900 > unrolled thread

[PATCH 0/3] ACPI / EC: Fix several code coverity issues

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] ACPI / EC: Fix several code coverity issues Lv Zheng <lv.zheng@intel.com> - 2015-09-24 09:00 +0200
    [PATCH 2/3] ACPI / EC: Fix query handler related issues Lv Zheng <lv.zheng@intel.com> - 2015-09-24 09:00 +0200
    [PATCH 1/3] ACPI / EC: Fix a memory leak issue in acpi_ec_query() Lv Zheng <lv.zheng@intel.com> - 2015-09-24 09:00 +0200
    Re: [PATCH 0/3] ACPI / EC: Fix several code coverity issues "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-10-05 22:20 +0200

#1231900 — [PATCH 0/3] ACPI / EC: Fix several code coverity issues

This patch fixes several code coverity issues in the EC driver.
One serious issue that can cause memory leak is marked as stable material.

Lv Zheng (3):
  ACPI / EC: Fix a memory leak issue in acpi_ec_query()
  ACPI / EC: Fix query handler related issues
  ACPI / EC: Fix a race issue in acpi_ec_guard_event()

 drivers/acpi/ec.c |  113 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 74 insertions(+), 39 deletions(-)

-- 
1.7.10

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

[toc] | [next] | [standalone]

#1231902 — [PATCH 2/3] ACPI / EC: Fix query handler related issues

1. acpi_ec_remove_query_handlers()
This patch refines the query handler removal logic implemented in
acpi_ec_remove_query_handler(), making it to invoke new
acpi_ec_remove_query_handlers() API, and ensuring all other removal code
paths to invoke the new API to honor the reference count of the query
handlers.

2. acpi_ec_get_query_handler_by_value()
This patch also refines the query handler search logic originally
implemented in acpi_ec_query(), collecting it into
acpi_ec_get_query_handler_by_value(). And since schedule_work() can ensure
the serilization of acpi_ec_event_handler(), we needn't put the
mutex_lock() around schedule_work().

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   73 +++++++++++++++++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 28 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 42c66b6..ddaaa9d 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -929,6 +929,23 @@ acpi_ec_get_query_handler(struct acpi_ec_query_handler *handler)
 	return handler;
 }
 
+static struct acpi_ec_query_handler *
+acpi_ec_get_query_handler_by_value(struct acpi_ec *ec, u8 value)
+{
+	struct acpi_ec_query_handler *handler;
+	bool found = false;
+
+	mutex_lock(&ec->mutex);
+	list_for_each_entry(handler, &ec->list, node) {
+		if (value == handler->query_bit) {
+			found = true;
+			break;
+		}
+	}
+	mutex_unlock(&ec->mutex);
+	return found ? acpi_ec_get_query_handler(handler) : NULL;
+}
+
 static void acpi_ec_query_handler_release(struct kref *kref)
 {
 	struct acpi_ec_query_handler *handler =
@@ -964,14 +981,15 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 }
 EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler);
 
-void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
+static void acpi_ec_remove_query_handlers(struct acpi_ec *ec,
+					  bool remove_all, u8 query_bit)
 {
 	struct acpi_ec_query_handler *handler, *tmp;
 	LIST_HEAD(free_list);
 
 	mutex_lock(&ec->mutex);
 	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
-		if (query_bit == handler->query_bit) {
+		if (remove_all || query_bit == handler->query_bit) {
 			list_del_init(&handler->node);
 			list_add(&handler->node, &free_list);
 		}
@@ -980,6 +998,11 @@ void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 	list_for_each_entry_safe(handler, tmp, &free_list, node)
 		acpi_ec_put_query_handler(handler);
 }
+
+void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
+{
+	acpi_ec_remove_query_handlers(ec, false, query_bit);
+}
 EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
 
 static struct acpi_ec_query *acpi_ec_create_query(u8 *pval)
@@ -1025,7 +1048,6 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 {
 	u8 value = 0;
 	int result;
-	struct acpi_ec_query_handler *handler;
 	struct acpi_ec_query *q;
 
 	q = acpi_ec_create_query(&value);
@@ -1043,25 +1065,26 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 	if (result)
 		goto err_exit;
 
-	mutex_lock(&ec->mutex);
-	result = -ENODATA;
-	list_for_each_entry(handler, &ec->list, node) {
-		if (value == handler->query_bit) {
-			result = 0;
-			q->handler = acpi_ec_get_query_handler(handler);
-			ec_dbg_evt("Query(0x%02x) scheduled",
-				   q->handler->query_bit);
-			/*
-			 * It is reported that _Qxx are evaluated in a
-			 * parallel way on Windows:
-			 * https://bugzilla.kernel.org/show_bug.cgi?id=94411
-			 */
-			if (!schedule_work(&q->work))
-				result = -EBUSY;
-			break;
-		}
+	q->handler = acpi_ec_get_query_handler_by_value(ec, value);
+	if (!q->handler) {
+		result = -ENODATA;
+		goto err_exit;
+	}
+
+	/*
+	 * It is reported that _Qxx are evaluated in a parallel way on
+	 * Windows:
+	 * https://bugzilla.kernel.org/show_bug.cgi?id=94411
+	 *
+	 * Put this log entry before schedule_work() in order to make
+	 * it appearing before any other log entries occurred during the
+	 * work queue execution.
+	 */
+	ec_dbg_evt("Query(0x%02x) scheduled", value);
+	if (!schedule_work(&q->work)) {
+		ec_dbg_evt("Query(0x%02x) overlapped", value);
+		result = -EBUSY;
 	}
-	mutex_unlock(&ec->mutex);
 
 err_exit:
 	if (result && q)
@@ -1354,19 +1377,13 @@ static int acpi_ec_add(struct acpi_device *device)
 static int acpi_ec_remove(struct acpi_device *device)
 {
 	struct acpi_ec *ec;
-	struct acpi_ec_query_handler *handler, *tmp;
 
 	if (!device)
 		return -EINVAL;
 
 	ec = acpi_driver_data(device);
 	ec_remove_handlers(ec);
-	mutex_lock(&ec->mutex);
-	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
-		list_del(&handler->node);
-		kfree(handler);
-	}
-	mutex_unlock(&ec->mutex);
+	acpi_ec_remove_query_handlers(ec, true, 0);
 	release_region(ec->data_addr, 1);
 	release_region(ec->command_addr, 1);
 	device->driver_data = NULL;
-- 
1.7.10

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

[toc] | [prev] | [next] | [standalone]

#1231904 — [PATCH 1/3] ACPI / EC: Fix a memory leak issue in acpi_ec_query()

When query handler is not found, "result" is actually stil 0, and
"struct acpi_ec_query" is not NULL, so the deletion code of
"struct acpi_ec_query" at the end of the function cannot be invoked.
As a consequence, memory leak can be observed.

The issue is introduced by this commit:
  Commit: 02b771b64b73226052d6e731a0987db3b47281e9
  Subject: ACPI / EC: Fix an issue caused by the serialized _Qxx

This patch fixes such memory leakage.

Cc: stable@vger.kernel.org  # 4.3.1+
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 2614a83..42c66b6 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1044,8 +1044,10 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 		goto err_exit;
 
 	mutex_lock(&ec->mutex);
+	result = -ENODATA;
 	list_for_each_entry(handler, &ec->list, node) {
 		if (value == handler->query_bit) {
+			result = 0;
 			q->handler = acpi_ec_get_query_handler(handler);
 			ec_dbg_evt("Query(0x%02x) scheduled",
 				   q->handler->query_bit);
-- 
1.7.10

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

[toc] | [prev] | [next] | [standalone]

#1239926

On Thursday, September 24, 2015 02:54:26 PM Lv Zheng wrote:
> This patch fixes several code coverity issues in the EC driver.
> One serious issue that can cause memory leak is marked as stable material.
> 
> Lv Zheng (3):
>   ACPI / EC: Fix a memory leak issue in acpi_ec_query()
>   ACPI / EC: Fix query handler related issues
>   ACPI / EC: Fix a race issue in acpi_ec_guard_event()
> 
>  drivers/acpi/ec.c |  113 +++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 74 insertions(+), 39 deletions(-)

Series applied, thanks!

Rafael

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web