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

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

On Wed, 14 Sep 2022 13:53:39 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Wed, 14 Sep 2022 12:02:56 -0700
> Nicolin Chen <nicolinc@nvidia.com> wrote:
> 
> > Hi Alex,
> > 
> > On Wed, Sep 14, 2022 at 12:10:29PM -0600, Alex Williamson 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!    
> >   
> > > 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:    
> > 
> > Your patch looks good and clean to me (some nits inline).
> > 
> > Would you please send and merge it, replacing mine?
> >   
> > > +       /*
> > > +         * Setup defaults for container pgsizes and dma_max_mappings if not
> > > +         * provided by kernel below.
> > >           */    
> > 
> > Indentation is misaligned at the first line.  
> 
> Oops, will run through checkpatch.
> 
> >   
> > > +        container->pgsizes = 4096;    
> > 
> > This might be a separate question/issue: I wonder if we should use
> > "sysconf(_SC_PAGE_SIZE)" here instead of 4096.
> > 
> > With a kernel using a larger page size, e.g. CONFIG_ARM64_64K_PAGES,
> > the IO page size is likely to be 64K too. If the ioctl fails, this
> > default 4K setup won't work.  
> 
> Perhaps, but IIRC this solution came about because we originally forgot
> to expose the IOMMU_INFO flag to indicate the pgsize field was valid.
> At the time we only supported 4K systems, so it made sense to provide
> this default, though it is indeed dated.
> 
> TBH, I don't really see why we should try to continue if the ioctl
> itself fails, so maybe this should be:
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index ace9562a9ba1..ad188b7649e6 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.
> -         */
>          ret = vfio_get_iommu_info(container, &info);
> +        if (ret) {

Clearly untested,

               ret = -errno;

> +            error_setg_errno(errp, -ret, "Failed to get VFIO IOMMU info");
> +            goto enable_discards_exit:;
> +        }
>  
> -        if (ret || !(info->flags & VFIO_IOMMU_INFO_PGSIZES)) {
> -            /* Assume 4k IOVA page size */
> -            info->iova_pgsizes = 4096;
> +        if (info->flags & VFIO_IOMMU_INFO_PGSIZES) {
> +            container->pgsizes = info->iova_pgsizes;
> +        } else {
> +            container->pgsizes = qemu_real_host_page_size();
>          }
> -        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->dma_max_mappings = 65535;
> -        if (!ret) {
> -            vfio_get_info_dma_avail(info, &container->dma_max_mappings);
> -            vfio_get_iommu_info_migration(container, info);
> +        if (!vfio_get_info_dma_avail(info, &container->dma_max_mappings)) {
> +            container->dma_max_mappings = 65535;
>          }
> +        vfio_get_iommu_info_migration(container, 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: