[PATCH V1] Revert "accel/amdxdna: Support read-only user-pointer BO mappings"

[PATCH V1] Revert "accel/amdxdna: Support read-only user-pointer BO mappings"

[Lizhi Hou]

This reverts commit f649e63d4a6423eda8eb208638849fd6396aedd7.

The read-only feature requires further consideration.

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

diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.c b/drivers/accel/amdxdna/amdxdna_ubuf.c
index 3769210c55cc..4c0647057759 100644
--- a/drivers/accel/amdxdna/amdxdna_ubuf.c
+++ b/drivers/accel/amdxdna/amdxdna_ubuf.c
@@ -125,26 +125,6 @@ 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)
 {
@@ -154,7 +134,6 @@ 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);
 
@@ -193,10 +172,6 @@ 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;
@@ -219,7 +194,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,
-					  (readonly ? 0 : FOLL_WRITE) | FOLL_LONGTERM,
+					  FOLL_WRITE | FOLL_LONGTERM,
 					  &ubuf->pages[start]);
 		if (ret >= 0) {
 			start += ret;
@@ -236,7 +211,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 = (readonly ? O_RDONLY : O_RDWR) | O_CLOEXEC;
+	exp_info.flags = O_RDWR | O_CLOEXEC;
 
 	dbuf = dma_buf_export(&exp_info);
 	if (IS_ERR(dbuf)) {
-- 
2.34.1

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

[Christian König]

On 5/21/26 18:29, Lizhi Hou wrote:
> This reverts commit f649e63d4a6423eda8eb208638849fd6396aedd7.
> 
> The read-only feature requires further consideration.
> 
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>

Acked-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/accel/amdxdna/amdxdna_ubuf.c | 29 ++--------------------------
>  1 file changed, 2 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.c b/drivers/accel/amdxdna/amdxdna_ubuf.c
> index 3769210c55cc..4c0647057759 100644
> --- a/drivers/accel/amdxdna/amdxdna_ubuf.c
> +++ b/drivers/accel/amdxdna/amdxdna_ubuf.c
> @@ -125,26 +125,6 @@ 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)
>  {
> @@ -154,7 +134,6 @@ 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);
>  
> @@ -193,10 +172,6 @@ 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;
> @@ -219,7 +194,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,
> -					  (readonly ? 0 : FOLL_WRITE) | FOLL_LONGTERM,
> +					  FOLL_WRITE | FOLL_LONGTERM,
>  					  &ubuf->pages[start]);
>  		if (ret >= 0) {
>  			start += ret;
> @@ -236,7 +211,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 = (readonly ? O_RDONLY : O_RDWR) | O_CLOEXEC;
> +	exp_info.flags = O_RDWR | O_CLOEXEC;
>  
>  	dbuf = dma_buf_export(&exp_info);
>  	if (IS_ERR(dbuf)) {

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

[Claude Code Review Bot]

Overall Series Review

Subject: Revert "accel/amdxdna: Support read-only user-pointer BO mappings"
Author: Lizhi Hou <lizhi.hou@amd.com>
Patches: 2
Reviewed: 2026-05-25T19:46:43.479862

---

This is a single-patch series that reverts commit `f649e63d4a64` ("accel/amdxdna: Support read-only user-pointer BO mappings") from the amdxdna accelerator driver. The patch is mechanically correct as a revert — it removes the `readonly_va_entry()` function, the `readonly` tracking variable, and changes `pin_user_pages_fast()` and `dma_buf_export()` to always use writable/RDWR semantics.

The commit message is minimal: *"The read-only feature requires further consideration."* This is vague. For a revert of a feature that was presumably reviewed and merged, the rationale should explain **what** needs further consideration — whether there's a correctness bug, a security issue, an API concern, or a design disagreement. Reviewers and future readers of `git log` deserve more than this.

That said, the reverted code did have real problems worth reverting for, which makes this a reasonable change on technical grounds even if the commit message doesn't articulate them.

**Verdict:** The revert is mechanically sound and likely justified, but the commit message needs improvement.

---

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Commit message issues:**

The message "The read-only feature requires further consideration" is insufficiently detailed for a revert. It should explain the specific problems. The original `readonly_va_entry()` function had at least two notable issues:

1. **Race condition / TOCTOU:** The function checks VMA flags (`vm_flags & VM_WRITE`) under `mmap_read_lock`, but by the time `pin_user_pages_fast()` is called (which does its own VMA lookup internally), the VMA could have changed — it could have been split, unmapped, or had its permissions changed via `mprotect()`. This is a classic time-of-check-to-time-of-use race. The `pin_user_pages_fast` GUP path already handles write permission checking internally when `FOLL_WRITE` is (or isn't) passed, so duplicating that logic at a different point in time is fragile.

2. **Spanning multiple VMAs:** The check `vma->vm_end - va_ent->vaddr < va_ent->len` assumes the entire range is covered by a single VMA, but a user-pointer range could span multiple VMAs with different permissions. `find_vma()` only returns the first one. This means the read-only detection could be incorrect for multi-VMA ranges.

These would be good things to mention in the commit message.

**Code changes — correctness:**

The diff itself is a clean revert:

- Removes `readonly_va_entry()` entirely — correct.
- Removes `bool readonly = true;` declaration — correct.
- Removes the `readonly_va_entry()` call in the validation loop — correct.
- Changes `(readonly ? 0 : FOLL_WRITE) | FOLL_LONGTERM` to `FOLL_WRITE | FOLL_LONGTERM` — correct, always pins as writable.
- Changes `(readonly ? O_RDONLY : O_RDWR) | O_CLOEXEC` to `O_RDWR | O_CLOEXEC` — correct, always exports as read-write.

The post-revert state matches what existed before `f649e63d4a64` was applied.

**Minor nit:** The revert message doesn't include a `Cc: stable@` tag. If the original commit went into a released kernel and the TOCTOU race is considered a security-relevant bug, a stable backport annotation might be appropriate. But this is at the maintainer's discretion.

**Recommendation:** Request an improved commit message that explains the specific technical concerns motivating the revert (TOCTOU race, single-VMA assumption, or whatever the actual driver is). The code change itself is correct and can be accepted as-is once the message is improved.

---
Generated by Claude Code Patch Reviewer