Thank you for your contribution! Sashiko AI review found 1 potential issue(s) 
to consider:
- [Medium] Math bug in vfio_pci_dma_buf_find_pfn() breaks direct DMABUF mmap() 
by conflating VFIO and DMABUF VMA coordinate spaces.
--

--- Patch [5]: [PATCH v5 5/9] vfio/pci: Convert BAR mmap() to use a DMABUF ---
commit 9a789f4b1551b3573dc4e2e6ba3af07e0aa65ce5
Author: Matt Evans <[email protected]>
vfio/pci: Convert BAR mmap() to use a DMABUF

This commit converts the VFIO device fd fops->mmap to create a DMABUF
representing the BAR mapping. The VMA fault handler is updated to look
up PFNs from the corresponding DMABUF instead. This is intended to support
future code for direct mmap()ing of BAR DMABUFs.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c 
> b/drivers/vfio/pci/vfio_pci_core.c
> index 3f11a9624b9c0..adce779c722c7 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
[ ... ]
> @@ -1778,23 +1766,42 @@ static vm_fault_t vfio_pci_mmap_huge_fault(struct 
> vm_fault *vmf,
>                                          unsigned int order)
>  {
[ ... ]
> +     scoped_guard(rwsem_read, &vdev->memory_lock) {
> +             if (!priv->revoked) {
> +                     int pres = vfio_pci_dma_buf_find_pfn(priv, vma,
> +                                                          vmf->address,
> +                                                          order, &pfn);

[Severity: Medium]
Will this actually support direct DMABUF mmap()s as the commit message
suggests?

Looking at the math inside vfio_pci_dma_buf_find_pfn():

drivers/vfio/pci/vfio_pci_dmabuf.c:vfio_pci_dma_buf_find_pfn() {
    ...
        unsigned long vma_off = ((vma->vm_pgoff - priv->vma_pgoff_adjust) <<
                                 PAGE_SHIFT) & VFIO_PCI_OFFSET_MASK;
    ...
}

If a direct DMABUF mmap has a vma->vm_pgoff of 0, won't 0 - 
priv->vma_pgoff_adjust
underflow and cause the bounds checks to fail?

Additionally, inserting the VFIO VMA into the DMABUF's i_mmap tree with a
shifted offset seems to conflate coordinate spaces. Could this break future
targeted unmap_mapping_range() calls?

> +
> +                     if (pres == 0)
> +                             ret = vfio_pci_vmf_insert_pfn(vdev, vmf,
> +                                                           pfn, order);
> +                     else if (pres == -EAGAIN)
> +                             ret = VM_FAULT_FALLBACK;
> +             }
>       }

-- 
Sashiko AI review ยท 
https://sashiko.dev/#/patchset/[email protected]?part=5

Reply via email to