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

[Igor Mammedov]

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:
> > > > > > 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.
> > 
> > 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 and I still doubt that explicit recording of every
allocation is necessary. I'd rather use QOM tree for AML objects.

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. With
external  init/free_all API to allow explicitly do this operations
before/after table is build. It would still provide explicit
alloc/free but would keep AML API clean/simple since we won't have to
pass alloc list to every API function which are called quite a lot
times. It also would allow transparently for users switch from this
allocation scheme to QOM one when such need arises.

> > only wonder how things would look with Igor's option 2.2 above.
> > The caller still only needs to free the final table, but it
> > also becomes safe to use the same object references multiple
> > times before freeing the table. Using QOM also seems reasonable
> > to me, as it appears it's the accepted way to do garbage
> > collection in QEMU. Is it possible to do 2.2 with QOM?
> 
> I'd rather not go there: QOM was really invented for introspection,
> and for long-lived heavy-weight objects.  And to me, code using QOM is
> harder to understand than simple alloc/free. It's worth it where we
> need the features it offers, e.g. it has run-time checks where we
> previously just did a cast. But in this case I'd rather use something
> simpler and with compile-time checks.

I'll post example on top of v2 that does it QOM way as Drew suggested
for discussion. Resut looks relatively simple and keeps API clean,
it replaces alloc/free with widely used object_new/object_unref
and allows to free table tree including all its children in one call
object_unref(table).
It also allows to extend generic AML object to types like AML tables
blob (there is patch in example) or AML table (haven't tried yet but
see need for it) and automatically enforces type checks whenever we do
casts, and we have to do casts to access type specific fields
internally. At the same time it keeps API simple and uniform (single
typed Aml* as parameters/return values) from user POV.
Making type for every AML object probably would be overkill right now
but we could do it for generic AML object, AML table and AML tables
blob objects, that helps to generalize table building functions that
has been just copied from x86 to ARM series in latest ACPI for ARM
series. Later if there would be need it would be possible to extend
AML types internally without touching/breaking API users.

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