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

Groups > linux.kernel > #1659372 > unrolled thread

[PATCH v5] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v5] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently Lv Zheng <lv.zheng@intel.com> - 2017-06-07 07:00 +0200
    Re: [PATCH v5] ACPICA: Tables: Add mechanism to allow to balance late  stage acpi_get_table() independently Dan Williams <dan.j.williams@intel.com> - 2017-06-07 08:50 +0200

#1659372 — [PATCH v5] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently

Considering this case:
1. A program opens a sysfs table file 65535 times, it can increase
   validation_count and first increment cause the table to be mapped:
    validation_count = 65535
2. AML execution causes "Load" to be executed on the same table, this time
   it cannot increase validation_count, so validation_count remains:
    validation_count = 65535
3. The program closes sysfs table file 65535 times, it can decrease
   validation_count and the last decrement cause the table to be unmapped:
    validation_count = 0
4. AML code still accessing the loaded table, kernel crash can be observed.

This is because orginally ACPICA doesn't support unmapping tables during
OS late stage. So the current code only allows unmapping tables during OS
early stage, and for late stage, no acpi_put_table() clones should be
invoked, especially cases that can trigger frequent invocations of
acpi_get_table()/acpi_put_table() are forbidden:
1. sysfs table accesses
2. dynamic Load/Unload opcode executions
3. acpi_load_table()
4. etc.
Such frequent acpi_put_table() balance changes have to be done altogether.

This philosophy is not convenient for Linux driver writers. Since the API
is just there, developers will start to use acpi_put_table() during late
stage. So we need to consider a better mechanism to allow them to safely
invoke acpi_put_table().

This patch provides such a mechanism by adding a validation_count
threashold. When it is reached, the validation_count can no longer be
incremented/decremented to invalidate the table descriptor (means
preventing table unmappings) so that acpi_put_table() balance changes can be
done independently to each others.

Note: code added in acpi_tb_put_table() is actually a no-op but changes the
warning message into a warning once message. Lv Zheng.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/acpica/tbutils.c | 36 +++++++++++++++++++++++++++---------
 include/acpi/actbl.h          | 13 +++++++++++++
 2 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
index cd96026..4048523 100644
--- a/drivers/acpi/acpica/tbutils.c
+++ b/drivers/acpi/acpica/tbutils.c
@@ -416,9 +416,19 @@ acpi_tb_get_table(struct acpi_table_desc *table_desc,
 		}
 	}
 
-	table_desc->validation_count++;
-	if (table_desc->validation_count == 0) {
-		table_desc->validation_count--;
+	if (table_desc->validation_count < ACPI_MAX_TABLE_VALIDATIONS) {
+		table_desc->validation_count++;
+
+		/*
+		 * Ensure the warning message can only be displayed once. The
+		 * warning message occurs when the "get" operations are performed
+		 * more than "put" operations, causing count overflow.
+		 */
+		if (table_desc->validation_count >= ACPI_MAX_TABLE_VALIDATIONS) {
+			ACPI_WARNING((AE_INFO,
+				      "Table %p, Validation count overflows\n",
+				      table_desc));
+		}
 	}
 
 	*out_table = table_desc->pointer;
@@ -445,13 +455,21 @@ void acpi_tb_put_table(struct acpi_table_desc *table_desc)
 
 	ACPI_FUNCTION_TRACE(acpi_tb_put_table);
 
-	if (table_desc->validation_count == 0) {
-		ACPI_WARNING((AE_INFO,
-			      "Table %p, Validation count is zero before decrement\n",
-			      table_desc));
-		return_VOID;
+	if (table_desc->validation_count < ACPI_MAX_TABLE_VALIDATIONS) {
+		table_desc->validation_count--;
+
+		/*
+		 * Ensure the warning message can only be displayed once. The
+		 * warning message occurs when the "put" operations are performed
+		 * more than "get" operations, causing count underflow.
+		 */
+		if (table_desc->validation_count >= ACPI_MAX_TABLE_VALIDATIONS) {
+			ACPI_WARNING((AE_INFO,
+				      "Table %p, Validation count underflows\n",
+				      table_desc));
+			return_VOID;
+		}
 	}
-	table_desc->validation_count--;
 
 	if (table_desc->validation_count == 0) {
 
diff --git a/include/acpi/actbl.h b/include/acpi/actbl.h
index d92543f..f42e6d5 100644
--- a/include/acpi/actbl.h
+++ b/include/acpi/actbl.h
@@ -374,6 +374,19 @@ struct acpi_table_desc {
 	u16 validation_count;
 };
 
+/*
+ * Maximum validation count, when it is reached, validation count can no
+ * longer be changed. Which means, the table can no longer be invalidated.
+ * This mechanism is implemented for backward compatibility, where in OS
+ * late stage, old drivers are not facilitated with paired validations and
+ * invalidations.
+ * The maximum validation count can be defined to any value, but should be
+ * greater than the maximum number of OS early stage mapping slots as it
+ * must be ensured that no early stage mappings can be leaked to the late
+ * stage.
+ */
+#define ACPI_MAX_TABLE_VALIDATIONS          ACPI_UINT16_MAX
+
 /* Masks for Flags field above */
 
 #define ACPI_TABLE_ORIGIN_EXTERNAL_VIRTUAL  (0)	/* Virtual address, external maintained */
-- 
2.7.4

[toc] | [next] | [standalone]

#1659410 — Re: [PATCH v5] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently

On Tue, Jun 6, 2017 at 9:54 PM, Lv Zheng <lv.zheng@intel.com> wrote:
> Considering this case:
> 1. A program opens a sysfs table file 65535 times, it can increase
>    validation_count and first increment cause the table to be mapped:
>     validation_count = 65535
> 2. AML execution causes "Load" to be executed on the same table, this time
>    it cannot increase validation_count, so validation_count remains:
>     validation_count = 65535
> 3. The program closes sysfs table file 65535 times, it can decrease
>    validation_count and the last decrement cause the table to be unmapped:
>     validation_count = 0
> 4. AML code still accessing the loaded table, kernel crash can be observed.
>
> This is because orginally ACPICA doesn't support unmapping tables during
> OS late stage. So the current code only allows unmapping tables during OS
> early stage, and for late stage, no acpi_put_table() clones should be
> invoked, especially cases that can trigger frequent invocations of
> acpi_get_table()/acpi_put_table() are forbidden:
> 1. sysfs table accesses
> 2. dynamic Load/Unload opcode executions
> 3. acpi_load_table()
> 4. etc.
> Such frequent acpi_put_table() balance changes have to be done altogether.
>
> This philosophy is not convenient for Linux driver writers. Since the API
> is just there, developers will start to use acpi_put_table() during late
> stage. So we need to consider a better mechanism to allow them to safely
> invoke acpi_put_table().
>
> This patch provides such a mechanism by adding a validation_count
> threashold. When it is reached, the validation_count can no longer be
> incremented/decremented to invalidate the table descriptor (means
> preventing table unmappings) so that acpi_put_table() balance changes can be
> done independently to each others.
>
> Note: code added in acpi_tb_put_table() is actually a no-op but changes the
> warning message into a warning once message. Lv Zheng.
>

This still seems to be unnecessary gymnastics to keep the validation
count around and make it work for random drivers. Which ACPI tables
might be hot removed? If it's only a small handful of tables why not
teach the code that handles those exceptional cases to manage a
dedicated reference count mechanism? That way the other cases can be
left alone and not worry about balancing their references.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web