[PATCH V2] accel/amdxdna: Support read-only user-pointer BO mappings

[PATCH V2] accel/amdxdna: Support read-only user-pointer BO mappings

[Lizhi Hou]

From: Max Zhen <max.zhen@amd.com>

Update the amdxdna user-pointer (ubuf) BO path to support creating buffer
objects from read-only user mappings.

Detect read-only VMAs by checking VMA permissions across all user virtual
address ranges associated with the BO. When all entries are read-only, pin
user pages without FOLL_WRITE and export the resulting dmabuf as read-only
(O_RDONLY).

This allows userptr BOs backed by read-only mappings to be safely imported
and used without requiring write access, which was previously rejected due
to unconditional FOLL_WRITE usage.

Signed-off-by: Max Zhen <max.zhen@amd.com>
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/amdxdna_ubuf.c | 29 ++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.c b/drivers/accel/amdxdna/amdxdna_ubuf.c
index 4c0647057759..3769210c55cc 100644
--- a/drivers/accel/amdxdna/amdxdna_ubuf.c
+++ b/drivers/accel/amdxdna/amdxdna_ubuf.c
@@ -125,6 +125,26 @@ static const struct dma_buf_ops amdxdna_ubuf_dmabuf_ops = {
 	.vunmap = amdxdna_ubuf_vunmap,
 };
 
+static int readonly_va_entry(struct amdxdna_drm_va_entry *va_ent)
+{
+	struct mm_struct *mm = current->mm;
+	struct vm_area_struct *vma;
+	int ret;
+
+	mmap_read_lock(mm);
+
+	vma = find_vma(mm, va_ent->vaddr);
+	if (!vma ||
+	    vma->vm_start > va_ent->vaddr ||
+	    vma->vm_end - va_ent->vaddr < va_ent->len)
+		ret = -ENOENT;
+	else
+		ret = vma->vm_flags & VM_WRITE ? 0 : 1;
+
+	mmap_read_unlock(mm);
+	return ret;
+}
+
 struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
 				 u32 num_entries, void __user *va_entries)
 {
@@ -134,6 +154,7 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
 	struct amdxdna_ubuf_priv *ubuf;
 	u32 npages, start = 0;
 	struct dma_buf *dbuf;
+	bool readonly = true;
 	int i, ret;
 	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
 
@@ -172,6 +193,10 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
 			ret = -EINVAL;
 			goto free_ent;
 		}
+
+		/* Pin pages as writable as long as not all entries are read-only. */
+		if (readonly && readonly_va_entry(&va_ent[i]) != 1)
+			readonly = false;
 	}
 
 	ubuf->nr_pages = exp_info.size >> PAGE_SHIFT;
@@ -194,7 +219,7 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
 		npages = va_ent[i].len >> PAGE_SHIFT;
 
 		ret = pin_user_pages_fast(va_ent[i].vaddr, npages,
-					  FOLL_WRITE | FOLL_LONGTERM,
+					  (readonly ? 0 : FOLL_WRITE) | FOLL_LONGTERM,
 					  &ubuf->pages[start]);
 		if (ret >= 0) {
 			start += ret;
@@ -211,7 +236,7 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
 
 	exp_info.ops = &amdxdna_ubuf_dmabuf_ops;
 	exp_info.priv = ubuf;
-	exp_info.flags = O_RDWR | O_CLOEXEC;
+	exp_info.flags = (readonly ? O_RDONLY : O_RDWR) | O_CLOEXEC;
 
 	dbuf = dma_buf_export(&exp_info);
 	if (IS_ERR(dbuf)) {
-- 
2.34.1

Re: [PATCH V2] accel/amdxdna: Support read-only user-pointer BO mappings

[Mario Limonciello]

On 3/31/26 12:26, Lizhi Hou wrote:
> From: Max Zhen <max.zhen@amd.com>
> 
> Update the amdxdna user-pointer (ubuf) BO path to support creating buffer
> objects from read-only user mappings.
> 
> Detect read-only VMAs by checking VMA permissions across all user virtual
> address ranges associated with the BO. When all entries are read-only, pin
> user pages without FOLL_WRITE and export the resulting dmabuf as read-only
> (O_RDONLY).
> 
> This allows userptr BOs backed by read-only mappings to be safely imported
> and used without requiring write access, which was previously rejected due
> to unconditional FOLL_WRITE usage.
> 
> Signed-off-by: Max Zhen <max.zhen@amd.com>
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> ---
>   drivers/accel/amdxdna/amdxdna_ubuf.c | 29 ++++++++++++++++++++++++++--
>   1 file changed, 27 insertions(+), 2 deletions(-)
Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
> 
> diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.c b/drivers/accel/amdxdna/amdxdna_ubuf.c
> index 4c0647057759..3769210c55cc 100644
> --- a/drivers/accel/amdxdna/amdxdna_ubuf.c
> +++ b/drivers/accel/amdxdna/amdxdna_ubuf.c
> @@ -125,6 +125,26 @@ static const struct dma_buf_ops amdxdna_ubuf_dmabuf_ops = {
>   	.vunmap = amdxdna_ubuf_vunmap,
>   };
>   
> +static int readonly_va_entry(struct amdxdna_drm_va_entry *va_ent)
> +{
> +	struct mm_struct *mm = current->mm;
> +	struct vm_area_struct *vma;
> +	int ret;
> +
> +	mmap_read_lock(mm);
> +
> +	vma = find_vma(mm, va_ent->vaddr);
> +	if (!vma ||
> +	    vma->vm_start > va_ent->vaddr ||
> +	    vma->vm_end - va_ent->vaddr < va_ent->len)
> +		ret = -ENOENT;
> +	else
> +		ret = vma->vm_flags & VM_WRITE ? 0 : 1;
> +
> +	mmap_read_unlock(mm);
> +	return ret;
> +}
> +
>   struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
>   				 u32 num_entries, void __user *va_entries)
>   {
> @@ -134,6 +154,7 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
>   	struct amdxdna_ubuf_priv *ubuf;
>   	u32 npages, start = 0;
>   	struct dma_buf *dbuf;
> +	bool readonly = true;
>   	int i, ret;
>   	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
>   
> @@ -172,6 +193,10 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
>   			ret = -EINVAL;
>   			goto free_ent;
>   		}
> +
> +		/* Pin pages as writable as long as not all entries are read-only. */
> +		if (readonly && readonly_va_entry(&va_ent[i]) != 1)
> +			readonly = false;
>   	}
>   
>   	ubuf->nr_pages = exp_info.size >> PAGE_SHIFT;
> @@ -194,7 +219,7 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
>   		npages = va_ent[i].len >> PAGE_SHIFT;
>   
>   		ret = pin_user_pages_fast(va_ent[i].vaddr, npages,
> -					  FOLL_WRITE | FOLL_LONGTERM,
> +					  (readonly ? 0 : FOLL_WRITE) | FOLL_LONGTERM,
>   					  &ubuf->pages[start]);
>   		if (ret >= 0) {
>   			start += ret;
> @@ -211,7 +236,7 @@ struct dma_buf *amdxdna_get_ubuf(struct drm_device *dev,
>   
>   	exp_info.ops = &amdxdna_ubuf_dmabuf_ops;
>   	exp_info.priv = ubuf;
> -	exp_info.flags = O_RDWR | O_CLOEXEC;
> +	exp_info.flags = (readonly ? O_RDONLY : O_RDWR) | O_CLOEXEC;
>   
>   	dbuf = dma_buf_export(&exp_info);
>   	if (IS_ERR(dbuf)) {

Claude review: accel/amdxdna: Support read-only user-pointer BO mappings

[Claude Code Review Bot]

Overall Series Review

Subject: accel/amdxdna: Support read-only user-pointer BO mappings
Author: Lizhi Hou <lizhi.hou@amd.com>
Patches: 2
Reviewed: 2026-04-01T07:35:33.389139

---

This is a single patch (V2) that adds read-only user-pointer BO support to the amdxdna accelerator driver. The motivation is sound: allowing userptr BOs backed by read-only mappings (e.g., read-only mmap'd files) to be pinned and exported without requiring write access, which previously failed unconditionally due to `FOLL_WRITE`.

The approach is reasonable — check VMA permissions, skip `FOLL_WRITE` when all entries are read-only, and export the dmabuf with `O_RDONLY`. However, there are several issues around the VMA checking function, TOCTOU considerations, and missing enforcement on the DMA-BUF side that should be addressed.

---

---
Generated by Claude Code Patch Reviewer

Claude review: accel/amdxdna: Support read-only user-pointer BO mappings

[Claude Code Review Bot]

Patch Review

**1. `readonly_va_entry` has an awkward return convention**

```c
static int readonly_va_entry(struct amdxdna_drm_va_entry *va_ent)
```

This function returns three distinct values: negative error (`-ENOENT`), `0` for writable, and `1` for read-only. The caller treats anything `!= 1` as "not read-only":

```c
if (readonly && readonly_va_entry(&va_ent[i]) != 1)
    readonly = false;
```

This means a VMA lookup failure (e.g., the address range isn't mapped at all) silently falls back to `FOLL_WRITE` rather than reporting an error. While `pin_user_pages_fast` would catch this later, it would be cleaner to either: (a) return a `bool` and handle errors separately, or (b) propagate the error so the caller can fail early with a meaningful message rather than deferring to a less informative `pin_user_pages_fast` failure.

**2. Multi-VMA ranges are not handled**

```c
vma = find_vma(mm, va_ent->vaddr);
if (!vma ||
    vma->vm_start > va_ent->vaddr ||
    vma->vm_end - va_ent->vaddr < va_ent->len)
    ret = -ENOENT;
```

If a single `va_entry` spans multiple VMAs, this check will fail because the first VMA won't cover the entire length. The function returns `-ENOENT`, causing fallback to `FOLL_WRITE`. This is safe (more permissive) but means the read-only optimization silently doesn't work for legitimate multi-VMA ranges. Consider using `vma_iterator` to walk all VMAs covering the range, or at least add a comment documenting this limitation.

**3. TOCTOU race between VMA check and page pinning**

The VMA flags are checked under `mmap_read_lock` in `readonly_va_entry`, but the lock is dropped before `pin_user_pages_fast` is called. Between those two points, `mprotect()` could change the VMA from read-only to writable (or vice versa). 

The dangerous direction: if the VMA is read-only at check time but becomes writable before pinning, pages are pinned without `FOLL_WRITE`. This means the pinned pages might be CoW copies. If the device then DMA-writes to them, it could corrupt memory without the mm subsystem knowing about the write (no page fault to break CoW). In practice this is a narrow race and self-inflicted by the user, but it's worth acknowledging.

**4. No enforcement of read-only on the DMA-BUF mapping side**

When `readonly` is true, the dmabuf is exported with `O_RDONLY`:

```c
exp_info.flags = (readonly ? O_RDONLY : O_RDWR) | O_CLOEXEC;
```

But `amdxdna_ubuf_map` does not check or enforce the DMA direction against the read-only state:

```c
static struct sg_table *amdxdna_ubuf_map(struct dma_buf_attachment *attach,
                                         enum dma_data_direction direction)
```

An importer could attach with `DMA_FROM_DEVICE` (device writes to memory) and the map callback would happily create a writable DMA mapping to read-only-pinned pages. Consider storing the `readonly` flag in `struct amdxdna_ubuf_priv` and rejecting `DMA_FROM_DEVICE` / `DMA_BIDIRECTIONAL` directions in `amdxdna_ubuf_map` when the buffer is read-only. This would be the actual enforcement point.

**5. Minor: `vma->vm_end - va_ent->vaddr` potential underflow**

```c
vma->vm_end - va_ent->vaddr < va_ent->len
```

If `vma->vm_start > va_ent->vaddr`, we reach this check only when the first condition `vma->vm_start > va_ent->vaddr` is false (due to the `||` chain). So `va_ent->vaddr >= vma->vm_start`, which means `va_ent->vaddr < vma->vm_end` (since `find_vma` returns the first VMA with `vm_end > addr`), so the subtraction is safe. This is fine, just noting for completeness.

**6. The `readonly` variable should be stored for later use**

The `readonly` determination is made during setup but only used ephemerally for the pin and export flags. If the buffer is later re-attached or needs to enforce access control, there's no record of whether it was read-only. Consider adding a `bool readonly` field to `struct amdxdna_ubuf_priv`.

**Summary**: The patch enables a useful feature but the protection model is incomplete — the read-only intent is detected and used for page pinning, but not enforced at the DMA-BUF map callback level. The VMA checking function could also be more robust in its error handling and multi-VMA support.

---
Generated by Claude Code Patch Reviewer