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

[Claude Code Review Bot <claude-review@example.com>]

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