Re: [PATCH v11 rdma-next 3/7] RDMA/efa: Use the common mmap_xa helpers

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Sep 05, 2019 at 01:01:13PM +0300, Michal Kalderon wrote:
>  static int __efa_mmap(struct efa_dev *dev, struct efa_ucontext *ucontext,
> -		      struct vm_area_struct *vma, u64 key, u64 length)
> +		      struct vm_area_struct *vma, u64 key, size_t length)
>  {
> -	struct efa_mmap_entry *entry;
> +	struct rdma_user_mmap_entry *rdma_entry;
> +	struct efa_user_mmap_entry *entry;
>  	unsigned long va;
>  	u64 pfn;
>  	int err;
>  
> -	entry = mmap_entry_get(dev, ucontext, key, length);
> -	if (!entry) {
> +	rdma_entry = rdma_user_mmap_entry_get(&ucontext->ibucontext, key,
> +					      length, vma);
> +	if (!rdma_entry) {

This allocates memory and assigns it to vma->vm_private

>  		ibdev_dbg(&dev->ibdev, "key[%#llx] does not have valid entry\n",
>  			  key);
>  		return -EINVAL;
>  	}
> +	entry = to_emmap(rdma_entry);
> +	if (entry->length != length) {
> +		ibdev_dbg(&dev->ibdev,
> +			  "key[%#llx] does not have valid length[%#zx] expected[%#zx]\n",
> +			  key, length, entry->length);
> +		err = -EINVAL;
> +		goto err;

Now we take an error..

> +err:
> +	rdma_user_mmap_entry_put(&ucontext->ibucontext,
> +				 rdma_entry);

And this leaks the struct rdma_umap_priv

I said this already, but it looks wrong that rdma_umap_priv_init() is
testing vm_private_data. rdma_user_mmap_io should accept the struct
rdma_user_mmap_entry pointer so it can directly and always create the
priv instead of trying to pass allocated memory through the vm_private

The only place that should set vm_private_data is rdma_user_mmap_io()
and once it succeeds the driver must return success back through
file_operations->mmap()

This hidden detail should also be noted in the comment for
rdma_user_mmap_io..

Jason