Re: [PATCH v11 2/8] mm/hmm: add hmm_range_fault_unlocked_timeout() for mmap lock-drop support

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Unconditional dereference of hmm_vma_walk->locked in hmm_do_fault() on VM_FAULT_RETRY causes a kernel panic.
- [High] hmm_range_fault_unlocked_timeout() causes an infinite 100% CPU busy loop on faults that drop the mmap lock (like disk I/O) by unconditionally passing FAULT_FLAG_ALLOW_RETRY without tracking retries, and continuously resetting the timeout.
--

--- Patch [2]: [PATCH v11 2/8] mm/hmm: add hmm_range_fault_unlocked_timeout() for mmap lock-drop support ---
commit 05dcbcb5ba6adcdc92b16e648fa82986518acbaa
Author: Stanislav Kinsburskii <[email protected]>

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

This commit introduces hmm_range_fault_unlocked_timeout() to allow HMM
fault callers to handle mapping faults that drop the mmap_read_lock, such
as userfaultfd regions. It restructures the internal fault loop to track
the lock state and gracefully retry when the lock is dropped.

> --- a/mm/hmm.c
> +++ b/mm/hmm.c
[ ... ]
> @@ -641,6 +651,9 @@ static int hmm_do_fault(struct mm_struct *mm,
>  	unsigned int fault_flags = FAULT_FLAG_REMOTE;
>  	struct vm_area_struct *vma;
>  
>  	if (hmm_vma_walk->locked)
>  		fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;

[Severity: High]
Does this missing FAULT_FLAG_TRIED tracking cause a livelock on file-backed
faults?

When filemap_fault() sees FAULT_FLAG_ALLOW_RETRY without FAULT_FLAG_TRIED,
it initiates background I/O, drops the lock, and returns VM_FAULT_RETRY
immediately. Because FAULT_FLAG_TRIED is never set on subsequent retries,
this will continually drop the lock and retry until the I/O completes.

[ ... ]
>  	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 = false;

[Severity: Critical]
If hmm_range_fault() is called, locked is NULL. Can this dereference a NULL
pointer if a fault handler returns VM_FAULT_RETRY?

Even without FAULT_FLAG_ALLOW_RETRY, some fault handlers (like
exfat_page_mkwrite or BPF arena_vm_fault) return VM_FAULT_RETRY
unconditionally when internal locks fail. If this happens during a legacy
hmm_range_fault() call, *hmm_vma_walk->locked will cause a panic.

[ ... ]
>  static int hmm_range_fault_locked(struct hmm_range *range, bool *locked)
>  {
>  	struct hmm_vma_walk hmm_vma_walk = {
>  		.range = range,
>  		.locked = locked,
>  		.last = range->start,
>  	};

[Severity: High]
When a lock is dropped, the outer function reacquires it and calls
hmm_range_fault_locked() again. Because hmm_vma_walk.last is re-initialized
to range->start on every call, forward progress is lost and the walk
restarts from the beginning of the range.

[ ... ]
> +int hmm_range_fault_unlocked_timeout(struct hmm_range *range,
> +				     unsigned long timeout)
> +{
[ ... ]
> +	do {
> +		/*
> +		 * If the previous fault dropped mmap_lock, then the fault
> +		 * handler made progress. Restart the retry timeout in that
> +		 * case, but keep the existing deadline for ordinary -EBUSY
> +		 * retries.
> +		 */
> +		if (timeout && !locked)
> +			deadline = jiffies + timeout;

[Severity: High]
Coupled with the missing FAULT_FLAG_TRIED logic above, does this continually
reset the timeout?

Since filemap_fault() drops the lock and returns VM_FAULT_RETRY instantly
while waiting for I/O, locked will be false on every iteration. The timeout
deadline will be pushed forward infinitely, creating a 100% CPU busy loop
that entirely bypasses the caller's timeout policy until the I/O finishes.

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