> From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma-
> owner@xxxxxxxxxxxxxxx> On Behalf Of Jason Gunthorpe
>
> 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..
I actually misunderstood you before and thought that we want to maintain all mappings in umap.
Now I understand you meant only the io mappings, since they're not referenced counted by the mm system.
I'll revert the changes and add an additional parameter to rdma_user_mmap_io ->
However, this means more changes in drivers that call this function and don't use the mmap_xa,
Should I add an additional interface ? or are you OK with me sending NULL to the additional parameter from
all drivers using the interface ? (hns, mlx4, mlx5)
Also, who should increase the refcnt of rdma_user_mmap_entry when it is set to the vma private data ?
The caller of rdma_user_mmap_io or inside rdma_user_mmap_io function ?
This will definitely simplify things.
>
> Jason
