Re: [PATCH 2/4] vfio: selftests: Add memfd test to vfio_dma_mapping_perf_test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing SKIP fallback for hugepage allocation failures.
- [Low] MAP_SHARED (an mmap flag) is incorrectly passed to memfd_create() via memfd_flags.
- [Low] memfd_create() return value is incorrectly validated against <= 0, which rejects fd 0 and leaks it.
--

commit ffa43469ab562932001b32361e9549a362e44c3c
Author: Aaron Lewis <aaronlewis@google.com>

vfio: selftests: Add memfd test to vfio_dma_mapping_perf_test

Add a second test to "vfio_dma_mapping_perf_test" to evaluate memfd
latencies. A key motivator for adding this test is to demonstrate that
the IOMMU_IOAS_MAP_FILE ioctl is significantly faster than other methods
of mapping DMA regions.

> diff --git a/tools/testing/selftests/vfio/vfio_dma_mapping_perf_test.c b/tools/testing/selftests/vfio/vfio_dma_mapping_perf_test.c
> index 87d84450fc47e..4639bda7ebaa3 100644
> --- a/tools/testing/selftests/vfio/vfio_dma_mapping_perf_test.c
> +++ b/tools/testing/selftests/vfio/vfio_dma_mapping_perf_test.c
[ ... ]
> +#define FIXTURE_VARIANT_ADD_IOMMU_MODE(_iommu_mode, _name, _mmap_flags, _memfd_flags) \
> +FIXTURE_VARIANT_ADD(vfio_dma_mapping_perf_memfd_test, _iommu_mode ## _ ## _name) {    \
> +	.iommu_mode = #_iommu_mode,						      \
> +	.mmap_flags = MAP_SHARED | (_mmap_flags),				      \
> +	.memfd_flags = MAP_SHARED | (_memfd_flags),				      \

[Severity: Low]
Does this incorrectly pass the mmap flag MAP_SHARED to memfd_create() via
memfd_flags?

MAP_SHARED (0x01) aliases with MFD_CLOEXEC (0x0001U) in the memfd flag
namespace, which silently sets close-on-exec rather than failing at runtime.
Could conflating the namespaces here cause confusion?

> +}
> +
[ ... ]
> +static void *setup_memfd(int *fd, u64 size, int mmap_flags, int mfd_flags)
> +{
> +	void *buf = MAP_FAILED;
> +	struct timespec start;
> +
> +	timer_start(&start);
> +	*fd = memfd_create("vfio_dma_mapping_perf_memfd_test", mfd_flags);
> +	printf("Completed memfd_create() in %.2lfms\n", timer_elapsed_ms(start));
> +	if (*fd <= 0)
> +		return MAP_FAILED;

[Severity: Low]
Could this leak fd 0 if memfd_create() successfully returns it?

File descriptors can validly be 0 if the test is run with stdin closed.
Would it be better to check if (*fd < 0) instead so it doesn't reject fd 0?

> +
> +	if (ftruncate(*fd, size))
> +		goto out;
[ ... ]
> +TEST_F(vfio_dma_mapping_perf_memfd_test, dma_map_unmap_from_file)
> +{
> +	const u64 size = SZ_1G;
> +	struct dma_region region;
> +	struct timespec start;
> +	u64 unmapped;
> +	int rc, fd;
> +
> +	region.vaddr = setup_memfd(&fd, size, variant->mmap_flags, variant->memfd_flags);
> +	ASSERT_NE(region.vaddr, MAP_FAILED);

[Severity: Medium]
Will this unconditional ASSERT_NE cause the test suite to fail on systems
that do not have sufficient hugepages configured (e.g., for MFD_HUGE_1GB or
MFD_HUGE_2MB) instead of gracefully skipping the unsupported variants?

> +
> +	region.iova = iova_allocator_alloc(self->iova_allocator, size);
> +	region.size = size;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701203311.326798-1-aaronlewis@google.com?part=2