Re: [PATCH v10 6/8] RDMA/umem: Use hmm_range_fault_unlocked_timeout() for ODP faults

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

Pre-existing issues:
- [High] Missing check for unstable address space before iterating over an external mm_struct's VMAs, risking a crash if the OOM reaper has marked it MMF_UNSTABLE.
- [High] Unbounded interval invalidation retry loop in a workqueue context lacks a timeout or yield, leading to a potential CPU soft lockup.
--

--- Patch [6]: [PATCH v10 6/8] RDMA/umem: Use hmm_range_fault_unlocked_timeout() for ODP faults ---
commit b7408a657980d2af72d45f322c843edb9faecf2e
Author: Stanislav Kinsburskii <[email protected]>

RDMA/umem: Use hmm_range_fault_unlocked_timeout() for ODP faults

ib_umem_odp_map_dma_and_lock() takes mmap_read_lock() only around
hmm_range_fault(), then retries -EBUSY until HMM_RANGE_DEFAULT_TIMEOUT
expires.

Use hmm_range_fault_unlocked_timeout() instead. The HMM helper now owns
the mmap lock and refreshes range->notifier_seq for its internal retries.
ODP keeps using HMM_RANGE_DEFAULT_TIMEOUT for each HMM fault attempt,
while interval invalidation retries continue to be handled by the existing
outer loop.

ODP still validates the interval notifier sequence while holding umem_mutex
before DMA mapping pages.

> diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
> index 404fa1cc32542..9cc21cd762d98 100644
> --- a/drivers/infiniband/core/umem_odp.c
> +++ b/drivers/infiniband/core/umem_odp.c

[ ... ]

> @@ -363,26 +363,18 @@ int ib_umem_odp_map_dma_and_lock(struct ib_umem_odp *umem_odp, u64 user_virt,
>  	}
>  
>  	range.hmm_pfns = &(umem_odp->map.pfn_list[pfn_start_idx]);
> -	timeout = jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> +	timeout = msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
>  
>  retry:
> -	current_seq = range.notifier_seq =
> -		mmu_interval_read_begin(&umem_odp->notifier);
> -
> -	mmap_read_lock(owning_mm);
> -	ret = hmm_range_fault(&range);
> -	mmap_read_unlock(owning_mm);
> -	if (unlikely(ret)) {
> -		if (ret == -EBUSY && !time_after(jiffies, timeout))
> -			goto retry;
> +	ret = hmm_range_fault_unlocked_timeout(&range, timeout);
> +	if (unlikely(ret))
>  		goto out_put_mm;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed we are passing an
external mm to the HMM helper here without checking for an unstable address
space.

When we call hmm_range_fault_unlocked_timeout(), it takes the
mmap_read_lock_killable() on the external owning_mm and traverses the VMA
tree locklessly:

mm/hmm.c:hmm_range_fault_unlocked_timeout() {
        ...
        ret = mmap_read_lock_killable(mm);
        ...
        ret = hmm_range_fault_locked(range, &locked);
        ...
}

Could this crash if the external mm is being concurrently torn down?

For instance, if the OOM reaper has marked the mm as MMF_UNSTABLE
or if a dup_mmap() failure has left XA_ZERO_ENTRY markers in the maple
tree.

Does the HMM helper or this caller need to invoke
check_stable_address_space(mm) after acquiring the mmap lock before
walking the page tables?

> -	}
>  
>  	start_idx = (range.start - ib_umem_start(umem_odp)) >> page_shift;
>  	dma_index = start_idx;
>  
>  	mutex_lock(&umem_odp->umem_mutex);
> -	if (mmu_interval_read_retry(&umem_odp->notifier, current_seq)) {
> +	if (mmu_interval_read_retry(&umem_odp->notifier, range.notifier_seq)) {
>  		mutex_unlock(&umem_odp->umem_mutex);
>  		goto retry;
>  	}

[Severity: High]
This is a pre-existing issue, but does this interval invalidation retry
loop lack an overall timeout or a way to yield?

If a user-space program concurrently loops on madvise(MADV_DONTNEED) for
a memory region, it triggers the MMU notifier, causing
mmu_interval_read_retry() to repeatedly return true.

Because hmm_range_fault_unlocked_timeout() takes a relative timeout,
calling it again here on every retry resets the deadline for the HMM fault
attempt. If the pages are already faulted, the helper returns 0 quickly,
and this thread will loop unconditionally back to the retry label.

Can this unbounded loop monopolize a CPU core indefinitely and lead to a
soft lockup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6