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
