From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51057) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZlbnL-0008GY-1a for qemu-devel@nongnu.org; Mon, 12 Oct 2015 08:01:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZlbnF-0001ay-7b for qemu-devel@nongnu.org; Mon, 12 Oct 2015 08:01:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49672) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZlbnF-0001al-08 for qemu-devel@nongnu.org; Mon, 12 Oct 2015 08:01:29 -0400 References: <1444576764-15344-1-git-send-email-tianyu.lan@intel.com> From: Paolo Bonzini Message-ID: <561BA112.5030800@redhat.com> Date: Mon, 12 Oct 2015 14:01:22 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini , Lan Tianyu Cc: xen-devel@lists.xensource.com, mjt@tls.msk.ru, qemu-devel@nongnu.org On 12/10/2015 13:09, Stefano Stabellini wrote: > On Sun, 11 Oct 2015, Lan Tianyu wrote: >> From: > >> >> msix->mmio is added to XenPCIPassthroughState's object as property. >> object_finalize_child_property is called for XenPCIPassthroughState's >> object, which calls object_property_del_all, which is going to try to >> delete msix->mmio. object_finalize_child_property() will access >> msix->mmio's obj. But the whole msix struct has already been freed >> by xen_pt_msix_delete. This will cause segment fault when msix->mmio >> has been overwritten. >> >> This patch is to fix the issue. >> >> Signed-off-by: Lan Tianyu > > Looks good to me. Paolo? Also looks good to me. Thanks! Paolo >> hw/xen/xen_pt.c | 8 ++++++++ >> hw/xen/xen_pt.h | 1 + >> hw/xen/xen_pt_config_init.c | 2 +- >> hw/xen/xen_pt_msi.c | 13 ++++++++++++- >> 4 files changed, 22 insertions(+), 2 deletions(-) >> >> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c >> index 2b54f52..aa96288 100644 >> --- a/hw/xen/xen_pt.c >> +++ b/hw/xen/xen_pt.c >> @@ -938,10 +938,18 @@ static void xen_pci_passthrough_class_init(ObjectClass *klass, void *data) >> dc->props = xen_pci_passthrough_properties; >> }; >> >> +static void xen_pci_passthrough_finalize(Object *obj) >> +{ >> + XenPCIPassthroughState *s = XEN_PT_DEVICE(obj); >> + >> + xen_pt_msix_delete(s); >> +} >> + >> static const TypeInfo xen_pci_passthrough_info = { >> .name = TYPE_XEN_PT_DEVICE, >> .parent = TYPE_PCI_DEVICE, >> .instance_size = sizeof(XenPCIPassthroughState), >> + .instance_finalize = xen_pci_passthrough_finalize, >> .class_init = xen_pci_passthrough_class_init, >> }; >> >> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h >> index 3bc22eb..c545280 100644 >> --- a/hw/xen/xen_pt.h >> +++ b/hw/xen/xen_pt.h >> @@ -305,6 +305,7 @@ void xen_pt_msi_disable(XenPCIPassthroughState *s); >> >> int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base); >> void xen_pt_msix_delete(XenPCIPassthroughState *s); >> +void xen_pt_msix_unmap(XenPCIPassthroughState *s); >> int xen_pt_msix_update(XenPCIPassthroughState *s); >> int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index); >> void xen_pt_msix_disable(XenPCIPassthroughState *s); >> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c >> index 4a5bc11..0efee11 100644 >> --- a/hw/xen/xen_pt_config_init.c >> +++ b/hw/xen/xen_pt_config_init.c >> @@ -2079,7 +2079,7 @@ void xen_pt_config_delete(XenPCIPassthroughState *s) >> >> /* free MSI/MSI-X info table */ >> if (s->msix) { >> - xen_pt_msix_delete(s); >> + xen_pt_msix_unmap(s); >> } >> g_free(s->msi); >> >> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c >> index e3d7194..82de2bc 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(OBJECT(&msix->mmio)); >> >> g_free(s->msix); >> s->msix = NULL; >> -- >> 1.7.9.5 >> From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region Date: Mon, 12 Oct 2015 14:01:22 +0200 Message-ID: <561BA112.5030800@redhat.com> References: <1444576764-15344-1-git-send-email-tianyu.lan@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org To: Stefano Stabellini , Lan Tianyu Cc: xen-devel@lists.xensource.com, mjt@tls.msk.ru, qemu-devel@nongnu.org List-Id: xen-devel@lists.xenproject.org On 12/10/2015 13:09, Stefano Stabellini wrote: > On Sun, 11 Oct 2015, Lan Tianyu wrote: >> From: > >> >> msix->mmio is added to XenPCIPassthroughState's object as property. >> object_finalize_child_property is called for XenPCIPassthroughState's >> object, which calls object_property_del_all, which is going to try to >> delete msix->mmio. object_finalize_child_property() will access >> msix->mmio's obj. But the whole msix struct has already been freed >> by xen_pt_msix_delete. This will cause segment fault when msix->mmio >> has been overwritten. >> >> This patch is to fix the issue. >> >> Signed-off-by: Lan Tianyu > > Looks good to me. Paolo? Also looks good to me. Thanks! Paolo >> hw/xen/xen_pt.c | 8 ++++++++ >> hw/xen/xen_pt.h | 1 + >> hw/xen/xen_pt_config_init.c | 2 +- >> hw/xen/xen_pt_msi.c | 13 ++++++++++++- >> 4 files changed, 22 insertions(+), 2 deletions(-) >> >> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c >> index 2b54f52..aa96288 100644 >> --- a/hw/xen/xen_pt.c >> +++ b/hw/xen/xen_pt.c >> @@ -938,10 +938,18 @@ static void xen_pci_passthrough_class_init(ObjectClass *klass, void *data) >> dc->props = xen_pci_passthrough_properties; >> }; >> >> +static void xen_pci_passthrough_finalize(Object *obj) >> +{ >> + XenPCIPassthroughState *s = XEN_PT_DEVICE(obj); >> + >> + xen_pt_msix_delete(s); >> +} >> + >> static const TypeInfo xen_pci_passthrough_info = { >> .name = TYPE_XEN_PT_DEVICE, >> .parent = TYPE_PCI_DEVICE, >> .instance_size = sizeof(XenPCIPassthroughState), >> + .instance_finalize = xen_pci_passthrough_finalize, >> .class_init = xen_pci_passthrough_class_init, >> }; >> >> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h >> index 3bc22eb..c545280 100644 >> --- a/hw/xen/xen_pt.h >> +++ b/hw/xen/xen_pt.h >> @@ -305,6 +305,7 @@ void xen_pt_msi_disable(XenPCIPassthroughState *s); >> >> int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base); >> void xen_pt_msix_delete(XenPCIPassthroughState *s); >> +void xen_pt_msix_unmap(XenPCIPassthroughState *s); >> int xen_pt_msix_update(XenPCIPassthroughState *s); >> int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index); >> void xen_pt_msix_disable(XenPCIPassthroughState *s); >> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c >> index 4a5bc11..0efee11 100644 >> --- a/hw/xen/xen_pt_config_init.c >> +++ b/hw/xen/xen_pt_config_init.c >> @@ -2079,7 +2079,7 @@ void xen_pt_config_delete(XenPCIPassthroughState *s) >> >> /* free MSI/MSI-X info table */ >> if (s->msix) { >> - xen_pt_msix_delete(s); >> + xen_pt_msix_unmap(s); >> } >> g_free(s->msi); >> >> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c >> index e3d7194..82de2bc 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(OBJECT(&msix->mmio)); >> >> g_free(s->msix); >> s->msix = NULL; >> -- >> 1.7.9.5 >>