Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Modifying vma->vm_pgoff in the dma-buf mmap exporter breaks the dma-buf interval tree, preventing memory revocation. - [High] The patch completely bans mapping the BO at a non-zero offset, which breaks userspace compatibility for partial mappings. --
commit f5b497b44e071da0d7d3ac56c9b4055ed5127ba8 Author: Lizhi Hou <[email protected]> Subject: accel/amdxdna: Fix unsafe use of handle_mm_fault() This commit replaces the use of handle_mm_fault() from the mmap callback with a VM_MIXEDMAP and vm_insert_pages() approach for shmem BOs. For imported BOs, it marks the mapping as invalid and relies on the first command submission to fault in the pages. > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c > b/drivers/accel/amdxdna/amdxdna_gem.c > index 3f05fa8811af1..612f36a2c0725 100644 > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c [ ... ] > @@ -490,16 +491,12 @@ 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 { > + if (vma->vm_pgoff - drm_vma_node_start(&to_gobj(abo)->vma_node)) > + return -EINVAL; [Severity: High] Does this check artificially reject valid non-zero offsets that were previously supported? Userspace applications that attempt to mmap a sub-region of a Buffer Object will receive an unexpected -EINVAL error. If a userspace process calls mmap() on the DRM device fd, passing an offset that includes both the DRM mmap node offset and a logical offset within the BO, it seems this strict equality check will fail and ban partial mappings. > + > + if (is_import_bo(abo)) { > vma->vm_private_data = NULL; [ ... ] > 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); > + vma->vm_pgoff += drm_vma_node_start(&gobj->vma_node); [Severity: High] Could modifying vma->vm_pgoff here corrupt the logical offset used for linking the VMA into the dma-buf address_space interval tree? After the callback returns, the core memory management (mmap_region) links the VMA into the dma-buf's file mapping interval tree using this highly inflated vm_pgoff. Because of this, memory revocation mechanisms for dma-bufs might fail to find and unmap this VMA, leaving stale PTEs that can lead to a use-after-free if the backing memory is freed. > ret = drm_gem_shmem_mmap(&abo->base, vma); > if (ret) > goto put_obj; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
