RE: [PATCH 2/4] mm: add vmf_insert_pfn_order() and vmf_pfn_order_ok() helpers

[Farber, Eliav]

On 11/12/26 14:02, David Hildenbrand (Arm) wrote:
>> +    return !order || (addr >= vma->vm_start &&
>> +                      addr + (PAGE_SIZE << order) <= vma->vm_end &&
>> +                      IS_ALIGNED(pfn, 1UL << order));
> 
> To make this somewhat consumable for a human,
> 
> if (!order)
>       return true;
> return start >= vma->vm_start && end <= vma->vm_end;

We crossed in flight: v2 went out about six minutes before your mail, so
you were reading v1. I posted it to address sashiko-bot's review of v1,
which found a write-flag asymmetry in this patch and a COW problem in
patches 3 and 4. vmf_pfn_order_ok() is unchanged there, so all three of
your comments apply to v2 as posted.

Agreed on the shape. Your version drops the pfn term, which is your next
question, so let me answer that and then propose deleting the helper.

> And maybe rejecting IS_ALIGNED(pfn, 1UL << order) with WARN_ONCE() as this
> sounds like the user is doing something stupid?

A misaligned pfn is not a caller bug, it is what userspace asked for. For
/dev/mem, vm_pgoff is the base pfn, so an mmap() at a page-aligned but not
PMD-aligned physical offset gives a pfn that cannot be mapped at PMD order.
Falling back to PTE order is the correct answer. The selftest covers this
directly: it maps /dev/mem at offset +4K onto a PMD-aligned VA, so the
window term passes and only the pfn term fails, and it asserts both the 4K
fallback and that the data read back is the data at that physical address.
A WARN there would fire on an ordinary mmap().

Dropping the term is worse than a WARN, because nothing below catches it.
x86 pfn_pmd() masks the pfn:

        static inline pmd_t pfn_pmd(unsigned long page_nr, pgprot_t pgprot)
        {
                phys_addr_t pfn = (phys_addr_t)page_nr << PAGE_SHIFT;
                pfn ^= protnone_mask(pgprot_val(pgprot));
                pfn &= PHYSICAL_PMD_PAGE_MASK;

so a misaligned pfn is silently rounded down and the mapping points at the
wrong physical memory. pfn_pud() does the same with PHYSICAL_PUD_PAGE_MASK.

> But looking again, this all looks like sanity checks where the user is just
> doing something stupid?
> 
> Why is that required at all? I'd get rid of this entirely.

The check is required on the fault path because mm asks the handler to do
it. From __thp_vma_allowable_orders():

        /*
         * Check alignment for file vma and size for both file and anon vma by
         * filtering out the unsuitable orders.
         *
         * Skip the check for page fault. Huge fault does the check in fault
         * handlers.
         */
        if (!in_pf) {

thp_vma_suitable_order() is the same check: the order window within the
VMA, plus, for a non-anonymous VMA, IS_ALIGNED(vm_start >> PAGE_SHIFT -
vm_pgoff, ...), which for a PFNMAP whose vm_pgoff is the base pfn is the
pfn alignment. mm's own huge handler honours that contract:
do_huge_pmd_anonymous_page() opens with

        if (!thp_vma_suitable_order(vma, haddr, PMD_ORDER))
                return VM_FAULT_FALLBACK;

and the pfn-mapping handlers each carry a private copy:
is_aligned_for_order() in include/linux/vfio_pci_core.h for vfio-pci and
nvgrace-gpu, device-dax re-derives it from dev_dax->align, and fs/dax.c has
dax_fault_check_fallback(), which tests the PMD colour and both ends of the
window.

So I agree with where you are going, just not that it can be deleted. I
would rather move both terms into mm than add a seventh copy. insert_pmd()
and insert_pud() are the right place: they already reject
addr < vm_start || addr >= vm_end, and they already hold the pfn in
fop.pfn. What is missing there is the end of the window and, in the
non-folio branch, the alignment. Concretely:

  - Add to insert_pmd(), beside the start test:
    addr + HPAGE_PMD_SIZE > vma->vm_end, and, in the !fop.is_folio case,
    !IS_ALIGNED(fop.pfn, HPAGE_PMD_NR), both returning VM_FAULT_FALLBACK.
    insert_pud() the same with HPAGE_PUD_SIZE and HPAGE_PUD_NR. The
    alignment test is gated on !fop.is_folio, and the window test is a
    no-op for the folio callers, since dax_fault_check_fallback() already
    rejects (pmd_addr + PMD_SIZE) > vm_end before calling.
  - Drop vmf_pfn_order_ok() entirely. No caller needs it and no new driver
    does geometry.

That removes the helper you object to, fixes an unchecked window end that
exists today for every implementor, and leaves vmf_insert_pfn_order() as
the order dispatch plus the huge-zero and pfn_modify_allowed() cases.

> Why do we even care about the order? Why isn't it sufficient that we get called
> with "insert this pfn range" and we see how to best map it (pfn/pmd/pud)?
> 
> Why does the caller even have to care about *how* something is mapped?

It does not, and it does not choose. mm picks the order and tells the
handler which one: __handle_mm_fault() tries create_huge_pud() ->
->huge_fault(vmf, PUD_ORDER), descends to create_huge_pmd() ->
->huge_fault(vmf, PMD_ORDER) on VM_FAULT_FALLBACK, then to
handle_pte_fault(). vmf_insert_pfn_order() only dispatches on the level mm
reached, which is what vfio_pci_vmf_insert_pfn() already open-codes with
the same !order / PMD_ORDER / PUD_ORDER ladder. The patch de-duplicates
that; it does not introduce the order-based shape.

Changing the interface to "here is a pfn range, you pick" means changing
->huge_fault itself, for all of its implementors. The folio-backed ones
need the order, because they look up a folio of that size. That seems like
a bigger discussion than this series, but I am happy to have it.

For VM_PFNMAP specifically there is a narrower version of your idea that
does work, and patch 1 is most of it: pfn_range_track() already gives mm
the pfn range at mmap() time, so the fault path could insert without
calling the driver at all. What stops it is per-fault policy, not page
table walking: /dev/mem must re-check range_is_allowed() because
revoke_iomem() can zap the mapping after mmap(), UIO must test idev->info
under info_lock so a fault racing unbind gets SIGBUS, and vfio-pci holds
memory_lock and checks __vfio_pci_memory_enabled(). That is why
->huge_fault exists for these mappings.

Does the insert_pmd()/insert_pud() move look right to you? The missing
window-end test is pre-existing, so it could equally go on the
huge-pfnmap mm fixes series rather than inside v3. Say which you prefer
and I will send it that way.

What changed in this patch between v1 and v2:

  https://lore.kernel.org/all/20261112125638.12209-3-farbere@xxxxxxxxxx/

Only the order-0 path. The sashiko-bot review on this thread observed that
vmf_insert_pfn_order() honoured the fault's write flag for PMD and PUD but
not for order 0, where the pte comes from vma->vm_page_prot. v2 passes it
at order 0 too, through an extracted __vmf_insert_pfn_prot(..., mkwrite),
so the v1 sentence you quoted above the hunk, "The order-0 path needs
nothing: wp_pfn_shared() resolves that fault", is gone. I could not
reproduce a user-visible failure on either new target: /dev/mem and UIO
both modify vm_page_prot, so vma_wants_writenotify() takes its escape. That
change therefore rests on consistency with the huge paths, not on a
demonstrated bug. It does mean a first write at order 0 no longer calls
->pfn_mkwrite, which the kernel-doc now states. v2's cover letter has the
rest of the changelog.

--
Thanks,
Eliav