Re: [PATCH V4 15/19] accel/amdxdna: Make populate_range common for AIE2 and AIE4

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/amdxdna: data race on hmm_range during concurrent faulting
- [High] mm: cross-process mmap_lock dependency blocks NPU job submission
--

commit c2a130c1477d63bd08139e79291eca214fcf225a
Author: David Zhang <yidong.zhang@amd.com>

accel/amdxdna: Make populate_range common for AIE2 and AIE4

This commit moves aie2_populate_range() from aie2_ctx.c into amdxdna_gem.c
as amdxdna_populate_range(), making it common for both AIE2 and AIE4
command submissions.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index e1b72cef51bdf..320604ec157f7 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c

[ ... ]

> @@ -337,6 +337,79 @@ void amdxdna_umap_put(struct amdxdna_umap *mapp)
>  	kref_put(&mapp->refcnt, amdxdna_umap_release);
>  }
>  
> +int amdxdna_populate_range(struct amdxdna_gem_obj *abo)
> +{
> +	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
> +	struct amdxdna_umap *mapp;
> +	unsigned long timeout;
> +	struct mm_struct *mm;
> +	bool found;
> +	int ret;
> +
> +	timeout = msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> +again:
> +	found = false;
> +	down_write(&xdna->notifier_lock);
> +	list_for_each_entry(mapp, &abo->mem.umap_list, node) {

[Severity: High]
Does this code iterate over all mappings across all processes without
filtering by the current process?

If an unprivileged process imports a shared dmabuf, maps it, and holds its
mmap_write_lock indefinitely (e.g., via userfaultfd or FUSE), it could cause
this function to block indefinitely when attempting to acquire the
mmap_read_lock for that other process's mm. 

Could this result in a denial of service by permanently blocking job
submission for the process that exported the dmabuf?

> +		/*
> +		 * Skip entries that have already been unmapped.
> +		 *
> +		 * If userspace unmaps the address and later submits I/O using
> +		 * it, the IOMMU will reject the access and report a fault.
> +		 * Ignore such entries here.
> +		 */
> +		if (mapp->unmapped)
> +			continue;
> +
> +		if (mapp->invalid && kref_get_unless_zero(&mapp->refcnt)) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	if (!found) {

[ ... ]

> +	if (!mmget_not_zero(mm)) {
> +		amdxdna_umap_put(mapp);
> +		return -EFAULT;
> +	}
> +
> +	ret = hmm_range_fault_unlocked_timeout(&mapp->range, timeout);

[Severity: High]
Can multiple concurrent job submissions on the same GEM object cause a data
race here?

When multiple hardware contexts submit jobs referencing the same GEM object
with invalidated memory mappings, aie2_cmd_submit() drops GEM reservations
and notifier_lock before calling amdxdna_populate_range(). Because
mapp->invalid remains true until the fault is completed, concurrent
submissions will find the exact same amdxdna_umap object and invoke
hmm_range_fault_unlocked_timeout() simultaneously on the identical
&mapp->range structure.

The HMM API requires struct hmm_range to be thread-local or synchronized by
the caller. The internal mmap_read_lock only protects the VMA, not the
hmm_range struct itself.

Could these unsynchronized writes to range->notifier_seq and the
range->hmm_pfns array break the mmu_interval_read_retry() logic, circumvent
MMU invalidations, or lead to memory corruption?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009225343.3079074-1-yidong.zhang@amd.com?part=15