Re: [Qemu-devel] Use-after-free during unrealize in system_reset

[Andreas Färber]

Am 09.06.2014 19:02, schrieb Bandan Das:
> Paolo Bonzini <address@hidden> writes:
> 
>> Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
>>> Tested-by: Michael S. Tsirkin <address@hidden>
>>
>> You probably tested the reversal, actually. :)
>>
>> Actually, there is a reason for it.  "Unassembling" the device
>> (unparent) should come after "powering it down" (unrealize).
> 
> Yes, exactly. But to be honest, I was not sure if I was doing the 
> right thing and hence posted this series as a RFC.
> 
>> However, the bus is missing a recursive unrealization of the devices
>> below it prior to calling bc->unrealize:
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index e65a5aa..4282491 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -567,32 +567,35 @@ static void bus_set_realized(Object *obj, bool
>> value, Error **errp)
>>  {
>>      BusState *bus = BUS(obj);
>>      BusClass *bc = BUS_GET_CLASS(bus);
>> +    BusChild *kid;
>>      Error *local_err = NULL;
>>
>>      if (value && !bus->realized) {
>>          if (bc->realize) {
>>              bc->realize(bus, &local_err);
>> -
>> -            if (local_err != NULL) {
>> -                goto error;
>> -            }
>> -
>>          }
>> +
>> +        /* TODO: recursive realization */
>>      } else if (!value && bus->realized) {
>> -        if (bc->unrealize) {
>> +        QTAILQ_FOREACH(kid, &bus->children, sibling) {
>> +            DeviceState *dev = kid->child;
>> +            object_property_set_bool(OBJECT(dev), false, "realized",
>> +                                     &local_err);
>> +            if (local_err != NULL) {
>> +                break;
>> +            }
>> +        }
>> +        if (bc->unrealize && local_err == NULL) {
>>              bc->unrealize(bus, &local_err);
> 
> It also seems that my original patch included unrealizing children, but
> that didn't get in for some reason -
> https://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03204.html
> Andreas might have had a reason not to include it however..

Yes, we had talked about the future recursive (un)realization at KVM
Forum 2013, but your RFC patch implementing this for buses seemed to get
ahead of the game. I therefore took only the part of the patch that did
not contradict Paolo's objection to unchecked recursive realization -
and I was in a hurry to get it into 2.0 before the freeze, sorry.

Doing only recursive unrealization sounds like a good compromise to me,
since the concerns we had were all about recursive realization and
prereqs not being realized yet. Only suggestion would be to do the error
handling refactoring in a separate patch, but it'll better align with
the qdev.c code then.

Still, isn't this an indication that devices relied on the PCI bus bug
of not unrealizing its state all the time and we may need to go back as
far as ~1.7 (the initial finalize based fix) for resolving it?

Regards,
Andreas

> 
>> -
>> -            if (local_err != NULL) {
>> -                goto error;
>> -            }
>>          }
>>      }
>>
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>>      bus->realized = value;
>> -    return;
>> -
>> -error:
>> -    error_propagate(errp, local_err);
>>  }
>>
>>  void qbus_create_inplace(void *bus, size_t size, const char *typename,
>>
>>
>> This seems to fix the bug too.
>>
>> Paolo
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg