Re: [PATCH V3] accel/amdxdna: Fix unsafe use of handle_mm_fault()

[Lizhi Hou <lizhi.hou@amd.com>]

Applied to drm-misc-next

On 9/10/26 15:39, Max Zhen wrote:
>
>
> On 9/10/2026 Thu 14:13, Lizhi Hou wrote:
>> handle_mm_fault() must not be called from the mmap callback because
>> the VMA has not yet been linked. The handle_mm_fault() API contract
>> assumes that the VMA is already linked.
>>
>> Remove the handle_mm_fault() call from the mmap callback. For imported
>> BOs, mark the mapping as invalid and rely on the first command 
>> submission
>> to fault in the pages.
>>
>> For shmem BOs, set the VM_MIXEDMAP flag and use vm_insert_pages().
>> Implement amdxdna_gem_mixed_vm_ops to handle the page faults.
>>
>> Fixes: e486147c912f ("accel/amdxdna: Add BO import and export")
>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> Reviewed-by: Max Zhen <max.zhen@amd.com>
>> ---
>> v2 & v3:
>>    Fix sashiko comment.
>>
>>   drivers/accel/amdxdna/amdxdna_gem.c | 151 ++++++++++++++++++++++------
>>   1 file changed, 118 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c 
>> b/drivers/accel/amdxdna/amdxdna_gem.c
>> index 0d165b66c1fc..a12a762b3a6b 100644
>> --- a/drivers/accel/amdxdna/amdxdna_gem.c
>> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/dma-direct.h>
>>   #include <linux/iosys-map.h>
>>   #include <linux/pagemap.h>
>> +#include <linux/swap.h>
>>   #include <linux/vmalloc.h>
>>     #include "amdxdna_cbuf.h"
>> @@ -490,16 +491,16 @@ static int amdxdna_insert_pages(struct 
>> amdxdna_gem_obj *abo,
>>   {
>>       struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
>>       unsigned long num_pages = vma_pages(vma);
>> -    unsigned long offset = 0;
>>       int ret;
>>   -    if (!is_import_bo(abo)) {
>> -        ret = drm_gem_shmem_mmap(&abo->base, vma);
>> -        if (ret) {
>> -            XDNA_ERR(xdna, "Failed shmem mmap %d", ret);
>> -            return ret;
>> -        }
>> -    } else {
>> +    /*
>> +     * Until today there is not any use case to mmap with non-zero
>> +     * offset. Put an explicit check here.
>> +     */
>> +    if (vma->vm_pgoff - drm_vma_node_start(&to_gobj(abo)->vma_node))
>> +        return -EINVAL;
>> +
>> +    if (is_import_bo(abo)) {
>>           vma->vm_private_data = NULL;
>>           vma->vm_ops = NULL;
>>           ret = dma_buf_mmap(abo->dma_buf, vma, 0);
>> @@ -508,23 +509,28 @@ static int amdxdna_insert_pages(struct 
>> amdxdna_gem_obj *abo,
>>               return ret;
>>           }
>>   +        amdxdna_mark_mapp_invalid(abo, vma);
>> +
>>           /* Drop the reference drm_gem_mmap_obj() acquired.*/
>>           drm_gem_object_put(to_gobj(abo));
>> +        return 0;
>>       }
>>   -    do {
>> -        vm_fault_t fault_ret;
>> -
>> -        fault_ret = handle_mm_fault(vma, vma->vm_start + offset,
>> -                        FAULT_FLAG_WRITE, NULL);
>> -        if (fault_ret & VM_FAULT_ERROR) {
>> -            XDNA_ERR(xdna, "Fault in page failed");
>> -            amdxdna_mark_mapp_invalid(abo, vma);
>> -            break;
>> -        }
>> +    ret = drm_gem_shmem_mmap(&abo->base, vma);
>> +    if (ret) {
>> +        XDNA_ERR(xdna, "Failed shmem mmap %d", ret);
>> +        return ret;
>> +    }
>>   -        offset += PAGE_SIZE;
>> -    } while (--num_pages);
>> +    vm_flags_mod(vma, VM_MIXEDMAP, VM_PFNMAP);
>> +    ret = vm_insert_pages(vma, vma->vm_start, abo->base.pages, 
>> &num_pages);
>> +    if (ret) {
>> +        XDNA_ERR(xdna, "Failed to insert pages %d", ret);
>> +        dma_resv_lock(to_gobj(abo)->resv, NULL);
>> +        drm_gem_shmem_put_pages_locked(&abo->base);
>> +        dma_resv_unlock(to_gobj(abo)->resv);
>> +        return ret;
>> +    }
>>         return 0;
>>   }
>> @@ -536,6 +542,10 @@ static int amdxdna_gem_obj_mmap(struct 
>> drm_gem_object *gobj,
>>       struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
>>       int ret;
>>   +    XDNA_DBG(xdna, "BO map_offset 0x%llx type %d userptr 0x%lx 
>> size 0x%lx",
>> +         drm_vma_node_offset_addr(&gobj->vma_node), abo->type,
>> +         vma->vm_start, gobj->size);
>> +
>>       ret = amdxdna_hmm_register(abo, vma);
>>       if (ret)
>>           return ret;
>> @@ -546,9 +556,6 @@ static int amdxdna_gem_obj_mmap(struct 
>> drm_gem_object *gobj,
>>           goto hmm_unreg;
>>       }
>>   -    XDNA_DBG(xdna, "BO map_offset 0x%llx type %d userptr 0x%lx 
>> size 0x%lx",
>> -         drm_vma_node_offset_addr(&gobj->vma_node), abo->type,
>> -         vma->vm_start, gobj->size);
>>       return 0;
>>     hmm_unreg:
>> @@ -556,14 +563,99 @@ static int amdxdna_gem_obj_mmap(struct 
>> drm_gem_object *gobj,
>>       return ret;
>>   }
>>   +/*
>> + * VM operations for amdxdna shmem VMAs that use VM_MIXEDMAP.
>> + *
>> + * drm_gem_shmem_vm_ops cannot be used on VM_MIXEDMAP VMAs because 
>> its fault
>> + * handler calls vmf_insert_pfn() → vmf_insert_pfn_prot() which 
>> contains:
>> + *   BUG_ON((vma->vm_flags & VM_MIXEDMAP) && pfn_valid(pfn))
>> + * All amdxdna shmem pages are ordinary struct pages so pfn_valid() 
>> is always
>> + * true, making the combination fatal.
>> + *
>> + * These ops use vmf_insert_page() (struct-page based) instead, 
>> which is the
>> + * correct API for VM_MIXEDMAP VMAs backed by real struct pages.  
>> The open and
>> + * close handlers replicate drm_gem_shmem_vm_open/close using only 
>> exported
>> + * symbols.
>> + */
>> +static vm_fault_t amdxdna_gem_mixedmap_fault(struct vm_fault *vmf)
>> +{
>> +    struct vm_area_struct *vma = vmf->vma;
>> +    struct drm_gem_object *gobj = vma->vm_private_data;
>> +    struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(gobj);
>> +    loff_t num_pages = gobj->size >> PAGE_SHIFT;
>> +    vm_fault_t ret = VM_FAULT_SIGBUS;
>> +    pgoff_t page_offset;
>> +    struct page *page;
>> +
>> +    /*
>> +     * Partial free of vma is unexpected. Otherwise, the wrong page
>> +     * will be faulted in and the user application may crash itself.
>> +     */
>> +    page_offset = vmf->pgoff - vma->vm_pgoff;
>> +
>> +    dma_resv_lock(gobj->resv, NULL);
>> +
>> +    if (!shmem->pages || shmem->madv < 0 || page_offset >= num_pages)
>> +        goto out;
>> +
>> +    page = shmem->pages[page_offset];
>> +    if (WARN_ON_ONCE(!page))
>> +        goto out;
>> +
>> +    /*
>> +     * Use vmf_insert_page() (struct-page path) not vmf_insert_pfn()
>> +     * (PFN path) because this VMA carries VM_MIXEDMAP.
>> +     */
>> +    ret = vmf_insert_page(vma, vmf->address, page);
>> +    if (ret == VM_FAULT_NOPAGE)
>> +        folio_mark_accessed(page_folio(page));
>> +
>> +out:
>> +    dma_resv_unlock(gobj->resv);
>> +    return ret;
>> +}
>> +
>> +static void amdxdna_gem_mixedmap_vm_open(struct vm_area_struct *vma)
>> +{
>> +    struct drm_gem_object *gobj = vma->vm_private_data;
>> +    struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(gobj);
>> +
>> +    /*
>> +     * Bump pages_use_count so the page array stays alive for the new
>> +     * mapping copy created by fork().  Mirrors 
>> drm_gem_shmem_vm_open().
>> +     */
>> +    dma_resv_lock(gobj->resv, NULL);
>> +    drm_WARN_ON_ONCE(gobj->dev, 
>> !refcount_inc_not_zero(&shmem->pages_use_count));
>> +    dma_resv_unlock(gobj->resv);
>> +
>> +    drm_gem_vm_open(vma);
>> +}
>> +
>> +static void amdxdna_gem_mixedmap_vm_close(struct vm_area_struct *vma)
>> +{
>> +    struct drm_gem_object *gobj = vma->vm_private_data;
>> +    struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(gobj);
>> +
>> +    dma_resv_lock(gobj->resv, NULL);
>> +    drm_gem_shmem_put_pages_locked(shmem);
>> +    dma_resv_unlock(gobj->resv);
>> +
>> +    drm_gem_vm_close(vma);
>> +}
>> +
>> +static const struct vm_operations_struct amdxdna_gem_mixedmap_vm_ops 
>> = {
>> +    .fault  = amdxdna_gem_mixedmap_fault,
>> +    .open   = amdxdna_gem_mixedmap_vm_open,
>> +    .close  = amdxdna_gem_mixedmap_vm_close,
>> +};
>> +
>>   static int amdxdna_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct 
>> vm_area_struct *vma)
>>   {
>>       struct drm_gem_object *gobj = dma_buf->priv;
>>       struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
>> -    unsigned long num_pages = vma_pages(vma);
>>       int ret;
>>   -    vma->vm_ops = &drm_gem_shmem_vm_ops;
>> +    vma->vm_ops = &amdxdna_gem_mixedmap_vm_ops;
>>       vma->vm_private_data = gobj;
>>         drm_gem_object_get(gobj);
>> @@ -573,16 +665,9 @@ static int amdxdna_gem_dmabuf_mmap(struct 
>> dma_buf *dma_buf, struct vm_area_struc
>>         /* The buffer is based on memory pages. Fix the flag. */
>>       vm_flags_mod(vma, VM_MIXEDMAP, VM_PFNMAP);
>> -    ret = vm_insert_pages(vma, vma->vm_start, abo->base.pages,
>> -                  &num_pages);
>> -    if (ret)
>> -        goto close_vma;
>>         return 0;
>>   -close_vma:
>> -    vma->vm_ops->close(vma);
>> -    return ret;
>>   put_obj:
>>       drm_gem_object_put(gobj);
>>       return ret;
>> @@ -878,7 +963,7 @@ static const struct drm_gem_object_funcs 
>> amdxdna_gem_shmem_funcs = {
>>       .vmap = amdxdna_gem_obj_vmap,
>>       .vunmap = amdxdna_gem_obj_vunmap,
>>       .mmap = amdxdna_gem_obj_mmap,
>> -    .vm_ops = &drm_gem_shmem_vm_ops,
>> +    .vm_ops = &amdxdna_gem_mixedmap_vm_ops,
>>       .export = amdxdna_gem_prime_export,
>>   };
>