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

[Jonathan Cameron]

On Fri, 21 Apr 2023 08:26:43 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> 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

Left over garbage from a previous attempt to fix.  Good spot.
I'll clean that out and resend shortly.

Thanks,

Jonathan
 
> 
> >   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;  
>