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

[Igor Mammedov]

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: 
> > > http://www.cs.hunter.cuny.edu/~sweiss/course_materials/csci493.70/lecture_notes/GTK_memory_mngmt.pdf
> > >    In particular adopt behavior of GInitiallyUnowned usage model
> > > 
> > >    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 below.
> 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)
> or
>  * 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_append(method,
>       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);
>   aml_free(store);
>   aml_free(loc0);
>   aml_free(str);

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_append(dev,
    aml_name_def(p, "_STA", aml_int(p, 0xF /* present */))
);
aml_append(ssdt, dev);

aml_append(acpi_tables_blob, ssdt);

free_aml_alloc_list(p);


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
content.

> 
> > 
> > Good idea! I think this will address the issue.
> > 
> > 
> > > 3. talloc implementation Amit've mentioned,
> > >    perhaps it might work since it allows to set destructors for
> > >    managed pointers. With this we might get clear abort when
> > >    dereferencing freed pointer see talloc_set()
> > 
> > 
> > I think it's a separate discussion. Maybe talloc is a good
> > allocator to use in qemu, but using a separate allocator
> > just for acpi generation would be an overkill.
> > 
> > > 
> > > > 
> > > > Just pass AcpiAml* everywhere, add APIs to allocate and free it
> > > > together with the internal buffer.
> > > > This makes it trivial to see that value is not misused:
> > > > just check it's between alloc and free - and that there are
> > > > no leaks - just check we call free on each value.
> > > > We can write a semantic patch to catch missing free calls,
> > > > it's easy.
> > > > 
> > > > 
> > > > > > 
> > > > > > > As for moving to to another file, during all this series lowlevel
> > > > > > > build_(some_aml_related_costruct_helper)s are moved into this file
> > > > > > > and should be make static to hide from user lowlevel helpers
> > > > > > > (including build_package).
> > > > > > > That will leave only high level API available.
> > > > > > > 
> > > > > > > TODO for me: make sure that moved lowlevel helpers are static
> > > > > > > 
> > > > > > >