Re: [PATCH v6 4/4] mshv: Use hmm_range_fault_unlocked() for region faults

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing check for fatal signals in retry loop handling HMM faults.
--

commit 4f92a5b1f4b0160351016ede4e0c8d47d308cbfa
Author: Stanislav Kinsburskii <skinsburskii@gmail.com>

mshv: Use hmm_range_fault_unlocked() for region faults

Convert mshv_region_hmm_fault_and_lock() to use hmm_range_fault_unlocked()
instead of taking mmap_read_lock() around hmm_range_fault() directly.
This allows the MSHV fault path to handle mappings whose fault handlers may
drop mmap_lock.

> diff --git a/drivers/hv/mshv_regions.c b/drivers/hv/mshv_regions.c
> index 6d65e5b421529..04676f06c5c74 100644
> --- a/drivers/hv/mshv_regions.c
> +++ b/drivers/hv/mshv_regions.c
> @@ -388,13 +388,11 @@ int mshv_region_get(struct mshv_mem_region *region)
>   *
>   * This function performs the following steps:
>   * 1. Reads the notifier sequence for the HMM range.
> - * 2. Acquires a read lock on the memory map.
> - * 3. Handles HMM faults for the specified range.
> - * 4. Releases the read lock on the memory map.
> - * 5. If successful, locks the memory region mutex.
> - * 6. Verifies if the notifier sequence has changed during the operation.
> + * 2. Handles HMM faults for the specified range.
> + * 3. If successful, locks the memory region mutex.
> + * 4. Verifies if the notifier sequence has changed during the operation.
>   *    If it has, releases the mutex and returns -EBUSY to match with
> - *    hmm_range_fault() return code for repeating.
> + *    hmm_range_fault_unlocked() so the caller retries the range fault.

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying the
fault retry logic, does the retry loop in mshv_region_range_fault() need to
check for fatal signals?

When mmu_interval_read_retry() detects a collision, this function returns
-EBUSY, which bubbles up to the caller in drivers/hv/mshv_regions.c:

mshv_region_range_fault() {
        ...
        do {
                ret = mshv_region_hmm_fault_and_lock(region, &range);
        } while (ret == -EBUSY);
        ...
}

If a userspace program continuously invalidates the memory range concurrently
(for example, via madvise(MADV_DONTNEED)), mmu_interval_read_retry() can
repeatedly return true.

Without a check for fatal_signal_pending(current) inside the loop or before
returning -EBUSY, could this cause the kernel thread to spin indefinitely and
ignore fatal signals like SIGKILL?

>   *
>   * Return: 0 on success, a negative error code otherwise.
>   */
> @@ -404,9 +402,7 @@ static int mshv_region_hmm_fault_and_lock(struct mshv_mem_region *region,
>  	int ret;
>  
>  	range->notifier_seq = mmu_interval_read_begin(range->notifier);
> -	mmap_read_lock(region->mreg_mni.mm);
> -	ret = hmm_range_fault(range);
> -	mmap_read_unlock(region->mreg_mni.mm);
> +	ret = hmm_range_fault_unlocked(range);
>  	if (ret)
>  		return ret;
>

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