mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Alex Williamson <alex@shazbot.org>
To: Mina Almasry <almasrymina@google.com>
Cc: Peter Xu <peterx@redhat.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-kselftest@vger.kernel.org,
	David Matlack <dmatlack@google.com>,
	Shuah Khan <shuah@kernel.org>, Kevin Tian <kevin.tian@intel.com>,
	Jason Gunthorpe <jgg@ziepe.ca>,
	alex@shazbot.org
Subject: Re: [PATCH v2 2/2] vfio: selftests: Test BAR write faults after clear_refs
Date: Mon, 2 Nov 2026 17:11:49 -0700	[thread overview]
Message-ID: <20261102171149.5d4ed369@shazbot.org> (raw)
In-Reply-To: <20261101062049.998160-3-almasrymina@google.com>

On Sun,  1 Nov 2026 06:20:49 +0000
Mina Almasry <almasrymina@google.com> wrote:

> Add a regression test in vfio_pci_device_test to verify that huge_fault
> on writable MAP_SHARED VFIO PCI BAR mappings handles write faults after
> soft-dirty tracking clears VM_SOFTDIRTY and downgrades vma->vm_page_prot
> to read-only via /proc/self/clear_refs ("4").
> 
> Specifically, for each mappable read/write BAR (already aligned up to
> SZ_1G by vfio_pci_bar_map() with MADV_HUGEPAGE), verify both:
> 
> 1. Read-then-write fault sequence after clear_refs, exercising in-place
>    huge PMD/PUD write-protection upgrade via wp_huge_pmd()/wp_huge_pud().
> 2. Direct initial write fault on a freshly mapped unpopulated BAR VMA
>    after clear_refs, exercising vmf_insert_pfn_pmd()/vmf_insert_pfn_pud()
>    with FAULT_FLAG_WRITE set.
> 
> Skip gracefully when /proc/self/clear_refs is absent or soft-dirty
> tracking (CONFIG_MEM_SOFT_DIRTY) is disabled, or when the device exposes
> no mappable read/write BARs.
> 
> Cc: David Matlack <dmatlack@google.com>
> Signed-off-by: Mina Almasry <almasrymina@google.com>
> ---
>  .../selftests/vfio/vfio_pci_device_test.c     | 83 +++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/tools/testing/selftests/vfio/vfio_pci_device_test.c b/tools/testing/selftests/vfio/vfio_pci_device_test.c
> index 93c11fd5e081..75ccc557efb9 100644
> --- a/tools/testing/selftests/vfio/vfio_pci_device_test.c
> +++ b/tools/testing/selftests/vfio/vfio_pci_device_test.c
> @@ -1,10 +1,13 @@
>  // SPDX-License-Identifier: GPL-2.0-only
> +#include <errno.h>
>  #include <fcntl.h>
>  #include <stdlib.h>
> +#include <unistd.h>
>  
>  #include <sys/ioctl.h>
>  #include <sys/mman.h>
>  
> +#include <linux/compiler.h>
>  #include <linux/limits.h>
>  #include <linux/pci_regs.h>
>  #include <linux/sizes.h>
> @@ -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);

-ENOENT seems to map to the open() when /proc/self/clear_refs is not
present.  We can't clear soft-dirty by virtue that the file is absent.
Fine.

-EINVAL seems to be intended to catch:

clear_refs_write():

        if (type < CLEAR_REFS_ALL || type >= CLEAR_REFS_LAST)
                return -EINVAL;

But that's only testing for a known type, not that it's supported.
This is never triggered, which can lead to a false pass if soft-dirty
really isn't supported and silently does nothing.


> +	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;

Where do we filter out BARs that wouldn't trigger a PMD/PUD fault?  If
we're only testing sub-PMD BARs, this is another false pass.

If we do find a BAR to trigger a PMD/PUD mapping, what's the value in
continuing to exhaustively search and test the remaining BARs?

> +
> +		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);

Sashiko notes[1] an issue here that might be a false positive, but the
comment should at least note that a matching driver could already have
touched the BAR via read/write but doesn't invalidate the test... if so.
Thanks,

Alex

[1]https://sashiko.dev/#/patchset/20261101062049.998160-1-almasrymina@google.com

> +
> +		/*
> +		 * Replace the VMA with a fresh unpopulated mapping at the same
> +		 * hugepage-aligned address, clear soft-dirty again, and fault
> +		 * with an immediate store to exercise initial write insertion
> +		 * (vmf_insert_pfn_pmd/pud with write=true).
> +		 */
> +		vaddr = mmap(bar->vaddr, bar->info.size, PROT_READ | PROT_WRITE,
> +			     MAP_SHARED | MAP_FIXED, self->device->fd,
> +			     bar->info.offset);
> +		ASSERT_EQ(bar->vaddr, vaddr);
> +		madvise(bar->vaddr, bar->info.size, MADV_HUGEPAGE);
> +
> +		ASSERT_EQ(0, clear_soft_dirty_refs());
> +
> +		WRITE_ONCE(*(u32 *)bar->vaddr, val);
> +		(void)READ_ONCE(*(u32 *)bar->vaddr);
> +
> +		tested = true;
> +	}
> +
> +	vfio_pci_cmd_clear(self->device, PCI_COMMAND_MEMORY);
> +
> +	if (!tested)
> +		SKIP(return, "No mappable read/write BAR on %s\n", device_bdf);
> +}
> +
>  FIXTURE(vfio_pci_irq_test) {
>  	struct iommu *iommu;
>  	struct vfio_pci_device *device;


      parent reply	other threads:[~2026-11-03  0:11 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-11-01  6:20 [PATCH v2 0/2] vfio/pci: fix huge_fault write handling and add selftest Mina Almasry
2026-11-01  6:20 ` [PATCH v2 1/2] vfio/pci: pass FAULT_FLAG_WRITE to vmf_insert_pfn_pmd/pud() Mina Almasry
2026-11-01  6:20 ` [PATCH v2 2/2] vfio: selftests: Test BAR write faults after clear_refs Mina Almasry
2026-11-02 21:45   ` David Matlack
2026-11-03  0:11   ` Alex Williamson [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20261102171149.5d4ed369@shazbot.org \
    --to=alex@shazbot.org \
    --cc=akpm@linux-foundation.org \
    --cc=almasrymina@google.com \
    --cc=dmatlack@google.com \
    --cc=jgg@ziepe.ca \
    --cc=kevin.tian@intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=peterx@redhat.com \
    --cc=shuah@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®