From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49932) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZjSNa-0004bq-94 for qemu-devel@nongnu.org; Tue, 06 Oct 2015 09:34:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZjSNW-0004Qb-Af for qemu-devel@nongnu.org; Tue, 06 Oct 2015 09:34:06 -0400 Received: from mail-wi0-x22f.google.com ([2a00:1450:400c:c05::22f]:35260) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZjSNW-0004QP-20 for qemu-devel@nongnu.org; Tue, 06 Oct 2015 09:34:02 -0400 Received: by wicge5 with SMTP id ge5so167246409wic.0 for ; Tue, 06 Oct 2015 06:34:01 -0700 (PDT) Sender: Paolo Bonzini References: <1443595897-405-1-git-send-email-tianyu.lan@intel.com> From: Paolo Bonzini Message-ID: <5613CDC4.1070602@redhat.com> Date: Tue, 6 Oct 2015 15:33:56 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [Fix 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: mjt@tls.msk.ru, qemu-devel@nongnu.org, jbeulich@suse.com, konrad.wilk@oracle.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