Re: [RFC PATCH 7/7] vfio/selftests: Add observability test for IO page table usage

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests: vfio_nr_pages_test skips IOMMUFD due to zero-initialized fd assumption
- [Medium] selftests: vfio_nr_pages_test crashes when mapping PROT_NONE memory
--

commit d7003d8f6957a90f40b858dea08cd7ccf897ea8b
Author: Pranjal Shrivastava <praan@google.com>

vfio/selftests: Add observability test for IO page table usage

This commit adds a new selftest, vfio_nr_pages_test, to validate the
iommu-nr-pages fdinfo metric across IOMMU backends. The test performs
sparse mappings to force deep page table allocation and checks that the
metric increases and remains allocated upon unmapping.

> diff --git a/tools/testing/selftests/vfio/vfio_nr_pages_test.c b/tools/testing/selftests/vfio/vfio_nr_pages_test.c
> new file mode 100644
> index 0000000000000..aef6b1dd8e19d
> --- /dev/null
> +++ b/tools/testing/selftests/vfio/vfio_nr_pages_test.c
> @@ -0,0 +1,119 @@

[ ... ]

> +static long read_nr_pages(struct iommu *iommu)
> +{
> +	char path[256];
> +	char line[128];
> +	FILE *f;
> +	long val = -1;
> +	int fd = iommu->container_fd != -1 ? iommu->container_fd : iommu->iommufd;

[Severity: Medium]
Will this ternary operation erroneously yield 0 instead of the actual
iommufd file descriptor?

In IOMMUFD mode, iommu->container_fd is implicitly initialized to 0 via
calloc_assert() in iommu_init(), while iommu->iommufd is initialized to the
actual file descriptor.

This causes the ternary operator iommu->container_fd != -1 to evaluate to
true, assigning fd = 0.

The test then reads from /proc/self/fdinfo/0, fails to find the metric, and
silently skips execution for the IOMMUFD backend.

[ ... ]

> +TEST_F(vfio_nr_pages_test, sparse_mapping_sticky_reclamation)
> +{
> +	long nr_pages_initial, nr_pages_mapped, nr_pages_final;
> +	struct dma_region region1 = {0};
> +	struct dma_region region2 = {0};
> +
> +	if (!self->iommu || !self->device)
> +		SKIP(return, "Fixture setup failed");
> +
> +	nr_pages_initial = read_nr_pages(self->iommu);
> +	if (nr_pages_initial < 0)
> +		SKIP(return, "iommu-nr-pages not reported by this IOMMU");
> +
> +	/* Map a page at start */
> +	region1.size = SZ_4K;
> +	region1.vaddr = mmap_reserve(region1.size, region1.size, 0);
> +	ASSERT_NE(MAP_FAILED, region1.vaddr);
> +	region1.iova = 0x100000;
> +	iommu_map(self->iommu, &region1);

[Severity: Medium]
Does mapping this PROT_NONE region cause the test to crash?

mmap_reserve() provisions memory using mmap() with strictly PROT_NONE
protections. The test immediately passes this region to iommu_map()
without calling mprotect() or over-mapping with PROT_READ | PROT_WRITE.

The kernel will attempt to pin these pages using get_user_pages(), which
will fail with -EFAULT due to lack of read/write permissions.

The iommu_map() wrapper asserts success, causing the test to unconditionally
crash.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001224531.765278-1-praan@google.com?part=7