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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jan 27, 2015 at 11:29:09PM +0100, Igor Mammedov wrote:
> On Mon, 26 Jan 2015 18:17:55 +0200
> "Michael S. Tsirkin" <mst@redhat.com> 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 <imammedo@redhat.com> wrote:
> > > > 
> > > > > On Sat, 24 Jan 2015 18:33:50 +0200
> > > > > "Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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

Just make names shorter, something like "p" will do the trick.
It's hard to argue about subjective ugly/not ugly.

Have you seen Rusty's design manifesto?
You current code is at best at level 2:
read the implementation and you will get it right.
Moving to QOM gets you to level 4:
follow common convention and you'll get it right.
But alloc/free is at level 7:
the obvious use is (probably) the correct one.

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

Having just tried to fix QOM induced memory leaks, I'm not impressed.
Basically, the rule that people seem to agree on is that
it's always a mistake to create QOM objects in response
to guest activity, and this is what we are doing here, after all.

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