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

[Marcel Apfelbaum <marcel@redhat.com>]

On 01/28/2015 12:29 AM, 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
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.
Thanks,
Marcel

[...]