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

[Bandan Das]

Andreas Färber <address@hidden> writes:

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

That's ok. I only regret that despite being posted as a RFC and way before
the 2.0 freeze deadline, the patches received absolutely no feedback. If 
we had discussed Paolo's objections when the patches were posted, someone
might have pointed out what could go wrong.

Bandan

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