[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_appen

From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()
Date: Thu, 05 Feb 2015 16:09:06 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0

On 01/28/2015 12:29 AM, Igor Mammedov wrote:
On Mon, 26 Jan 2015 18:17:55 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

On Mon, Jan 26, 2015 at 04:34:01PM +0100, Andrew Jones wrote:
On Mon, Jan 26, 2015 at 04:09:20PM +0100, Igor Mammedov wrote:
On Mon, 26 Jan 2015 10:57:21 +0100
Igor Mammedov <address@hidden> wrote:

On Sat, 24 Jan 2015 18:33:50 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

On Fri, Jan 23, 2015 at 06:56:20PM +0100, Igor Mammedov wrote:
On Fri, 23 Jan 2015 15:55:11 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

I refuse to give up on cleaner and simpler API yet :)

Your patches are almost there, they are pretty clean, the
only issue I think is this passing of AcpiAml by value,
sometimes freeing buffer in the process, sometimes not.
Currently buffer is allocated by API and is always freed
whenever it's passed to another API function.
That's why it makes user not to care about memory mgmt.

The only limitation of it is if you store AcpiAml return
value into some variable you are responsible to use it only
once for passing to another API function. Reusing this
variable's value (pass it to API function second time)
would cause cause use-after-free and freeing-freed bugs.
Like this: AcpiAml table =
acpi_definition_block("SSDT",...); AcpiAml scope =
acpi_scope("PCI0"); aml_append(&table, scope); // <- here
scope becomes invalid // a bug
aml_append(&table, scope); // use-after-free +
freeing-freed bugs

There are several approaches to look for resolving above
issues: 1. Adopt and use memory mgmt model used by GTK+
    in nutshell:
In particular adopt behavior of GInitiallyUnowned usage

    that will allow to keep convenient chained call style
and if necessary reuse objects returned by API by
explicitly referencing/dereferencing them if needed.

Hmm, it's still easy to misuse. I think I prefer option 2
That's basically what we have/use in QOM with object_new(FOO) +
object_unref() I have no idea why we invented our own Object
infrastructure when we could just use GObject one from already
used glib.

2. It's possible to drop freeing inside API completely and
    record(store in list) every new object inside a table
context. When table is constructed, list of created objects
could be safely freed.
    With that it would be safe to reuse every AcpiAml object
    and avoid free-after-use issues with limitation that
created AcpiAml objects shouldn't be used after table was
closed. It should cover all practical use of API, i.e. no
cross table AcpiAml objects.

So each aml_alloc function gets pointer to this list,
and adds the new element there.
Eventually we do free_all to free all elements,
so there isn't even an aml_free to mis-use.
I'm thinking a little bit different about implementation though.
I still don't like the use of explicit alloc/free being called
by API user since it doesn't allow chained API calls and
I think it's unnecessary complication see below why.

Here is what's true about current API and a I'd like to with it:

   1. Every API call (except aml_append) makes aml_alloc(), it's
just like a wrapper about object_new(FOO). (current + new impl.)

   2 Every API call that takes AML type as input argument
   2.1 consumes (frees) it (current impl.)
       (it's easy to fix use after free concern too,
        just pass AML by pointer and zero-out memory before it's
freed and assert whenever one of input arguments is not correct,
        i.e. it was reused second time)
       There is no need for following steps after this one.
   2.2 takes ownership of GInitiallyUnowned and adds it to its
list of its children.
   3. Free children when AML object is destroyed (i.e. ref count
zero) That way when toplevel table object (definition block in
42/47) is added to ACPI blob we can unref it, which will cause
      its whole children tree freed, except for AML objects where
      API user explicitly took extra reference (i.e. wanted them
      to reuse in another table)

I'd prefer:
  *  2.1 way to address your current concern of use-after-free
     as the most simplest one (no reuse is possible however)
  * follow already used by QEMU QOM/GObject pattern of
    implicit alloc/free

since they allow to construct AML in a more simple/manageable
way i.e.
       aml_store(aml_string("foo"), aml_local(0)))

v.s. explicit headache of alloc/free, which doesn't fix
      use-after-free anyway and just adds more boiler plate
      plus makes code har to read read

   str = aml_alloc();
   aml_string(str, "foo");
   loc0 = aml_alloc();
   aml_local(loc0, 0);
   store = aml_alloc();
   aml_store(store, str, loc0);
   aml_append(method, store);

Here is a compromise what I and Michael came to on a phone call:

Externally API usage would look like:

AmlAllocList *p = some_list_alloc();

Aml *ssdt = aml_def_block(p, "SSDT", ...);
Aml *dev = aml_device(p, "PCI0");
     aml_name_def(p, "_STA", aml_int(p, 0xF /* present */))
aml_append(ssdt, dev);

aml_append(acpi_tables_blob, ssdt);


Each of aml_foo() will take other Aml arguments by pointer.
Also every aml_foo(), except of aml_append() will allocate
Aml struct and return pointer to it and also add this pointer
into AmlAllocList which is passed as first argument to each
aml_foo() call.
aml_append() becomes nondestructive function and just adds
child(2nd arg) to the parent context (1st arg).

After API user is done with building table and pushed it
into tables blob, he/she calls free_aml_alloc_list() to free
all Aml objects created during process of building the table

Hmm, passing 'p' around somewhat muddies an otherwise clean
interface, but the concern with aml_append silently freeing
memory still accessible by the caller is definitely valid. I
I've tried redo series with passing alloc list as first argument,
looks ugly as hell
I don't like that either.

 and I still doubt that explicit recording of every
allocation is necessary.
This would be nice since is simple, it doesn't hurt
and it releases us from the use-after-free problem.

 I'd rather use QOM tree for AML objects.
If you must :(, it looks like an overkill since those objects will live
a very short life and your proposed table does the trick.
Keeping references and a whole tree seems unnecessary.

If we still don't want to use QOM and considering that ACPI
tables are build in one thread/function, I'd prefer to hide
allocation list inside API making it static variable for now.
Why not, would be an internal static variable acceptable?
Do we have to worry of multiple threads for this?
If not, I would suggest going with that and close the day.

My opinion, of course.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]