On 2025/11/13 7:39, Peter Xu wrote:
On Sat, Nov 08, 2025 at 11:07:46AM +0900, Akihiko Odaki wrote:
On 2025/11/08 1:40, Peter Xu wrote:
On Fri, Nov 07, 2025 at 11:16:56AM +0900, Akihiko Odaki wrote:
On 2025/11/07 2:50, Peter Xu wrote:
On Thu, Nov 06, 2025 at 11:23:32AM +0900, Akihiko Odaki wrote:
Generally speaking, we will not necessarily "always" get an issue report
when things went wrong with memory management. A bug in memory management
may not cause an immediate crash but corrupt the memory state which you will
find only later. The end result of memory corruption may look random and
result in a hard-to-debug issue report. A user may not even bother writing
an issue report at all; this is especially true for this kind of corner
cases that rarely happen.
There should have been no such a hazard of memory corruption if the code did
exactly what the documentation said in the first place. The consistency of
the code and the documentation is essential, especially for this kind of
complex and fundamental code.
Do you have encountered such case before?
I wasn't expecting that, because what you were saying looks more like what
Linux kernel would have a bug in mm. QEMU is still special as it has the
default unassigned MR trapping everything by default, meanwhile normally
what is moving is MMIO / alias regions rather than real ramblocks. It
means when things go wrong we have much higher chance to trap them
properly.
When I said "memory management" I meant the methods we use to allocate and
free memory (the Linux equivalents would be kmalloc()/free()/kref), not the
MM tracking or unassigned MR trapping behavior you mentioned. The unassigned
MR trap and MMIO/alias movement are a separate concern and don’t change the
underlying risk.
Concrete example: imagine an alias is allocated with g_new() and freed
immediately after object_unparent(). If that alias accidentally becomes the
FlatView root, destroying the FlatView later will call memory_region_unref()
and produce a use-after-free. We cannot predict what memory_region_unref()
will read or write in that scenario — the result can be arbitrary memory
corruption that surfaces much later as a hard-to-debug, intermittent
problem. Users often won’t file an issue for these rare corner cases.
OK I see what you meant now. Yes it's a valid concern.
I also confess though that I'm pretty conservative on fixing things with
hypothetical issues. In general, I prefer fixing things with explicit
problems, so we know how to measure and justify a fix (depending on how
aggressive the fix is and how much maintanence burden it will bring to
QEMU). Without a real problem, it's harder to quantify that even if such
evaluation will also normally be subjective too.
Regarding your preference to fix only explicit problems: I understand the
conservatism, but here are the facts we need to weigh:
- The documentation claims we may free aliases because
memory_region_ref() is never called, yet there is code that does call
memory_region_ref().
- The patch adds code to align behavior with the documentation.
The significance of both potential impacts (the behavioral divergence for
devices other than pci-bridge, and the added complexity needed for
consistency) may be subjective and hypothetical, but that applies equally to
both sides.
In this case, the long-term reliability and maintainability of QEMU depend
on having the code behave as documented. Correctness should take precedence
over simplicity.
Fair enough.
Let's then still try to see whether we can brainstorm something that can
still at least avoid the "let's clear a remote pointer in a finalize(),
because it looks safe" approach.. I'm not sure if I'm the only one that
feels nervous with it.
Fundamentally, if you can remotely clear a pointer, it means it's not used
at all. In practise, that's correct because as I also discussed before I
don't think RCU readers use flatview->root at all. It was almost only
because we have some very light references on flatview->root. The major
"hidden" reference is actually the key in flat_views hash, however I don't
think it will have any stale root MR VA as key, as long as after a proper
commit() completes.
"As long as after a proper commit() completes" is what we are trying to
avoid. Think of the following sequence:
memory_region_transaction_begin()
object_unparent(mr)
g_free(mr)
mr = g_new0(MemoryRegion, 1) // reuse the storage for another MR
address_space_init(as, mr, "as")
memory_region_transaction_commit()
address_space_init() will use the value keyed with the dangling pointer in
flat_views.
I agree with your analysis, however I don't think such use case exists in
practise..
I added below and qemu's qtests run all smooth:
void address_space_init(AddressSpace *as, MemoryRegion *root, const char
*name)
{
+ assert(memory_region_transaction_depth == 0 &&
+ memory_region_update_pending == false);
memory_region_ref(root);
as->root = root;
as->current_map = NULL;
We can either keep the assertion (until it'll trigger any time, but I doubt
it..), or we can also solve this by other ways, I can think of two right
now:
It is a convoluted way to express that flat_views may lose a valid
reference of a memory region at some point. This patch explicitly states
that.
(1) Make sure address_space_init() skips the last two steps to update
topology and ioeventfds if memory_region_transaction_depth>0.
(2) Introduce one more field for MemoryRegion, which is to allocate a
globally unique KEY for the flat_views hash, only needed if the MR
can be root candidates.
(2) will need more work, as we may need to have some way to make sure the
allocator will not generate duplicate KEYs (hence u64 counter may not be
ideal, as it may provide duplicates when wraps over u64.. even though I
also don't know if people will hit it at all..).
So I wonder if we can just keep the solution (0) above, which is to assert
it.
Removing a dangling reference is safer than leaving it. Some code that
assumes the reference will not be gone may get surprised and cause a NULL
dereference, but that almost always result in SIGSEGV instead of memory
corruptions. Such segfault is as easy to debug as SIGABRT triggered by
assertions.
Yes it'll be easier to debug indeed. However IMHO this is still a bad plan
to wait for it whenever overlooked. It'll still be a disaster if a
customer crashed on the NULL pointer reference somewhere, so IMHO we should
avoid the first crash from design.
I confess I also added some assertions above. However since we don't have
a huge lot of object_unparent(mr) use cases, we can walk all of them and
make sure the assertion will never happen. IMHO it's still slightly better
than remote pointer resets.
Pointer resetting doesn't even require walking all of object_unparent(),
so in this sense pointer resetting is better.
Regards,
Akihiko Odaki
