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
