[PATCH 0/3] accel/amdxdna: honour the SYNC_BO range

[PATCH 0/3] accel/amdxdna: honour the SYNC_BO range

[Taimuraz Kaitmazov]

SYNC_BO carries an offset and a size, but amdxdna_flush_bo() honours them
only on the vmap path. An imported BO is tested for first and flushes its
whole scatterlist, so a sync costs what the BO is worth rather than what
the caller asked to maintain: on npu4 an imported 64 MiB BO cost 1056 us
to sync at every size from 4 KiB up. Patch 3 reorders the arms so the
vmap path is tried first, and indexes the page-array fallback from the
requested offset.

Reaching an imported BO through vmap needs two things in front of it.
Patch 1 refuses an I/O memory mapping, which the driver currently stores
as if it were an ordinary kernel address; that one stands on its own and
does not depend on the rest of the series, so it can be taken separately
or first. Patch 2 adds a probe that does not log, so that patch 3 does
not make an exporter without a vmap op print on every ioctl; it is a
refactor and does not depend on patch 1 either. Only patch 3 depends on
both: on patch 2 for the probe, and on patch 1 to keep an I/O mapping
out of the flush it adds.

The measurements in patch 3 were taken with the equivalent change in
AMD's out-of-tree xdna-driver, where this merged as #1541. That version
and this one differ in two places, neither of them on the path the
measurement exercises: it carries a second page-array fallback that
mainline has no field for, reached only when the mapping fails and the BO
is neither imported nor shmem backed, and the mapping helper has a
different name. The flush and the helper are otherwise identical. This
version is compile-tested; it has not been booted.

Patch 1 is from inspection rather than a reproducer. The exporter I can
test against is amdgpu, and amdgpu is the case that cannot reach it: it
implements .pin, so a non peer to peer attachment like this driver's
forces the buffer to GTT before anything maps it. Reproducing it needs a
GPU whose exporter has no .pin, which I do not have paired with an NPU
here.

Taimuraz Kaitmazov (3):
  accel/amdxdna: refuse an I/O memory mapping of an imported BO
  accel/amdxdna: add a quiet variant of amdxdna_gem_vmap()
  accel/amdxdna: flush only the requested range in amdxdna_flush_bo

 drivers/accel/amdxdna/amdxdna_gem.c | 53 +++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 15 deletions(-)

-- 
2.55.0

[PATCH 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO

[Taimuraz Kaitmazov]

amdxdna_gem_obj_vmap() accepts whatever dma_buf_vmap() returns and only
rejects a NULL vaddr. struct iosys_map is a union discriminated by
is_iomem, so an exporter that answers with an I/O mapping leaves a
void __iomem pointer in map->vaddr, and amdxdna_gem_vmap() stores it in
abo->mem.kva, which callers use as an ordinary kernel address:
amdxdna_cmd_set_error() memsets and memcpys through it.

amdxdna_drm_va_tbl takes a dmabuf_fd, so a BO of type AMDXDNA_BO_SHARE
or AMDXDNA_BO_CMD can be any exporter's buffer. Whether such a buffer is
still in a bus aperture when it is mapped depends on the exporter.
amdxdna attaches without importer ops, so an exporter that implements
.pin has it called before the mapping, and amdgpu's removes VRAM from
the allowed domains as soon as one attachment cannot do peer to peer,
which this driver's cannot; an amdgpu buffer is therefore in GTT
before any of this runs. An exporter using drm_gem_prime_dmabuf_ops
has no .pin at all, nothing moves the buffer, and drm_gem_ttm_vmap()
answers with iosys_map_set_vaddr_iomem() for a VRAM resident object.
nouveau and radeon are in that group, so an NPU paired with one of
those GPUs reaches this.

Refuse the mapping, so it never reaches a caller that cannot use it.
vmw_gem_vmap() does the same for the same reason. Unlike that one, this
path is reachable from an unprivileged ioctl, so it does not warn;
-EOPNOTSUPP is what drm_gem_vmap_locked() already returns here for an
exporter with no vmap op.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index 1f190b319..b66ec9e48 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -683,10 +683,16 @@ static int amdxdna_gem_obj_vmap(struct drm_gem_object *obj, struct iosys_map *ma
 
 	dma_resv_assert_held(obj->resv);
 
-	if (is_import_bo(abo))
+	if (is_import_bo(abo)) {
 		ret = dma_buf_vmap(abo->dma_buf, map);
-	else
+		/* Callers use mem.kva as an ordinary kernel address. */
+		if (!ret && map->is_iomem) {
+			dma_buf_vunmap(abo->dma_buf, map);
+			return -EOPNOTSUPP;
+		}
+	} else {
 		ret = drm_gem_shmem_object_vmap(obj, map);
+	}
 	if (ret)
 		return ret;
 	if (!map->vaddr)
-- 
2.55.0

[PATCH 2/3] accel/amdxdna: add a quiet variant of amdxdna_gem_vmap()

[Taimuraz Kaitmazov]

amdxdna_gem_vmap() logs an error whenever the mapping fails, which suits
its callers: each of them treats a failure as fatal to the operation it
is performing. The next patch adds one that does not, and an exporter
that implements no vmap op fails every call without anything caching
that, so a logging probe would print on every ioctl.

Split the mapping out into __amdxdna_gem_vmap(), which returns the error,
and leave amdxdna_gem_vmap() as that plus the log. No caller changes: it
still returns the address, or NULL after logging.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index b66ec9e48..d0c846b02 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -191,12 +191,8 @@ amdxdna_gem_destroy_obj(struct amdxdna_gem_obj *abo)
 	kfree(abo);
 }
 
-/*
- * Obtains a kernel virtual address on the BO (usually of small size).
- * The mapping is established on the first call and stays valid until
- * amdxdna_gem_vunmap() is called.
- */
-void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
+/* Returns the error instead of logging it. */
+static void *__amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
 {
 	struct iosys_map map = IOSYS_MAP_INIT_VADDR(NULL);
 	int ret;
@@ -210,13 +206,28 @@ void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
 	if (!abo->mem.kva) {
 		ret = drm_gem_vmap(to_gobj(abo), &map);
 		if (ret)
-			XDNA_ERR(abo->client->xdna, "Vmap bo failed, ret %d", ret);
-		else
-			abo->mem.kva = map.vaddr;
+			return ERR_PTR(ret);
+		abo->mem.kva = map.vaddr;
 	}
 	return abo->mem.kva;
 }
 
+/*
+ * Obtains a kernel virtual address on the BO (usually of small size).
+ * The mapping is established on the first call and stays valid until
+ * amdxdna_gem_vunmap() is called.
+ */
+void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
+{
+	void *kva = __amdxdna_gem_vmap(abo);
+
+	if (IS_ERR(kva)) {
+		XDNA_ERR(abo->client->xdna, "Vmap bo failed, ret %ld", PTR_ERR(kva));
+		return NULL;
+	}
+	return kva;
+}
+
 /*
  * Free mapping established through amdxdna_gem_vmap()
  */
-- 
2.55.0

[PATCH 3/3] accel/amdxdna: flush only the requested range in amdxdna_flush_bo

[Taimuraz Kaitmazov]

SYNC_BO carries an offset and a size, but amdxdna_flush_bo() honours
them only on the vmap path: an imported BO is tested for first and
flushes its whole scatterlist, and the page-array fallback flushes every
page of the BO. A sync costs what the BO is worth rather than what the
caller asked to maintain.

amdxdna_gem_obj_vmap() maps an imported BO through dma_buf_vmap(), so
try the vmap path first and leave drm_clflush_sg() as the fallback for
an exporter that cannot serve one. Index the page-array fallback from
the requested offset. An imported BO now holds a kernel mapping from its
first sync until it is freed, as a shmem BO already does.

Measured on npu4, 64 MiB BO, pinned, minimum of 50 runs: an imported BO
cost 1056 us to sync at every size from 4 KiB up, and now tracks the
driver-owned BO at 0.7 us for 4 KiB, 17 us for 1 MiB and 1056 us for the
whole BO. The driver-owned column does not move.

An earlier version walked the scatterlist a page at a time instead. It
fixed the range case but cost about 179 ns per page of barrier and call
overhead, taking the whole-BO sync from 1056 to 3989 us, so this one
reuses the mapping instead.

This does not bracket the flush with dma_buf_begin_cpu_access() and
dma_buf_end_cpu_access(). The driver has never called them, here or
anywhere else, so the omission predates this change; what changes is that
the vmap path now serves an imported BO by default, which is where the
exporter's own coherency hook would matter most. Adding the bracket is
follow-up work rather than part of this one: the calls carry a direction
but no range, so pairing them with a ranged flush wants its own reasoning
and its own measurement.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index d0c846b02..4886f7c08 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -1234,6 +1234,8 @@ int amdxdna_drm_get_bo_info_ioctl(struct drm_device *dev, void *data, struct drm
 
 static int amdxdna_flush_bo(struct amdxdna_gem_obj *abo, u64 offset, u64 size)
 {
+	unsigned long first, nr_pages;
+	void *kva;
 	u64 end;
 
 	if (offset >= abo->mem.size)
@@ -1243,12 +1245,16 @@ static int amdxdna_flush_bo(struct amdxdna_gem_obj *abo, u64 offset, u64 size)
 		return -EINVAL;
 
 	size = min(abo->mem.size, end) - offset;
-	if (is_import_bo(abo))
+	first = offset >> PAGE_SHIFT;
+	nr_pages = (PAGE_ALIGN(offset + size) >> PAGE_SHIFT) - first;
+
+	kva = __amdxdna_gem_vmap(abo);
+	if (!IS_ERR(kva))
+		drm_clflush_virt_range(kva + offset, size);
+	else if (is_import_bo(abo))
 		drm_clflush_sg(abo->base.sgt);
-	else if (amdxdna_gem_vmap(abo))
-		drm_clflush_virt_range(amdxdna_gem_vmap(abo) + offset, size);
 	else if (abo->base.pages)
-		drm_clflush_pages(abo->base.pages, abo->mem.size >> PAGE_SHIFT);
+		drm_clflush_pages(&abo->base.pages[first], nr_pages);
 	else
 		return -EINVAL;
 
-- 
2.55.0

Re: [PATCH 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing `amdxdna_gem_put_obj()` on the error path in `amdxdna_cmd_set_error` leaks a GEM object reference.
--

--- Patch [1]: [PATCH 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO ---
commit c573f7ee611c1dede95280b62a62d7abef0e2670
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>

accel/amdxdna: refuse an I/O memory mapping of an imported BO

amdxdna_gem_obj_vmap() accepts whatever dma_buf_vmap() returns and only
rejects a NULL vaddr. struct iosys_map is a union discriminated by
is_iomem, so an exporter that answers with an I/O mapping leaves a
void __iomem pointer in map->vaddr, and amdxdna_gem_vmap() stores it in
abo->mem.kva, which callers use as an ordinary kernel address:
amdxdna_cmd_set_error() memsets and memcpys through it.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 1f190b319bb6e..b66ec9e482894 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -683,10 +683,16 @@ static int amdxdna_gem_obj_vmap(struct drm_gem_object *obj, struct iosys_map *ma
>  
>  	dma_resv_assert_held(obj->resv);
>  
> -	if (is_import_bo(abo))
> +	if (is_import_bo(abo)) {
>  		ret = dma_buf_vmap(abo->dma_buf, map);
> -	else
> +		/* Callers use mem.kva as an ordinary kernel address. */
> +		if (!ret && map->is_iomem) {
> +			dma_buf_vunmap(abo->dma_buf, map);
> +			return -EOPNOTSUPP;
> +		}

[Severity: High]
This isn't a bug introduced by this patch, but refusing the mapping here
exposes a reference leak in amdxdna_cmd_set_error().

If unprivileged userspace imports an IOMEM DMABUF and submits it as an
ERT_CMD_CHAIN command that causes an error, we reach this code in
drivers/accel/amdxdna/amdxdna_ctx.c:

	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) {
		cc = amdxdna_cmd_get_payload(abo, NULL);
		cc->error_index = (cmd_idx < cc->command_count) ? cmd_idx : 0;
		abo = amdxdna_gem_get_obj(client, cc->data[0], AMDXDNA_BO_SHARE);
		if (!abo)
			return -EINVAL;
		cmd = amdxdna_gem_vmap(abo);
		if (!cmd)
			return -ENOMEM;
	}

Because amdxdna_gem_vmap() will now return NULL for the IOMEM BO, the code
returns -ENOMEM immediately. Does this leak the GEM object reference acquired
by amdxdna_gem_get_obj() since it bypasses the amdxdna_gem_put_obj() cleanup
located at the end of the function?

> +	} else {
>  		ret = drm_gem_shmem_object_vmap(obj, map);
> +	}
>  	if (ret)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811204556.875037-1-taimuraz@kaitmazov.com?part=1

Re: [PATCH 2/3] accel/amdxdna: add a quiet variant of amdxdna_gem_vmap()

[sashiko-bot]

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

Pre-existing issues:
- [High] Accessing `abo->client->xdna` in the error logging path of `amdxdna_gem_vmap()` (and in job execution paths like `aie2_init_exec_req()`) causes a NULL pointer dereference if the user closes the GEM handle concurrently.
--

--- Patch [2]: [PATCH 2/3] accel/amdxdna: add a quiet variant of amdxdna_gem_vmap() ---
commit d54f02782d28a33a9fbd50ba981aaddfeb1cebb5
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>

accel/amdxdna: add a quiet variant of amdxdna_gem_vmap()

This commit splits amdxdna_gem_vmap() into an inner function that returns
an error code and an outer function that retains the logging behavior. This
allows callers who expect mapping failures to avoid spamming the log.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index b66ec9e482894..d0c846b02694b 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c

[ ... ]

> @@ -210,13 +206,28 @@ void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
>  	if (!abo->mem.kva) {
>  		ret = drm_gem_vmap(to_gobj(abo), &map);
>  		if (ret)
> -			XDNA_ERR(abo->client->xdna, "Vmap bo failed, ret %d", ret);
> -		else
> -			abo->mem.kva = map.vaddr;
> +			return ERR_PTR(ret);
> +		abo->mem.kva = map.vaddr;
>  	}
>  	return abo->mem.kva;
>  }
>  
> +/*
> + * Obtains a kernel virtual address on the BO (usually of small size).
> + * The mapping is established on the first call and stays valid until
> + * amdxdna_gem_vunmap() is called.
> + */
> +void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
> +{
> +	void *kva = __amdxdna_gem_vmap(abo);
> +
> +	if (IS_ERR(kva)) {
> +		XDNA_ERR(abo->client->xdna, "Vmap bo failed, ret %ld", PTR_ERR(kva));

[Severity: High]
This is a pre-existing issue, but could this access of abo->client risk
a NULL pointer dereference?

If an unprivileged user submits a job referencing a GEM BO and immediately
closes the BO handle via DRM_IOCTL_GEM_CLOSE, amdxdna_gem_obj_close() gets
called and sets abo->client to NULL:

amdxdna_gem_obj_close() {
    ...
    abo->client = NULL;
}

The DRM scheduler still holds a reference to the GEM object itself. When the
job runs asynchronously, it may call amdxdna_gem_vmap(). If the mapping fails,
we reach this error logging path.

Because the patch splits the function and __amdxdna_gem_vmap() releases
abo->lock before returning, does this widen the race window and allow a
concurrent handle close to nullify abo->client exactly before it is
dereferenced here?

> +		return NULL;
> +	}
> +	return kva;
> +}
> +
>  /*
>   * Free mapping established through amdxdna_gem_vmap()
>   */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811204556.875037-1-taimuraz@kaitmazov.com?part=2

Re: [PATCH 3/3] accel/amdxdna: flush only the requested range in amdxdna_flush_bo

[sashiko-bot]

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

New issues:
- [High] Holding a vmap mapping for the lifetime of an imported DMA-buf violates the dma-buf API and permanently pins the buffer in the exporter.

Pre-existing issues:
- [High] Userspace can trigger a kernel page fault (DoS) by passing size=0 to the AMDXDNA_DRM_SYNC_BO ioctl.
- [High] CPU cache flushes on imported DMA-BUFs are performed without dma_buf_begin_cpu_access() and dma_buf_end_cpu_access().
--

--- Patch [3]: [PATCH 3/3] accel/amdxdna: flush only the requested range in amdxdna_flush_bo ---
commit b478fba210b28db0ebe941498872d630befd0f64
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>

accel/amdxdna: flush only the requested range in amdxdna_flush_bo

This commit updates amdxdna_flush_bo() to only flush the requested range of a
buffer object instead of the entire buffer. It tries the vmap path first for
imported BOs, falling back to other flush methods. This reduces the time
spent syncing large BOs.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index d0c846b02694b..4886f7c082da1 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
[ ... ]
> @@ -1243,12 +1245,16 @@ static int amdxdna_flush_bo(struct amdxdna_gem_obj *abo, u64 offset, u64 size)
>  		return -EINVAL;
>  
>  	size = min(abo->mem.size, end) - offset;
> -	if (is_import_bo(abo))
> +	first = offset >> PAGE_SHIFT;
> +	nr_pages = (PAGE_ALIGN(offset + size) >> PAGE_SHIFT) - first;
> +
> +	kva = __amdxdna_gem_vmap(abo);

[Severity: High]
Does this code pin the imported buffer in the exporter indefinitely?

When __amdxdna_gem_vmap() is called on an imported BO in
drivers/accel/amdxdna/amdxdna_gem.c, it uses dma_buf_vmap() which establishes a
persistent mapping stored in abo->mem.kva.

According to the dma-buf API, mappings must not be held for the lifetime of the
buffer because they prevent the exporter (like AMDGPU) from migrating the
buffer. By replacing the transient drm_clflush_sg() fallback with a persistent
vmap, userspace can intentionally or unintentionally pin VRAM in the exporter,
potentially leading to GPU memory exhaustion or eviction failures.

> +	if (!IS_ERR(kva))
> +		drm_clflush_virt_range(kva + offset, size);

[Severity: High]
This is a pre-existing issue, but can this cause a kernel page fault if
userspace passes size=0 to the AMDXDNA_DRM_SYNC_BO ioctl?

In amdxdna_drm_sync_bo_ioctl(), if args->size is 0, it gets passed to
amdxdna_flush_bo(). This calculates size = 0 and calls drm_clflush_virt_range()
with a length of 0.

In drm_clflush_virt_range() on x86, if length is 0, the for loop is skipped,
but it unconditionally executes:

    clflushopt(end - 1);

Since vmap allocations are often preceded by unmapped guard pages, accessing
kva - 1 can trigger an unhandled kernel page fault, potentially allowing a
local denial of service.

[Severity: High]
This isn't a bug introduced by this patch, but does this perform CPU cache
flushes on imported DMA-BUFs without invoking dma_buf_begin_cpu_access() and
dma_buf_end_cpu_access()?

The dma-buf API mandates that importers must bracket any CPU access to imported
buffers with these hooks. Bypassing the exporter's specific cache maintenance
requirements can potentially lead to cache coherency issues or data corruption.

> +	else if (is_import_bo(abo))
>  		drm_clflush_sg(abo->base.sgt);
> -	else if (amdxdna_gem_vmap(abo))
> -		drm_clflush_virt_range(amdxdna_gem_vmap(abo) + offset, size);
>  	else if (abo->base.pages)
> -		drm_clflush_pages(abo->base.pages, abo->mem.size >> PAGE_SHIFT);
> +		drm_clflush_pages(&abo->base.pages[first], nr_pages);
>  	else
>  		return -EINVAL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811204556.875037-1-taimuraz@kaitmazov.com?part=3