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

["David Hildenbrand (Arm)" <david@kernel.org>]

On 11/12/26 16:55, Farber, Eliav wrote:
> 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,

First, wait for human review. Always.

Second, read Documentation/process/submitting-patches.rst

"Wait for a minimum of one week before resubmitting or pinging reviewers
- possibly longer during busy times like merge windows."

Because we have much other priorities right now :)

> 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@amazon.com/
> 
> 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.

Before I reply to anything in here: did an LLM compose most of these walls of
text? :)

-- 
Cheers,

David