Re: [PATCH v5 18/18] Documentation/userfaultfd: document RWP working set tracking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] PAGEMAP_SCAN example matches unpopulated pages as "cold", leading to an infinite loop of evicting memory holes.
- [Medium] The documentation falsely claims that UFFDIO_API does not fail when unsupported features are requested.
--

commit a8b39975821d1562e1c0e6bd26ea6ff5f6e32ba6
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

Documentation/userfaultfd: document RWP working set tracking

This patch adds an admin-guide section covering UFFDIO_REGISTER_MODE_RWP.
It details sync and async fault models, UFFDIO_RWPROTECT semantics, and
covers a typical VMM working-set-tracking workflow.

> diff --git a/Documentation/admin-guide/mm/userfaultfd.rst b/Documentation/admin-guide/mm/userfaultfd.rst
> index 1e533639fd509..1db99b5355f71 100644
> --- a/Documentation/admin-guide/mm/userfaultfd.rst
> +++ b/Documentation/admin-guide/mm/userfaultfd.rst

[ ... ]

> +Feature availability:
> +
> +RWP is built on top of two kernel primitives: a spare PTE bit owned by
> +userfaultfd (CONFIG_HAVE_ARCH_USERFAULTFD_WP) and architecture support
> +for present-but-inaccessible PTEs (CONFIG_ARCH_HAS_PTE_PROTNONE). When both
> +are available on a 64-bit kernel, the build selects
> +CONFIG_USERFAULTFD_RWP=y and the VM_UFFD_RWP VMA flag becomes
> +available.
> +
> +UFFD_FEATURE_RWP and UFFD_FEATURE_RWP_ASYNC are masked out of the
> +features returned by UFFDIO_API when the running kernel or architecture
> +cannot support them — for example 32-bit kernels (where VM_UFFD_RWP is
> +unavailable), kernels built without CONFIG_USERFAULTFD_RWP, and
> +architectures whose ptes cannot carry the uffd bit at runtime (e.g. riscv
> +without the SVRSW60T59B extension). UFFDIO_API does not fail;
> +unsupported bits are simply absent from uffdio_api.features on return.

[Severity: Medium]
Is this description of UFFDIO_API behavior accurate?

Looking at userfaultfd_api() in mm/userfaultfd.c, the kernel validates
the requested features against what is supported, and branches to an
error path returning -EINVAL if unsupported features are requested:

	if (features & ~uffdio_api.features)
		goto err_out;

Because the ioctl fails, the userfaultfd context remains uninitialized,
and any subsequent ioctls on that fd (such as UFFDIO_REGISTER) will fail.
Should the documentation advise users to first query available features
with a zero-feature UFFDIO_API call rather than assuming it will not fail?

[ ... ]

> +Cold Page Detection with PAGEMAP_SCAN:
> +
> +RWP-protected PTEs carry the uffd PTE bit; the fault-resolution path
> +clears it. PAGEMAP_SCAN reports PAGE_IS_ACCESSED once the bit is
> +clear on a VM_UFFD_RWP VMA, so inverting it efficiently reports the
> +still-protected (cold) pages::
> +
> +    struct pm_scan_arg arg = {
> +        .size = sizeof(arg),
> +        .start = guest_mem_start,
> +        .end = guest_mem_end,
> +        .vec = (uint64_t)regions,
> +        .vec_len = regions_len,
> +        .category_mask = PAGE_IS_ACCESSED,
> +        .category_inverted = PAGE_IS_ACCESSED,
> +        .return_mask = PAGE_IS_ACCESSED,
> +    };

[Severity: Medium]
Will this configuration also match unpopulated pages (memory holes) as
"cold" regions?

Since pagemap_page_category() returns 0 for unpopulated pages, evaluating
(0 ^ PAGE_IS_ACCESSED) & PAGE_IS_ACCESSED inside 
pagemap_scan_is_interesting_page() results in true. 

Later in the documentation, the VMM loop example uses this same mask:

[ ... ]

> +        /* 3. Find cold pages (uffd bit still set) */
> +        ioctl(pagemap_fd, PAGEMAP_SCAN, &(struct pm_scan_arg){
> +            .category_mask = PAGE_IS_ACCESSED,
> +            .category_inverted = PAGE_IS_ACCESSED,
> +            .return_mask = PAGE_IS_ACCESSED,
> +            ...
> +        });
> +
> +        /* 4. Evict cold pages (vCPU faults block on guest_mem) */
> +        for each cold range:
> +            /* Read from io_mem -- bypasses RWP, no fault. */
> +            pwrite(storage_fd, (char *)io_mem + cold_offset,
> +                   len, cold_offset);
> +            /* Drop the page from the shared file. */
> +            fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> +                      cold_offset, len);

If unpopulated ranges are included in the results, does this create an
infinite loop where the VMM continually reads zeroes from io_mem (allocating
new zero pages), writes them to storage, and calls fallocate() on an existing
hole? 

Since a hole never acquires the PAGE_IS_ACCESSED bit, the next scan would
find the exact same ranges. Should PAGE_IS_PRESENT be included in
category_mask (but not category_inverted) to safely skip memory holes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526130509.2748441-1-kirill@shutemov.name?part=18