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

[Nicolin Chen]

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.

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

Thanks!
Nic