Re: [PATCH V2 5/9] iommufd: IOMMU_IOAS_MAP_FILE implementation

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Sep 24, 2024 at 08:05:34AM -0700, Steve Sistare wrote:

> @@ -199,7 +200,44 @@ static int conv_iommu_prot(u32 map_flags)
>  
>  int iommufd_ioas_map_file(struct iommufd_ucmd *ucmd)
>  {
> -	return -ENOTTY;
> +	struct iommu_ioas_map_file *cmd = ucmd->cmd;
> +	unsigned long iova = cmd->iova;
> +	struct iommufd_ioas *ioas;
> +	unsigned int flags = 0;
> +	int rc;
> +	struct file *file;
> +	unsigned long end;
> +
> +	if (cmd->iova >= ULONG_MAX || cmd->length >= ULONG_MAX)
> +		return -EOVERFLOW;
> +
> +	if (check_add_overflow(cmd->start, cmd->length - 1, &end))
> +		return -EOVERFLOW;

Do we check that length != 0 before doing this math someplace?

> +	ioas = iommufd_get_ioas(ucmd->ictx, cmd->ioas_id);
> +	if (IS_ERR(ioas))
> +		return PTR_ERR(ioas);
> +
> +	if (!(cmd->flags & IOMMU_IOAS_MAP_FIXED_IOVA))
> +		flags = IOPT_ALLOC_IOVA;
> +
> +	file = fget(cmd->fd);
> +	if (!file)
> +		return -EBADF;
> +
> +	rc = iopt_map_file_pages(ucmd->ictx, &ioas->iopt, &iova,
> +				 file, cmd->start,
> +				 cmd->length,
> +				 conv_iommu_prot(cmd->flags), flags);
> +	if (rc)
> +		goto out_put;
> +
> +	cmd->iova = iova;
> +	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
> +out_put:
> +	iommufd_put_object(ucmd->ictx, &ioas->obj);
> +	fput(file);

I don't see a second fput, what does fput on the success path?

>  int iommufd_ioas_map(struct iommufd_ucmd *ucmd)
> diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
> index df5ba4f..0a432dc2 100644
> --- a/drivers/iommu/iommufd/pages.c
> +++ b/drivers/iommu/iommufd/pages.c
> @@ -902,7 +902,8 @@ static int update_mm_locked_vm(struct iopt_pages *pages, unsigned long npages,
>  		mmap_read_unlock(pages->source_mm);
>  		user->locked = 0;
>  		/* If we had the lock then we also have a get */
> -	} else if ((!user || !user->upages) &&
> +
> +	} else if ((!user || (!user->upages && !user->ufolios)) &&
>  		   pages->source_mm != current->mm) {
>  		if (!mmget_not_zero(pages->source_mm))
>  			return -EINVAL;

Does this hunk go in an earlier previous patch?

Jason