netdev - RE: [PATCH v6 rdma-next 1/6] RDMA/core: Create mmap database and cookie helper functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 26 Jul 2019 08:42:07 +0000
From:   Michal Kalderon <mkalderon@...vell.com>
To:     Jason Gunthorpe <jgg@...pe.ca>
CC:     Kamal Heib <kamalheib1@...il.com>,
        Ariel Elior <aelior@...vell.com>,
        "dledford@...hat.com" <dledford@...hat.com>,
        "galpress@...zon.com" <galpress@...zon.com>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH v6 rdma-next 1/6] RDMA/core: Create mmap database and
 cookie helper functions

> From: linux-rdma-owner@...r.kernel.org <linux-rdma-
> owner@...r.kernel.org> On Behalf Of Jason Gunthorpe
> 
> On Thu, Jul 25, 2019 at 07:34:15PM +0000, Michal Kalderon wrote:
> > > > +	ibdev_dbg(ucontext->device,
> > > > +		  "mmap: obj[0x%p] key[%#llx] addr[%#llx] len[%#llx]
> > > removed\n",
> > > > +		  entry->obj, key, entry->address, entry->length);
> > > > +
> > > > +	return entry;
> > > > +}
> > > > +EXPORT_SYMBOL(rdma_user_mmap_entry_get);
> > >
> > > It is a mistake we keep making, and maybe the war is hopelessly lost
> > > now, but functions called from a driver should not be part of the
> > > ib_uverbs module
> > > - ideally uverbs is an optional module. They should be in ib_core.
> > >
> > > Maybe put this in ib_core_uverbs.c ?
> 
> > But if there isn't ib_uverbs user apps can't be run right ? and then
> > these functions Won't get called anyway ?
> 
> Right, but, we don't want loading the driver to force creating
> /dev/infiniband/uverbs - so the driver support component of uverbs should
> live in ib_core, and the /dev/ component should be in ib_uverbs
> 
> > > > +	xa_lock(&ucontext->mmap_xa);
> > > > +	if (check_add_overflow(ucontext->mmap_xa_page,
> > > > +			       (u32)(length >> PAGE_SHIFT),
> > >
> > > Should this be divide round up ?
> 
> > For cases that length is not rounded to PAGE_SHIFT?
> 
> It should never happen, but yes
ok
> 
> > >
> > > > +			       &next_mmap_page))
> > > > +		goto err_unlock;
> > >
> > > I still don't like that this algorithm latches into a permanent
> > > failure when the xa_page wraps.
> > >
> > > It seems worth spending a bit more time here to tidy this.. Keep
> > > using the mmap_xa_page scheme, but instead do something like
> > >
> > > alloc_cyclic_range():
> > >
> > > while () {
> > >    // Find first empty element in a cyclic way
> > >    xa_page_first = mmap_xa_page;
> > >    xa_find(xa, &xa_page_first, U32_MAX, XA_FREE_MARK)
> > >
> > >    // Is there a enough room to have the range?
> > >    if (check_add_overflow(xa_page_first, npages, &xa_page_end)) {
> > >       mmap_xa_page = 0;
> > >       continue;
> > >    }
> > >
> > >    // See if the element before intersects
> > >    elm = xa_find(xa, &zero, xa_page_end, 0);
> > >    if (elm && intersects(xa_page_first, xa_page_last, elm->first, elm-
> >last)) {
> > >       mmap_xa_page = elm->last + 1;
> > >       continue
> > >    }
> > >
> > >    // xa_page_first -> xa_page_end should now be free
> > >    xa_insert(xa, xa_page_start, entry);
> > >    mmap_xa_page = xa_page_end + 1;
> > >    return xa_page_start;
> > > }
> > >
> > > Approximately, please check it.
> 
> > But we don't free entires from the xa_array ( only when ucontext is
> > destroyed) so how will There be an empty element after we wrap ?
> 
> Oh!
> 
> That should be fixed up too, in the general case if a user is
> creating/destroying driver objects in loop we don't want memory usage to
> be unbounded.
> 
> The rdma_user_mmap stuff has VMA ops that can refcount the xa entry and
> now that this is core code it is easy enough to harmonize the two things and
> track the xa side from the struct rdma_umap_priv
> 
> The question is, does EFA or qedr have a use model for this that allows a
> userspace verb to create/destroy in a loop? ie do we need to fix this right
> now?
The mapping occurs for every qp and cq creation. So yes. 
So do you mean add a ref-cnt to the xarray entry and from umap decrease the refcnt and free? 

> 
> Jason