qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH v2] hw/pci-bridge: Fix release ordering by embedding PCIB


From: Thomas Huth
Subject: Re: [RFC PATCH v2] hw/pci-bridge: Fix release ordering by embedding PCIBridgeWindows within PCIBridge
Date: Thu, 27 Apr 2023 10:22:46 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0

On 21/04/2023 14.25, Jonathan Cameron wrote:
The lifetime of the PCIBridgeWindows instance accessed via the windows pointer
in struct PCIBridge is managed separately from the PCIBridge itself.

Triggered by ./qemu-system-x86_64 -M x-remote -display none -monitor stdio
QEMU monitor: device_add cxl-downstream

In some error handling paths (such as the above due to attaching a 
cxl-downstream
port anything other than a cxl-upstream port) the g_free() of the PCIBridge
windows in pci_bridge_region_cleanup() is called before the final call of
flatview_uref() in address_space_set_flatview() ultimately from
drain_call_rcu()

At one stage this resulted in a crash, currently can still be observed using
valgrind which records a use after free.

When present, only one instance is allocated. pci_bridge_update_mappings()
can operate directly on an instance rather than creating a new one and
swapping it in.  Thus there appears to be no reason to not directly
couple the lifetimes of the two structures by embedding the PCIBridgeWindows
within the PCIBridge removing the need for the problematic separate free.

Patch is same as was posted deep in the discussion.
20230403171232.000020bb@huawei.com/">https://lore.kernel.org/qemu-devel/20230403171232.000020bb@huawei.com/

Posted as an RFC as only lightly tested and I'm not sure what the reasoning
behind the separation of lifetimes originally was. As such perhaps this is
not the best route to fixing the issue.

I'm pretty ignorant when it comes to flatviews etc., but from my distant point of view, this rather sounds like a workaround than a real fix ... anyway, embedding the structure instead of allocating it is certainly a good change anyway, and it fixes the problem for this rather obscure scenario, so I'm fine if we only go ahead with this patch.

Reviewed-by: Thomas Huth <thuth@redhat.com>




reply via email to

[Prev in Thread] Current Thread [Next in Thread]