On Mon, 12 Sep 2022 13:51:30 -0700
Nicolin Chen <nicol...@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 <nicol...@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:
