From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50146) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHEtV-0006Mf-82 for qemu-devel@nongnu.org; Mon, 20 Jul 2015 13:30:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZHEtS-0003LC-2Y for qemu-devel@nongnu.org; Mon, 20 Jul 2015 13:30:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57157) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHEtR-0003L3-S7 for qemu-devel@nongnu.org; Mon, 20 Jul 2015 13:30:22 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 1782FC9425 for ; Mon, 20 Jul 2015 17:30:21 +0000 (UTC) References: <20150720164002.2178.42525.stgit@gimli.home> From: Laszlo Ersek Message-ID: <55AD302A.8010209@redhat.com> Date: Mon, 20 Jul 2015 19:30:18 +0200 MIME-Version: 1.0 In-Reply-To: <20150720164002.2178.42525.stgit@gimli.home> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] vfio/pci: Cleanup vfio_early_setup_msix() error path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson , qemu-devel@nongnu.org On 07/20/15 18:41, Alex Williamson wrote: > With the addition of the Chelsio quirk we have an error path out of > vfio_early_setup_msix() that doesn't free the allocated VFIOMSIXInfo > struct. This doesn't introduce a leak as it still gets freed in the > vfio_put_device() path, but it's complicated and sloppy to rely on > that. Restructure to free the allocated data on error and only link > it into the vdev on success. > > Signed-off-by: Alex Williamson > Reported-by: Laszlo Ersek > --- > hw/vfio/pci.c | 27 +++++++++++++-------------- > 1 file changed, 13 insertions(+), 14 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 70d82d4..8c6127a 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -2204,6 +2204,7 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev) > uint16_t ctrl; > uint32_t table, pba; > int fd = vdev->vbasedev.fd; > + VFIOMSIXInfo *msix; > > pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX); > if (!pos) { > @@ -2229,21 +2230,19 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev) > table = le32_to_cpu(table); > pba = le32_to_cpu(pba); > > - vdev->msix = g_malloc0(sizeof(*(vdev->msix))); > - vdev->msix->table_bar = table & PCI_MSIX_FLAGS_BIRMASK; > - vdev->msix->table_offset = table & ~PCI_MSIX_FLAGS_BIRMASK; > - vdev->msix->pba_bar = pba & PCI_MSIX_FLAGS_BIRMASK; > - vdev->msix->pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK; > - vdev->msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1; > + msix = g_malloc0(sizeof(*msix)); > + msix->table_bar = table & PCI_MSIX_FLAGS_BIRMASK; > + msix->table_offset = table & ~PCI_MSIX_FLAGS_BIRMASK; > + msix->pba_bar = pba & PCI_MSIX_FLAGS_BIRMASK; > + msix->pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK; > + msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1; > > /* > * Test the size of the pba_offset variable and catch if it extends outside > * of the specified BAR. If it is the case, we need to apply a hardware > * specific quirk if the device is known or we have a broken configuration. > */ > - if (vdev->msix->pba_offset >= > - vdev->bars[vdev->msix->pba_bar].region.size) { > - > + if (msix->pba_offset >= vdev->bars[msix->pba_bar].region.size) { > PCIDevice *pdev = &vdev->pdev; > uint16_t vendor = pci_get_word(pdev->config + PCI_VENDOR_ID); > uint16_t device = pci_get_word(pdev->config + PCI_DEVICE_ID); > @@ -2255,18 +2254,18 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev) > * is 0x1000, so we hard code that here. > */ > if (vendor == PCI_VENDOR_ID_CHELSIO && (device & 0xff00) == 0x5800) { > - vdev->msix->pba_offset = 0x1000; > + msix->pba_offset = 0x1000; > } else { > error_report("vfio: Hardware reports invalid configuration, " > "MSIX PBA outside of specified BAR"); > + g_free(msix); > return -EINVAL; > } > } > > - trace_vfio_early_setup_msix(vdev->vbasedev.name, pos, > - vdev->msix->table_bar, > - vdev->msix->table_offset, > - vdev->msix->entries); > + trace_vfio_early_setup_msix(vdev->vbasedev.name, pos, msix->table_bar, > + msix->table_offset, msix->entries); > + vdev->msix = msix; > > return 0; > } > Reviewed-by: Laszlo Ersek