Re: [PATCH v9 1/8] mm/hmm: move page fault handling out of walk callbacks

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

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 <jgg@nvidia.com>
> Signed-off-by: Stanislav Kinsburskii <skinsburskii@gmail.com>
> ---


[...]


>  
>  		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) <david@kernel.org>


-- 
Cheers,

David