Re: [PATCH v11 7/8] accel/amdxdna: Use hmm_range_fault_unlocked_timeout() for range population

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

Pre-existing issues:
- [High] Dereferencing `mapp->vma` without holding `mmap_lock` or a VMA lock leads to potential Use-After-Free.
--

--- Patch [7]: [PATCH v11 7/8] accel/amdxdna: Use hmm_range_fault_unlocked_timeout() for range population ---
commit 49a458a983bff0bc836f15feb7b3aa4763d7eb91
Author: Stanislav Kinsburskii <[email protected]>

accel/amdxdna: Use hmm_range_fault_unlocked_timeout() for range population

This patch updates aie2_populate_range() to use the new
hmm_range_fault_unlocked_timeout() helper. The helper manages the mmap lock
and retry sequences internally. The driver now scopes the HMM retry timeout to
the helper while maintaining its outer invalidation loop.

> diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
> index 54486960cbf5e..b5b4ca263002d 100644
> --- a/drivers/accel/amdxdna/aie2_ctx.c
> +++ b/drivers/accel/amdxdna/aie2_ctx.c

[ ... ]

> @@ -1061,24 +1061,9 @@ static int aie2_populate_range(struct amdxdna_gem_obj *abo)

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
use-after-free with the VMA just before this block in aie2_populate_range():

        XDNA_DBG(xdna, "populate memory range %lx %lx",
                 mapp->vma->vm_start, mapp->vma->vm_end);
        mm = mapp->notifier.mm;
        if (!mmget_not_zero(mm)) {

Is it safe to dereference mapp->vma here? 

Since aie2_populate_range() only holds the xdna->notifier_lock at this point,
it doesn't appear to be tied to the VMA's lifetime. If a concurrent munmap
from another thread triggers an invalidation and frees the VMA entirely, could
the logging statement access a freed VMA structure when reading vm_start and
vm_end?

>  		return -EFAULT;
>  	}
>  
> -	mapp->range.notifier_seq = mmu_interval_read_begin(&mapp->notifier);
> -	mmap_read_lock(mm);
> -	ret = hmm_range_fault(&mapp->range);
> -	mmap_read_unlock(mm);
> -	if (ret) {
> -		if (time_after(jiffies, timeout)) {
> -			ret = -ETIME;
> -			goto put_mm;
> -		}
> -
> -		if (ret == -EBUSY) {
> -			amdxdna_umap_put(mapp);
> -			mmput(mm);
> -			goto again;
> -		}
> -
> +	ret = hmm_range_fault_unlocked_timeout(&mapp->range, timeout);
> +	if (ret)
>  		goto put_mm;

[ ... ]

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