[PATCH 0/2] ACPICA: Fix a race in GenericSerialBus (I2C) and GPIO handling

[PATCH 0/2] ACPICA: Fix a race in GenericSerialBus (I2C) and GPIO handling

[Hans de Goede]

Hi All,

On one of my machines I noticed the following errors being logged:

[   52.892807] i2c i2c-0: adapter quirk: no zero length (addr 0x0078, size 0, read)
[   52.893037] i2c i2c-0: i2c read 0 bytes from client@0x78 starting at reg 0x0 failed, error: -95

The second line is coming from the Linux I2C ACPI OpRegion handling and
after a bunch of debugging I've found out that there is a rather obvious
(once you see it) and nasty race condition in the handling of I2C and GPIO
opregions in acpi_ev_address_space_dispatch(). See the first patch in this
series (the second patch is a follow-up cleanup patch removing some code
duplication).

TBH I'm surprised that this issue has gone unnoticed as long as it has,
but I guess that it mostly leads to unreproducable sporadic problems
making it hard to debug and I got lucky that I had a machine where the
race seems to trigger about once every 20 seconds.

I know that ACPICA patches are normally merged through the ACPICA upstream
but given that this is a serious bug, I believe that in this case it might
be best to add the fix directly to Linux and then port it to ACPICA from
there.

Regards,

Hans

[PATCH 1/2] ACPICA: Fix race in GenericSerialBus (I2C) and GPIO OpRegion parameter handling

[Hans de Goede]

The handling of the GenericSerialBus (I2C) and GPIO OpRegions in
acpi_ev_address_space_dispatch() passes a number of extra parameters to
the address-space handler through the address-space context pointer
(instead of using more function parameters).

The context is shared between threads, so if multiple threads try to call
the handler for the same address-space at the same time, then
a second thread could change the parameters of a first thread while the
handler is running for the first thread.

An example of this race hitting is the Lenovo Yoga Tablet2 1015L, where
there are both AttribBytes accesses and AttribByte accesses to the same
address-space. The AttribBytes access stores the number of bytes to
transfer in context->access_length. Where as for the AttribByte access the
number of bytes to transfer is always 1 and field_obj->field.access_length
is unused (so 0). Both types of accesses racing from different threads
leads to the following problem:

1. Thread a. starts an AttribBytes access, stores a non 0 value
from field_obj->field.access_length in context->access_length
2. Thread b. starts an AttribByte access, stores 0 in
context->access_length
3. Thread a. calls i2c_acpi_space_handler() (under Linux). Which
sees that the access-type is ACPI_GSB_ACCESS_ATTRIB_MULTIBYTE and
calls acpi_gsb_i2c_read_bytes(..., context->access_length)
4. At this point context->access_length is 0 (set by thread b.)
rather then the field_obj->field.access_length value from thread a.
This 0 length reads leads to the following errors being logged:

 i2c i2c-0: adapter quirk: no zero length (addr 0x0078, size 0, read)
 i2c i2c-0: i2c read 0 bytes from client@0x78 starting at reg 0x0 failed, error: -95

Note this is just an example of the problems which this race can cause.
There are likely many more (sporadic) problems caused by this race.

This commit adds a new context_mutex to acpi_object_addr_handler
and makes acpi_ev_address_space_dispatch() take that mutex when
using the shared context to pass extra parameters to an address-space
handler, fixing this race.

Note the new mutex must be taken *after* exiting the interpreter,
therefor the existing acpi_ex_exit_interpreter() call is moved to above
the code which stores the extra parameters in the context.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpica/acobject.h  |  1 +
 drivers/acpi/acpica/evhandler.c |  6 ++++
 drivers/acpi/acpica/evregion.c  | 61 ++++++++++++++++++++++++---------
 drivers/acpi/acpica/evxfregn.c  |  1 +
 4 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/acpica/acobject.h b/drivers/acpi/acpica/acobject.h
index 9f0219a8cb98..dd7efafcb103 100644
--- a/drivers/acpi/acpica/acobject.h
+++ b/drivers/acpi/acpica/acobject.h
@@ -284,6 +284,7 @@ struct acpi_object_addr_handler {
 	acpi_adr_space_handler handler;
 	struct acpi_namespace_node *node;	/* Parent device */
 	void *context;
+	acpi_mutex context_mutex;
 	acpi_adr_space_setup setup;
 	union acpi_operand_object *region_list;	/* Regions using this handler */
 	union acpi_operand_object *next;
diff --git a/drivers/acpi/acpica/evhandler.c b/drivers/acpi/acpica/evhandler.c
index 5884eba047f7..347199f29afe 100644
--- a/drivers/acpi/acpica/evhandler.c
+++ b/drivers/acpi/acpica/evhandler.c
@@ -489,6 +489,12 @@ acpi_ev_install_space_handler(struct acpi_namespace_node *node,
 
 	/* Init handler obj */
 
+	status = acpi_os_create_mutex(&handler_obj->address_space.context_mutex);
+	if (ACPI_FAILURE(status)) {
+		acpi_ut_remove_reference(handler_obj);
+		goto unlock_and_exit;
+	}
+
 	handler_obj->address_space.space_id = (u8)space_id;
 	handler_obj->address_space.handler_flags = flags;
 	handler_obj->address_space.region_list = NULL;
diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c
index 21ff341e34a4..8e84eb0641e0 100644
--- a/drivers/acpi/acpica/evregion.c
+++ b/drivers/acpi/acpica/evregion.c
@@ -112,6 +112,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	union acpi_operand_object *region_obj2;
 	void *region_context = NULL;
 	struct acpi_connection_info *context;
+	acpi_mutex context_mutex;
+	bool context_locked;
 	acpi_physical_address address;
 
 	ACPI_FUNCTION_TRACE(ev_address_space_dispatch);
@@ -136,6 +138,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	}
 
 	context = handler_desc->address_space.context;
+	context_mutex = handler_desc->address_space.context_mutex;
+	context_locked = false;
 
 	/*
 	 * It may be the case that the region has never been initialized.
@@ -204,6 +208,23 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	handler = handler_desc->address_space.handler;
 	address = (region_obj->region.address + region_offset);
 
+	ACPI_DEBUG_PRINT((ACPI_DB_OPREGION,
+			  "Handler %p (@%p) Address %8.8X%8.8X [%s]\n",
+			  &region_obj->region.handler->address_space, handler,
+			  ACPI_FORMAT_UINT64(address),
+			  acpi_ut_get_region_name(region_obj->region.
+						  space_id)));
+
+	if (!(handler_desc->address_space.handler_flags &
+	      ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
+		/*
+		 * For handlers other than the default (supplied) handlers, we must
+		 * exit the interpreter because the handler *might* block -- we don't
+		 * know what it will do, so we can't hold the lock on the interpreter.
+		 */
+		acpi_ex_exit_interpreter();
+	}
+
 	/*
 	 * Special handling for generic_serial_bus and general_purpose_io:
 	 * There are three extra parameters that must be passed to the
@@ -212,6 +233,11 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	 *   2) Length of the above buffer
 	 *   3) Actual access length from the access_as() op
 	 *
+	 * Since we pass these extra parameters via the context, which is
+	 * shared between threads, we must lock the context to avoid these
+	 * parameters being changed from another thread before the handler
+	 * has completed running.
+	 *
 	 * In addition, for general_purpose_io, the Address and bit_width fields
 	 * are defined as follows:
 	 *   1) Address is the pin number index of the field (bit offset from
@@ -221,6 +247,13 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	if ((region_obj->region.space_id == ACPI_ADR_SPACE_GSBUS) &&
 	    context && field_obj) {
 
+		status = acpi_os_acquire_mutex(context_mutex, ACPI_WAIT_FOREVER);
+		if (ACPI_FAILURE(status)) {
+			goto re_enter_interpreter;
+		}
+
+		context_locked = true;
+
 		/* Get the Connection (resource_template) buffer */
 
 		context->connection = field_obj->field.resource_buffer;
@@ -230,6 +263,13 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	if ((region_obj->region.space_id == ACPI_ADR_SPACE_GPIO) &&
 	    context && field_obj) {
 
+		status = acpi_os_acquire_mutex(context_mutex, ACPI_WAIT_FOREVER);
+		if (ACPI_FAILURE(status)) {
+			goto re_enter_interpreter;
+		}
+
+		context_locked = true;
+
 		/* Get the Connection (resource_template) buffer */
 
 		context->connection = field_obj->field.resource_buffer;
@@ -239,28 +279,14 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 		bit_width = field_obj->field.bit_length;
 	}
 
-	ACPI_DEBUG_PRINT((ACPI_DB_OPREGION,
-			  "Handler %p (@%p) Address %8.8X%8.8X [%s]\n",
-			  &region_obj->region.handler->address_space, handler,
-			  ACPI_FORMAT_UINT64(address),
-			  acpi_ut_get_region_name(region_obj->region.
-						  space_id)));
-
-	if (!(handler_desc->address_space.handler_flags &
-	      ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
-		/*
-		 * For handlers other than the default (supplied) handlers, we must
-		 * exit the interpreter because the handler *might* block -- we don't
-		 * know what it will do, so we can't hold the lock on the interpreter.
-		 */
-		acpi_ex_exit_interpreter();
-	}
-
 	/* Call the handler */
 
 	status = handler(function, address, bit_width, value, context,
 			 region_obj2->extra.region_context);
 
+	if (context_locked)
+		acpi_os_release_mutex(context_mutex);
+
 	if (ACPI_FAILURE(status)) {
 		ACPI_EXCEPTION((AE_INFO, status, "Returned by Handler for [%s]",
 				acpi_ut_get_region_name(region_obj->region.
@@ -277,6 +303,7 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 		}
 	}
 
+re_enter_interpreter:
 	if (!(handler_desc->address_space.handler_flags &
 	      ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
 		/*
diff --git a/drivers/acpi/acpica/evxfregn.c b/drivers/acpi/acpica/evxfregn.c
index da97fd0c6b51..74d0ee8a5736 100644
--- a/drivers/acpi/acpica/evxfregn.c
+++ b/drivers/acpi/acpica/evxfregn.c
@@ -201,6 +201,7 @@ acpi_remove_address_space_handler(acpi_handle device,
 
 			/* Now we can delete the handler object */
 
+			acpi_os_release_mutex(handler_obj->address_space.context_mutex);
 			acpi_ut_remove_reference(handler_obj);
 			goto unlock_and_exit;
 		}
-- 
2.28.0

[PATCH 2/2] ACPICA: Remove some code duplication from acpi_ev_address_space_dispatch

[Hans de Goede]

The handling of the space_id == ACPI_ADR_SPACE_GSBUS and
space_id == ACPI_ADR_SPACE_GPIO cases is almost identical,
fold the 2 cases into 1 to remove some code duplication.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpica/evregion.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c
index 8e84eb0641e0..d1ca997c9c4c 100644
--- a/drivers/acpi/acpica/evregion.c
+++ b/drivers/acpi/acpica/evregion.c
@@ -244,7 +244,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	 *      the previous Connection)
 	 *   2) bit_width is the actual bit length of the field (number of pins)
 	 */
-	if ((region_obj->region.space_id == ACPI_ADR_SPACE_GSBUS) &&
+	if ((region_obj->region.space_id == ACPI_ADR_SPACE_GSBUS ||
+	     region_obj->region.space_id == ACPI_ADR_SPACE_GPIO) &&
 	    context && field_obj) {
 
 		status = acpi_os_acquire_mutex(context_mutex, ACPI_WAIT_FOREVER);
@@ -259,24 +260,11 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 		context->connection = field_obj->field.resource_buffer;
 		context->length = field_obj->field.resource_length;
 		context->access_length = field_obj->field.access_length;
-	}
-	if ((region_obj->region.space_id == ACPI_ADR_SPACE_GPIO) &&
-	    context && field_obj) {
 
-		status = acpi_os_acquire_mutex(context_mutex, ACPI_WAIT_FOREVER);
-		if (ACPI_FAILURE(status)) {
-			goto re_enter_interpreter;
+		if (region_obj->region.space_id == ACPI_ADR_SPACE_GPIO) {
+			address = field_obj->field.pin_number_index;
+			bit_width = field_obj->field.bit_length;
 		}
-
-		context_locked = true;
-
-		/* Get the Connection (resource_template) buffer */
-
-		context->connection = field_obj->field.resource_buffer;
-		context->length = field_obj->field.resource_length;
-		context->access_length = field_obj->field.access_length;
-		address = field_obj->field.pin_number_index;
-		bit_width = field_obj->field.bit_length;
 	}
 
 	/* Call the handler */
-- 
2.28.0

RE: [PATCH 1/2] ACPICA: Fix race in GenericSerialBus (I2C) and GPIO OpRegion parameter handling

[Moore, Robert]

Hans,
Could you make a pull request for this (and any related patches) on our github?
Thanks,
Bob


-----Original Message-----
From: Hans de Goede <hdegoede@redhat.com> 
Sent: Saturday, December 26, 2020 6:28 AM
To: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>; Moore, Robert <robert.moore@intel.com>; Kaneda, Erik <erik.kaneda@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>; linux-acpi@vger.kernel.org; devel@acpica.org
Subject: [PATCH 1/2] ACPICA: Fix race in GenericSerialBus (I2C) and GPIO OpRegion parameter handling

The handling of the GenericSerialBus (I2C) and GPIO OpRegions in
acpi_ev_address_space_dispatch() passes a number of extra parameters to the address-space handler through the address-space context pointer (instead of using more function parameters).

The context is shared between threads, so if multiple threads try to call the handler for the same address-space at the same time, then a second thread could change the parameters of a first thread while the handler is running for the first thread.

An example of this race hitting is the Lenovo Yoga Tablet2 1015L, where there are both AttribBytes accesses and AttribByte accesses to the same address-space. The AttribBytes access stores the number of bytes to transfer in context->access_length. Where as for the AttribByte access the number of bytes to transfer is always 1 and field_obj->field.access_length is unused (so 0). Both types of accesses racing from different threads leads to the following problem:

1. Thread a. starts an AttribBytes access, stores a non 0 value from field_obj->field.access_length in context->access_length 2. Thread b. starts an AttribByte access, stores 0 in
context->access_length
3. Thread a. calls i2c_acpi_space_handler() (under Linux). Which sees that the access-type is ACPI_GSB_ACCESS_ATTRIB_MULTIBYTE and calls acpi_gsb_i2c_read_bytes(..., context->access_length) 4. At this point context->access_length is 0 (set by thread b.) rather then the field_obj->field.access_length value from thread a.
This 0 length reads leads to the following errors being logged:

 i2c i2c-0: adapter quirk: no zero length (addr 0x0078, size 0, read)  i2c i2c-0: i2c read 0 bytes from client@0x78 starting at reg 0x0 failed, error: -95

Note this is just an example of the problems which this race can cause.
There are likely many more (sporadic) problems caused by this race.

This commit adds a new context_mutex to acpi_object_addr_handler and makes acpi_ev_address_space_dispatch() take that mutex when using the shared context to pass extra parameters to an address-space handler, fixing this race.

Note the new mutex must be taken *after* exiting the interpreter, therefor the existing acpi_ex_exit_interpreter() call is moved to above the code which stores the extra parameters in the context.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpica/acobject.h  |  1 +
 drivers/acpi/acpica/evhandler.c |  6 ++++  drivers/acpi/acpica/evregion.c  | 61 ++++++++++++++++++++++++---------  drivers/acpi/acpica/evxfregn.c  |  1 +
 4 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/acpica/acobject.h b/drivers/acpi/acpica/acobject.h index 9f0219a8cb98..dd7efafcb103 100644
--- a/drivers/acpi/acpica/acobject.h
+++ b/drivers/acpi/acpica/acobject.h
@@ -284,6 +284,7 @@ struct acpi_object_addr_handler {
 	acpi_adr_space_handler handler;
 	struct acpi_namespace_node *node;	/* Parent device */
 	void *context;
+	acpi_mutex context_mutex;
 	acpi_adr_space_setup setup;
 	union acpi_operand_object *region_list;	/* Regions using this handler */
 	union acpi_operand_object *next;
diff --git a/drivers/acpi/acpica/evhandler.c b/drivers/acpi/acpica/evhandler.c index 5884eba047f7..347199f29afe 100644
--- a/drivers/acpi/acpica/evhandler.c
+++ b/drivers/acpi/acpica/evhandler.c
@@ -489,6 +489,12 @@ acpi_ev_install_space_handler(struct acpi_namespace_node *node,
 
 	/* Init handler obj */
 
+	status = acpi_os_create_mutex(&handler_obj->address_space.context_mutex);
+	if (ACPI_FAILURE(status)) {
+		acpi_ut_remove_reference(handler_obj);
+		goto unlock_and_exit;
+	}
+
 	handler_obj->address_space.space_id = (u8)space_id;
 	handler_obj->address_space.handler_flags = flags;
 	handler_obj->address_space.region_list = NULL; diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c index 21ff341e34a4..8e84eb0641e0 100644
--- a/drivers/acpi/acpica/evregion.c
+++ b/drivers/acpi/acpica/evregion.c
@@ -112,6 +112,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	union acpi_operand_object *region_obj2;
 	void *region_context = NULL;
 	struct acpi_connection_info *context;
+	acpi_mutex context_mutex;
+	bool context_locked;
 	acpi_physical_address address;
 
 	ACPI_FUNCTION_TRACE(ev_address_space_dispatch);
@@ -136,6 +138,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	}
 
 	context = handler_desc->address_space.context;
+	context_mutex = handler_desc->address_space.context_mutex;
+	context_locked = false;
 
 	/*
 	 * It may be the case that the region has never been initialized.
@@ -204,6 +208,23 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	handler = handler_desc->address_space.handler;
 	address = (region_obj->region.address + region_offset);
 
+	ACPI_DEBUG_PRINT((ACPI_DB_OPREGION,
+			  "Handler %p (@%p) Address %8.8X%8.8X [%s]\n",
+			  &region_obj->region.handler->address_space, handler,
+			  ACPI_FORMAT_UINT64(address),
+			  acpi_ut_get_region_name(region_obj->region.
+						  space_id)));
+
+	if (!(handler_desc->address_space.handler_flags &
+	      ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
+		/*
+		 * For handlers other than the default (supplied) handlers, we must
+		 * exit the interpreter because the handler *might* block -- we don't
+		 * know what it will do, so we can't hold the lock on the interpreter.
+		 */
+		acpi_ex_exit_interpreter();
+	}
+
 	/*
 	 * Special handling for generic_serial_bus and general_purpose_io:
 	 * There are three extra parameters that must be passed to the @@ -212,6 +233,11 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	 *   2) Length of the above buffer
 	 *   3) Actual access length from the access_as() op
 	 *
+	 * Since we pass these extra parameters via the context, which is
+	 * shared between threads, we must lock the context to avoid these
+	 * parameters being changed from another thread before the handler
+	 * has completed running.
+	 *
 	 * In addition, for general_purpose_io, the Address and bit_width fields
 	 * are defined as follows:
 	 *   1) Address is the pin number index of the field (bit offset from
@@ -221,6 +247,13 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	if ((region_obj->region.space_id == ACPI_ADR_SPACE_GSBUS) &&
 	    context && field_obj) {
 
+		status = acpi_os_acquire_mutex(context_mutex, ACPI_WAIT_FOREVER);
+		if (ACPI_FAILURE(status)) {
+			goto re_enter_interpreter;
+		}
+
+		context_locked = true;
+
 		/* Get the Connection (resource_template) buffer */
 
 		context->connection = field_obj->field.resource_buffer; @@ -230,6 +263,13 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	if ((region_obj->region.space_id == ACPI_ADR_SPACE_GPIO) &&
 	    context && field_obj) {
 
+		status = acpi_os_acquire_mutex(context_mutex, ACPI_WAIT_FOREVER);
+		if (ACPI_FAILURE(status)) {
+			goto re_enter_interpreter;
+		}
+
+		context_locked = true;
+
 		/* Get the Connection (resource_template) buffer */
 
 		context->connection = field_obj->field.resource_buffer; @@ -239,28 +279,14 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 		bit_width = field_obj->field.bit_length;
 	}
 
-	ACPI_DEBUG_PRINT((ACPI_DB_OPREGION,
-			  "Handler %p (@%p) Address %8.8X%8.8X [%s]\n",
-			  &region_obj->region.handler->address_space, handler,
-			  ACPI_FORMAT_UINT64(address),
-			  acpi_ut_get_region_name(region_obj->region.
-						  space_id)));
-
-	if (!(handler_desc->address_space.handler_flags &
-	      ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
-		/*
-		 * For handlers other than the default (supplied) handlers, we must
-		 * exit the interpreter because the handler *might* block -- we don't
-		 * know what it will do, so we can't hold the lock on the interpreter.
-		 */
-		acpi_ex_exit_interpreter();
-	}
-
 	/* Call the handler */
 
 	status = handler(function, address, bit_width, value, context,
 			 region_obj2->extra.region_context);
 
+	if (context_locked)
+		acpi_os_release_mutex(context_mutex);
+
 	if (ACPI_FAILURE(status)) {
 		ACPI_EXCEPTION((AE_INFO, status, "Returned by Handler for [%s]",
 				acpi_ut_get_region_name(region_obj->region.
@@ -277,6 +303,7 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 		}
 	}
 
+re_enter_interpreter:
 	if (!(handler_desc->address_space.handler_flags &
 	      ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
 		/*
diff --git a/drivers/acpi/acpica/evxfregn.c b/drivers/acpi/acpica/evxfregn.c index da97fd0c6b51..74d0ee8a5736 100644
--- a/drivers/acpi/acpica/evxfregn.c
+++ b/drivers/acpi/acpica/evxfregn.c
@@ -201,6 +201,7 @@ acpi_remove_address_space_handler(acpi_handle device,
 
 			/* Now we can delete the handler object */
 
+			acpi_os_release_mutex(handler_obj->address_space.context_mutex);
 			acpi_ut_remove_reference(handler_obj);
 			goto unlock_and_exit;
 		}
--
2.28.0

[Devel] Re: [PATCH 1/2] ACPICA: Fix race in GenericSerialBus (I2C) and GPIO OpRegion parameter handling

[Moore, Robert]

[-- Attachment #1: Type: text/plain, Size: 9670 bytes --]

Hans,
Could you make a pull request for this (and any related patches) on our github?
Thanks,
Bob


-----Original Message-----
From: Hans de Goede <hdegoede(a)redhat.com> 
Sent: Saturday, December 26, 2020 6:28 AM
To: Rafael J . Wysocki <rjw(a)rjwysocki.net>; Len Brown <lenb(a)kernel.org>; Moore, Robert <robert.moore(a)intel.com>; Kaneda, Erik <erik.kaneda(a)intel.com>
Cc: Hans de Goede <hdegoede(a)redhat.com>; linux-acpi(a)vger.kernel.org; devel(a)acpica.org
Subject: [PATCH 1/2] ACPICA: Fix race in GenericSerialBus (I2C) and GPIO OpRegion parameter handling

The handling of the GenericSerialBus (I2C) and GPIO OpRegions in
acpi_ev_address_space_dispatch() passes a number of extra parameters to the address-space handler through the address-space context pointer (instead of using more function parameters).

The context is shared between threads, so if multiple threads try to call the handler for the same address-space at the same time, then a second thread could change the parameters of a first thread while the handler is running for the first thread.

An example of this race hitting is the Lenovo Yoga Tablet2 1015L, where there are both AttribBytes accesses and AttribByte accesses to the same address-space. The AttribBytes access stores the number of bytes to transfer in context->access_length. Where as for the AttribByte access the number of bytes to transfer is always 1 and field_obj->field.access_length is unused (so 0). Both types of accesses racing from different threads leads to the following problem:

1. Thread a. starts an AttribBytes access, stores a non 0 value from field_obj->field.access_length in context->access_length 2. Thread b. starts an AttribByte access, stores 0 in
context->access_length
3. Thread a. calls i2c_acpi_space_handler() (under Linux). Which sees that the access-type is ACPI_GSB_ACCESS_ATTRIB_MULTIBYTE and calls acpi_gsb_i2c_read_bytes(..., context->access_length) 4. At this point context->access_length is 0 (set by thread b.) rather then the field_obj->field.access_length value from thread a.
This 0 length reads leads to the following errors being logged:

 i2c i2c-0: adapter quirk: no zero length (addr 0x0078, size 0, read)  i2c i2c-0: i2c read 0 bytes from client(a)0x78 starting at reg 0x0 failed, error: -95

Note this is just an example of the problems which this race can cause.
There are likely many more (sporadic) problems caused by this race.

This commit adds a new context_mutex to acpi_object_addr_handler and makes acpi_ev_address_space_dispatch() take that mutex when using the shared context to pass extra parameters to an address-space handler, fixing this race.

Note the new mutex must be taken *after* exiting the interpreter, therefor the existing acpi_ex_exit_interpreter() call is moved to above the code which stores the extra parameters in the context.

Signed-off-by: Hans de Goede <hdegoede(a)redhat.com>
---
 drivers/acpi/acpica/acobject.h  |  1 +
 drivers/acpi/acpica/evhandler.c |  6 ++++  drivers/acpi/acpica/evregion.c  | 61 ++++++++++++++++++++++++---------  drivers/acpi/acpica/evxfregn.c  |  1 +
 4 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/acpica/acobject.h b/drivers/acpi/acpica/acobject.h index 9f0219a8cb98..dd7efafcb103 100644
--- a/drivers/acpi/acpica/acobject.h
+++ b/drivers/acpi/acpica/acobject.h
@@ -284,6 +284,7 @@ struct acpi_object_addr_handler {
 	acpi_adr_space_handler handler;
 	struct acpi_namespace_node *node;	/* Parent device */
 	void *context;
+	acpi_mutex context_mutex;
 	acpi_adr_space_setup setup;
 	union acpi_operand_object *region_list;	/* Regions using this handler */
 	union acpi_operand_object *next;
diff --git a/drivers/acpi/acpica/evhandler.c b/drivers/acpi/acpica/evhandler.c index 5884eba047f7..347199f29afe 100644
--- a/drivers/acpi/acpica/evhandler.c
+++ b/drivers/acpi/acpica/evhandler.c
@@ -489,6 +489,12 @@ acpi_ev_install_space_handler(struct acpi_namespace_node *node,
 
 	/* Init handler obj */
 
+	status = acpi_os_create_mutex(&handler_obj->address_space.context_mutex);
+	if (ACPI_FAILURE(status)) {
+		acpi_ut_remove_reference(handler_obj);
+		goto unlock_and_exit;
+	}
+
 	handler_obj->address_space.space_id = (u8)space_id;
 	handler_obj->address_space.handler_flags = flags;
 	handler_obj->address_space.region_list = NULL; diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c index 21ff341e34a4..8e84eb0641e0 100644
--- a/drivers/acpi/acpica/evregion.c
+++ b/drivers/acpi/acpica/evregion.c
@@ -112,6 +112,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	union acpi_operand_object *region_obj2;
 	void *region_context = NULL;
 	struct acpi_connection_info *context;
+	acpi_mutex context_mutex;
+	bool context_locked;
 	acpi_physical_address address;
 
 	ACPI_FUNCTION_TRACE(ev_address_space_dispatch);
@@ -136,6 +138,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	}
 
 	context = handler_desc->address_space.context;
+	context_mutex = handler_desc->address_space.context_mutex;
+	context_locked = false;
 
 	/*
 	 * It may be the case that the region has never been initialized.
@@ -204,6 +208,23 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	handler = handler_desc->address_space.handler;
 	address = (region_obj->region.address + region_offset);
 
+	ACPI_DEBUG_PRINT((ACPI_DB_OPREGION,
+			  "Handler %p (@%p) Address %8.8X%8.8X [%s]\n",
+			  &region_obj->region.handler->address_space, handler,
+			  ACPI_FORMAT_UINT64(address),
+			  acpi_ut_get_region_name(region_obj->region.
+						  space_id)));
+
+	if (!(handler_desc->address_space.handler_flags &
+	      ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
+		/*
+		 * For handlers other than the default (supplied) handlers, we must
+		 * exit the interpreter because the handler *might* block -- we don't
+		 * know what it will do, so we can't hold the lock on the interpreter.
+		 */
+		acpi_ex_exit_interpreter();
+	}
+
 	/*
 	 * Special handling for generic_serial_bus and general_purpose_io:
 	 * There are three extra parameters that must be passed to the @@ -212,6 +233,11 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	 *   2) Length of the above buffer
 	 *   3) Actual access length from the access_as() op
 	 *
+	 * Since we pass these extra parameters via the context, which is
+	 * shared between threads, we must lock the context to avoid these
+	 * parameters being changed from another thread before the handler
+	 * has completed running.
+	 *
 	 * In addition, for general_purpose_io, the Address and bit_width fields
 	 * are defined as follows:
 	 *   1) Address is the pin number index of the field (bit offset from
@@ -221,6 +247,13 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	if ((region_obj->region.space_id == ACPI_ADR_SPACE_GSBUS) &&
 	    context && field_obj) {
 
+		status = acpi_os_acquire_mutex(context_mutex, ACPI_WAIT_FOREVER);
+		if (ACPI_FAILURE(status)) {
+			goto re_enter_interpreter;
+		}
+
+		context_locked = true;
+
 		/* Get the Connection (resource_template) buffer */
 
 		context->connection = field_obj->field.resource_buffer; @@ -230,6 +263,13 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 	if ((region_obj->region.space_id == ACPI_ADR_SPACE_GPIO) &&
 	    context && field_obj) {
 
+		status = acpi_os_acquire_mutex(context_mutex, ACPI_WAIT_FOREVER);
+		if (ACPI_FAILURE(status)) {
+			goto re_enter_interpreter;
+		}
+
+		context_locked = true;
+
 		/* Get the Connection (resource_template) buffer */
 
 		context->connection = field_obj->field.resource_buffer; @@ -239,28 +279,14 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 		bit_width = field_obj->field.bit_length;
 	}
 
-	ACPI_DEBUG_PRINT((ACPI_DB_OPREGION,
-			  "Handler %p (@%p) Address %8.8X%8.8X [%s]\n",
-			  &region_obj->region.handler->address_space, handler,
-			  ACPI_FORMAT_UINT64(address),
-			  acpi_ut_get_region_name(region_obj->region.
-						  space_id)));
-
-	if (!(handler_desc->address_space.handler_flags &
-	      ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
-		/*
-		 * For handlers other than the default (supplied) handlers, we must
-		 * exit the interpreter because the handler *might* block -- we don't
-		 * know what it will do, so we can't hold the lock on the interpreter.
-		 */
-		acpi_ex_exit_interpreter();
-	}
-
 	/* Call the handler */
 
 	status = handler(function, address, bit_width, value, context,
 			 region_obj2->extra.region_context);
 
+	if (context_locked)
+		acpi_os_release_mutex(context_mutex);
+
 	if (ACPI_FAILURE(status)) {
 		ACPI_EXCEPTION((AE_INFO, status, "Returned by Handler for [%s]",
 				acpi_ut_get_region_name(region_obj->region.
@@ -277,6 +303,7 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 		}
 	}
 
+re_enter_interpreter:
 	if (!(handler_desc->address_space.handler_flags &
 	      ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
 		/*
diff --git a/drivers/acpi/acpica/evxfregn.c b/drivers/acpi/acpica/evxfregn.c index da97fd0c6b51..74d0ee8a5736 100644
--- a/drivers/acpi/acpica/evxfregn.c
+++ b/drivers/acpi/acpica/evxfregn.c
@@ -201,6 +201,7 @@ acpi_remove_address_space_handler(acpi_handle device,
 
 			/* Now we can delete the handler object */
 
+			acpi_os_release_mutex(handler_obj->address_space.context_mutex);
 			acpi_ut_remove_reference(handler_obj);
 			goto unlock_and_exit;
 		}
--
2.28.0

Re: [PATCH 1/2] ACPICA: Fix race in GenericSerialBus (I2C) and GPIO OpRegion parameter handling

[Hans de Goede]

Hi,

On 1/4/21 11:17 PM, Moore, Robert wrote:
> Hans,
> Could you make a pull request for this (and any related patches) on our github?

Done:

https://github.com/acpica/acpica/pull/658

Regards,

Hans



> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com> 
> Sent: Saturday, December 26, 2020 6:28 AM
> To: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>; Moore, Robert <robert.moore@intel.com>; Kaneda, Erik <erik.kaneda@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>; linux-acpi@vger.kernel.org; devel@acpica.org
> Subject: [PATCH 1/2] ACPICA: Fix race in GenericSerialBus (I2C) and GPIO OpRegion parameter handling
> 
> The handling of the GenericSerialBus (I2C) and GPIO OpRegions in
> acpi_ev_address_space_dispatch() passes a number of extra parameters to the address-space handler through the address-space context pointer (instead of using more function parameters).
> 
> The context is shared between threads, so if multiple threads try to call the handler for the same address-space at the same time, then a second thread could change the parameters of a first thread while the handler is running for the first thread.
> 
> An example of this race hitting is the Lenovo Yoga Tablet2 1015L, where there are both AttribBytes accesses and AttribByte accesses to the same address-space. The AttribBytes access stores the number of bytes to transfer in context->access_length. Where as for the AttribByte access the number of bytes to transfer is always 1 and field_obj->field.access_length is unused (so 0). Both types of accesses racing from different threads leads to the following problem:
> 
> 1. Thread a. starts an AttribBytes access, stores a non 0 value from field_obj->field.access_length in context->access_length 2. Thread b. starts an AttribByte access, stores 0 in
> context->access_length
> 3. Thread a. calls i2c_acpi_space_handler() (under Linux). Which sees that the access-type is ACPI_GSB_ACCESS_ATTRIB_MULTIBYTE and calls acpi_gsb_i2c_read_bytes(..., context->access_length) 4. At this point context->access_length is 0 (set by thread b.) rather then the field_obj->field.access_length value from thread a.
> This 0 length reads leads to the following errors being logged:
> 
>  i2c i2c-0: adapter quirk: no zero length (addr 0x0078, size 0, read)  i2c i2c-0: i2c read 0 bytes from client@0x78 starting at reg 0x0 failed, error: -95
> 
> Note this is just an example of the problems which this race can cause.
> There are likely many more (sporadic) problems caused by this race.
> 
> This commit adds a new context_mutex to acpi_object_addr_handler and makes acpi_ev_address_space_dispatch() take that mutex when using the shared context to pass extra parameters to an address-space handler, fixing this race.
> 
> Note the new mutex must be taken *after* exiting the interpreter, therefor the existing acpi_ex_exit_interpreter() call is moved to above the code which stores the extra parameters in the context.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/acpica/acobject.h  |  1 +
>  drivers/acpi/acpica/evhandler.c |  6 ++++  drivers/acpi/acpica/evregion.c  | 61 ++++++++++++++++++++++++---------  drivers/acpi/acpica/evxfregn.c  |  1 +
>  4 files changed, 52 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/acobject.h b/drivers/acpi/acpica/acobject.h index 9f0219a8cb98..dd7efafcb103 100644
> --- a/drivers/acpi/acpica/acobject.h
> +++ b/drivers/acpi/acpica/acobject.h
> @@ -284,6 +284,7 @@ struct acpi_object_addr_handler {
>  	acpi_adr_space_handler handler;
>  	struct acpi_namespace_node *node;	/* Parent device */
>  	void *context;
> +	acpi_mutex context_mutex;
>  	acpi_adr_space_setup setup;
>  	union acpi_operand_object *region_list;	/* Regions using this handler */
>  	union acpi_operand_object *next;
> diff --git a/drivers/acpi/acpica/evhandler.c b/drivers/acpi/acpica/evhandler.c index 5884eba047f7..347199f29afe 100644
> --- a/drivers/acpi/acpica/evhandler.c
> +++ b/drivers/acpi/acpica/evhandler.c
> @@ -489,6 +489,12 @@ acpi_ev_install_space_handler(struct acpi_namespace_node *node,
>  
>  	/* Init handler obj */
>  
> +	status = acpi_os_create_mutex(&handler_obj->address_space.context_mutex);
> +	if (ACPI_FAILURE(status)) {
> +		acpi_ut_remove_reference(handler_obj);
> +		goto unlock_and_exit;
> +	}
> +
>  	handler_obj->address_space.space_id = (u8)space_id;
>  	handler_obj->address_space.handler_flags = flags;
>  	handler_obj->address_space.region_list = NULL; diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c index 21ff341e34a4..8e84eb0641e0 100644
> --- a/drivers/acpi/acpica/evregion.c
> +++ b/drivers/acpi/acpica/evregion.c
> @@ -112,6 +112,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
>  	union acpi_operand_object *region_obj2;
>  	void *region_context = NULL;
>  	struct acpi_connection_info *context;
> +	acpi_mutex context_mutex;
> +	bool context_locked;
>  	acpi_physical_address address;
>  
>  	ACPI_FUNCTION_TRACE(ev_address_space_dispatch);
> @@ -136,6 +138,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
>  	}
>  
>  	context = handler_desc->address_space.context;
> +	context_mutex = handler_desc->address_space.context_mutex;
> +	context_locked = false;
>  
>  	/*
>  	 * It may be the case that the region has never been initialized.
> @@ -204,6 +208,23 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
>  	handler = handler_desc->address_space.handler;
>  	address = (region_obj->region.address + region_offset);
>  
> +	ACPI_DEBUG_PRINT((ACPI_DB_OPREGION,
> +			  "Handler %p (@%p) Address %8.8X%8.8X [%s]\n",
> +			  &region_obj->region.handler->address_space, handler,
> +			  ACPI_FORMAT_UINT64(address),
> +			  acpi_ut_get_region_name(region_obj->region.
> +						  space_id)));
> +
> +	if (!(handler_desc->address_space.handler_flags &
> +	      ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
> +		/*
> +		 * For handlers other than the default (supplied) handlers, we must
> +		 * exit the interpreter because the handler *might* block -- we don't
> +		 * know what it will do, so we can't hold the lock on the interpreter.
> +		 */
> +		acpi_ex_exit_interpreter();
> +	}
> +
>  	/*
>  	 * Special handling for generic_serial_bus and general_purpose_io:
>  	 * There are three extra parameters that must be passed to the @@ -212,6 +233,11 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
>  	 *   2) Length of the above buffer
>  	 *   3) Actual access length from the access_as() op
>  	 *
> +	 * Since we pass these extra parameters via the context, which is
> +	 * shared between threads, we must lock the context to avoid these
> +	 * parameters being changed from another thread before the handler
> +	 * has completed running.
> +	 *
>  	 * In addition, for general_purpose_io, the Address and bit_width fields
>  	 * are defined as follows:
>  	 *   1) Address is the pin number index of the field (bit offset from
> @@ -221,6 +247,13 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
>  	if ((region_obj->region.space_id == ACPI_ADR_SPACE_GSBUS) &&
>  	    context && field_obj) {
>  
> +		status = acpi_os_acquire_mutex(context_mutex, ACPI_WAIT_FOREVER);
> +		if (ACPI_FAILURE(status)) {
> +			goto re_enter_interpreter;
> +		}
> +
> +		context_locked = true;
> +
>  		/* Get the Connection (resource_template) buffer */
>  
>  		context->connection = field_obj->field.resource_buffer; @@ -230,6 +263,13 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
>  	if ((region_obj->region.space_id == ACPI_ADR_SPACE_GPIO) &&
>  	    context && field_obj) {
>  
> +		status = acpi_os_acquire_mutex(context_mutex, ACPI_WAIT_FOREVER);
> +		if (ACPI_FAILURE(status)) {
> +			goto re_enter_interpreter;
> +		}
> +
> +		context_locked = true;
> +
>  		/* Get the Connection (resource_template) buffer */
>  
>  		context->connection = field_obj->field.resource_buffer; @@ -239,28 +279,14 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
>  		bit_width = field_obj->field.bit_length;
>  	}
>  
> -	ACPI_DEBUG_PRINT((ACPI_DB_OPREGION,
> -			  "Handler %p (@%p) Address %8.8X%8.8X [%s]\n",
> -			  &region_obj->region.handler->address_space, handler,
> -			  ACPI_FORMAT_UINT64(address),
> -			  acpi_ut_get_region_name(region_obj->region.
> -						  space_id)));
> -
> -	if (!(handler_desc->address_space.handler_flags &
> -	      ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
> -		/*
> -		 * For handlers other than the default (supplied) handlers, we must
> -		 * exit the interpreter because the handler *might* block -- we don't
> -		 * know what it will do, so we can't hold the lock on the interpreter.
> -		 */
> -		acpi_ex_exit_interpreter();
> -	}
> -
>  	/* Call the handler */
>  
>  	status = handler(function, address, bit_width, value, context,
>  			 region_obj2->extra.region_context);
>  
> +	if (context_locked)
> +		acpi_os_release_mutex(context_mutex);
> +
>  	if (ACPI_FAILURE(status)) {
>  		ACPI_EXCEPTION((AE_INFO, status, "Returned by Handler for [%s]",
>  				acpi_ut_get_region_name(region_obj->region.
> @@ -277,6 +303,7 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
>  		}
>  	}
>  
> +re_enter_interpreter:
>  	if (!(handler_desc->address_space.handler_flags &
>  	      ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
>  		/*
> diff --git a/drivers/acpi/acpica/evxfregn.c b/drivers/acpi/acpica/evxfregn.c index da97fd0c6b51..74d0ee8a5736 100644
> --- a/drivers/acpi/acpica/evxfregn.c
> +++ b/drivers/acpi/acpica/evxfregn.c
> @@ -201,6 +201,7 @@ acpi_remove_address_space_handler(acpi_handle device,
>  
>  			/* Now we can delete the handler object */
>  
> +			acpi_os_release_mutex(handler_obj->address_space.context_mutex);
>  			acpi_ut_remove_reference(handler_obj);
>  			goto unlock_and_exit;
>  		}
> --
> 2.28.0
> 

RE: [PATCH 0/2] ACPICA: Fix a race in GenericSerialBus (I2C) and GPIO handling

[Kaneda, Erik]

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Saturday, December 26, 2020 6:28 AM
> To: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
> Moore, Robert <robert.moore@intel.com>; Kaneda, Erik
> <erik.kaneda@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>; linux-acpi@vger.kernel.org;
> devel@acpica.org
> Subject: [PATCH 0/2] ACPICA: Fix a race in GenericSerialBus (I2C) and GPIO
> handling
> 
> Hi All,
> 
> On one of my machines I noticed the following errors being logged:
> 
> [   52.892807] i2c i2c-0: adapter quirk: no zero length (addr 0x0078, size 0,
> read)
> [   52.893037] i2c i2c-0: i2c read 0 bytes from client@0x78 starting at reg 0x0
> failed, error: -95
> 
> The second line is coming from the Linux I2C ACPI OpRegion handling and
> after a bunch of debugging I've found out that there is a rather obvious
> (once you see it) and nasty race condition in the handling of I2C and GPIO
> opregions in acpi_ev_address_space_dispatch(). See the first patch in this
> series (the second patch is a follow-up cleanup patch removing some code
> duplication).
> 
> TBH I'm surprised that this issue has gone unnoticed as long as it has,
> but I guess that it mostly leads to unreproducable sporadic problems
> making it hard to debug and I got lucky that I had a machine where the
> race seems to trigger about once every 20 seconds.
> 
> I know that ACPICA patches are normally merged through the ACPICA
> upstream
> but given that this is a serious bug, I believe that in this case it might
> be best to add the fix directly to Linux and then port it to ACPICA from
> there.

Thanks for this changeset! I'm taking a look right now

Erik
> 
> Regards,
> 
> Hans

Re: [PATCH 0/2] ACPICA: Fix a race in GenericSerialBus (I2C) and GPIO handling

[Hans de Goede]

Hi,

On 1/11/21 7:39 PM, Kaneda, Erik wrote:
> 
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: Saturday, December 26, 2020 6:28 AM
>> To: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
>> Moore, Robert <robert.moore@intel.com>; Kaneda, Erik
>> <erik.kaneda@intel.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>; linux-acpi@vger.kernel.org;
>> devel@acpica.org
>> Subject: [PATCH 0/2] ACPICA: Fix a race in GenericSerialBus (I2C) and GPIO
>> handling
>>
>> Hi All,
>>
>> On one of my machines I noticed the following errors being logged:
>>
>> [   52.892807] i2c i2c-0: adapter quirk: no zero length (addr 0x0078, size 0,
>> read)
>> [   52.893037] i2c i2c-0: i2c read 0 bytes from client@0x78 starting at reg 0x0
>> failed, error: -95
>>
>> The second line is coming from the Linux I2C ACPI OpRegion handling and
>> after a bunch of debugging I've found out that there is a rather obvious
>> (once you see it) and nasty race condition in the handling of I2C and GPIO
>> opregions in acpi_ev_address_space_dispatch(). See the first patch in this
>> series (the second patch is a follow-up cleanup patch removing some code
>> duplication).
>>
>> TBH I'm surprised that this issue has gone unnoticed as long as it has,
>> but I guess that it mostly leads to unreproducable sporadic problems
>> making it hard to debug and I got lucky that I had a machine where the
>> race seems to trigger about once every 20 seconds.
>>
>> I know that ACPICA patches are normally merged through the ACPICA
>> upstream
>> but given that this is a serious bug, I believe that in this case it might
>> be best to add the fix directly to Linux and then port it to ACPICA from
>> there.
> 
> Thanks for this changeset! I'm taking a look right now

Ok, note I already ported it to ACPICA git and submitted a pull-req
(on Bob's request): https://github.com/acpica/acpica/pull/658

Regards,

Hans

Re: [PATCH 0/2] ACPICA: Fix a race in GenericSerialBus (I2C) and GPIO handling

[Hans de Goede]

Hi,

On 12/26/20 3:28 PM, Hans de Goede wrote:
> Hi All,
> 
> On one of my machines I noticed the following errors being logged:
> 
> [   52.892807] i2c i2c-0: adapter quirk: no zero length (addr 0x0078, size 0, read)
> [   52.893037] i2c i2c-0: i2c read 0 bytes from client@0x78 starting at reg 0x0 failed, error: -95
> 
> The second line is coming from the Linux I2C ACPI OpRegion handling and
> after a bunch of debugging I've found out that there is a rather obvious
> (once you see it) and nasty race condition in the handling of I2C and GPIO
> opregions in acpi_ev_address_space_dispatch(). See the first patch in this
> series (the second patch is a follow-up cleanup patch removing some code
> duplication).
> 
> TBH I'm surprised that this issue has gone unnoticed as long as it has,
> but I guess that it mostly leads to unreproducable sporadic problems
> making it hard to debug and I got lucky that I had a machine where the
> race seems to trigger about once every 20 seconds.
> 
> I know that ACPICA patches are normally merged through the ACPICA upstream
> but given that this is a serious bug, I believe that in this case it might
> be best to add the fix directly to Linux and then port it to ACPICA from
> there.

ping ?

This was submitted 2 full months ago; and despite this:

1. Fixing a serious bug in ACPICA
2. The fix being pretty simple (and AFAICT obviously correct)

This is still awaiting review upstream:
https://github.com/acpica/acpica/pull/658

I must say that it feels to me that the upstream ACPICA process is broken here.

I submitted a pull-req for this, as requested and after that there has
been zero progress.

The pull-req even has a 26 day old "this looks good to me" comment from Erik,
followed by silence... ?

Rafael, can you please consider just directly picking these 2 fixes into
your acpi branch, so that we can get this nasty race condition fixed ?

Regards,

Hans

Re: [PATCH 0/2] ACPICA: Fix a race in GenericSerialBus (I2C) and GPIO handling

[Rafael J. Wysocki]

On Mon, Feb 15, 2021 at 6:52 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 12/26/20 3:28 PM, Hans de Goede wrote:
> > Hi All,
> >
> > On one of my machines I noticed the following errors being logged:
> >
> > [   52.892807] i2c i2c-0: adapter quirk: no zero length (addr 0x0078, size 0, read)
> > [   52.893037] i2c i2c-0: i2c read 0 bytes from client@0x78 starting at reg 0x0 failed, error: -95
> >
> > The second line is coming from the Linux I2C ACPI OpRegion handling and
> > after a bunch of debugging I've found out that there is a rather obvious
> > (once you see it) and nasty race condition in the handling of I2C and GPIO
> > opregions in acpi_ev_address_space_dispatch(). See the first patch in this
> > series (the second patch is a follow-up cleanup patch removing some code
> > duplication).
> >
> > TBH I'm surprised that this issue has gone unnoticed as long as it has,
> > but I guess that it mostly leads to unreproducable sporadic problems
> > making it hard to debug and I got lucky that I had a machine where the
> > race seems to trigger about once every 20 seconds.
> >
> > I know that ACPICA patches are normally merged through the ACPICA upstream
> > but given that this is a serious bug, I believe that in this case it might
> > be best to add the fix directly to Linux and then port it to ACPICA from
> > there.
>
> ping ?
>
> This was submitted 2 full months ago; and despite this:
>
> 1. Fixing a serious bug in ACPICA
> 2. The fix being pretty simple (and AFAICT obviously correct)
>
> This is still awaiting review upstream:
> https://github.com/acpica/acpica/pull/658
>
> I must say that it feels to me that the upstream ACPICA process is broken here.
>
> I submitted a pull-req for this, as requested and after that there has
> been zero progress.
>
> The pull-req even has a 26 day old "this looks good to me" comment from Erik,
> followed by silence... ?
>
> Rafael, can you please consider just directly picking these 2 fixes into
> your acpi branch, so that we can get this nasty race condition fixed ?

I will do that later this week, thanks!

[Devel] Re: [PATCH 0/2] ACPICA: Fix a race in GenericSerialBus (I2C) and GPIO handling

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 2149 bytes --]

On Mon, Feb 15, 2021 at 6:52 PM Hans de Goede <hdegoede(a)redhat.com> wrote:
>
> Hi,
>
> On 12/26/20 3:28 PM, Hans de Goede wrote:
> > Hi All,
> >
> > On one of my machines I noticed the following errors being logged:
> >
> > [   52.892807] i2c i2c-0: adapter quirk: no zero length (addr 0x0078, size 0, read)
> > [   52.893037] i2c i2c-0: i2c read 0 bytes from client(a)0x78 starting at reg 0x0 failed, error: -95
> >
> > The second line is coming from the Linux I2C ACPI OpRegion handling and
> > after a bunch of debugging I've found out that there is a rather obvious
> > (once you see it) and nasty race condition in the handling of I2C and GPIO
> > opregions in acpi_ev_address_space_dispatch(). See the first patch in this
> > series (the second patch is a follow-up cleanup patch removing some code
> > duplication).
> >
> > TBH I'm surprised that this issue has gone unnoticed as long as it has,
> > but I guess that it mostly leads to unreproducable sporadic problems
> > making it hard to debug and I got lucky that I had a machine where the
> > race seems to trigger about once every 20 seconds.
> >
> > I know that ACPICA patches are normally merged through the ACPICA upstream
> > but given that this is a serious bug, I believe that in this case it might
> > be best to add the fix directly to Linux and then port it to ACPICA from
> > there.
>
> ping ?
>
> This was submitted 2 full months ago; and despite this:
>
> 1. Fixing a serious bug in ACPICA
> 2. The fix being pretty simple (and AFAICT obviously correct)
>
> This is still awaiting review upstream:
> https://github.com/acpica/acpica/pull/658
>
> I must say that it feels to me that the upstream ACPICA process is broken here.
>
> I submitted a pull-req for this, as requested and after that there has
> been zero progress.
>
> The pull-req even has a 26 day old "this looks good to me" comment from Erik,
> followed by silence... ?
>
> Rafael, can you please consider just directly picking these 2 fixes into
> your acpi branch, so that we can get this nasty race condition fixed ?

I will do that later this week, thanks!

RE: [PATCH 0/2] ACPICA: Fix a race in GenericSerialBus (I2C) and GPIO handling

[Moore, Robert]

We've been busy implementing ACPI 6.4 support in ACPICA. Sorry for the delay.
Bob


-----Original Message-----
From: Rafael J. Wysocki <rafael@kernel.org> 
Sent: Monday, February 15, 2021 10:26 AM
To: Hans de Goede <hdegoede@redhat.com>
Cc: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>; Moore, Robert <robert.moore@intel.com>; Kaneda, Erik <erik.kaneda@intel.com>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>; open list:ACPI COMPONENT ARCHITECTURE (ACPICA) <devel@acpica.org>
Subject: Re: [PATCH 0/2] ACPICA: Fix a race in GenericSerialBus (I2C) and GPIO handling

On Mon, Feb 15, 2021 at 6:52 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 12/26/20 3:28 PM, Hans de Goede wrote:
> > Hi All,
> >
> > On one of my machines I noticed the following errors being logged:
> >
> > [   52.892807] i2c i2c-0: adapter quirk: no zero length (addr 0x0078, size 0, read)
> > [   52.893037] i2c i2c-0: i2c read 0 bytes from client@0x78 starting at reg 0x0 failed, error: -95
> >
> > The second line is coming from the Linux I2C ACPI OpRegion handling 
> > and after a bunch of debugging I've found out that there is a rather 
> > obvious (once you see it) and nasty race condition in the handling 
> > of I2C and GPIO opregions in acpi_ev_address_space_dispatch(). See 
> > the first patch in this series (the second patch is a follow-up 
> > cleanup patch removing some code duplication).
> >
> > TBH I'm surprised that this issue has gone unnoticed as long as it 
> > has, but I guess that it mostly leads to unreproducable sporadic 
> > problems making it hard to debug and I got lucky that I had a 
> > machine where the race seems to trigger about once every 20 seconds.
> >
> > I know that ACPICA patches are normally merged through the ACPICA 
> > upstream but given that this is a serious bug, I believe that in 
> > this case it might be best to add the fix directly to Linux and then 
> > port it to ACPICA from there.
>
> ping ?
>
> This was submitted 2 full months ago; and despite this:
>
> 1. Fixing a serious bug in ACPICA
> 2. The fix being pretty simple (and AFAICT obviously correct)
>
> This is still awaiting review upstream:
> https://github.com/acpica/acpica/pull/658
>
> I must say that it feels to me that the upstream ACPICA process is broken here.
>
> I submitted a pull-req for this, as requested and after that there has 
> been zero progress.
>
> The pull-req even has a 26 day old "this looks good to me" comment 
> from Erik, followed by silence... ?
>
> Rafael, can you please consider just directly picking these 2 fixes 
> into your acpi branch, so that we can get this nasty race condition fixed ?

I will do that later this week, thanks!

[Devel] Re: [PATCH 0/2] ACPICA: Fix a race in GenericSerialBus (I2C) and GPIO handling

[Moore, Robert]

[-- Attachment #1: Type: text/plain, Size: 2788 bytes --]

We've been busy implementing ACPI 6.4 support in ACPICA. Sorry for the delay.
Bob


-----Original Message-----
From: Rafael J. Wysocki <rafael(a)kernel.org> 
Sent: Monday, February 15, 2021 10:26 AM
To: Hans de Goede <hdegoede(a)redhat.com>
Cc: Rafael J . Wysocki <rjw(a)rjwysocki.net>; Len Brown <lenb(a)kernel.org>; Moore, Robert <robert.moore(a)intel.com>; Kaneda, Erik <erik.kaneda(a)intel.com>; ACPI Devel Maling List <linux-acpi(a)vger.kernel.org>; open list:ACPI COMPONENT ARCHITECTURE (ACPICA) <devel(a)acpica.org>
Subject: Re: [PATCH 0/2] ACPICA: Fix a race in GenericSerialBus (I2C) and GPIO handling

On Mon, Feb 15, 2021 at 6:52 PM Hans de Goede <hdegoede(a)redhat.com> wrote:
>
> Hi,
>
> On 12/26/20 3:28 PM, Hans de Goede wrote:
> > Hi All,
> >
> > On one of my machines I noticed the following errors being logged:
> >
> > [   52.892807] i2c i2c-0: adapter quirk: no zero length (addr 0x0078, size 0, read)
> > [   52.893037] i2c i2c-0: i2c read 0 bytes from client(a)0x78 starting at reg 0x0 failed, error: -95
> >
> > The second line is coming from the Linux I2C ACPI OpRegion handling 
> > and after a bunch of debugging I've found out that there is a rather 
> > obvious (once you see it) and nasty race condition in the handling 
> > of I2C and GPIO opregions in acpi_ev_address_space_dispatch(). See 
> > the first patch in this series (the second patch is a follow-up 
> > cleanup patch removing some code duplication).
> >
> > TBH I'm surprised that this issue has gone unnoticed as long as it 
> > has, but I guess that it mostly leads to unreproducable sporadic 
> > problems making it hard to debug and I got lucky that I had a 
> > machine where the race seems to trigger about once every 20 seconds.
> >
> > I know that ACPICA patches are normally merged through the ACPICA 
> > upstream but given that this is a serious bug, I believe that in 
> > this case it might be best to add the fix directly to Linux and then 
> > port it to ACPICA from there.
>
> ping ?
>
> This was submitted 2 full months ago; and despite this:
>
> 1. Fixing a serious bug in ACPICA
> 2. The fix being pretty simple (and AFAICT obviously correct)
>
> This is still awaiting review upstream:
> https://github.com/acpica/acpica/pull/658
>
> I must say that it feels to me that the upstream ACPICA process is broken here.
>
> I submitted a pull-req for this, as requested and after that there has 
> been zero progress.
>
> The pull-req even has a 26 day old "this looks good to me" comment 
> from Erik, followed by silence... ?
>
> Rafael, can you please consider just directly picking these 2 fixes 
> into your acpi branch, so that we can get this nasty race condition fixed ?

I will do that later this week, thanks!