Re: [Qemu-devel] [Fix PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region

[Paolo Bonzini <pbonzini@redhat.com>]

On 05/10/2015 18:53, Stefano Stabellini wrote:
>> This patch is to fix the issue via moving MSIX MMIO memory region into
>> struct XenPCIPassthroughState and free it together with pt device's obj.
> 
> Given that all the MSI-X related info are in XenPTMSIX, I would prefer
> to keep the mmio memory region there, if possible.
> 
> Couldn't you just unhook msix->mmio from XenPCIPassthroughState's object
> in xen_pt_msix_delete?  Calling object_property_del_child or
> object_unparent?

This is the right thing to do, but there are two separate things to fix.

One is the use-after-free of msix->mmio, the other is that freeing 
s->msix and in general xen_pt_config_delete should be done from the 
.instance_finalize callback.  This is documented in docs/memory.txt.

This is an attempt at a patch (not even compiled):

diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index e3d7194..e476bac 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -610,7 +610,7 @@ error_out:
     return rc;
 }
 
-void xen_pt_msix_delete(XenPCIPassthroughState *s)
+void xen_pt_msix_unmap(XenPCIPassthroughState *s)
 {
     XenPTMSIX *msix = s->msix;
 
@@ -627,6 +627,17 @@ void xen_pt_msix_delete(XenPCIPassthroughState *s)
     }
 
     memory_region_del_subregion(&s->bar[msix->bar_index], &msix->mmio);
+}
+
+void xen_pt_msix_delete(XenPCIPassthroughState *s)
+{
+    XenPTMSIX *msix = s->msix;
+
+    if (!msix) {
+        return;
+    }
+
+    object_unparent(&msix->mmio);
 
     g_free(s->msix);
     s->msix = NULL;

where xen_pt_config_unmap would be called from xen_pt_destroy, and the call
to xen_pt_config_delete would be moved to xen_pci_passthrough_info's
instance_finalize member.

Paolo