Re: [RFC PATCH] hw/pci-bridge: Fix release ordering by embedding PCIBridgeWindows within PCIBridge

[Philippe Mathieu-Daudé]

Hi Jonathan,

On 20/4/23 16:59, Jonathan Cameron via 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.
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>   hw/pci/pci_bridge.c         | 20 ++++++++------------
>   include/hw/pci/pci_bridge.h |  3 ++-
>   2 files changed, 10 insertions(+), 13 deletions(-)


> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> index 01670e9e65..ac75ec0c1b 100644
> --- a/include/hw/pci/pci_bridge.h
> +++ b/include/hw/pci/pci_bridge.h
> @@ -30,6 +30,7 @@
>   #include "hw/pci/pci_bus.h"
>   #include "hw/cxl/cxl.h"
>   #include "qom/object.h"
> +#include "qemu/rcu.h"

Where is this header is used?

>   typedef struct PCIBridgeWindows PCIBridgeWindows;
>   
> @@ -73,7 +74,7 @@ struct PCIBridge {
>       MemoryRegion address_space_mem;
>       MemoryRegion address_space_io;
>   
> -    PCIBridgeWindows *windows;
> +    PCIBridgeWindows windows;
>   
>       pci_map_irq_fn map_irq;
>       const char *bus_name;