Re: [PATCH] mm/hmm: honor write requests for owned device-private pages

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm/hmm: infinite fault loop in hmm_range_fault_locked() with amdkfd SVM
--

commit 1f96e5c5d80c40ba632786bdf9197a9a37329854
Author: Aveline Noir <jm5905938@gmail.com>

mm/hmm: honor write requests for owned device-private pages

This commit updates hmm_vma_handle_pte() and hmm_vma_handle_absent_pmd() to
check read-only, owner-matching device-private entries for write faults when
HMM_PFN_REQ_WRITE is requested, rather than returning their PFNs directly.

> diff --git a/mm/hmm.c b/mm/hmm.c
> index 2f1e98c6b6440..92957541ba95c 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -268,15 +268,22 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
>  		const softleaf_t entry = softleaf_from_pte(pte);
>  
>  		/*
> -		 * Don't fault in device private pages owned by the caller,
> -		 * just report the PFN.
> +		 * Don't fault in device private pages owned by the caller unless
> +		 * a write fault is required.
>  		 */
>  		if (softleaf_is_device_private(entry) &&
>  		    page_pgmap(softleaf_to_page(entry))->owner ==
>  		    range->dev_private_owner) {
>  			cpu_flags = HMM_PFN_VALID;
> -			if (softleaf_is_device_private_write(entry))
> +			if (softleaf_is_device_private_write(entry)) {
>  				cpu_flags |= HMM_PFN_WRITE;
> +			} else {
> +				required_fault =
> +					hmm_pte_need_fault(hmm_vma_walk, pfn_req_flags,
> +							   cpu_flags);
> +				if (required_fault)
> +					goto fault;
> +			}
>  			new_pfn_flags = softleaf_to_pfn(entry) | cpu_flags;
>  			goto out;
>  		}
> @@ -358,8 +365,15 @@ static int hmm_vma_handle_absent_pmd(struct mm_walk *walk, unsigned long start,
>  		unsigned long pfn = softleaf_to_pfn(entry);
>  		unsigned long i;
>  
> -		if (softleaf_is_device_private_write(entry))
> +		if (softleaf_is_device_private_write(entry)) {
>  			cpu_flags |= HMM_PFN_WRITE;
> +		} else if ((range->default_flags | range->pfn_flags_mask) &
> +			   HMM_PFN_REQ_WRITE) {
> +			required_fault = hmm_range_need_fault(hmm_vma_walk,
> +							      hmm_pfns, npages, cpu_flags);
> +			if (required_fault)
> +				return hmm_record_fault(addr, end, required_fault, walk);
> +		}

[Severity: High]
Can triggering a fault on an owner-matching SOFTLEAF_DEVICE_PRIVATE_READ
entry in hmm_vma_handle_pte() or hmm_vma_handle_absent_pmd() cause an
infinite loop in hmm_range_fault_locked() when used with amdkfd SVM?

An anonymous VM_WRITE VMA can contain an owner-matching read-only
device-private entry (SOFTLEAF_DEVICE_PRIVATE_READ) either after fork() via
copy_nonpresent_pte(), or when svm_migrate_vma_to_vram() migrates a present
read-only PTE (!pte_write(pte), such as a read-faulted anonymous page) to
VRAM via migrate_vma_collect_pmd() and remove_migration_pte().

When validating and mapping an SVM range in a VM_WRITE VMA, amdkfd calls
hmm_range_fault() with default_flags = HMM_PFN_REQ_FAULT | HMM_PFN_REQ_WRITE
and dev_private_owner set to the GPU owner while p->svms.faulting_task is set
to current:

drivers/gpu/drm/amd/amdkfd/kfd_svm.c:svm_range_validate_and_map() {
    ...
			WRITE_ONCE(p->svms.faulting_task, current);
			range = amdgpu_hmm_range_alloc(NULL);
			if (likely(range))
				r = amdgpu_hmm_range_get_pages(&prange->notifier, addr, npages,
							       readonly, owner, range);
			else
				r = -ENOMEM;
			WRITE_ONCE(p->svms.faulting_task, NULL);
    ...
}

With this change, hmm_vma_handle_pte() and hmm_vma_handle_absent_pmd() see
HMM_PFN_REQ_WRITE without HMM_PFN_WRITE on the owner-matching read-only
device-private entry and return HMM_FAULT_PENDING at addr.

In mm/hmm.c, hmm_range_fault_locked() then invokes:

  hmm_do_fault()
    -> handle_mm_fault()
      -> do_swap_page()
        -> pgmap->ops->migrate_to_ram() (svm_migrate_to_ram())

In drivers/gpu/drm/amd/amdkfd/kfd_migrate.c, svm_migrate_to_ram() checks
p->svms.faulting_task to avoid deadlocking on svms.lock and
prange->migrate_mutex:

	if (READ_ONCE(p->svms.faulting_task) == current) {
		pr_debug("skipping ram migration\n");
		r = 0;
		goto out_unref_process;
	}

Because svm_migrate_to_ram() returns 0 without migrating the page to RAM,
modifying the PTE, or firing an MMU notifier, hmm_do_fault() returns -EBUSY:

mm/hmm.c:hmm_range_fault_locked() {
	do {
		/* If range is no longer valid force retry. */
		if (mmu_interval_check_retry(range->notifier,
					     range->notifier_seq))
			return -EBUSY;
		ret = walk_page_range(mm, hmm_vma_walk.last, range->end,
				      &hmm_walk_ops, &hmm_vma_walk);
		if (ret == HMM_FAULT_PENDING) {
			ret = hmm_do_fault(mm, &hmm_vma_walk);
			if (ret == HMM_FAULT_UNLOCKED) {
				if (fatal_signal_pending(current))
					return -EINTR;
				return -EBUSY;
			}
		}
	} while (ret == -EBUSY);
}

Since no MMU notifier fired, mmu_interval_check_retry() returns false and
walk_page_range() restarts at the same hmm_vma_walk.last address. Won't this
spin forever on the unchanged SOFTLEAF_DEVICE_PRIVATE_READ entry while
holding mmap_read_lock(mm) (or mmap_write_lock(mm)), svms.lock, and
prange->migrate_mutex?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179343062238.2.2773026701442863377@gmail.com?part=1