From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54885) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZjU0n-0004AL-Gz for qemu-devel@nongnu.org; Tue, 06 Oct 2015 11:18:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZjU0h-0006jO-Jn for qemu-devel@nongnu.org; Tue, 06 Oct 2015 11:18:41 -0400 Received: from mga09.intel.com ([134.134.136.24]:48158) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZjU0h-0006iz-DT for qemu-devel@nongnu.org; Tue, 06 Oct 2015 11:18:35 -0400 Message-ID: <5613E645.1050209@intel.com> Date: Tue, 06 Oct 2015 23:18:29 +0800 From: "Lan, Tianyu" MIME-Version: 1.0 References: <1443595897-405-1-git-send-email-tianyu.lan@intel.com> <5613CDC4.1070602@redhat.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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 , Paolo Bonzini Cc: mjt@tls.msk.ru, qemu-devel@nongnu.org, jbeulich@suse.com, konrad.wilk@oracle.com On 10/6/2015 9:49 PM, Stefano Stabellini wrote: > On Tue, 6 Oct 2015, Paolo Bonzini wrote: >> 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. > > Thanks for the explanation and the code. It makes sense to me. > > Lan, could you please write up a patch based on this approach and test > it? > Sure. I will update patch following the guide. Thanks Paolo & Stefano.