Bug 6514 - ACPI-CA memory leak due to optionally stored AML Object passed through "child" Method
Summary: ACPI-CA memory leak due to optionally stored AML Object passed through "child...
Status: CLOSED CODE_FIX
Alias: None
Product: ACPI
Classification: Unclassified
Component: ACPICA-Core (show other bugs)
Hardware: i386 Linux
: P2 normal
Assignee: fiodor.f.suietov
URL:
Keywords:
: 7163 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-05-08 04:32 UTC by fiodor.f.suietov
Modified: 2006-12-02 10:47 UTC (History)
5 users (show)

See Also:
Kernel Version: 2.6
Subsystem:
Regression: ---
Bisected commit-id:


Attachments
ASL code to reproduce bug (360 bytes, text/plain)
2006-05-08 04:46 UTC, fiodor.f.suietov
Details
Proposed patch (669 bytes, patch)
2006-05-08 05:01 UTC, fiodor.f.suietov
Details | Diff
Proposed patch 2 (1.38 KB, patch)
2006-05-08 05:19 UTC, fiodor.f.suietov
Details | Diff
Patch for 2.6.16/2.6.17 (2.56 KB, patch)
2006-09-20 13:00 UTC, fiodor.f.suietov
Details | Diff

Description fiodor.f.suietov 2006-05-08 04:32:09 UTC
Most recent kernel where this bug did not occur:
Distribution:
Hardware Environment:
Software Environment:
Problem Description:
The Linux kernel ACPI interpreter fails the following AML test
when it is compiled into a simulator.
So if an OEM BIOS includes this code, Linux would fail.

Steps to reproduce:
Run interpreter with the .asl demo code attached below.
Comment 1 fiodor.f.suietov 2006-05-08 04:46:30 UTC
Created attachment 8042 [details]
ASL code to reproduce bug


ACPICA AML interpreter (acpiexec) should be ran with -s flag (slack mode).

Output of the demo:

Executing \MAIN
[ACPI Debug]  Integer: 0x00000000000000F1
Outstanding: 0x1 allocations after execution
Execution of \MAIN returned object 00326E38 Buflen 10
  [Integer] = 00000000000000F1
Comment 2 fiodor.f.suietov 2006-05-08 04:50:43 UTC
DESCRIPTION: 

In ACPICA AML interpreter Slack mode outstanding allocations
are detected when AML-code looks like the following lines:

Method (SBYT, 1)
{
    Store (Arg0, Debug)
}

Method(MAIN)
{
    Or (0xf0, 0x01, Local0)
    SBYT (Local0)
}

The problem was initially held up by FreeBSD:
> -----Original Message-----
> From: Nate Lawson [mailto:nate@root.org]
> Sent: Monday, February 13, 2006 2:16 PM
> To: Jung-uk Kim
> Cc: Moore, Robert
> Subject: Re: ACPI-CA memory leak
> 
> Judging from your patch, my thought is that the store from Local1 to
> DATS is not being reference-counted properly and so the local object is
> never freed.

The above proposed example reproduces a very probable way of that case.

Both optional storing to Local0 and manipulation with it in a called
Method (SBYT) are essential here.

Note: there is no any outstanding allocations when
      Store (Or (0xf0, 0x01), Local0) is used instead
      of Or(0xf0, 0x01, Local0).

Looks like the following code in the function AcpiDsDoImplicitReturn
(file interpreter/dispatcher/dsutils.c) takes effect in our case:

    /*
     * Delete any "stale" implicit return value first. However, in
     * complex statements, the implicit return value can be
     * bubbled up several levels, so we don't clear the value if it
     * is the same as the ReturnDesc.
     */
    if (WalkState->ImplicitReturnObj)
    {
        if (WalkState->ImplicitReturnObj == ReturnDesc)
        {
            return (TRUE);
        }
        AcpiDsClearImplicitReturn (WalkState);
    }

The RefCount of the Local0 object (from the example) incremented for
MAIN--WalkState->ImplicitReturnObj was additionally incremented for
the terminated method SBYT--WalkState->ImplicitReturnObj (ReturnDesc).
So, after returning from the called method (SBYT) to the current method
(MAIN) the previous MAIN--WalkState->ImplicitReturnObj should explicitly
be cleared through the call to AcpiDsClearImplicitReturn.

The following patch can solve this problem:

--- dsutils.c.orig	2006-03-28 12:48:09.146714200 +0400
+++ dsutils.c	2006-03-28 12:48:37.087169200 +0400
@@ -228,8 +228,4 @@
     if (WalkState->ImplicitReturnObj)
     {
-        if (WalkState->ImplicitReturnObj == ReturnDesc)
-        {
-            return (TRUE);
-        }
         AcpiDsClearImplicitReturn (WalkState);
     }

Although ASL tests are not specifically designed to cover Slack mode issues,
this problem evidently is touched by them. There are different numbers of
outstanding allocations on Slack and non-Slack mode. After this patch some
Slack numbers became less than before.

However there is no certainty now that the proposed update is the exact
solution of the problem. This bug should be additionally investigated along
with other events of outstanding allocations.

INTERNAL BUG NUMBER:

   211
Comment 3 fiodor.f.suietov 2006-05-08 04:56:45 UTC
Additional consideration.

The same result (outstanding allocation) takes place if
explicit Return operator will be used in SBYT method above:

Method (SBYT, 1)
{
-    Store (Arg0, Debug)
+    Return (Arg0)
}

Evidently the RefCount of the original Local0 object (passed
into SBYT) incremented for SBYT--WalkState->ReturnDesc gives
the same negative effect.

The mentioned above fragment of AcpiDsDoImplicitReturn resolves
Implicit Return processing issue when the same Object could become
the Result Object of several successive AML operators ("bubbled up
several levels"). In that case would be superfluous decrementing Object's
RefCount (by means AcpiDsClearImplicitReturn) to increment it then:

    /* Save the implicit return value, add a reference if requested */

    WalkState->ImplicitReturnObj = ReturnDesc;
    if (AddReference)
    {
        AcpiUtAddReference (ReturnDesc);
    }

The AddReference parameter of AcpiDsDoImplicitReturn is FALSE
above when this function is invoked after an AML method return so
that RefCount of the previous Implicit Return Object is decremented
and can become Zero (the Object will be deleted if any) but RefCount
of the new Implicit Return Object will not be incremented. This is
right for a called method Return Object in the most cases but does
not work when it is the same as the previous Implicit Return Object.

That additional issue could be fixed by the following patch:

diff -r -u source3/components/interpreter/dispatcher/dsutils.c 
source/components/interpreter/dispatcher/dsutils.c
--- source3/components/interpreter/dispatcher/dsutils.c	2006-04-24 
12:25:15.000000000 +0400
+++ source/components/interpreter/dispatcher/dsutils.c	2006-05-06 
11:00:06.979158800 +0400
@@ -228,6 +228,10 @@
     {
         if (WalkState->ImplicitReturnObj == ReturnDesc)
         {
+            if (!AddReference)
+            { /* Delete additional reference corresponding to ReturnDesc */
+                AcpiUtRemoveReference (ReturnDesc);
+            }
             return (TRUE);
         }
         AcpiDsClearImplicitReturn (WalkState);
Comment 4 fiodor.f.suietov 2006-05-08 05:01:07 UTC
Created attachment 8043 [details]
Proposed patch

The update was tested by the ASLTS test suite runs
(all the test cases) on the following systems:

- CYGWIN_NT-5.1 1.5.18(0.132/4/2) 2005-07-02 20:30 i686 unknown unknown Cygwin
- Linux 2.6.17-rc1-mm2 #1 SMP Thu Apr 13 14:03:28 MSD 2006 i686 i686 i386
GNU/Linux

for the following (all) modes supported by the ASLTS test suite:

- 32-bit norm mode
- 64-bit norm mode
- 32-bit slack mode
- 32-bit slack mode

The tests were run (on the systems described above) before update,
then the tests were run on those systems after update. Then results
of two runs were compared. All is Ok (the numbers of outstanding
allocations after update in norm mode are the same and in slack mode
are equal to or less than before update, no new errors).
Comment 5 fiodor.f.suietov 2006-05-08 05:15:13 UTC
Additional consideration 2.

Actually a relevant to the proposed patch action is
provided for normal Return Object processing in the function
AcpiDsRestartControlMethod (file interpreter/dispatcher/dsmethod.c)
if "the return value will not be used by the calling method":

        if (WalkState->ReturnUsed)
        {
            /* Save the return value from the previous method */
...
        else if (!AcpiDsDoImplicitReturn (ReturnDesc, WalkState, FALSE))
        {
            /*
             * Delete the return value if it will not be used by the
             * calling method
             */
            AcpiUtRemoveReference (ReturnDesc);
        }

So the following patch concerned with AcpiDsRestartControlMethod
and resolving the problem too looks like more preferable:

diff -r -u source3/components/interpreter/dispatcher/dsmethod.c 
source/components/interpreter/dispatcher/dsmethod.c
--- source3/components/interpreter/dispatcher/dsmethod.c	2006-04-24 
12:25:14.000000000 +0400
+++ source/components/interpreter/dispatcher/dsmethod.c	2006-05-08 
14:36:20.791169200 +0400
@@ -492,6 +492,7 @@
     ACPI_OPERAND_OBJECT     *ReturnDesc)
 {
     ACPI_STATUS             Status;
+    UINT32                  IsImplicitSelfsame;
 
 
     ACPI_FUNCTION_TRACE_PTR (DsRestartControlMethod, WalkState);
@@ -511,6 +512,9 @@
 
     if (ReturnDesc)
     {
+        /* Opportunely calculate Implicit Return specific condition */
+        IsImplicitSelfsame = (WalkState->ImplicitReturnObj == ReturnDesc);
+
         /* Are we actually going to use the return value? */
 
         if (WalkState->ReturnUsed)
@@ -539,7 +543,12 @@
          * NOTE: this is optional because the ASL language does not actually
          * support this behavior.
          */
-        else if (!AcpiDsDoImplicitReturn (ReturnDesc, WalkState, FALSE))
+        else if (!AcpiDsDoImplicitReturn (ReturnDesc, WalkState, FALSE) ||
+                /*
+                 * Take into account "complex statements"
+                 * processing in AcpiDsDoImplicitReturn
+                 */
+                IsImplicitSelfsame)
         {
             /*
              * Delete the return value if it will not be used by the
Comment 6 fiodor.f.suietov 2006-05-08 05:19:43 UTC
Created attachment 8044 [details]
Proposed patch 2
Comment 7 Robert Moore 2006-05-12 14:26:24 UTC
Essentials of patch 2 integrated and released in ACPICA version 20060512
Comment 8 Len Brown 2006-06-25 21:49:46 UTC
ACPICA 20060608 shipped in 2.6.17-git9, closed. 
Comment 9 Len Brown 2006-09-19 19:42:53 UTC
*** Bug 7163 has been marked as a duplicate of this bug. ***
Comment 10 Len Brown 2006-09-19 19:46:33 UTC
re-opening because we need a version of this patch for 2.6.16/2.6.17 
 
Comment 11 fiodor.f.suietov 2006-09-20 13:00:57 UTC
Created attachment 9059 [details]
Patch for 2.6.16/2.6.17 

Extracted from patch-2.6.18.rc1
Comment 12 Thomas Renninger 2006-09-25 00:16:51 UTC
Thanks for also adding the backport. It's very much appreciated!
If there pop up similar things, it would be nice if you can add me into CC list 
if I am not already. It's very easy to oversee such important fixes.

Thanks.
Comment 13 Adrian Bunk 2006-12-02 10:47:44 UTC
Fixed since 2.6.18, patch for older kernels available.

Note You need to log in before you can comment on or make changes to this bug.