On Tue, Sep 16, 2025 at 03:11:59PM +0100, Lorenzo Stoakes wrote:
> -static int iommufd_fops_mmap(struct file *filp, struct vm_area_struct *vma)
> +static int iommufd_fops_mmap_prepare(struct vm_area_desc *desc)
> {
> + struct file *filp = desc->file;
> struct iommufd_ctx *ictx = filp->private_data;
> - size_t length = vma->vm_end - vma->vm_start;
> + const size_t length = vma_desc_size(desc);
> struct iommufd_mmap *immap;
> - int rc;
>
> if (!PAGE_ALIGNED(length))
> return -EINVAL;
This is for sure redundant? Ie vma_desc_size() is always page
multiples? Lets drop it
> - if (!(vma->vm_flags & VM_SHARED))
> + if (!(desc->vm_flags & VM_SHARED))
> return -EINVAL;
This should be that no COW helper David found
> - /* vma->vm_pgoff carries a page-shifted start position to an immap */
> - immap = mtree_load(&ictx->mt_mmap, vma->vm_pgoff << PAGE_SHIFT);
> + /* desc->pgoff carries a page-shifted start position to an immap */
> + immap = mtree_load(&ictx->mt_mmap, desc->pgoff << PAGE_SHIFT);
> if (!immap)
> return -ENXIO;
> /*
> * mtree_load() returns the immap for any contained mmio_addr, so only
> * allow the exact immap thing to be mapped
> */
> - if (vma->vm_pgoff != immap->vm_pgoff || length != immap->length)
> + if (desc->pgoff != immap->vm_pgoff || length != immap->length)
> return -ENXIO;
>
> - vma->vm_pgoff = 0;
I think this is an existing bug, I must have missed it when I reviewed
this. If we drop it then the vma will naturally get pgoff right?
> - vma->vm_private_data = immap;
> - vma->vm_ops = &iommufd_vma_ops;
> - vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> + desc->pgoff = 0;
> + desc->private_data = immap;
> + desc->vm_ops = &iommufd_vma_ops;
> + desc->page_prot = pgprot_noncached(desc->page_prot);
>
> - rc = io_remap_pfn_range(vma, vma->vm_start,
> - immap->mmio_addr >> PAGE_SHIFT, length,
> - vma->vm_page_prot);
> - if (rc)
> - return rc;
> + mmap_action_ioremap_full(desc, immap->mmio_addr >> PAGE_SHIFT);
> + desc->action.success_hook = iommufd_fops_mmap_success;
>
> - /* vm_ops.open won't be called for mmap itself. */
> - refcount_inc(&immap->owner->users);
Ooh this is racey existing bug, I'm going to send a patch for it
right now.. So success_hook won't work here.
@@ -551,15 +551,24 @@ static int iommufd_fops_mmap(struct file *filp, struct
vm_area_struct *vma)
return -EPERM;
/* vma->vm_pgoff carries a page-shifted start position to an immap */
+ mtree_lock(&ictx->mt_mmap);
immap = mtree_load(&ictx->mt_mmap, vma->vm_pgoff << PAGE_SHIFT);
- if (!immap)
+ if (!immap) {
+ mtree_unlock(&ictx->mt_mmap);
return -ENXIO;
+ }
+ /* vm_ops.open won't be called for mmap itself. */
+ refcount_inc(&immap->owner->users);
+ mtree_unlock(&ictx->mt_mmap);
+
/*
* mtree_load() returns the immap for any contained mmio_addr, so only
* allow the exact immap thing to be mapped
*/
- if (vma->vm_pgoff != immap->vm_pgoff || length != immap->length)
- return -ENXIO;
+ if (vma->vm_pgoff != immap->vm_pgoff || length != immap->length) {
+ rc = -ENXIO;
+ goto err_refcount;
+ }
vma->vm_pgoff = 0;
vma->vm_private_data = immap;
@@ -570,10 +579,11 @@ static int iommufd_fops_mmap(struct file *filp, struct
vm_area_struct *vma)
immap->mmio_addr >> PAGE_SHIFT, length,
vma->vm_page_prot);
if (rc)
- return rc;
+ goto err_refcount;
+ return 0;
- /* vm_ops.open won't be called for mmap itself. */
- refcount_inc(&immap->owner->users);
+err_refcount:
+ refcount_dec(&immap->owner->users);
return rc;
}
