Re: [PATCH v2 1/4] fixup! mm/hmm: add hmm_range_fault_unlocked_timeout() for mmap lock-drop support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] hmm_range_fault_locked() performs VMA iteration on an external mm_struct without verifying if the address space is stable, risking a crash on an unstable maple tree.
--

--- Patch [1]: [PATCH v2 1/4] fixup! mm/hmm: add hmm_range_fault_unlocked_timeout() for mmap lock-drop support ---
commit a32e804e265d5dd30e11762ac51a889fc0abfebc
Author: Stanislav Kinsburskii <skinsburskii@gmail.com>

fixup! mm/hmm: add hmm_range_fault_unlocked_timeout() for mmap lock-drop support

This commit updates the example in the HMM documentation to pass the notifier
stored in range to mmu_interval_read_retry(). It explicitly details the
relationship between the HMM walk and the invalidation check.

> diff --git a/Documentation/mm/hmm.rst b/Documentation/mm/hmm.rst
> index 5c88d2cf08183..fc1b8dc198250 100644
> --- a/Documentation/mm/hmm.rst
> +++ b/Documentation/mm/hmm.rst
> @@ -206,7 +206,7 @@ The usage pattern is::
>            goto out_put;
>  
>        take_lock(driver->update);
> -      if (mmu_interval_read_retry(&interval_sub, range.notifier_seq)) {
> +      if (mmu_interval_read_retry(range.notifier, range.notifier_seq)) {
>            release_lock(driver->update);
>            goto again;
>        }

[ ... ]

> @@ -225,7 +225,8 @@ The usage pattern is::
>  The driver->update lock is the same lock that the driver takes inside its
>  invalidate() callback. That lock must be held before calling
>  mmu_interval_read_retry() to avoid any race with a concurrent CPU page table
> -update.
> +update. The retry check must use the same notifier and sequence number stored
> +in ``range`` by ``hmm_range_fault_unlocked_timeout()``.
>  
>  Holding the mmap lock across HMM faults
>  =======================================

[Severity: High]
This is a pre-existing issue, but while reviewing the documentation for
HMM faults, I noticed hmm_range_fault_locked() in mm/hmm.c might be missing
a check for a stable address space before traversing VMAs:

mm/hmm.c:hmm_range_fault_locked() {
    ...
	mmap_assert_locked(mm);

	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);
    ...
}

Does this path need to call check_stable_address_space(mm) before iterating
the VMAs with walk_page_range()?

If the mm becomes unstable (for instance, if an OOM reaper sets MMF_UNSTABLE
and modifies the maple tree concurrently while a device driver processes a
page fault for a user process), could iterating the maple tree on an
external mm_struct encounter XA_ZERO_ENTRY markers and lead to undefined
behavior or a panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178406760622.1106335.2379450382728057793.stgit@skinsburskii?part=1