Re: [PATCH v3 13/13] iommufd: update to use mmap_prepare

[Jason Gunthorpe <jgg@nvidia.com>]

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;
 }