diff mbox series

[v2,7/8] ACPICA: AML Parser: ignore dispatcher error status during table load

Message ID 1553845052-22422-1-git-send-email-aaron.ma@canonical.com
State New
Headers show
Series iommu: add kernel dma protection | expand

Commit Message

Aaron Ma March 29, 2019, 7:37 a.m. UTC 
From: "Schmauss, Erik" <erik.schmauss@intel.com>

BugLink: https://bugs.launchpad.net/bugs/1820153

The dispatcher and the executer process the parse nodes During table
load. Error status from the evaluation confuses the AML parser. This
results in the parser failing to complete parsing of the current
scope op which becomes problematic. For the incorrect AML below, _ADR
never gets created.

definition_block(...)
{
   Scope (\_SB)
   {
     Device (PCI0){...}
     Name (OBJ1, 0x0)
     OBJ1 = PCI0 + 5 // Results in an operand error.
   } // \_SB not closed

   // parser looks for \_SB._SB.PCI0, results in AE_NOT_FOUND error
   // Entire scope block gets skipped.
   Scope (\_SB.PCI0)
   {
       Name (_ADR, 0x0)
   }
}

Fix the above error by properly completing the initial \_SB scope
after an error by clearing errors that occur during table load. In
the above case, this means that OBJ1 = PIC0 + 5 is skipped.

Fixes: 5088814a6e93 (ACPICA: AML parser: attempt to continue loading table after error)
Link: https://bugzilla.kernel.org/show_bug.cgi?id=200363
Tested-by: Bastien Nocera <hadess@hadess.net>
Signed-off-by: Erik Schmauss <erik.schmauss@intel.com>
Cc: 4.17+ <stable@vger.kernel.org> # 4.17+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
(cherry picked from commit 73c2a01c52b657f4a0ead6c95f64c5279efbd000)
Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
---
 drivers/acpi/acpica/psloop.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Khalid Elmously March 29, 2019, 8:06 a.m. UTC | #1 
On 2019-03-29 15:37:31 , Aaron Ma wrote:
> From: "Schmauss, Erik" <erik.schmauss@intel.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1820153
> 
> The dispatcher and the executer process the parse nodes During table
> load. Error status from the evaluation confuses the AML parser. This
> results in the parser failing to complete parsing of the current
> scope op which becomes problematic. For the incorrect AML below, _ADR
> never gets created.
> 
> definition_block(...)
> {
>    Scope (\_SB)
>    {
>      Device (PCI0){...}
>      Name (OBJ1, 0x0)
>      OBJ1 = PCI0 + 5 // Results in an operand error.
>    } // \_SB not closed
> 
>    // parser looks for \_SB._SB.PCI0, results in AE_NOT_FOUND error
>    // Entire scope block gets skipped.
>    Scope (\_SB.PCI0)
>    {
>        Name (_ADR, 0x0)
>    }
> }
> 
> Fix the above error by properly completing the initial \_SB scope
> after an error by clearing errors that occur during table load. In
> the above case, this means that OBJ1 = PIC0 + 5 is skipped.
> 
> Fixes: 5088814a6e93 (ACPICA: AML parser: attempt to continue loading table after error)
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=200363
> Tested-by: Bastien Nocera <hadess@hadess.net>
> Signed-off-by: Erik Schmauss <erik.schmauss@intel.com>
> Cc: 4.17+ <stable@vger.kernel.org> # 4.17+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> (cherry picked from commit 73c2a01c52b657f4a0ead6c95f64c5279efbd000)
> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> ---
>  drivers/acpi/acpica/psloop.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/acpi/acpica/psloop.c b/drivers/acpi/acpica/psloop.c
> index f437c3d54086..a5f35ff54510 100644
> --- a/drivers/acpi/acpica/psloop.c
> +++ b/drivers/acpi/acpica/psloop.c
> @@ -513,6 +513,18 @@ acpi_status acpi_ps_parse_loop(struct acpi_walk_state *walk_state)
>  			status =
>  			    acpi_ps_create_op(walk_state, aml_op_start, &op);
>  			if (ACPI_FAILURE(status)) {
> +				/*
> +				 * ACPI_PARSE_MODULE_LEVEL means that we are loading a table by
> +				 * executing it as a control method. However, if we encounter
> +				 * an error while loading the table, we need to keep trying to
> +				 * load the table rather than aborting the table load. Set the
> +				 * status to AE_OK to proceed with the table load.
> +				 */
> +				if ((walk_state->
> +				     parse_flags & ACPI_PARSE_MODULE_LEVEL)
> +				    && status == AE_ALREADY_EXISTS) {
> +					status = AE_OK;
> +				}
>  				if (status == AE_CTRL_PARSE_CONTINUE) {
>  					continue;
>  				}
> @@ -710,6 +722,20 @@ acpi_status acpi_ps_parse_loop(struct acpi_walk_state *walk_state)
>  			    acpi_ps_next_parse_state(walk_state, op, status);
>  			if (status == AE_CTRL_PENDING) {
>  				status = AE_OK;
> +			} else
> +			    if ((walk_state->
> +				 parse_flags & ACPI_PARSE_MODULE_LEVEL)
> +				&& ACPI_FAILURE(status)) {
> +				/*
> +				 * ACPI_PARSE_MODULE_LEVEL means that we are loading a table by
> +				 * executing it as a control method. However, if we encounter
> +				 * an error while loading the table, we need to keep trying to
> +				 * load the table rather than aborting the table load. Set the
> +				 * status to AE_OK to proceed with the table load. If we get a
> +				 * failure at this point, it means that the dispatcher got an
> +				 * error while processing Op (most likely an AML operand error.
> +				 */
> +				status = AE_OK;
>  			}
>  		}
> 

I believe I have Tyler's ACK for patch 8/8 based on his comment but I have no ACKs yet for 7/8.

I'm assuming these 2 patches don't necessarily need to come right after patch 1-6? Please let me know if that's not right.

Thanks
Aaron Ma March 29, 2019, 8:19 a.m. UTC | #2 
On March 29, 2019 8:06:00 AM UTC, Khaled Elmously <khalid.elmously@canonical.com> wrote:
>On 2019-03-29 15:37:31 , Aaron Ma wrote:
>> From: "Schmauss, Erik" <erik.schmauss@intel.com>
>> 
>> BugLink: https://bugs.launchpad.net/bugs/1820153
>> 
>> The dispatcher and the executer process the parse nodes During table
>> load. Error status from the evaluation confuses the AML parser. This
>> results in the parser failing to complete parsing of the current
>> scope op which becomes problematic. For the incorrect AML below, _ADR
>> never gets created.
>> 
>> definition_block(...)
>> {
>>    Scope (\_SB)
>>    {
>>      Device (PCI0){...}
>>      Name (OBJ1, 0x0)
>>      OBJ1 = PCI0 + 5 // Results in an operand error.
>>    } // \_SB not closed
>> 
>>    // parser looks for \_SB._SB.PCI0, results in AE_NOT_FOUND error
>>    // Entire scope block gets skipped.
>>    Scope (\_SB.PCI0)
>>    {
>>        Name (_ADR, 0x0)
>>    }
>> }
>> 
>> Fix the above error by properly completing the initial \_SB scope
>> after an error by clearing errors that occur during table load. In
>> the above case, this means that OBJ1 = PIC0 + 5 is skipped.
>> 
>> Fixes: 5088814a6e93 (ACPICA: AML parser: attempt to continue loading
>table after error)
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=200363
>> Tested-by: Bastien Nocera <hadess@hadess.net>
>> Signed-off-by: Erik Schmauss <erik.schmauss@intel.com>
>> Cc: 4.17+ <stable@vger.kernel.org> # 4.17+
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> (cherry picked from commit 73c2a01c52b657f4a0ead6c95f64c5279efbd000)
>> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
>> ---
>>  drivers/acpi/acpica/psloop.c | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>> 
>> diff --git a/drivers/acpi/acpica/psloop.c
>b/drivers/acpi/acpica/psloop.c
>> index f437c3d54086..a5f35ff54510 100644
>> --- a/drivers/acpi/acpica/psloop.c
>> +++ b/drivers/acpi/acpica/psloop.c
>> @@ -513,6 +513,18 @@ acpi_status acpi_ps_parse_loop(struct
>acpi_walk_state *walk_state)
>>  			status =
>>  			    acpi_ps_create_op(walk_state, aml_op_start, &op);
>>  			if (ACPI_FAILURE(status)) {
>> +				/*
>> +				 * ACPI_PARSE_MODULE_LEVEL means that we are loading a table by
>> +				 * executing it as a control method. However, if we encounter
>> +				 * an error while loading the table, we need to keep trying to
>> +				 * load the table rather than aborting the table load. Set the
>> +				 * status to AE_OK to proceed with the table load.
>> +				 */
>> +				if ((walk_state->
>> +				     parse_flags & ACPI_PARSE_MODULE_LEVEL)
>> +				    && status == AE_ALREADY_EXISTS) {
>> +					status = AE_OK;
>> +				}
>>  				if (status == AE_CTRL_PARSE_CONTINUE) {
>>  					continue;
>>  				}
>> @@ -710,6 +722,20 @@ acpi_status acpi_ps_parse_loop(struct
>acpi_walk_state *walk_state)
>>  			    acpi_ps_next_parse_state(walk_state, op, status);
>>  			if (status == AE_CTRL_PENDING) {
>>  				status = AE_OK;
>> +			} else
>> +			    if ((walk_state->
>> +				 parse_flags & ACPI_PARSE_MODULE_LEVEL)
>> +				&& ACPI_FAILURE(status)) {
>> +				/*
>> +				 * ACPI_PARSE_MODULE_LEVEL means that we are loading a table by
>> +				 * executing it as a control method. However, if we encounter
>> +				 * an error while loading the table, we need to keep trying to
>> +				 * load the table rather than aborting the table load. Set the
>> +				 * status to AE_OK to proceed with the table load. If we get a
>> +				 * failure at this point, it means that the dispatcher got an
>> +				 * error while processing Op (most likely an AML operand error.
>> +				 */
>> +				status = AE_OK;
>>  			}
>>  		}
>> 
>
>I believe I have Tyler's ACK for patch 8/8 based on his comment but I
>have no ACKs yet for 7/8.
>
>I'm assuming these 2 patches don't necessarily need to come right after
>patch 1-6? Please let me know if that's not right.
>

I think Tyler's ack for both patches is in bionic sru.

Just applying them are fine, no need to rebase branch.

Thanks,
Aaron


>Thanks
Tyler Hicks March 29, 2019, 3 p.m. UTC | #3 
On 2019-03-29 15:37:31, Aaron Ma wrote:
> From: "Schmauss, Erik" <erik.schmauss@intel.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1820153
> 
> The dispatcher and the executer process the parse nodes During table
> load. Error status from the evaluation confuses the AML parser. This
> results in the parser failing to complete parsing of the current
> scope op which becomes problematic. For the incorrect AML below, _ADR
> never gets created.
> 
> definition_block(...)
> {
>    Scope (\_SB)
>    {
>      Device (PCI0){...}
>      Name (OBJ1, 0x0)
>      OBJ1 = PCI0 + 5 // Results in an operand error.
>    } // \_SB not closed
> 
>    // parser looks for \_SB._SB.PCI0, results in AE_NOT_FOUND error
>    // Entire scope block gets skipped.
>    Scope (\_SB.PCI0)
>    {
>        Name (_ADR, 0x0)
>    }
> }
> 
> Fix the above error by properly completing the initial \_SB scope
> after an error by clearing errors that occur during table load. In
> the above case, this means that OBJ1 = PIC0 + 5 is skipped.
> 
> Fixes: 5088814a6e93 (ACPICA: AML parser: attempt to continue loading table after error)
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=200363
> Tested-by: Bastien Nocera <hadess@hadess.net>
> Signed-off-by: Erik Schmauss <erik.schmauss@intel.com>
> Cc: 4.17+ <stable@vger.kernel.org> # 4.17+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> (cherry picked from commit 73c2a01c52b657f4a0ead6c95f64c5279efbd000)
> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>

Acked-by: Tyler Hicks <tyhicks@canonical.com>

Tyler

> ---
>  drivers/acpi/acpica/psloop.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/acpi/acpica/psloop.c b/drivers/acpi/acpica/psloop.c
> index f437c3d54086..a5f35ff54510 100644
> --- a/drivers/acpi/acpica/psloop.c
> +++ b/drivers/acpi/acpica/psloop.c
> @@ -513,6 +513,18 @@ acpi_status acpi_ps_parse_loop(struct acpi_walk_state *walk_state)
>  			status =
>  			    acpi_ps_create_op(walk_state, aml_op_start, &op);
>  			if (ACPI_FAILURE(status)) {
> +				/*
> +				 * ACPI_PARSE_MODULE_LEVEL means that we are loading a table by
> +				 * executing it as a control method. However, if we encounter
> +				 * an error while loading the table, we need to keep trying to
> +				 * load the table rather than aborting the table load. Set the
> +				 * status to AE_OK to proceed with the table load.
> +				 */
> +				if ((walk_state->
> +				     parse_flags & ACPI_PARSE_MODULE_LEVEL)
> +				    && status == AE_ALREADY_EXISTS) {
> +					status = AE_OK;
> +				}
>  				if (status == AE_CTRL_PARSE_CONTINUE) {
>  					continue;
>  				}
> @@ -710,6 +722,20 @@ acpi_status acpi_ps_parse_loop(struct acpi_walk_state *walk_state)
>  			    acpi_ps_next_parse_state(walk_state, op, status);
>  			if (status == AE_CTRL_PENDING) {
>  				status = AE_OK;
> +			} else
> +			    if ((walk_state->
> +				 parse_flags & ACPI_PARSE_MODULE_LEVEL)
> +				&& ACPI_FAILURE(status)) {
> +				/*
> +				 * ACPI_PARSE_MODULE_LEVEL means that we are loading a table by
> +				 * executing it as a control method. However, if we encounter
> +				 * an error while loading the table, we need to keep trying to
> +				 * load the table rather than aborting the table load. Set the
> +				 * status to AE_OK to proceed with the table load. If we get a
> +				 * failure at this point, it means that the dispatcher got an
> +				 * error while processing Op (most likely an AML operand error.
> +				 */
> +				status = AE_OK;
>  			}
>  		}
>  
> -- 
> 2.17.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff mbox series

Patch

diff --git a/drivers/acpi/acpica/psloop.c b/drivers/acpi/acpica/psloop.c
index f437c3d54086..a5f35ff54510 100644
--- a/drivers/acpi/acpica/psloop.c
+++ b/drivers/acpi/acpica/psloop.c
@@ -513,6 +513,18 @@  acpi_status acpi_ps_parse_loop(struct acpi_walk_state *walk_state)
 			status =
 			    acpi_ps_create_op(walk_state, aml_op_start, &op);
 			if (ACPI_FAILURE(status)) {
+				/*
+				 * ACPI_PARSE_MODULE_LEVEL means that we are loading a table by
+				 * executing it as a control method. However, if we encounter
+				 * an error while loading the table, we need to keep trying to
+				 * load the table rather than aborting the table load. Set the
+				 * status to AE_OK to proceed with the table load.
+				 */
+				if ((walk_state->
+				     parse_flags & ACPI_PARSE_MODULE_LEVEL)
+				    && status == AE_ALREADY_EXISTS) {
+					status = AE_OK;
+				}
 				if (status == AE_CTRL_PARSE_CONTINUE) {
 					continue;
 				}
@@ -710,6 +722,20 @@  acpi_status acpi_ps_parse_loop(struct acpi_walk_state *walk_state)
 			    acpi_ps_next_parse_state(walk_state, op, status);
 			if (status == AE_CTRL_PENDING) {
 				status = AE_OK;
+			} else
+			    if ((walk_state->
+				 parse_flags & ACPI_PARSE_MODULE_LEVEL)
+				&& ACPI_FAILURE(status)) {
+				/*
+				 * ACPI_PARSE_MODULE_LEVEL means that we are loading a table by
+				 * executing it as a control method. However, if we encounter
+				 * an error while loading the table, we need to keep trying to
+				 * load the table rather than aborting the table load. Set the
+				 * status to AE_OK to proceed with the table load. If we get a
+				 * failure at this point, it means that the dispatcher got an
+				 * error while processing Op (most likely an AML operand error.
+				 */
+				status = AE_OK;
 			}
 		}