Re: Gem GTT mmaps..

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wednesday, February 4, 2009 3:42 pm Eric Anholt wrote:
> On Wed, 2009-02-04 at 15:02 -0800, Jesse Barnes wrote:
> > On Wednesday, February 4, 2009 2:32 pm Thomas Hellström wrote:
> > > Jesse,
> > >
> > > I have some concerns about the GEM GTT mmap functionality.
> >
> > Thanks for looking it over again; you would know since some of this code
> > came from you in the first place. :)
> >
> > > First, a gem object pointer is copied to map->offset and then to the
> > > vma->vm_private_data without proper reference counting. This pointer is
> > > used in i915_gem_fault() to access the gem object. However if the gem
> > > object is destroyed and a process then tries to access data in a vma
> > > mapping the (now destroyed) object, it would dereference a stale
> > > pointer into kernel space? Shouldn't those pointers be reference
> > > counted, and to account for fork(), a vm open and close would be needed
> > > to  reference count corresponding pointers of newly created and
> > > destroyed vmas?
> >
> > Yeah looks like we don't protect against vm_private_data pointing at a
> > freed or other object.  But rather than refcounting the pointers I wonder
> > if we could make the private data use the GEM object name instead, then
> > do the lookup in the fault handler?
>
> The object doesn't necessarily have a public name.  You do need to
> refcount the objects.

So if we leave the lookup reference around from the GTT mapping ioctl, that 
would take care of new mappings.  And if we added/removed references at VM 
open/close time, we should be covered for fork.  But is it ok to add a new 
unref in the finish ioctl for GTT mapped objects?  I don't think so, because 
we don't know for sure if the caller was the one that created the new fake 
offset (which would be one way of detecting whether it was GTT mapped).  
Seems like we need a new unmap ioctl?  Or we could put the mapping ref/unref 
in libdrm, where it would be tracked on a per-process basis...

-- 
Jesse Barnes, Intel Open Source Technology Center