Support pin user pages with read-only mapping pointer#1198
Conversation
Signed-off-by: Max Zhen <max.zhen@amd.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the AMD XDNA user-buffer (ubuf) dma-buf export path to support pinning user pages when the supplied user virtual address range is mapped read-only, by conditionally omitting FOLL_WRITE during long-term GUP.
Changes:
- Add a helper to detect whether a VA entry resides in a read-only VMA.
- Compute a
readonlydecision while validating VA entries. - Conditionally include
FOLL_WRITEinpin_user_pages_fast()based on the read-only decision.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ret = pin_user_pages_fast(va_ent[i].vaddr, npages, | ||
| FOLL_WRITE | FOLL_LONGTERM, | ||
| (readonly ? 0 : FOLL_WRITE) | FOLL_LONGTERM, | ||
| &ubuf->pages[start]); |
There was a problem hiding this comment.
Allowing pinning without FOLL_WRITE means this path can now accept pages from a read-only user VMA, but the exported dma-buf can still be mapped/used as writable elsewhere (the dma-buf is exported as O_RDWR, and the dma-buf mmap path doesn’t appear to enforce read-only). This can effectively upgrade a read-only mapping into a writable one via the dma-buf fd for file-backed/read-only mappings. Consider tracking “read-only” in amdxdna_ubuf_priv and enforcing it (e.g., export as O_RDONLY and/or reject/clear VM_WRITE on dma-buf mmap) when FOLL_WRITE is not used.
| if (ret < 0 || ret != npages) { | ||
| ret = -ENOMEM; |
There was a problem hiding this comment.
If pin_user_pages_fast() returns a partial success (0 < ret < npages), the code treats it as an error but doesn’t unpin the ret pages that were just pinned for this entry (since start isn’t incremented before goto destroy_pages). This can leak long-term pins. Consider handling the partial case explicitly by unpinning the newly pinned subset (or incrementing start before the error path) and by preserving the underlying error code where possible.
| if (ret < 0 || ret != npages) { | |
| ret = -ENOMEM; | |
| if (ret != npages) { | |
| if (ret > 0) | |
| unpin_user_pages(&ubuf->pages[start], ret); | |
| if (ret > 0) | |
| ret = -ENOMEM; |
| if (readonly && readonly_va_entry(&va_ent[i]) != 1) | ||
| readonly = false; |
There was a problem hiding this comment.
readonly_va_entry() can return a negative error (e.g., -ENOENT when the VA range isn’t covered by a VMA), but the caller treats any value other than 1 as “not read-only” and continues. This can mask invalid VA ranges and later fail in pin_user_pages_fast() with a misleading -ENOMEM. Consider propagating negative returns here (bail out of amdxdna_get_ubuf() with the specific error) instead of treating them as writable.
| if (readonly && readonly_va_entry(&va_ent[i]) != 1) | |
| readonly = false; | |
| if (readonly) { | |
| int ro = readonly_va_entry(&va_ent[i]); | |
| if (ro < 0) { | |
| ret = ro; | |
| goto free_ent; | |
| } | |
| if (ro != 1) | |
| readonly = false; | |
| } |
| for (i = 0; i < num_entries; i++) { | ||
| 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]); |
There was a problem hiding this comment.
The global readonly decision is applied to all VA entries when pinning. If the user supplies a mix of writable and read-only VMAs, setting readonly = false will add FOLL_WRITE for every entry, and pin_user_pages_fast() will fail on the read-only segments. Pin flags likely need to be determined per-entry (or entries should be grouped/pinned separately based on their VMA permissions) rather than using a single boolean for the whole table.
Signed-off-by: Max Zhen <max.zhen@amd.com>
Signed-off-by: Max Zhen <max.zhen@amd.com>
No description provided.