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

[Alex Williamson]

On Wed, 14 Sep 2022 12:10:29 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> 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) {

Or better still:

            if (!vfio_get_iommu_info(container, &info)) {

> +            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:
>