Re: [PATCH 1/2] qom: Reject temporary object resurrection

On 2026/06/15 17:34, Kevin Wolf wrote:
> Am 15.06.2026 um 06:11 hat Akihiko Odaki geschrieben:
>> If object_ref() is called during finalization, it will temporarily
>> "resurrect" the object. Although object_finalize() asserts that no
>> resurrecting reference remains before freeing the object, the assertion
>> cannot catch the case where the resurrecting reference is dropped before
>> freeing the object.
> 
> Ok, and why is that a problem?
> 
> In the cover letter, you referred to an earlier patch you had posted:
> 
>      The temporary object resurrection patch originated from:
>      https://lore.kernel.org/qemu-devel/[email protected]/
>      ("[PATCH v2 1/3] qom: Do not finalize twice")
> 
>  From that patch, it's clear that one problem is that .finalize() can be
> called multiple times, which you probably don't want. But what is the
> reason for switching from just ignoring the second .finalize() like in
> the old patch to completely forbidding a temporary refcount increase?

None. I thought merely skipping finalization is not a well-defined 
semantics (e.g., breaking Rust safety guarantee), but, reconsidering 
now, I think that's fine.

> 
>> One way to catch this would be to add a check in object_ref() to reject
>> resurrecting references before they are created. However, object_ref()
>> is frequently called so it is better to minimize the overhead.
> 
> We're talking about a single comparison with zero here. So if that's the
> worse alternative, the other one can't have any drawbacks.

True.

> 
>> To avoid adding the overhead, change how the reference count is
>> represented and let an existing assertion detect resurrection. More
>> concretely, obj->ref now stores the current reference count minus 1. The
>> stored count is therefore 0 after object_initialize(), and the final
>> object_unref() decrements it from 0 to UINT32_MAX and starts
>> finalization. A resurrecting object_ref() will then trip the existing
>> obj->ref < INT_MAX assertion.
> 
> A refcount field where 0 means that a last reference is still remaining
> is highly confusing. If we do want to forbid taking a temporary new
> reference in .finalize() for some reason, the explicit check in
> object_ref() sounds much better to me.

The rationale here is that the field is mostly contained in qom/object.c 
so the awkward semantics won't have a wide impact. But it's just 
"mostly" and there are two exceptions: block/throttle-groups.c and 
tests/unit/check-qom-proplist.c.

I withdraw this patch for the drawbacks discussed above.

Regards,
Akihiko Odaki