Re: [PATCH v2 3/4] fixup! accel/amdxdna: use hmm_range_fault_unlocked_timeout() for range population

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Livelock in aie2_populate_range() retry loop due to lack of absolute timeout.
- [High] Data race when concurrently populating shared BO mappings.
--

--- Patch [3]: [PATCH v2 3/4] fixup! accel/amdxdna: use hmm_range_fault_unlocked_timeout() for range population ---
commit 0ad43904da8ddad180e9ceac49d9b0988660e0ec
Author: Stanislav Kinsburskii <skinsburskii@gmail.com>

fixup! accel/amdxdna: use hmm_range_fault_unlocked_timeout() for range population
    
aie2_populate_range() now uses hmm_range_fault_unlocked_timeout() to let
HMM own mmap_lock acquisition and retry handling while populating an
invalid user mapping. The timeout passed to that helper is a relative
HMM retry budget, not an absolute deadline.

> diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
> index 548ba4315554f..21f2817751f9e 100644
> --- a/drivers/accel/amdxdna/aie2_ctx.c
> +++ b/drivers/accel/amdxdna/aie2_ctx.c
> @@ -1062,13 +1062,9 @@ static int aie2_populate_range(struct amdxdna_gem_obj *abo)
>  		return -EFAULT;
>  	}
>  
> -	ret = hmm_range_fault_unlocked_timeout(&mapp->range,
> -			max_t(long, timeout - jiffies, 1));
> -	if (ret) {
> -		if (ret == -EBUSY)
> -			ret = -ETIME;
> +	ret = hmm_range_fault_unlocked_timeout(&mapp->range, timeout);

[Severity: High]
This is a pre-existing issue, but is there a data race when concurrently
populating shared BO mappings?

In aie2_cmd_submit(), the job submission explicitly drops reservations and
locks before calling aie2_populate_range():

drivers/accel/amdxdna/aie2_ctx.c:aie2_cmd_submit() {
    ...
    if (abo->mem.map_invalid) {
        up_read(&xdna->notifier_lock);
        drm_gem_unlock_reservations(job->bos, job->bo_cnt, &acquire_ctx);
        ...
        ret = aie2_populate_range(abo);
    ...
}

If two threads concurrently submit jobs using a shared buffer object, they
could both see abo->mem.map_invalid as true, drop their locks, and
simultaneously call aie2_populate_range() for the same BO.

Could this cause concurrent uncoordinated modifications to the same
mapp->range structure, causing a data race on range->notifier_seq and
concurrent writes to the range->hmm_pfns array during the call to
hmm_range_fault_unlocked_timeout() here?

> +	if (ret)
>  		goto put_mm;
> -	}
>  
>  	down_write(&xdna->notifier_lock);
>  	if (mmu_interval_read_retry(&mapp->notifier, mapp->range.notifier_seq)) {

[Severity: High]
This is also a pre-existing issue, but does this create a livelock if an
unprivileged user repeatedly invalidates a memory range?

The code executes a 'goto again' if mmu_interval_read_retry() returns true,
passing a fresh timeout budget to hmm_range_fault_unlocked_timeout() on every
iteration.

Because there is no overarching absolute deadline within this loop, can the
thread spin indefinitely here?

The outer absolute timeout in aie2_cmd_submit() wouldn't provide protection
because aie2_populate_range() never returns while spinning.

[ ... ]

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