Re: [PATCH v2 2/2] vfio: selftests: Test BAR write faults after clear_refs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vfio: selftests: probe pre-faults BAR 0 before clear_soft_dirty_refs()
--

commit 43368d01d06eff4702b5d6cf5af459ab154dd5bc
Author: Mina Almasry <almasrymina@google.com>

vfio: selftests: Test BAR write faults after clear_refs

This commit adds a regression test in vfio_pci_device_test to verify that
huge_fault on writable MAP_SHARED VFIO PCI BAR mappings handles both
read-then-write and initial direct write faults after soft-dirty tracking
clears VM_SOFTDIRTY via /proc/self/clear_refs.

> diff --git a/tools/testing/selftests/vfio/vfio_pci_device_test.c b/tools/testing/selftests/vfio/vfio_pci_device_test.c
> index 93c11fd5e0818..75ccc557efb92 100644
> --- a/tools/testing/selftests/vfio/vfio_pci_device_test.c
> +++ b/tools/testing/selftests/vfio/vfio_pci_device_test.c

[ ... ]

> @@ -102,6 +105,86 @@ TEST_F(vfio_pci_device_test, validate_bars)
>  	}
>  }
>  
> +static int clear_soft_dirty_refs(void)
> +{
> +	ssize_t ret;
> +	int fd;
> +
> +	fd = open("/proc/self/clear_refs", O_WRONLY);
> +	if (fd < 0)
> +		return -errno;
> +
> +	ret = write(fd, "4\n", 2);
> +	if (ret < 0)
> +		ret = -errno;
> +	else
> +		VFIO_ASSERT_EQ(ret, 2);
> +
> +	VFIO_ASSERT_EQ(close(fd), 0);
> +	return ret < 0 ? ret : 0;
> +}
> +
> +TEST_F(vfio_pci_device_test, bar_write_fault_after_clear_refs)
> +{
> +	const u32 rw_mmap_flags = VFIO_REGION_INFO_FLAG_MMAP |
> +				  VFIO_REGION_INFO_FLAG_READ |
> +				  VFIO_REGION_INFO_FLAG_WRITE;
> +	struct vfio_pci_bar *bar;
> +	bool tested = false;
> +	int ret, i;
> +	u32 val;
> +
> +	ret = clear_soft_dirty_refs();
> +	if (ret == -ENOENT || ret == -EINVAL)
> +		SKIP(return, "soft-dirty clear_refs unsupported (%d)\n", ret);
> +	ASSERT_EQ(0, ret);
> +
> +	vfio_pci_cmd_set(self->device, PCI_COMMAND_MEMORY);
> +
> +	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> +		void *vaddr;
> +
> +		bar = &self->device->bars[i];
> +		if ((bar->info.flags & rw_mmap_flags) != rw_mmap_flags)
> +			continue;
> +
> +		ASSERT_NE(NULL, bar->vaddr);
> +
> +		/*
> +		 * clear_soft_dirty_refs() stripped write permissions from
> +		 * vma->vm_page_prot on the unpopulated BAR mapping created by
> +		 * vfio_pci_device_init(). Fault it read-only first, then write
> +		 * to exercise in-place write-upgrade (wp_huge_pmd/wp_huge_pud).
> +		 */
> +		val = READ_ONCE(*(u32 *)bar->vaddr);
> +		WRITE_ONCE(*(u32 *)bar->vaddr, val);

[Severity: Medium]
Is bar->vaddr guaranteed to be unpopulated at this point for BAR 0?

During FIXTURE_SETUP(vfio_pci_device_test), vfio_pci_device_init() maps the
BARs and then calls vfio_pci_driver_probe():

tools/testing/selftests/vfio/lib/vfio_pci_device.c:vfio_pci_device_init() {
	...
	vfio_pci_device_setup(device);
	vfio_pci_driver_probe(device);
	return device;
}

Several selftest drivers read from BAR 0 during probe:

- dsa_probe() -> dsa_int_handle_request_required() reads
  bar0 + IDXD_GENCAP_OFFSET
- ioat_probe() reads device->bars[0].vaddr + IOAT_VER_OFFSET
- nv_falcon_probe() reads bar0 + NV_PMC_BOOT_0

Because those reads happen before clear_soft_dirty_refs() is called,
VM_SOFTDIRTY is still set on the VMA and the fault installs a page table entry
that is already writable.

When clear_soft_dirty_refs() subsequently runs, clear_refs_write() updates
vma->vm_page_prot, but clear_refs_test_walk() skips walking page tables for
VM_PFNMAP VMAs:

fs/proc/task_mmu.c:clear_refs_test_walk() {
	...
	if (vma->vm_flags & VM_PFNMAP)
		return 1;
	...
}

As a result, the existing writable page table entry on BAR 0 remains in place,
and the READ_ONCE() and WRITE_ONCE() above will not fault or exercise
wp_huge_pmd() / wp_huge_pud() on BAR 0 for those devices.

Should the test replace the VMA with a fresh mmap(MAP_FIXED) and
madvise(MADV_HUGEPAGE) (or zap the BAR mappings) before the first
clear_soft_dirty_refs() call so that the read-then-write sequence always starts
from an unpopulated mapping?

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