On 7/15/26 20:15, Stanislav Kinsburskii wrote:
> hmm_range_fault() currently triggers page faults from inside the page-table
> walk callbacks: hmm_vma_walk_pmd(), hmm_vma_walk_pud(),
> hmm_vma_walk_hugetlb_entry() and the pte-level helper all call
> hmm_vma_fault(), which in turn calls handle_mm_fault() while the walker
> still holds nested locks. The pte spinlock is dropped explicitly by each
> caller, and the hugetlb path manually drops and retakes
> hugetlb_vma_lock_read around the fault to dodge a deadlock against the walk
> framework's unconditional unlock.
>
> This layering does not extend cleanly to fault handlers that may release
> mmap_lock (VM_FAULT_RETRY, VM_FAULT_COMPLETED). If the lock is dropped
> while walk_page_range() is mid-traversal, the VMA can be freed before the
> walk framework's matching hugetlb_vma_unlock_read(), turning that unlock
> into a use-after-free.
>
> Split the responsibilities the way get_user_pages() does. Walk callbacks
> become inspect-only: when they detect a range that needs to be faulted in,
> they record it in struct hmm_vma_walk and return a private sentinel
> (HMM_FAULT_PENDING). The outer loop in hmm_range_fault() then drops out of
> walk_page_range(), invokes a new helper hmm_do_fault() that calls
> handle_mm_fault() with only mmap_lock held, and restarts the walk so the
> now-present entries are collected into hmm_pfns.
>
> No functional change for existing callers. As a side effect the hugetlb
> callback no longer needs the hugetlb_vma_{un}lock_read dance, and every
> fault-path exit from the callbacks now releases the pte spinlock on a
> single, common path. This refactor is also a precursor for adding an
> unlockable variant of hmm_range_fault() in a follow-up patch.
>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Stanislav Kinsburskii <[email protected]>
> ---
[...]
>
> pfn = pud_pfn(pud) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
> @@ -564,21 +561,8 @@ static int hmm_vma_walk_hugetlb_entry(pte_t *pte,
> unsigned long hmask,
> required_fault =
> hmm_pte_need_fault(hmm_vma_walk, pfn_req_flags, cpu_flags);
> if (required_fault) {
> - int ret;
> -
> spin_unlock(ptl);
> - hugetlb_vma_unlock_read(vma);
> - /*
> - * Avoid deadlock: drop the vma lock before calling
> - * hmm_vma_fault(), which will itself potentially take and
> - * drop the vma lock. This is also correct from a
> - * protection point of view, because there is no further
> - * use here of either pte or ptl after dropping the vma
> - * lock.
> - */
> - ret = hmm_vma_fault(addr, end, required_fault, walk);
> - hugetlb_vma_lock_read(vma);
> - return ret;
> + return hmm_record_fault(addr, end, required_fault, walk);
Yes, that looks much better, as discussed. The downside is another vma_lookup()
in hmm_do_fault().
> }
>
> pfn = pte_pfn(entry) + ((start & ~hmask) >> PAGE_SHIFT);
> @@ -637,6 +621,44 @@ static const struct mm_walk_ops hmm_walk_ops = {
> .walk_lock = PGWALK_RDLOCK,
> };
>
> +/*
> + * hmm_do_fault - fault in a range recorded by a walk callback
> + *
> + * Called from the outer loop in hmm_range_fault() after a callback
> + * returned HMM_FAULT_PENDING. At this point we hold only mmap_lock;
> + * the page-table spinlock and any hugetlb_vma_lock acquired by the walk
> + * framework have already been released by the unwind.
> + *
> + * Returns -EBUSY on success (all pages faulted, caller should re-walk).
> + * Returns a negative errno on failure.
> + */
> +static int hmm_do_fault(struct mm_struct *mm,
> + struct hmm_vma_walk *hmm_vma_walk)
> +{
> + unsigned long addr = hmm_vma_walk->last;
> + unsigned long end = hmm_vma_walk->end;
> + unsigned int required_fault = hmm_vma_walk->required_fault;
end and required_fault could be const.
Reviewed-by: David Hildenbrand (Arm) <[email protected]>
--
Cheers,
David
