Re: [PATCH v6 2/4] mm/hmm: add hmm_range_fault_unlocked() for mmap lock-drop support

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `hmm_range_fault_unlocked()` uses uninterruptible `mmap_read_lock()` inside a retry sequence, which can delay or block fatal signals.
- [High] Unconditional dereference of `hmm_vma_walk->locked` in `hmm_do_fault()` can cause a kernel panic if a buggy fault handler returns `VM_FAULT_RETRY` or `VM_FAULT_COMPLETED` without `FAULT_FLAG_ALLOW_RETRY`.
--

commit 0808f6917e681bb06bd68894103530cfbabe07b9
Author: Stanislav Kinsburskii <[email protected]>

mm/hmm: add hmm_range_fault_unlocked() for mmap lock-drop support

This commit adds hmm_range_fault_unlocked() for callers that do not
need to hold mmap_lock across any work outside the HMM fault itself.
It takes mmap_read_lock() internally and allows handle_mm_fault() to
drop it during fault handling.

> diff --git a/mm/hmm.c b/mm/hmm.c
> index 2129b1ee4c35b..316116994209c 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
[ ... ]
> @@ -651,37 +663,33 @@ static int hmm_do_fault(struct mm_struct *mm,
>  		fault_flags |= FAULT_FLAG_WRITE;
>  	}
>  
> -	for (; addr < end; addr += PAGE_SIZE)
> -		if (handle_mm_fault(vma, addr, fault_flags, NULL) &
> -		    VM_FAULT_ERROR)
> -			return -EFAULT;
> +	for (; addr < end; addr += PAGE_SIZE) {
> +		vm_fault_t ret;
> +
> +		ret = handle_mm_fault(vma, addr, fault_flags, NULL);
> +
> +		if (ret & (VM_FAULT_COMPLETED | VM_FAULT_RETRY)) {
> +			*hmm_vma_walk->locked = 0;

[Severity: High]
Can this result in a NULL pointer dereference?

When the legacy hmm_range_fault() is called, it passes NULL for the
locked parameter, meaning hmm_vma_walk->locked will be NULL.

If a buggy fault handler incorrectly returns VM_FAULT_RETRY even when
FAULT_FLAG_ALLOW_RETRY is not set, it seems this would unconditionally
dereference the NULL pointer here.

> +			return HMM_FAULT_UNLOCKED;
> +		}
[ ... ]
> +int hmm_range_fault_unlocked(struct hmm_range *range)
> +{
> +	struct mm_struct *mm = range->notifier->mm;
> +	int locked = 1;
> +	int ret;
> +
> +	mmap_read_lock(mm);

[Severity: High]
Should this use mmap_read_lock_killable() instead of the uninterruptible
mmap_read_lock()?

Since this function is designed to be called inside a retry loop, if the
mmap lock is highly contended, it appears a task could enter an
uninterruptible sleep here and block fatal signals indefinitely. This might
defeat the purpose of the FAULT_FLAG_KILLABLE flag that is set during the
actual fault.

> +	ret = hmm_range_fault_locked(range, &locked);
> +	if (locked)
> +		mmap_read_unlock(mm);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(hmm_range_fault_unlocked);

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