Re: [PATCH] vfio/common: Do not g_free in vfio_get_iommu_info

[Alex Williamson <alex.williamson@redhat.com>]

On Mon, 12 Sep 2022 13:51:30 -0700
Nicolin Chen <nicolinc@nvidia.com> wrote:

> On Mon, Sep 12, 2022 at 02:38:52PM +0200, Cornelia Huck wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Fri, Sep 09 2022, Nicolin Chen <nicolinc@nvidia.com> wrote:
> >   
> > > Its caller vfio_connect_container() assigns a default value
> > > to info->iova_pgsizes, even if vfio_get_iommu_info() fails.
> > > This would result in a "Segmentation fault" error, when the
> > > VFIO_IOMMU_GET_INFO ioctl errors out.
> > >
> > > Since the caller has g_free already, drop the g_free in its
> > > rollback routine and add a line of comments to highlight it.  
> > 
> > There's basically two ways to fix this:
> > 
> > - return *info in any case, even on error
> > - free *info on error, and make sure that the caller doesn't try to
> >   access *info if the function returned !0
> > 
> > The problem with the first option is that the caller will access invalid
> > information if it neglects to check the return code, and that might lead
> > to not-that-obvious errors; in the second case, a broken caller would at
> > least fail quickly with a segfault. The current code is easier to fix
> > with the first option.
> > 
> > I think I'd prefer the second option; but obviously maintainer's choice.  
> 
> The caller does check rc all the time. So I made a smaller fix
> (the first option). Attaching the git-diff for the second one.
> 
> Alex, please let me know which one you prefer. Thanks!
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 51b2e05c76..74431411ab 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
[snip]

I think we can do better than that, I don't think we need to maintain
the existing grouping, and that FIXME comment is outdated and has
drifted from the relevant line of code.  What about:

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index ace9562a9ba1..8d8c54d59083 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -2111,29 +2111,31 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
     {
         struct vfio_iommu_type1_info *info;
 
-        /*
-         * FIXME: This assumes that a Type1 IOMMU can map any 64-bit
-         * IOVA whatsoever.  That's not actually true, but the current
-         * kernel interface doesn't tell us what it can map, and the
-         * existing Type1 IOMMUs generally support any IOVA we're
-         * going to actually try in practice.
+	/*
+         * Setup defaults for container pgsizes and dma_max_mappings if not
+         * provided by kernel below.
          */
-        ret = vfio_get_iommu_info(container, &info);
-
-        if (ret || !(info->flags & VFIO_IOMMU_INFO_PGSIZES)) {
-            /* Assume 4k IOVA page size */
-            info->iova_pgsizes = 4096;
-        }
-        vfio_host_win_add(container, 0, (hwaddr)-1, info->iova_pgsizes);
-        container->pgsizes = info->iova_pgsizes;
-
-        /* The default in the kernel ("dma_entry_limit") is 65535. */
+        container->pgsizes = 4096;
         container->dma_max_mappings = 65535;
+
+        ret = vfio_get_iommu_info(container, &info);
         if (!ret) {
+            if (info->flags & VFIO_IOMMU_INFO_PGSIZES) {
+                container->pgsizes = info->iova_pgsizes;
+            }
+
             vfio_get_info_dma_avail(info, &container->dma_max_mappings);
             vfio_get_iommu_info_migration(container, info);
+            g_free(info);
         }
-        g_free(info);
+
+        /*
+         * FIXME: We should parse VFIO_IOMMU_TYPE1_INFO_CAP_IOVA_RANGE
+         * information to get the actual window extent rather than assume
+         * a 64-bit IOVA address space.
+         */
+        vfio_host_win_add(container, 0, (hwaddr)-1, container->pgsizes);
+
         break;
     }
     case VFIO_SPAPR_TCE_v2_IOMMU: